Patchwork block: Use GCC_FMT_ATTR and fix a format error

login
register
mail settings
Submitter Stefan Weil
Date Sept. 23, 2010, 6:37 p.m.
Message ID <1285267031-22966-1-git-send-email-weil@mail.berlios.de>
Download mbox | patch
Permalink /patch/65566/
State New
Headers show

Comments

Stefan Weil - Sept. 23, 2010, 6:37 p.m.
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(-)
Blue Swirl - Sept. 23, 2010, 6:53 p.m.
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'.
Stefan Weil - Sept. 23, 2010, 7:03 p.m.
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.
Stefan Weil - Sept. 23, 2010, 7:11 p.m.
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
Blue Swirl - Sept. 23, 2010, 8:24 p.m.
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.
Stefan Weil - Sept. 23, 2010, 9:23 p.m.
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.
Blue Swirl - Sept. 25, 2010, 8:01 a.m.
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.
Stefan Weil - Sept. 25, 2010, 11:59 a.m.
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))
Stefan Weil - Oct. 13, 2010, 7:06 p.m.
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
Kevin Wolf - Oct. 14, 2010, 8:11 a.m.
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

Patch

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)));
     }
 }