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:58:59 +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: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.

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.

JC




reply via email to

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