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

[Stefan Weil]

Am 25.09.2010 10:01, schrieb Blue Swirl:
> On Thu, Sep 23, 2010 at 9:23 PM, Stefan Weil<address@hidden>  wrote:
>    
> > Am 23.09.2010 22:24, schrieb Blue Swirl:
> >      
> > > On Thu, Sep 23, 2010 at 7:11 PM, Stefan Weil<address@hidden>  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<address@hidden>
> > > > > >   wrote:
> > > > > >              
> > > > > > > Adding the gcc format attribute detects a format bug
> > > > > > > which is fixed here.
> > > > > > >
> > > > > > > Cc: Blue Swirl<address@hidden>
> > > > > > > Cc: Kevin Wolf<address@hidden>
> > > > > > > Signed-off-by: Stefan Weil<address@hidden>
> > > > > > > ---
> > > > > > >   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