From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=58279 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OzTPn-0007GH-Qh for qemu-devel@nongnu.org; Sat, 25 Sep 2010 07:59:40 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OzTPm-000278-HK for qemu-devel@nongnu.org; Sat, 25 Sep 2010 07:59:39 -0400 Received: from moutng.kundenserver.de ([212.227.126.171]:58795) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OzTPm-00026o-3R for qemu-devel@nongnu.org; Sat, 25 Sep 2010 07:59:38 -0400 Message-ID: <4C9DE425.4000702@mail.berlios.de> Date: Sat, 25 Sep 2010 13:59:33 +0200 From: Stefan Weil MIME-Version: 1.0 References: <1285267031-22966-1-git-send-email-weil@mail.berlios.de> <4C9BA48A.1090600@mail.berlios.de> <4C9BA64F.7030303@mail.berlios.de> <4C9BC53E.1010509@mail.berlios.de> In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: [Qemu-devel] Re: [PATCH] block: Use GCC_FMT_ATTR and fix a format error List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , 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 wrote: >> Am 23.09.2010 22:24, schrieb Blue Swirl: >>> >>> On Thu, Sep 23, 2010 at 7:11 PM, Stefan Weil >>> 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 >>>>>> wrote: >>>>>>> >>>>>>> Adding the gcc format attribute detects a format bug >>>>>>> which is fixed here. >>>>>>> >>>>>>> Cc: Blue Swirl >>>>>>> Cc: Kevin Wolf >>>>>>> Signed-off-by: Stefan Weil >>>>>>> --- >>>>>>> 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))