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

[malc]

On Thu, 18 Jun 2009, Jean-Christophe Dubois wrote:

> Le mercredi 17 juin 2009 23:51:12 malc, vous avez ЪЪcrit :
> > On Wed, 17 Jun 2009, 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.
> >
> > This patch is rather inconsistent w.r.t. lseek (perhaps some other calls
> > too).
> >
> > > 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..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);
> >
> > No check here.
> >
> > > -    read(s->fd, &bitmap_entry, 1);
> > > +    if (qemu_read(s->fd, &bitmap_entry, 1) != 1)
> > > +        return -1; // not allocated
> > >
> > >      if (!((bitmap_entry >> (extent_offset % 8)) & 1))
> > >      {
> > > diff --git a/block/cow.c b/block/cow.c
> > > index 84818f1..d8cd1df 100644
> > > --- a/block/cow.c
> > > +++ b/block/cow.c
> > > @@ -248,11 +248,19 @@ static int cow_create(const char *filename,
> > > QEMUOptionParameter *options) }
> > >      cow_header.sectorsize = cpu_to_be32(512);
> > >      cow_header.size = cpu_to_be64(image_sectors * 512);
> > > -    write(cow_fd, &cow_header, sizeof(cow_header));
> > > +    if (qemu_write(cow_fd, &cow_header, sizeof(cow_header)) !=
> > > sizeof(cow_header)) +        goto fail;
> > > +
> > >      /* resize to include at least all the bitmap */
> > > -    ftruncate(cow_fd, sizeof(cow_header) + ((image_sectors + 7) >> 3));
> > > +    if (ftruncate(cow_fd, sizeof(cow_header) + ((image_sectors + 7) >>
> > > 3))) +        goto fail;
> > > +
> > >      close(cow_fd);
> > >      return 0;
> > > +
> > > +fail:
> > > +    close(cow_fd);
> >
> > close can fail too, btw.
> >
> > > +    return -1;
> > >  }
> > >
> > >  static void cow_flush(BlockDriverState *bs)
> > > diff --git a/block/qcow.c b/block/qcow.c
> > > index 55a68a6..5e50db8 100644
> > > --- a/block/qcow.c
> > > +++ b/block/qcow.c
> > > @@ -801,17 +801,27 @@ static int qcow_create(const char *filename,
> > > QEMUOptionParameter *options) }
> > >
> > >      /* write all the data */
> > > -    write(fd, &header, sizeof(header));
> > > -    if (backing_file) {
> > > -        write(fd, backing_file, backing_filename_len);
> > > -    }
> > > -    lseek(fd, header_size, SEEK_SET);
> > > +    if (qemu_write(fd, &header, sizeof(header)) != sizeof(header))
> > > +        goto fail;
> > > +
> > > +    if (backing_file)
> > > +        if (qemu_write(fd, backing_file, backing_filename_len) !=
> > > backing_filename_len) +            goto fail;
> > > +
> > > +    if (lseek(fd, header_size, SEEK_SET) == -1)
> > > +        goto fail;
> >
> > But here it is checked..
> >
> > [..snip (more code with either behaviour)..]
> >
> > FWIW i'm all for it, but it would have been nice to have more coherence
> > there.
> 
> My goal was not to fix all the qemu source code at once. There are many, many 
> places in qemu where no error check is done on system calls. So I fixed a few 
> lseek and left some other ones alone. But believe me there are many more of 
> them in the source code. In order to be "coherent" my patch would need to be 
> a 
> lot, lot bigger.

And what was the motivation of fixing some and leaving others as is?

> BTW, there are other places in qemu sources where for example EINTR or 
> incomplete read/write are not handled correctly on read/write calls. But I am 
> not going to chase after them just to be told that you prefer to silence GCC 
> rather than checking for errors.

"You" as in...?

-- 
mailto:address@hidden