* Confused about SnapMapper::get_prefix
@ 2015-02-03 12:12 Ding Dinghua
2015-02-03 17:15 ` Gregory Farnum
0 siblings, 1 reply; 8+ messages in thread
From: Ding Dinghua @ 2015-02-03 12:12 UTC (permalink / raw)
To: ceph-devel
Hi all:
I don't understand why SnapMapper::get_prefix static_cast snap
to unsigned:
string SnapMapper::get_prefix(snapid_t snap)
{
char buf[100];
int len = snprintf(
buf, sizeof(buf),
"%.*X_", (int)(sizeof(snap)*2),
static_cast<unsigned>(snap));
return MAPPING_PREFIX + string(buf, len);
}
Will this limit snapshot count in pool to 2^32 -1 ?
Could anyone clarify ? Thanks
--
Ding Dinghua
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: Confused about SnapMapper::get_prefix 2015-02-03 12:12 Confused about SnapMapper::get_prefix Ding Dinghua @ 2015-02-03 17:15 ` Gregory Farnum 2015-02-03 17:21 ` Samuel Just 0 siblings, 1 reply; 8+ messages in thread From: Gregory Farnum @ 2015-02-03 17:15 UTC (permalink / raw) To: Ding Dinghua, sjust; +Cc: ceph-devel On Tue, Feb 3, 2015 at 4:12 AM, Ding Dinghua <dingdinghua85@gmail.com> wrote: > Hi all: > I don't understand why SnapMapper::get_prefix static_cast snap > to unsigned: > > string SnapMapper::get_prefix(snapid_t snap) > { > char buf[100]; > int len = snprintf( > buf, sizeof(buf), > "%.*X_", (int)(sizeof(snap)*2), > static_cast<unsigned>(snap)); > return MAPPING_PREFIX + string(buf, len); > } > > Will this limit snapshot count in pool to 2^32 -1 ? > > Could anyone clarify ? Thanks I think the code base is a little confused about whether snaps should be 32 or 64 bits in various places. :( That said, the size of unsigned can vary across architectures, so this should probably be sized more explicitly as whatever it's supposed to be on the disk... ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Confused about SnapMapper::get_prefix 2015-02-03 17:15 ` Gregory Farnum @ 2015-02-03 17:21 ` Samuel Just 2015-02-03 17:25 ` Samuel Just 0 siblings, 1 reply; 8+ messages in thread From: Samuel Just @ 2015-02-03 17:21 UTC (permalink / raw) To: Gregory Farnum; +Cc: Ding Dinghua, ceph-devel It looks like snapid_t is a uint64_t, but snprintf expects an unsigned there. -Sam On Tue, Feb 3, 2015 at 9:15 AM, Gregory Farnum <greg@gregs42.com> wrote: > On Tue, Feb 3, 2015 at 4:12 AM, Ding Dinghua <dingdinghua85@gmail.com> wrote: >> Hi all: >> I don't understand why SnapMapper::get_prefix static_cast snap >> to unsigned: >> >> string SnapMapper::get_prefix(snapid_t snap) >> { >> char buf[100]; >> int len = snprintf( >> buf, sizeof(buf), >> "%.*X_", (int)(sizeof(snap)*2), >> static_cast<unsigned>(snap)); >> return MAPPING_PREFIX + string(buf, len); >> } >> >> Will this limit snapshot count in pool to 2^32 -1 ? >> >> Could anyone clarify ? Thanks > > I think the code base is a little confused about whether snaps should > be 32 or 64 bits in various places. :( That said, the size of unsigned > can vary across architectures, so this should probably be sized more > explicitly as whatever it's supposed to be on the disk... ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Confused about SnapMapper::get_prefix 2015-02-03 17:21 ` Samuel Just @ 2015-02-03 17:25 ` Samuel Just 2015-02-04 4:02 ` Ding Dinghua 0 siblings, 1 reply; 8+ messages in thread From: Samuel Just @ 2015-02-03 17:25 UTC (permalink / raw) To: Gregory Farnum; +Cc: Ding Dinghua, ceph-devel Should probably be cast to long unsigned with lX conversion specifier? -Sam On Tue, Feb 3, 2015 at 9:21 AM, Samuel Just <sam.just@inktank.com> wrote: > It looks like snapid_t is a uint64_t, but snprintf expects an unsigned there. > -Sam > > On Tue, Feb 3, 2015 at 9:15 AM, Gregory Farnum <greg@gregs42.com> wrote: >> On Tue, Feb 3, 2015 at 4:12 AM, Ding Dinghua <dingdinghua85@gmail.com> wrote: >>> Hi all: >>> I don't understand why SnapMapper::get_prefix static_cast snap >>> to unsigned: >>> >>> string SnapMapper::get_prefix(snapid_t snap) >>> { >>> char buf[100]; >>> int len = snprintf( >>> buf, sizeof(buf), >>> "%.*X_", (int)(sizeof(snap)*2), >>> static_cast<unsigned>(snap)); >>> return MAPPING_PREFIX + string(buf, len); >>> } >>> >>> Will this limit snapshot count in pool to 2^32 -1 ? >>> >>> Could anyone clarify ? Thanks >> >> I think the code base is a little confused about whether snaps should >> be 32 or 64 bits in various places. :( That said, the size of unsigned >> can vary across architectures, so this should probably be sized more >> explicitly as whatever it's supposed to be on the disk... ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Confused about SnapMapper::get_prefix 2015-02-03 17:25 ` Samuel Just @ 2015-02-04 4:02 ` Ding Dinghua 2015-02-08 14:17 ` Ding Dinghua 0 siblings, 1 reply; 8+ messages in thread From: Ding Dinghua @ 2015-02-04 4:02 UTC (permalink / raw) To: sjust; +Cc: Gregory Farnum, ceph-devel I think so, snap_id is defined and used as uint64_t in ceph, but here static_cast may introduce bug, since two snap_id may get the same prefix, then same key in snap_mapper. diff --git a/src/osd/SnapMapper.cc b/src/osd/SnapMapper.cc index 315e2e2..27cc2b7 100644 --- a/src/osd/SnapMapper.cc +++ b/src/osd/SnapMapper.cc @@ -72,8 +72,7 @@ string SnapMapper::get_prefix(snapid_ t snap) char buf[100]; int len = snprintf( buf, sizeof(buf), - "%.*X_", (int)(sizeof(snap)*2), - static_cast<unsigned>(snap)); + "%.*llX_", (int)(sizeof(snap)*2), snap); 2015-02-04 1:25 GMT+08:00 Samuel Just <sam.just@inktank.com>: > Should probably be cast to long unsigned with lX conversion specifier? > -Sam > > On Tue, Feb 3, 2015 at 9:21 AM, Samuel Just <sam.just@inktank.com> wrote: >> It looks like snapid_t is a uint64_t, but snprintf expects an unsigned there. >> -Sam >> >> On Tue, Feb 3, 2015 at 9:15 AM, Gregory Farnum <greg@gregs42.com> wrote: >>> On Tue, Feb 3, 2015 at 4:12 AM, Ding Dinghua <dingdinghua85@gmail.com> wrote: >>>> Hi all: >>>> I don't understand why SnapMapper::get_prefix static_cast snap >>>> to unsigned: >>>> >>>> string SnapMapper::get_prefix(snapid_t snap) >>>> { >>>> char buf[100]; >>>> int len = snprintf( >>>> buf, sizeof(buf), >>>> "%.*X_", (int)(sizeof(snap)*2), >>>> static_cast<unsigned>(snap)); >>>> return MAPPING_PREFIX + string(buf, len); >>>> } >>>> >>>> Will this limit snapshot count in pool to 2^32 -1 ? >>>> >>>> Could anyone clarify ? Thanks >>> >>> I think the code base is a little confused about whether snaps should >>> be 32 or 64 bits in various places. :( That said, the size of unsigned >>> can vary across architectures, so this should probably be sized more >>> explicitly as whatever it's supposed to be on the disk... -- Ding Dinghua ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: Confused about SnapMapper::get_prefix 2015-02-04 4:02 ` Ding Dinghua @ 2015-02-08 14:17 ` Ding Dinghua 2015-02-08 15:07 ` Sage Weil 0 siblings, 1 reply; 8+ messages in thread From: Ding Dinghua @ 2015-02-08 14:17 UTC (permalink / raw) To: sjust; +Cc: Gregory Farnum, ceph-devel Ping.. I think so, snap_id is defined and used as uint64_t in ceph, but here static_cast may introduce bug, since two snap_id may get the same prefix, then same key in snap_mapper. diff --git a/src/osd/SnapMapper.cc b/src/osd/SnapMapper.cc index 315e2e2..27cc2b7 100644 --- a/src/osd/SnapMapper.cc +++ b/src/osd/SnapMapper.cc @@ -72,8 +72,7 @@ string SnapMapper::get_prefix(snapid_ t snap) char buf[100]; int len = snprintf( buf, sizeof(buf), - "%.*X_", (int)(sizeof(snap)*2), - static_cast<unsigned>(snap)); + "%.*llX_", (int)(sizeof(snap)*2), snap); 2015-02-04 12:02 GMT+08:00 Ding Dinghua <dingdinghua85@gmail.com>: > I think so, snap_id is defined and used as uint64_t in ceph, but here > static_cast may introduce bug, since two snap_id may get the same > prefix, then same key in snap_mapper. > > diff --git a/src/osd/SnapMapper.cc b/src/osd/SnapMapper.cc > index 315e2e2..27cc2b7 100644 > --- a/src/osd/SnapMapper.cc > +++ b/src/osd/SnapMapper.cc > @@ -72,8 +72,7 @@ string SnapMapper::get_prefix(snapid_ > t snap) > char buf[100]; > int len = snprintf( > buf, sizeof(buf), > - "%.*X_", (int)(sizeof(snap)*2), > - static_cast<unsigned>(snap)); > + "%.*llX_", (int)(sizeof(snap)*2), snap); > > 2015-02-04 1:25 GMT+08:00 Samuel Just <sam.just@inktank.com>: >> Should probably be cast to long unsigned with lX conversion specifier? >> -Sam >> >> On Tue, Feb 3, 2015 at 9:21 AM, Samuel Just <sam.just@inktank.com> wrote: >>> It looks like snapid_t is a uint64_t, but snprintf expects an unsigned there. >>> -Sam >>> >>> On Tue, Feb 3, 2015 at 9:15 AM, Gregory Farnum <greg@gregs42.com> wrote: >>>> On Tue, Feb 3, 2015 at 4:12 AM, Ding Dinghua <dingdinghua85@gmail.com> wrote: >>>>> Hi all: >>>>> I don't understand why SnapMapper::get_prefix static_cast snap >>>>> to unsigned: >>>>> >>>>> string SnapMapper::get_prefix(snapid_t snap) >>>>> { >>>>> char buf[100]; >>>>> int len = snprintf( >>>>> buf, sizeof(buf), >>>>> "%.*X_", (int)(sizeof(snap)*2), >>>>> static_cast<unsigned>(snap)); >>>>> return MAPPING_PREFIX + string(buf, len); >>>>> } >>>>> >>>>> Will this limit snapshot count in pool to 2^32 -1 ? >>>>> >>>>> Could anyone clarify ? Thanks >>>> >>>> I think the code base is a little confused about whether snaps should >>>> be 32 or 64 bits in various places. :( That said, the size of unsigned >>>> can vary across architectures, so this should probably be sized more >>>> explicitly as whatever it's supposed to be on the disk... > > > > -- > Ding Dinghua -- Ding Dinghua ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: Confused about SnapMapper::get_prefix 2015-02-08 14:17 ` Ding Dinghua @ 2015-02-08 15:07 ` Sage Weil 2015-02-09 17:23 ` Samuel Just 0 siblings, 1 reply; 8+ messages in thread From: Sage Weil @ 2015-02-08 15:07 UTC (permalink / raw) To: Ding Dinghua; +Cc: sjust, Gregory Farnum, ceph-devel This looks right to me. Sam? sage On Sun, 8 Feb 2015, Ding Dinghua wrote: > Ping.. > > I think so, snap_id is defined and used as uint64_t in ceph, but here > static_cast may introduce bug, since two snap_id may get the same > prefix, then same key in snap_mapper. > > diff --git a/src/osd/SnapMapper.cc b/src/osd/SnapMapper.cc > index 315e2e2..27cc2b7 100644 > --- a/src/osd/SnapMapper.cc > +++ b/src/osd/SnapMapper.cc > @@ -72,8 +72,7 @@ string SnapMapper::get_prefix(snapid_ > t snap) > char buf[100]; > int len = snprintf( > buf, sizeof(buf), > - "%.*X_", (int)(sizeof(snap)*2), > - static_cast<unsigned>(snap)); > + "%.*llX_", (int)(sizeof(snap)*2), snap); > > 2015-02-04 12:02 GMT+08:00 Ding Dinghua <dingdinghua85@gmail.com>: > > I think so, snap_id is defined and used as uint64_t in ceph, but here > > static_cast may introduce bug, since two snap_id may get the same > > prefix, then same key in snap_mapper. > > > > diff --git a/src/osd/SnapMapper.cc b/src/osd/SnapMapper.cc > > index 315e2e2..27cc2b7 100644 > > --- a/src/osd/SnapMapper.cc > > +++ b/src/osd/SnapMapper.cc > > @@ -72,8 +72,7 @@ string SnapMapper::get_prefix(snapid_ > > t snap) > > char buf[100]; > > int len = snprintf( > > buf, sizeof(buf), > > - "%.*X_", (int)(sizeof(snap)*2), > > - static_cast<unsigned>(snap)); > > + "%.*llX_", (int)(sizeof(snap)*2), snap); > > > > 2015-02-04 1:25 GMT+08:00 Samuel Just <sam.just@inktank.com>: > >> Should probably be cast to long unsigned with lX conversion specifier? > >> -Sam > >> > >> On Tue, Feb 3, 2015 at 9:21 AM, Samuel Just <sam.just@inktank.com> wrote: > >>> It looks like snapid_t is a uint64_t, but snprintf expects an unsigned there. > >>> -Sam > >>> > >>> On Tue, Feb 3, 2015 at 9:15 AM, Gregory Farnum <greg@gregs42.com> wrote: > >>>> On Tue, Feb 3, 2015 at 4:12 AM, Ding Dinghua <dingdinghua85@gmail.com> wrote: > >>>>> Hi all: > >>>>> I don't understand why SnapMapper::get_prefix static_cast snap > >>>>> to unsigned: > >>>>> > >>>>> string SnapMapper::get_prefix(snapid_t snap) > >>>>> { > >>>>> char buf[100]; > >>>>> int len = snprintf( > >>>>> buf, sizeof(buf), > >>>>> "%.*X_", (int)(sizeof(snap)*2), > >>>>> static_cast<unsigned>(snap)); > >>>>> return MAPPING_PREFIX + string(buf, len); > >>>>> } > >>>>> > >>>>> Will this limit snapshot count in pool to 2^32 -1 ? > >>>>> > >>>>> Could anyone clarify ? Thanks > >>>> > >>>> I think the code base is a little confused about whether snaps should > >>>> be 32 or 64 bits in various places. :( That said, the size of unsigned > >>>> can vary across architectures, so this should probably be sized more > >>>> explicitly as whatever it's supposed to be on the disk... > > > > > > > > -- > > Ding Dinghua > > > > -- > Ding Dinghua > -- > To unsubscribe from this list: send the line "unsubscribe ceph-devel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Confused about SnapMapper::get_prefix 2015-02-08 15:07 ` Sage Weil @ 2015-02-09 17:23 ` Samuel Just 0 siblings, 0 replies; 8+ messages in thread From: Samuel Just @ 2015-02-09 17:23 UTC (permalink / raw) To: Sage Weil; +Cc: Ding Dinghua, Gregory Farnum, ceph-devel I think that's right. Do you want to create a pull request against hammer? -Sam On Sun, Feb 8, 2015 at 7:07 AM, Sage Weil <sweil@redhat.com> wrote: > This looks right to me. Sam? > > sage > > > On Sun, 8 Feb 2015, Ding Dinghua wrote: > >> Ping.. >> >> I think so, snap_id is defined and used as uint64_t in ceph, but here >> static_cast may introduce bug, since two snap_id may get the same >> prefix, then same key in snap_mapper. >> >> diff --git a/src/osd/SnapMapper.cc b/src/osd/SnapMapper.cc >> index 315e2e2..27cc2b7 100644 >> --- a/src/osd/SnapMapper.cc >> +++ b/src/osd/SnapMapper.cc >> @@ -72,8 +72,7 @@ string SnapMapper::get_prefix(snapid_ >> t snap) >> char buf[100]; >> int len = snprintf( >> buf, sizeof(buf), >> - "%.*X_", (int)(sizeof(snap)*2), >> - static_cast<unsigned>(snap)); >> + "%.*llX_", (int)(sizeof(snap)*2), snap); >> >> 2015-02-04 12:02 GMT+08:00 Ding Dinghua <dingdinghua85@gmail.com>: >> > I think so, snap_id is defined and used as uint64_t in ceph, but here >> > static_cast may introduce bug, since two snap_id may get the same >> > prefix, then same key in snap_mapper. >> > >> > diff --git a/src/osd/SnapMapper.cc b/src/osd/SnapMapper.cc >> > index 315e2e2..27cc2b7 100644 >> > --- a/src/osd/SnapMapper.cc >> > +++ b/src/osd/SnapMapper.cc >> > @@ -72,8 +72,7 @@ string SnapMapper::get_prefix(snapid_ >> > t snap) >> > char buf[100]; >> > int len = snprintf( >> > buf, sizeof(buf), >> > - "%.*X_", (int)(sizeof(snap)*2), >> > - static_cast<unsigned>(snap)); >> > + "%.*llX_", (int)(sizeof(snap)*2), snap); >> > >> > 2015-02-04 1:25 GMT+08:00 Samuel Just <sam.just@inktank.com>: >> >> Should probably be cast to long unsigned with lX conversion specifier? >> >> -Sam >> >> >> >> On Tue, Feb 3, 2015 at 9:21 AM, Samuel Just <sam.just@inktank.com> wrote: >> >>> It looks like snapid_t is a uint64_t, but snprintf expects an unsigned there. >> >>> -Sam >> >>> >> >>> On Tue, Feb 3, 2015 at 9:15 AM, Gregory Farnum <greg@gregs42.com> wrote: >> >>>> On Tue, Feb 3, 2015 at 4:12 AM, Ding Dinghua <dingdinghua85@gmail.com> wrote: >> >>>>> Hi all: >> >>>>> I don't understand why SnapMapper::get_prefix static_cast snap >> >>>>> to unsigned: >> >>>>> >> >>>>> string SnapMapper::get_prefix(snapid_t snap) >> >>>>> { >> >>>>> char buf[100]; >> >>>>> int len = snprintf( >> >>>>> buf, sizeof(buf), >> >>>>> "%.*X_", (int)(sizeof(snap)*2), >> >>>>> static_cast<unsigned>(snap)); >> >>>>> return MAPPING_PREFIX + string(buf, len); >> >>>>> } >> >>>>> >> >>>>> Will this limit snapshot count in pool to 2^32 -1 ? >> >>>>> >> >>>>> Could anyone clarify ? Thanks >> >>>> >> >>>> I think the code base is a little confused about whether snaps should >> >>>> be 32 or 64 bits in various places. :( That said, the size of unsigned >> >>>> can vary across architectures, so this should probably be sized more >> >>>> explicitly as whatever it's supposed to be on the disk... >> > >> > >> > >> > -- >> > Ding Dinghua >> >> >> >> -- >> Ding Dinghua >> -- >> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> >> ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2015-02-09 17:23 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-02-03 12:12 Confused about SnapMapper::get_prefix Ding Dinghua 2015-02-03 17:15 ` Gregory Farnum 2015-02-03 17:21 ` Samuel Just 2015-02-03 17:25 ` Samuel Just 2015-02-04 4:02 ` Ding Dinghua 2015-02-08 14:17 ` Ding Dinghua 2015-02-08 15:07 ` Sage Weil 2015-02-09 17:23 ` Samuel Just
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.