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

[malc]

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.

-- 
mailto:address@hidden