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

[Stefan Weil <weil@mail.berlios.de>]

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