[Qemu-devel] Re: [PATCH] block: Use GCC_FMT_ATTR and fix a format error

[Kevin Wolf]

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<address@hidden>  wrote:
>>    
>>> Am 23.09.2010 22:24, schrieb Blue Swirl:
>>>      
>>>> On Thu, Sep 23, 2010 at 7:11 PM, Stefan Weil<address@hidden>  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<address@hidden>
>>>>>>>   wrote:
>>>>>>>              
>>>>>>>> Adding the gcc format attribute detects a format bug
>>>>>>>> which is fixed here.
>>>>>>>>
>>>>>>>> Cc: Blue Swirl<address@hidden>
>>>>>>>> Cc: Kevin Wolf<address@hidden>
>>>>>>>> Signed-off-by: Stefan Weil<address@hidden>
>>>>>>>> ---
>>>>>>>>   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