* [Qemu-devel] [PATCH] block: Use GCC_FMT_ATTR and fix a format error
@ 2010-09-23 18:37 Stefan Weil
2010-09-23 18:53 ` [Qemu-devel] " Blue Swirl
0 siblings, 1 reply; 11+ messages in thread
From: Stefan Weil @ 2010-09-23 18:37 UTC (permalink / raw)
To: QEMU Developers; +Cc: Blue Swirl, Kevin Wolf
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)));
}
}
--
1.7.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [Qemu-devel] Re: [PATCH] block: Use GCC_FMT_ATTR and fix a format error
2010-09-23 18:37 [Qemu-devel] [PATCH] block: Use GCC_FMT_ATTR and fix a format error Stefan Weil
@ 2010-09-23 18:53 ` Blue Swirl
2010-09-23 19:03 ` Stefan Weil
0 siblings, 1 reply; 11+ messages in thread
From: Blue Swirl @ 2010-09-23 18:53 UTC (permalink / raw)
To: Stefan Weil; +Cc: Kevin Wolf, QEMU Developers
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'.
^ permalink raw reply [flat|nested] 11+ messages in thread
* [Qemu-devel] Re: [PATCH] block: Use GCC_FMT_ATTR and fix a format error
2010-09-23 18:53 ` [Qemu-devel] " Blue Swirl
@ 2010-09-23 19:03 ` Stefan Weil
2010-09-23 19:11 ` Stefan Weil
0 siblings, 1 reply; 11+ messages in thread
From: Stefan Weil @ 2010-09-23 19:03 UTC (permalink / raw)
To: Blue Swirl; +Cc: Kevin Wolf, QEMU Developers
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* [Qemu-devel] Re: [PATCH] block: Use GCC_FMT_ATTR and fix a format error
2010-09-23 19:03 ` Stefan Weil
@ 2010-09-23 19:11 ` Stefan Weil
2010-09-23 20:24 ` Blue Swirl
0 siblings, 1 reply; 11+ messages in thread
From: Stefan Weil @ 2010-09-23 19:11 UTC (permalink / raw)
To: Blue Swirl; +Cc: Kevin Wolf, QEMU Developers
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [Qemu-devel] Re: [PATCH] block: Use GCC_FMT_ATTR and fix a format error
2010-09-23 19:11 ` Stefan Weil
@ 2010-09-23 20:24 ` Blue Swirl
2010-09-23 21:23 ` Stefan Weil
0 siblings, 1 reply; 11+ messages in thread
From: Blue Swirl @ 2010-09-23 20:24 UTC (permalink / raw)
To: Stefan Weil; +Cc: Kevin Wolf, QEMU Developers
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* [Qemu-devel] Re: [PATCH] block: Use GCC_FMT_ATTR and fix a format error
2010-09-23 20:24 ` Blue Swirl
@ 2010-09-23 21:23 ` Stefan Weil
2010-09-24 19:02 ` [Qemu-devel] " Stefan Weil
2010-09-25 8:01 ` [Qemu-devel] " Blue Swirl
0 siblings, 2 replies; 11+ messages in thread
From: Stefan Weil @ 2010-09-23 21:23 UTC (permalink / raw)
To: Blue Swirl; +Cc: Kevin Wolf, QEMU Developers
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* [Qemu-devel] [PATCH] block: Use GCC_FMT_ATTR and fix a format error
2010-09-23 21:23 ` Stefan Weil
@ 2010-09-24 19:02 ` Stefan Weil
2010-09-25 8:01 ` [Qemu-devel] " Blue Swirl
1 sibling, 0 replies; 11+ messages in thread
From: Stefan Weil @ 2010-09-24 19:02 UTC (permalink / raw)
To: QEMU Developers; +Cc: Blue Swirl, Kevin Wolf
Adding the gcc format attribute detects a format bug
which is fixed here.
v2:
Don't use type cast. BDRV_SECTOR_SIZE is unsigned long long,
so %lld should be the correct format specifier.
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..b2a12fe 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;
@@ -299,7 +300,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",
+ blkverify_err(acb, "contents mismatch in sector %lld",
acb->sector_num + (offset / BDRV_SECTOR_SIZE));
}
}
--
1.7.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [Qemu-devel] Re: [PATCH] block: Use GCC_FMT_ATTR and fix a format error
2010-09-23 21:23 ` Stefan Weil
2010-09-24 19:02 ` [Qemu-devel] " Stefan Weil
@ 2010-09-25 8:01 ` Blue Swirl
2010-09-25 11:59 ` Stefan Weil
2010-10-13 19:06 ` Stefan Weil
1 sibling, 2 replies; 11+ messages in thread
From: Blue Swirl @ 2010-09-25 8:01 UTC (permalink / raw)
To: Stefan Weil; +Cc: Kevin Wolf, QEMU Developers
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* [Qemu-devel] Re: [PATCH] block: Use GCC_FMT_ATTR and fix a format error
2010-09-25 8:01 ` [Qemu-devel] " Blue Swirl
@ 2010-09-25 11:59 ` Stefan Weil
2010-10-13 19:06 ` Stefan Weil
1 sibling, 0 replies; 11+ messages in thread
From: Stefan Weil @ 2010-09-25 11:59 UTC (permalink / raw)
To: Blue Swirl; +Cc: Kevin Wolf, QEMU Developers
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))
^ permalink raw reply [flat|nested] 11+ messages in thread
* [Qemu-devel] Re: [PATCH] block: Use GCC_FMT_ATTR and fix a format error
2010-09-25 8:01 ` [Qemu-devel] " Blue Swirl
2010-09-25 11:59 ` Stefan Weil
@ 2010-10-13 19:06 ` Stefan Weil
2010-10-14 8:11 ` Kevin Wolf
1 sibling, 1 reply; 11+ messages in thread
From: Stefan Weil @ 2010-10-13 19:06 UTC (permalink / raw)
To: Kevin Wolf; +Cc: Blue Swirl, QEMU Developers
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [Qemu-devel] Re: [PATCH] block: Use GCC_FMT_ATTR and fix a format error
2010-10-13 19:06 ` Stefan Weil
@ 2010-10-14 8:11 ` Kevin Wolf
0 siblings, 0 replies; 11+ messages in thread
From: Kevin Wolf @ 2010-10-14 8:11 UTC (permalink / raw)
To: Stefan Weil; +Cc: Blue Swirl, QEMU Developers
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
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2010-10-14 8:11 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-09-23 18:37 [Qemu-devel] [PATCH] block: Use GCC_FMT_ATTR and fix a format error Stefan Weil
2010-09-23 18:53 ` [Qemu-devel] " Blue Swirl
2010-09-23 19:03 ` Stefan Weil
2010-09-23 19:11 ` Stefan Weil
2010-09-23 20:24 ` Blue Swirl
2010-09-23 21:23 ` Stefan Weil
2010-09-24 19:02 ` [Qemu-devel] " Stefan Weil
2010-09-25 8:01 ` [Qemu-devel] " Blue Swirl
2010-09-25 11:59 ` Stefan Weil
2010-10-13 19:06 ` Stefan Weil
2010-10-14 8:11 ` Kevin Wolf
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).