Re: [Qemu-devel] [PATCH] suppress 'warn_unused_result' warning

[Daniel P. Berrange]

On Mon, May 11, 2009 at 03:15:11AM +0800, Chih-Min Chao wrote:
> The patch add error handling to functions with 'warn_unused_result' return
> value
> such as write, read, ftruncate, and realpath.

> @@ -239,11 +239,18 @@ static int cow_create(const char *filename, int64_t 
> image_sectors,
>      }
>      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 (write(cow_fd, &cow_header, sizeof(cow_header)) == -1)
> +        goto fail;

This isn't correct. You need to check that the write() actually 
wrote the number of bytes you asked it to., eg 

  if (write(cow_fd, &cow_header, sizeof(cow_header))  != sizeof(cow_header)
     goto fail;

would catch a short write, as well as other errors. Of course
you don't neccessarily want to fail on a short write, because
a reception of a signal can trigger a short write that can 
easily be recovered from by simply calling write() against for
the remainder of the data.

>      /* 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)) 
> == -1)
> +        goto fail;
> +
>      close(cow_fd);
>      return 0;
> +
> +fail:
> +    close(cow_fd);
> +    return -1;
>  }
>  
>  static void cow_flush(BlockDriverState *bs)
> diff --git a/block-qcow.c b/block-qcow.c
> index fc6b809..16138f3 100644
> --- a/block-qcow.c
> +++ b/block-qcow.c
> @@ -811,17 +811,28 @@ static int qcow_create(const char *filename, int64_t 
> total_size,
>      }
>  
>      /* write all the data */
> -    write(fd, &header, sizeof(header));
> +    if (write(fd, &header, sizeof(header)) == -1) {
> +        goto fail;
> +    }
> +
>      if (backing_file) {
> -        write(fd, backing_file, backing_filename_len);
> +        if (write(fd, backing_file, backing_filename_len) == -1) {
> +            goto fail;
> +        }
>      }
>      lseek(fd, header_size, SEEK_SET);
>      tmp = 0;
>      for(i = 0;i < l1_size; i++) {
> -        write(fd, &tmp, sizeof(tmp));
> +        if (write(fd, &tmp, sizeof(tmp)) == -1) {
> +            goto fail;
> +        }
>      }

Likewise all these are failing to check for a complete write.

If we want to make this robust for EINTR too, then a small wrapper
around raw read/write calls would likely be wanted to deal with
fact an a signal can cause EINTR, *or* a short write

  ssize_t safewrite(int fd, const void *buf, size_t count)
  {
        size_t nwritten = 0;
        while (count > 0) {
                ssize_t r = write(fd, buf, count);

                if (r < 0 && errno == EINTR)
                        continue;
                if (r < 0)
                        return r;
                if (r == 0)
                        return nwritten;
                buf = (const char *)buf + r;
                count -= r;
                nwritten += r;
        }
        return nwritten;
  }


Regards,
Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|