qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v4] avoid compilation warning/errors on up to da


From: Jean-Christophe Dubois
Subject: Re: [Qemu-devel] [PATCH v4] avoid compilation warning/errors on up to date
Date: Thu, 18 Jun 2009 00:41:31 +0200
User-agent: KMail/1.11.2 (Linux/2.6.28-13-generic; KDE/4.2.2; x86_64; ; )

Le mercredi 17 juin 2009 23:54:56 Anthony Liguori, vous avez écrit :
> Jean-Christophe Dubois wrote:
> > Some system calls are now requiring to have their return value checked.
> >
> > Without this a warning is emitted and in the case a qemu an error is
> > triggered as qemu is considering warnings as errors.
> >
> > For example:
> >
> > block/cow.c: In function ‘cow_create’:
> > block/cow.c:251: error: ignoring return value of ‘write’, declared with
> > attribute warn_unused_result
> > block/cow.c:253: error: ignoring return value of ‘ftruncate’, declared
> > with attribute warn_unused_result
> >
> > This is an attempt at removing all these warnings to allow a clean
> > compilation with up to date compilers/distributions.
> >
> > The second version fixes an error detected by Stuart Brady as well
> > as some coding style issues. Note however that some of the
> > modified files don't follow the qemu coding style (using tabs
> > instead of spaces).
> >
> > The Third version add one ftruncate() system call error handling that
> > was missing from V2 (in block/vvfat.c).
> >
> > The Fourth version is correctly handling EINTR error on read/write
> > system calls. read/write calls are replaced by qemu_read/qemu_write
> > functions that are handling EINTR and incomplete read/write under
> > the cover.
> >
> > Signed-off-by: Jean-Christophe DUBOIS <address@hidden>
>
> This really makes me nervous to be honest.  It's too much for one patch.

It is what is required to fix the compilation failure.

> What I'd rather do, is commit Gerd's patch and also add
> -U_FORTIFY_SOURCE to the default CFLAGS.  I've confirmed this fixes the
> default build on Ubuntu.

It certainly does. Qemu did compile for years with less strict compilers.

> I'd rather take this set of clean ups as a set of smaller, well
> thought-out patches.  Once that's done, we can remove -U_FORTIFY_SOURCE.

Well, without -U_FORTIFY_SOURCE the all set of patch is required. Once you add 
-U_FORTIFY_SOURCE I am skeptical it will be removed any time soon. Actually I 
bet more code without error checking will be added and the problem will just 
build up.

> > ---
> >  block.c           |    3 +-
> >  block/bochs.c     |    3 +-
> >  block/cow.c       |   12 +++++++++-
> >  block/qcow.c      |   22 +++++++++++++++-----
> >  block/qcow2.c     |   37 ++++++++++++++++++++++++++++-------
> >  block/raw-posix.c |    9 +++++--
> >  block/vmdk.c      |   54
> > +++++++++++++++++++++++++++++++++++----------------- block/vvfat.c     | 
> >  24 ++++++++++++++++------
> >  linux-user/mmap.c |    7 ++++-
> >  linux-user/path.c |    6 ++++-
> >  osdep.c           |    5 +++-
> >  qemu-common.h     |   31 ++++++++++++++++++++++++++++++
> >  slirp/misc.c      |    4 ++-
> >  usb-linux.c       |    3 +-
> >  vl.c              |   14 +++++++++---
> >  15 files changed, 177 insertions(+), 57 deletions(-)
> >
> > diff --git a/block.c b/block.c
> > index aca5a6d..c78d66a 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -371,7 +371,8 @@ int bdrv_open2(BlockDriverState *bs, const char
> > *filename, int flags, snprintf(backing_filename,
> > sizeof(backing_filename), "%s", filename);
> >          else
> > -            realpath(filename, backing_filename);
> > +            if (!realpath(filename, backing_filename))
> > +                return -1;
>
> There should be cleanup work to do, no?
>
> >          bdrv_qcow2 = bdrv_find_format("qcow2");
> >          options = parse_option_parameters("",
> > bdrv_qcow2->create_options, NULL); diff --git a/block/bochs.c
> > b/block/bochs.c
> > index bac81c4..1fbce9e 100644
> > --- a/block/bochs.c
> > +++ b/block/bochs.c
> > @@ -199,7 +199,8 @@ static inline int seek_to_sector(BlockDriverState
> > *bs, int64_t sector_num) // read in bitmap for current extent
> >      lseek(s->fd, bitmap_offset + (extent_offset / 8), SEEK_SET);
> >
> > -    read(s->fd, &bitmap_entry, 1);
> > +    if (qemu_read(s->fd, &bitmap_entry, 1) != 1)
> > +        return -1; // not allocated
>
> Why check errors on read but not lseek?  

Well, my first goal was to fix the compilation failure in a useful way. lseek() 
doesn't trigger the warning and so doesn't make the compilation fail.

Now, I would agree that lseek return code should also be checked. But can you 
guess how many lseek() (and other system calls) without error check there is 
in the qemu source code.

> If we're going to add error
> checking to this function, we should make it robust, not just enough
> error checking to silence GCC.

Why do you think my functions are not robust enough? Just curious ... If I can 
fix them, I'll do it. My goal was not to just silence GCC (I would use -
U_FORTIFY_SOURCE for this) but to actually handle the error in a meaningful 
way.

> We should just silence GCC with
> -U_FORTIFY_SOURCE.

You can do this but once done I doubt anybody will step up to actually fix the 
multiple place in the code where system calls are used without error checking. 
As there will be no more warning/error the problem will be 
forgotten/invisible.

> The same is true for most of the remainder.  This is definitely a good
> task worth doing but I think we should do it more carefully.

Once again, when all warnings are suppressed I doubt anybody will volunteer 
for this boring task. And all future code added to the project without system 
call error check will get unnoticed because you silenced GCC.

But at the end it is your call.

> Regards,
>
> Anthony Liguori





reply via email to

[Prev in Thread] Current Thread [Next in Thread]