* 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.