Re: [Qemu-devel] [PATCH v3] avoid compilation warning/errors on up to date compilers/glibc

[jcd]

----- "Anthony Liguori" <address@hidden> a é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).
> >
> > Signed-off-by: Jean-Christophe DUBOIS <address@hidden>
> >
> > ---
> >  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      |   38 ++++++++++++++++++++++++++++----------
> >  block/vvfat.c     |   24 +++++++++++++++++-------
> >  linux-user/mmap.c |    7 +++++--
> >  linux-user/path.c |    6 +++++-
> >  osdep.c           |    5 ++++-
> >  slirp/misc.c      |    3 ++-
> >  usb-linux.c       |    3 +--
> >  vl.c              |   14 ++++++++++----
> >  14 files changed, 137 insertions(+), 49 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;
> >  
> >          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..0d614eb 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 (read(s->fd, &bitmap_entry, 1) != 1)
> > +        return -1; // not allocated
> >   
> 
> This is no more correct than before.  read() can return EINTR and that
> 
> should be handled appropriately.  Elsewhere, read() can return partial
> 
> results and we ought to handle that properly.

It handles all cases except EINTR. And in the actual code base EINTR and __all 
other__ error cases are not handled. So as of today EINTR is not handled 
propely (as well as all other error cases).
So this consider EINTR as an error when it could be handled in a nicer way. 
Previously no ERRORS were considered as an error. What is best?
 
> Using -D_FORTIFY_SOURCES=0 will eliminate these warnings.  If we're 
> going to fix these things, we should fix them properly.

It just hides the problem and fix nothing but the compilation. ERRORS will 
still be ignored/discarded without any warning. If the glibc designers are 
insisting on the fact that some system calls should have their return value 
checked there must be a reason. Is this really the best practice here (to just 
ignore errors)?
 
> Regards,
> 
> Anthony Liguori