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