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@xxxxxxxxx>:
> > 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@xxxxxxxxxxx>:
> >> 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@xxxxxxxxxxx> 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@xxxxxxxxxxx> wrote:
> >>>> On Tue, Feb 3, 2015 at 4:12 AM, Ding Dinghua <dingdinghua85@xxxxxxxxx> 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@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
