From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=57689 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1P6Iu4-0003yN-HA for qemu-devel@nongnu.org; Thu, 14 Oct 2010 04:11:09 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1P6Iu2-0003AV-UC for qemu-devel@nongnu.org; Thu, 14 Oct 2010 04:11:08 -0400 Received: from mx1.redhat.com ([209.132.183.28]:6102) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1P6Iu2-0003AG-N4 for qemu-devel@nongnu.org; Thu, 14 Oct 2010 04:11:06 -0400 Message-ID: <4CB6BB37.3040900@redhat.com> Date: Thu, 14 Oct 2010 10:11:35 +0200 From: Kevin Wolf 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> <4CB6032E.9050802@mail.berlios.de> In-Reply-To: <4CB6032E.9050802@mail.berlios.de> Content-Type: text/plain; charset=UTF-8 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: 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 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. >> > > 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