| Message ID | 1285267031-22966-1-git-send-email-weil@mail.berlios.de |
|---|---|
| State | New |
| Headers | show |
On Thu, Sep 23, 2010 at 6:37 PM, Stefan Weil <weil@mail.berlios.de> wrote: > Adding the gcc format attribute detects a format bug > which is fixed here. > > Cc: Blue Swirl <blauwirbel@gmail.com> > Cc: Kevin Wolf <kwolf@redhat.com> > Signed-off-by: Stefan Weil <weil@mail.berlios.de> > --- > block/blkverify.c | 5 +++-- > 1 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/block/blkverify.c b/block/blkverify.c > index 8083464..b39fb67 100644 > --- a/block/blkverify.c > +++ b/block/blkverify.c > @@ -53,7 +53,8 @@ static AIOPool blkverify_aio_pool = { > .cancel = blkverify_aio_cancel, > }; > > -static void blkverify_err(BlkverifyAIOCB *acb, const char *fmt, ...) > +static void GCC_FMT_ATTR(2, 3) blkverify_err(BlkverifyAIOCB *acb, > + const char *fmt, ...) > { > va_list ap; > > @@ -300,7 +301,7 @@ static void blkverify_verify_readv(BlkverifyAIOCB *acb) > ssize_t offset = blkverify_iovec_compare(acb->qiov, &acb->raw_qiov); > if (offset != -1) { > blkverify_err(acb, "contents mismatch in sector %ld", > - acb->sector_num + (offset / BDRV_SECTOR_SIZE)); > + (long)(acb->sector_num + (offset / BDRV_SECTOR_SIZE))); sector_num is int64_t, so the correct fix is to change '%ld' to '%" PRId64'.
Am 23.09.2010 20:53, schrieb Blue Swirl: > On Thu, Sep 23, 2010 at 6:37 PM, Stefan Weil<weil@mail.berlios.de> wrote: > >> Adding the gcc format attribute detects a format bug >> which is fixed here. >> >> Cc: Blue Swirl<blauwirbel@gmail.com> >> Cc: Kevin Wolf<kwolf@redhat.com> >> Signed-off-by: Stefan Weil<weil@mail.berlios.de> >> --- >> block/blkverify.c | 5 +++-- >> 1 files changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/block/blkverify.c b/block/blkverify.c >> index 8083464..b39fb67 100644 >> --- a/block/blkverify.c >> +++ b/block/blkverify.c >> @@ -53,7 +53,8 @@ static AIOPool blkverify_aio_pool = { >> .cancel = blkverify_aio_cancel, >> }; >> >> -static void blkverify_err(BlkverifyAIOCB *acb, const char *fmt, ...) >> +static void GCC_FMT_ATTR(2, 3) blkverify_err(BlkverifyAIOCB *acb, >> + const char *fmt, ...) >> { >> va_list ap; >> >> @@ -300,7 +301,7 @@ static void blkverify_verify_readv(BlkverifyAIOCB *acb) >> ssize_t offset = blkverify_iovec_compare(acb->qiov,&acb->raw_qiov); >> if (offset != -1) { >> blkverify_err(acb, "contents mismatch in sector %ld", >> - acb->sector_num + (offset / BDRV_SECTOR_SIZE)); >> + (long)(acb->sector_num + (offset / BDRV_SECTOR_SIZE))); >> > sector_num is int64_t, so the correct fix is to change '%ld' to '%" PRId64'. > > I noticed that, too. But offset is ssize_t. Can you always be sure that (int64_t + ssize_t) results in a int64_t? I don't think it's so easy.
Am 23.09.2010 21:03, schrieb Stefan Weil: > Am 23.09.2010 20:53, schrieb Blue Swirl: >> On Thu, Sep 23, 2010 at 6:37 PM, Stefan Weil<weil@mail.berlios.de> >> wrote: >>> Adding the gcc format attribute detects a format bug >>> which is fixed here. >>> >>> Cc: Blue Swirl<blauwirbel@gmail.com> >>> Cc: Kevin Wolf<kwolf@redhat.com> >>> Signed-off-by: Stefan Weil<weil@mail.berlios.de> >>> --- >>> block/blkverify.c | 5 +++-- >>> 1 files changed, 3 insertions(+), 2 deletions(-) >>> >>> diff --git a/block/blkverify.c b/block/blkverify.c >>> index 8083464..b39fb67 100644 >>> --- a/block/blkverify.c >>> +++ b/block/blkverify.c >>> @@ -53,7 +53,8 @@ static AIOPool blkverify_aio_pool = { >>> .cancel = blkverify_aio_cancel, >>> }; >>> >>> -static void blkverify_err(BlkverifyAIOCB *acb, const char *fmt, ...) >>> +static void GCC_FMT_ATTR(2, 3) blkverify_err(BlkverifyAIOCB *acb, >>> + const char *fmt, ...) >>> { >>> va_list ap; >>> >>> @@ -300,7 +301,7 @@ static void >>> blkverify_verify_readv(BlkverifyAIOCB *acb) >>> ssize_t offset = >>> blkverify_iovec_compare(acb->qiov,&acb->raw_qiov); >>> if (offset != -1) { >>> blkverify_err(acb, "contents mismatch in sector %ld", >>> - acb->sector_num + (offset / BDRV_SECTOR_SIZE)); >>> + (long)(acb->sector_num + (offset / >>> BDRV_SECTOR_SIZE))); >> sector_num is int64_t, so the correct fix is to change '%ld' to '%" >> PRId64'. >> > > I noticed that, too. But offset is ssize_t. > Can you always be sure that (int64_t + ssize_t) results in a int64_t? > I don't think it's so easy. I think you are correct, the format should use PRId64. The type cast is still necessary, but should cast to int64_t. (needed when int64_t == long and ssize_t == long long). If you agree, I'll send a new patch. Stefan
On Thu, Sep 23, 2010 at 7:11 PM, Stefan Weil <weil@mail.berlios.de> wrote: > Am 23.09.2010 21:03, schrieb Stefan Weil: >> >> Am 23.09.2010 20:53, schrieb Blue Swirl: >>> >>> On Thu, Sep 23, 2010 at 6:37 PM, Stefan Weil<weil@mail.berlios.de> >>> wrote: >>>> >>>> Adding the gcc format attribute detects a format bug >>>> which is fixed here. >>>> >>>> Cc: Blue Swirl<blauwirbel@gmail.com> >>>> Cc: Kevin Wolf<kwolf@redhat.com> >>>> Signed-off-by: Stefan Weil<weil@mail.berlios.de> >>>> --- >>>> block/blkverify.c | 5 +++-- >>>> 1 files changed, 3 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/block/blkverify.c b/block/blkverify.c >>>> index 8083464..b39fb67 100644 >>>> --- a/block/blkverify.c >>>> +++ b/block/blkverify.c >>>> @@ -53,7 +53,8 @@ static AIOPool blkverify_aio_pool = { >>>> .cancel = blkverify_aio_cancel, >>>> }; >>>> >>>> -static void blkverify_err(BlkverifyAIOCB *acb, const char *fmt, ...) >>>> +static void GCC_FMT_ATTR(2, 3) blkverify_err(BlkverifyAIOCB *acb, >>>> + const char *fmt, ...) >>>> { >>>> va_list ap; >>>> >>>> @@ -300,7 +301,7 @@ static void blkverify_verify_readv(BlkverifyAIOCB >>>> *acb) >>>> ssize_t offset = blkverify_iovec_compare(acb->qiov,&acb->raw_qiov); >>>> if (offset != -1) { >>>> blkverify_err(acb, "contents mismatch in sector %ld", >>>> - acb->sector_num + (offset / BDRV_SECTOR_SIZE)); >>>> + (long)(acb->sector_num + (offset / >>>> BDRV_SECTOR_SIZE))); >>> >>> sector_num is int64_t, so the correct fix is to change '%ld' to '%" >>> PRId64'. >>> >> >> I noticed that, too. But offset is ssize_t. >> Can you always be sure that (int64_t + ssize_t) results in a int64_t? >> I don't think it's so easy. > > I think you are correct, the format should use PRId64. > The type cast is still necessary, but should cast to int64_t. > (needed when int64_t == long and ssize_t == long long). > > If you agree, I'll send a new patch. It's also possible to cast offset to int64_t. Or perhaps even the type of the return value of blkverify_iovec_compare should be changed to int64_t.
Am 23.09.2010 22:24, schrieb Blue Swirl: > On Thu, Sep 23, 2010 at 7:11 PM, Stefan Weil <weil@mail.berlios.de> wrote: >> Am 23.09.2010 21:03, schrieb Stefan Weil: >>> >>> Am 23.09.2010 20:53, schrieb Blue Swirl: >>>> >>>> On Thu, Sep 23, 2010 at 6:37 PM, Stefan Weil<weil@mail.berlios.de> >>>> wrote: >>>>> >>>>> Adding the gcc format attribute detects a format bug >>>>> which is fixed here. >>>>> >>>>> Cc: Blue Swirl<blauwirbel@gmail.com> >>>>> Cc: Kevin Wolf<kwolf@redhat.com> >>>>> Signed-off-by: Stefan Weil<weil@mail.berlios.de> >>>>> --- >>>>> block/blkverify.c | 5 +++-- >>>>> 1 files changed, 3 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/block/blkverify.c b/block/blkverify.c >>>>> index 8083464..b39fb67 100644 >>>>> --- a/block/blkverify.c >>>>> +++ b/block/blkverify.c >>>>> @@ -53,7 +53,8 @@ static AIOPool blkverify_aio_pool = { >>>>> .cancel = blkverify_aio_cancel, >>>>> }; >>>>> >>>>> -static void blkverify_err(BlkverifyAIOCB *acb, const char *fmt, ...) >>>>> +static void GCC_FMT_ATTR(2, 3) blkverify_err(BlkverifyAIOCB *acb, >>>>> + const char *fmt, ...) >>>>> { >>>>> va_list ap; >>>>> >>>>> @@ -300,7 +301,7 @@ static void blkverify_verify_readv(BlkverifyAIOCB >>>>> *acb) >>>>> ssize_t offset = >>>>> blkverify_iovec_compare(acb->qiov,&acb->raw_qiov); >>>>> if (offset != -1) { >>>>> blkverify_err(acb, "contents mismatch in sector %ld", >>>>> - acb->sector_num + (offset / BDRV_SECTOR_SIZE)); >>>>> + (long)(acb->sector_num + (offset / >>>>> BDRV_SECTOR_SIZE))); >>>> >>>> sector_num is int64_t, so the correct fix is to change '%ld' to '%" >>>> PRId64'. >>>> >>> >>> I noticed that, too. But offset is ssize_t. >>> Can you always be sure that (int64_t + ssize_t) results in a int64_t? >>> I don't think it's so easy. >> >> I think you are correct, the format should use PRId64. >> The type cast is still necessary, but should cast to int64_t. >> (needed when int64_t == long and ssize_t == long long). >> >> If you agree, I'll send a new patch. > > It's also possible to cast offset to int64_t. Or perhaps even the type > of the return value of blkverify_iovec_compare should be changed to > int64_t. Unless BDRV_SECTOR_SIZE is changed, too, this would still need a type cast. So we have two possible solutions: (1) Use %lld (should work because BDRV_SECTOR_SIZE is unsigned long long). (2) Use PRId64. This needs changes for BDRV_SECTOR_SIZE and blkverify_iovec_compare. Any preferences? I tend to (2), but that change is less local.
On Thu, Sep 23, 2010 at 9:23 PM, Stefan Weil <weil@mail.berlios.de> wrote: > Am 23.09.2010 22:24, schrieb Blue Swirl: >> >> On Thu, Sep 23, 2010 at 7:11 PM, Stefan Weil <weil@mail.berlios.de> wrote: >>> >>> Am 23.09.2010 21:03, schrieb Stefan Weil: >>>> >>>> Am 23.09.2010 20:53, schrieb Blue Swirl: >>>>> >>>>> On Thu, Sep 23, 2010 at 6:37 PM, Stefan Weil<weil@mail.berlios.de> >>>>> wrote: >>>>>> >>>>>> Adding the gcc format attribute detects a format bug >>>>>> which is fixed here. >>>>>> >>>>>> Cc: Blue Swirl<blauwirbel@gmail.com> >>>>>> Cc: Kevin Wolf<kwolf@redhat.com> >>>>>> Signed-off-by: Stefan Weil<weil@mail.berlios.de> >>>>>> --- >>>>>> block/blkverify.c | 5 +++-- >>>>>> 1 files changed, 3 insertions(+), 2 deletions(-) >>>>>> >>>>>> diff --git a/block/blkverify.c b/block/blkverify.c >>>>>> index 8083464..b39fb67 100644 >>>>>> --- a/block/blkverify.c >>>>>> +++ b/block/blkverify.c >>>>>> @@ -53,7 +53,8 @@ static AIOPool blkverify_aio_pool = { >>>>>> .cancel = blkverify_aio_cancel, >>>>>> }; >>>>>> >>>>>> -static void blkverify_err(BlkverifyAIOCB *acb, const char *fmt, ...) >>>>>> +static void GCC_FMT_ATTR(2, 3) blkverify_err(BlkverifyAIOCB *acb, >>>>>> + const char *fmt, ...) >>>>>> { >>>>>> va_list ap; >>>>>> >>>>>> @@ -300,7 +301,7 @@ static void blkverify_verify_readv(BlkverifyAIOCB >>>>>> *acb) >>>>>> ssize_t offset = blkverify_iovec_compare(acb->qiov,&acb->raw_qiov); >>>>>> if (offset != -1) { >>>>>> blkverify_err(acb, "contents mismatch in sector %ld", >>>>>> - acb->sector_num + (offset / BDRV_SECTOR_SIZE)); >>>>>> + (long)(acb->sector_num + (offset / >>>>>> BDRV_SECTOR_SIZE))); >>>>> >>>>> sector_num is int64_t, so the correct fix is to change '%ld' to '%" >>>>> PRId64'. >>>>> >>>> >>>> I noticed that, too. But offset is ssize_t. >>>> Can you always be sure that (int64_t + ssize_t) results in a int64_t? >>>> I don't think it's so easy. >>> >>> I think you are correct, the format should use PRId64. >>> The type cast is still necessary, but should cast to int64_t. >>> (needed when int64_t == long and ssize_t == long long). >>> >>> If you agree, I'll send a new patch. >> >> It's also possible to cast offset to int64_t. Or perhaps even the type >> of the return value of blkverify_iovec_compare should be changed to >> int64_t. > > Unless BDRV_SECTOR_SIZE is changed, too, this would > still need a type cast. So we have two possible solutions: > > (1) Use %lld (should work because BDRV_SECTOR_SIZE is unsigned long long). > (2) Use PRId64. This needs changes for BDRV_SECTOR_SIZE and > blkverify_iovec_compare. Or (3) Use PRId64, change blkverify_iovec_compare, leave BDRV_SECTOR_SIZE unchanged but add a cast to int64_t here. Grepping for BDRV_SECTOR_SIZE shows that it is used in several places in size_t or off_t expressions, so long long is as good as any other large type. I think Kevin should decide.
Am 25.09.2010 10:01, schrieb Blue Swirl: > On Thu, Sep 23, 2010 at 9:23 PM, Stefan Weil <weil@mail.berlios.de> wrote: >> Am 23.09.2010 22:24, schrieb Blue Swirl: >>> >>> On Thu, Sep 23, 2010 at 7:11 PM, Stefan Weil <weil@mail.berlios.de> >>> wrote: >>>> >>>> Am 23.09.2010 21:03, schrieb Stefan Weil: >>>>> >>>>> Am 23.09.2010 20:53, schrieb Blue Swirl: >>>>>> >>>>>> On Thu, Sep 23, 2010 at 6:37 PM, Stefan Weil<weil@mail.berlios.de> >>>>>> wrote: >>>>>>> >>>>>>> Adding the gcc format attribute detects a format bug >>>>>>> which is fixed here. >>>>>>> >>>>>>> Cc: Blue Swirl<blauwirbel@gmail.com> >>>>>>> Cc: Kevin Wolf<kwolf@redhat.com> >>>>>>> Signed-off-by: Stefan Weil<weil@mail.berlios.de> >>>>>>> --- >>>>>>> block/blkverify.c | 5 +++-- >>>>>>> 1 files changed, 3 insertions(+), 2 deletions(-) >>>>>>> >>>>>>> diff --git a/block/blkverify.c b/block/blkverify.c >>>>>>> index 8083464..b39fb67 100644 >>>>>>> --- a/block/blkverify.c >>>>>>> +++ b/block/blkverify.c >>>>>>> @@ -53,7 +53,8 @@ static AIOPool blkverify_aio_pool = { >>>>>>> .cancel = blkverify_aio_cancel, >>>>>>> }; >>>>>>> >>>>>>> -static void blkverify_err(BlkverifyAIOCB *acb, const char *fmt, >>>>>>> ...) >>>>>>> +static void GCC_FMT_ATTR(2, 3) blkverify_err(BlkverifyAIOCB *acb, >>>>>>> + const char *fmt, ...) >>>>>>> { >>>>>>> va_list ap; >>>>>>> >>>>>>> @@ -300,7 +301,7 @@ static void >>>>>>> blkverify_verify_readv(BlkverifyAIOCB >>>>>>> *acb) >>>>>>> ssize_t offset = >>>>>>> blkverify_iovec_compare(acb->qiov,&acb->raw_qiov); >>>>>>> if (offset != -1) { >>>>>>> blkverify_err(acb, "contents mismatch in sector %ld", >>>>>>> - acb->sector_num + (offset / >>>>>>> BDRV_SECTOR_SIZE)); >>>>>>> + (long)(acb->sector_num + (offset / >>>>>>> BDRV_SECTOR_SIZE))); >>>>>> >>>>>> sector_num is int64_t, so the correct fix is to change '%ld' to '%" >>>>>> PRId64'. >>>>>> >>>>> >>>>> I noticed that, too. But offset is ssize_t. >>>>> Can you always be sure that (int64_t + ssize_t) results in a int64_t? >>>>> I don't think it's so easy. >>>> >>>> I think you are correct, the format should use PRId64. >>>> The type cast is still necessary, but should cast to int64_t. >>>> (needed when int64_t == long and ssize_t == long long). >>>> >>>> If you agree, I'll send a new patch. >>> >>> It's also possible to cast offset to int64_t. Or perhaps even the type >>> of the return value of blkverify_iovec_compare should be changed to >>> int64_t. >> >> Unless BDRV_SECTOR_SIZE is changed, too, this would >> still need a type cast. So we have two possible solutions: >> >> (1) Use %lld (should work because BDRV_SECTOR_SIZE is unsigned long >> long). >> (2) Use PRId64. This needs changes for BDRV_SECTOR_SIZE and >> blkverify_iovec_compare. > > Or > (3) Use PRId64, change blkverify_iovec_compare, leave BDRV_SECTOR_SIZE > unchanged but add a cast to int64_t here. > > Grepping for BDRV_SECTOR_SIZE shows that it is used in several places > in size_t or off_t expressions, so long long is as good as any other > large type. > > I think Kevin should decide. > BDRV_SECTOR_MASK is the critical value. This should work: #define BDRV_SECTOR_SIZE 512 #define BDRV_SECTOR_MASK (int64_t)(~(511ULL))
Am 25.09.2010 10:01, schrieb Blue Swirl: > On Thu, Sep 23, 2010 at 9:23 PM, Stefan Weil<weil@mail.berlios.de> wrote: > >> Am 23.09.2010 22:24, schrieb Blue Swirl: >> >>> On Thu, Sep 23, 2010 at 7:11 PM, Stefan Weil<weil@mail.berlios.de> wrote: >>> >>>> Am 23.09.2010 21:03, schrieb Stefan Weil: >>>> >>>>> Am 23.09.2010 20:53, schrieb Blue Swirl: >>>>> >>>>>> On Thu, Sep 23, 2010 at 6:37 PM, Stefan Weil<weil@mail.berlios.de> >>>>>> wrote: >>>>>> >>>>>>> Adding the gcc format attribute detects a format bug >>>>>>> which is fixed here. >>>>>>> >>>>>>> Cc: Blue Swirl<blauwirbel@gmail.com> >>>>>>> Cc: Kevin Wolf<kwolf@redhat.com> >>>>>>> Signed-off-by: Stefan Weil<weil@mail.berlios.de> >>>>>>> --- >>>>>>> block/blkverify.c | 5 +++-- >>>>>>> 1 files changed, 3 insertions(+), 2 deletions(-) >>>>>>> >>>>>>> diff --git a/block/blkverify.c b/block/blkverify.c >>>>>>> index 8083464..b39fb67 100644 >>>>>>> --- a/block/blkverify.c >>>>>>> +++ b/block/blkverify.c >>>>>>> @@ -53,7 +53,8 @@ static AIOPool blkverify_aio_pool = { >>>>>>> .cancel = blkverify_aio_cancel, >>>>>>> }; >>>>>>> >>>>>>> -static void blkverify_err(BlkverifyAIOCB *acb, const char *fmt, ...) >>>>>>> +static void GCC_FMT_ATTR(2, 3) blkverify_err(BlkverifyAIOCB *acb, >>>>>>> + const char *fmt, ...) >>>>>>> { >>>>>>> va_list ap; >>>>>>> >>>>>>> @@ -300,7 +301,7 @@ static void blkverify_verify_readv(BlkverifyAIOCB >>>>>>> *acb) >>>>>>> ssize_t offset = blkverify_iovec_compare(acb->qiov,&acb->raw_qiov); >>>>>>> if (offset != -1) { >>>>>>> blkverify_err(acb, "contents mismatch in sector %ld", >>>>>>> - acb->sector_num + (offset / BDRV_SECTOR_SIZE)); >>>>>>> + (long)(acb->sector_num + (offset / >>>>>>> BDRV_SECTOR_SIZE))); >>>>>>> >>>>>> sector_num is int64_t, so the correct fix is to change '%ld' to '%" >>>>>> PRId64'. >>>>>> >>>>>> >>>>> I noticed that, too. But offset is ssize_t. >>>>> Can you always be sure that (int64_t + ssize_t) results in a int64_t? >>>>> I don't think it's so easy. >>>>> >>>> I think you are correct, the format should use PRId64. >>>> The type cast is still necessary, but should cast to int64_t. >>>> (needed when int64_t == long and ssize_t == long long). >>>> >>>> If you agree, I'll send a new patch. >>>> >>> It's also possible to cast offset to int64_t. Or perhaps even the type >>> of the return value of blkverify_iovec_compare should be changed to >>> int64_t. >>> >> Unless BDRV_SECTOR_SIZE is changed, too, this would >> still need a type cast. So we have two possible solutions: >> >> (1) Use %lld (should work because BDRV_SECTOR_SIZE is unsigned long long). >> (2) Use PRId64. This needs changes for BDRV_SECTOR_SIZE and >> blkverify_iovec_compare. >> > Or > (3) Use PRId64, change blkverify_iovec_compare, leave BDRV_SECTOR_SIZE > unchanged but add a cast to int64_t here. > > Grepping for BDRV_SECTOR_SIZE shows that it is used in several places > in size_t or off_t expressions, so long long is as good as any other > large type. > > I think Kevin should decide. > Kevin, how should this get fixed? I suggest committing my last patch version sent on 2010-09-24 ("[PATCH] block: Use GCC_FMT_ATTR and fix a format error"), but I don't mind if you have a different solution. Regards, Stefan
Am 13.10.2010 21:06, schrieb Stefan Weil: > Am 25.09.2010 10:01, schrieb Blue Swirl: >> On Thu, Sep 23, 2010 at 9:23 PM, Stefan Weil<weil@mail.berlios.de> wrote: >> >>> Am 23.09.2010 22:24, schrieb Blue Swirl: >>> >>>> On Thu, Sep 23, 2010 at 7:11 PM, Stefan Weil<weil@mail.berlios.de> wrote: >>>> >>>>> Am 23.09.2010 21:03, schrieb Stefan Weil: >>>>> >>>>>> Am 23.09.2010 20:53, schrieb Blue Swirl: >>>>>> >>>>>>> On Thu, Sep 23, 2010 at 6:37 PM, Stefan Weil<weil@mail.berlios.de> >>>>>>> wrote: >>>>>>> >>>>>>>> Adding the gcc format attribute detects a format bug >>>>>>>> which is fixed here. >>>>>>>> >>>>>>>> Cc: Blue Swirl<blauwirbel@gmail.com> >>>>>>>> Cc: Kevin Wolf<kwolf@redhat.com> >>>>>>>> Signed-off-by: Stefan Weil<weil@mail.berlios.de> >>>>>>>> --- >>>>>>>> block/blkverify.c | 5 +++-- >>>>>>>> 1 files changed, 3 insertions(+), 2 deletions(-) >>>>>>>> >>>>>>>> diff --git a/block/blkverify.c b/block/blkverify.c >>>>>>>> index 8083464..b39fb67 100644 >>>>>>>> --- a/block/blkverify.c >>>>>>>> +++ b/block/blkverify.c >>>>>>>> @@ -53,7 +53,8 @@ static AIOPool blkverify_aio_pool = { >>>>>>>> .cancel = blkverify_aio_cancel, >>>>>>>> }; >>>>>>>> >>>>>>>> -static void blkverify_err(BlkverifyAIOCB *acb, const char *fmt, ...) >>>>>>>> +static void GCC_FMT_ATTR(2, 3) blkverify_err(BlkverifyAIOCB *acb, >>>>>>>> + const char *fmt, ...) >>>>>>>> { >>>>>>>> va_list ap; >>>>>>>> >>>>>>>> @@ -300,7 +301,7 @@ static void blkverify_verify_readv(BlkverifyAIOCB >>>>>>>> *acb) >>>>>>>> ssize_t offset = blkverify_iovec_compare(acb->qiov,&acb->raw_qiov); >>>>>>>> if (offset != -1) { >>>>>>>> blkverify_err(acb, "contents mismatch in sector %ld", >>>>>>>> - acb->sector_num + (offset / BDRV_SECTOR_SIZE)); >>>>>>>> + (long)(acb->sector_num + (offset / >>>>>>>> BDRV_SECTOR_SIZE))); >>>>>>>> >>>>>>> sector_num is int64_t, so the correct fix is to change '%ld' to '%" >>>>>>> PRId64'. >>>>>>> >>>>>>> >>>>>> I noticed that, too. But offset is ssize_t. >>>>>> Can you always be sure that (int64_t + ssize_t) results in a int64_t? >>>>>> I don't think it's so easy. >>>>>> >>>>> I think you are correct, the format should use PRId64. >>>>> The type cast is still necessary, but should cast to int64_t. >>>>> (needed when int64_t == long and ssize_t == long long). >>>>> >>>>> If you agree, I'll send a new patch. >>>>> >>>> It's also possible to cast offset to int64_t. Or perhaps even the type >>>> of the return value of blkverify_iovec_compare should be changed to >>>> int64_t. >>>> >>> Unless BDRV_SECTOR_SIZE is changed, too, this would >>> still need a type cast. So we have two possible solutions: >>> >>> (1) Use %lld (should work because BDRV_SECTOR_SIZE is unsigned long long). >>> (2) Use PRId64. This needs changes for BDRV_SECTOR_SIZE and >>> blkverify_iovec_compare. >>> >> Or >> (3) Use PRId64, change blkverify_iovec_compare, leave BDRV_SECTOR_SIZE >> unchanged but add a cast to int64_t here. >> >> Grepping for BDRV_SECTOR_SIZE shows that it is used in several places >> in size_t or off_t expressions, so long long is as good as any other >> large type. >> >> I think Kevin should decide. >> > > Kevin, how should this get fixed? > > I suggest committing my last patch version sent on 2010-09-24 > ("[PATCH] block: Use GCC_FMT_ATTR and fix a format error"), > but I don't mind if you have a different solution. I think I would have used PRId64 and cast the whole thing to int64_t, but I don't really care as long as it works. I haven't heard any complaints about your patch being broken, and nobody else has sent a different patch, so I'll apply it. Kevin
diff --git a/block/blkverify.c b/block/blkverify.c index 8083464..b39fb67 100644 --- a/block/blkverify.c +++ b/block/blkverify.c @@ -53,7 +53,8 @@ static AIOPool blkverify_aio_pool = { .cancel = blkverify_aio_cancel, }; -static void blkverify_err(BlkverifyAIOCB *acb, const char *fmt, ...) +static void GCC_FMT_ATTR(2, 3) blkverify_err(BlkverifyAIOCB *acb, + const char *fmt, ...) { va_list ap; @@ -300,7 +301,7 @@ static void blkverify_verify_readv(BlkverifyAIOCB *acb) ssize_t offset = blkverify_iovec_compare(acb->qiov, &acb->raw_qiov); if (offset != -1) { blkverify_err(acb, "contents mismatch in sector %ld", - acb->sector_num + (offset / BDRV_SECTOR_SIZE)); + (long)(acb->sector_num + (offset / BDRV_SECTOR_SIZE))); } }
Adding the gcc format attribute detects a format bug which is fixed here. Cc: Blue Swirl <blauwirbel@gmail.com> Cc: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Stefan Weil <weil@mail.berlios.de> --- block/blkverify.c | 5 +++-- 1 files changed, 3 insertions(+), 2 deletions(-)