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

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