| Message ID | CANf+_kLDDEAy2Ra-3U-zXiuHnTG+q9TO2it5eWmRYzpBYm4i-w@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 > > -- 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
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 >> >> -- 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
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>: