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@xxxxxxxxxx> 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@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
