[Qemu-devel] [PATCH] [UPDATED] Add host_device support to qemu-img.

[Qemu-devel] [PATCH] [UPDATED] Add host_device support to qemu-img.

[Nolan]

This patch allows the use a host_device as the destination for "qemu-img
convert".

I added a ->bdrv_create function host_device.  It merely verifies that
the device exists and is large enough.

A check is needed in the qemu-img convert loop to ensure that we write
out all 0 sectors to the host_device.  Otherwise they end up with stale
garbage where all zero sectors were expected.

I also made the check against bdrv_is_allocated enabled for everything
_except_ host devices, since there is no point in making the block
backend write a bunch of zeros just so that we can memcmp them
immediately afterwards.  Host devices can't benefit from this because
there is no way to differentiate between a sector being unallocated
because it was never written, or because it was written with all zeros
and then made a trip through qemu-img convert.

Finally, there is an unrelated fix for a typo in the error message
printed if the destination device does not support ->bdrv_create.

Signed-off-by: Nolan Leake <nolan <at> sigbus.net>

Index: block-raw-posix.c
===================================================================
--- block-raw-posix.c	(revision 6963)
+++ block-raw-posix.c	(working copy)
@@ -1378,11 +1378,47 @@
 }
 #endif /* !linux && !FreeBSD */
 
+#if defined(__linux__) || defined(__FreeBSD__)
+static int hdev_create(const char *filename, int64_t total_size,
+                       const char *backing_file, int flags)
+{
+    int fd;
+    int ret = 0;
+    struct stat stat_buf;
+
+    if (flags || backing_file)
+        return -ENOTSUP;
+
+    fd = open(filename, O_WRONLY | O_BINARY);
+    if (fd < 0)
+        return -EIO;
+
+    if (fstat(fd, &stat_buf) < 0)
+        ret = -EIO;
+    else if (!S_ISBLK(stat_buf.st_mode))
+        ret = -EIO;
+    else if (lseek(fd, 0, SEEK_END) < total_size * 512)
+        ret = -ENOSPC;
+
+    close(fd);
+    return ret;
+}
+
+#else  /* !(linux || freebsd) */
+
+static int hdev_create(const char *filename, int64_t total_size,
+                       const char *backing_file, int flags)
+{
+    return -ENOTSUP;
+}
+#endif
+
 BlockDriver bdrv_host_device = {
     .format_name	= "host_device",
     .instance_size	= sizeof(BDRVRawState),
     .bdrv_open		= hdev_open,
     .bdrv_close		= raw_close,
+    .bdrv_create        = hdev_create,
     .bdrv_flush		= raw_flush,
 
 #ifdef CONFIG_AIO
Index: qemu-img.c
===================================================================
--- qemu-img.c	(revision 6963)
+++ qemu-img.c	(working copy)
@@ -493,7 +493,7 @@
     ret = bdrv_create(drv, out_filename, total_sectors, out_baseimg, flags);
     if (ret < 0) {
         if (ret == -ENOTSUP) {
-            error("Formatting not supported for file format '%s'", fmt);
+            error("Formatting not supported for file format '%s'", out_fmt);
         } else {
             error("Error while formatting '%s'", out_filename);
         }
@@ -592,18 +592,17 @@
             if (n > bs_offset + bs_sectors - sector_num)
                 n = bs_offset + bs_sectors - sector_num;
 
-            /* If the output image is being created as a copy on write image,
-               assume that sectors which are unallocated in the input image
-               are present in both the output's and input's base images (no
-               need to copy them). */
-            if (out_baseimg) {
-               if (!bdrv_is_allocated(bs[bs_i], sector_num - bs_offset, n, &n1)) {
-                  sector_num += n1;
-                  continue;
-               }
-               /* The next 'n1' sectors are allocated in the input image. Copy
-                  only those as they may be followed by unallocated sectors. */
-               n = n1;
+            if (drv != &bdrv_host_device) {
+                if (!bdrv_is_allocated(bs[bs_i], sector_num - bs_offset,
+                                       n, &n1)) {
+                    sector_num += n1;
+                    continue;
+                }
+                /* The next 'n1' sectors are allocated in the input image. Copy
+                   only those as they may be followed by unallocated sectors. */
+                n = n1;
+            } else {
+                n1 = n;
             }
 
             if (bdrv_read(bs[bs_i], sector_num - bs_offset, buf, n) < 0) 
@@ -615,8 +614,13 @@
             while (n > 0) {
                 /* If the output image is being created as a copy on write image,
                    copy all sectors even the ones containing only NUL bytes,
-                   because they may differ from the sectors in the base image. */
-                if (out_baseimg || is_allocated_sectors(buf1, n, &n1)) {
+                   because they may differ from the sectors in the base image.
+
+                   If the output is to a host device, we also write out
+                   sectors that are entirely 0, since whatever data was
+                   already there is garbage, not 0s. */
+                if (drv == &bdrv_host_device || out_baseimg ||
+                    is_allocated_sectors(buf1, n, &n1)) {
                     if (bdrv_write(out_bs, sector_num, buf1, n1) < 0)
                         error("error while writing");
                 }

Re: [Qemu-devel] [PATCH] [UPDATED] Add host_device support to qemu-img.

[malc]

On Wed, 1 Apr 2009, Nolan wrote:

> This patch allows the use a host_device as the destination for "qemu-img
> convert".
> 
> I added a ->bdrv_create function host_device.  It merely verifies that
> the device exists and is large enough.
> 
> A check is needed in the qemu-img convert loop to ensure that we write
> out all 0 sectors to the host_device.  Otherwise they end up with stale
> garbage where all zero sectors were expected.
> 
> I also made the check against bdrv_is_allocated enabled for everything
> _except_ host devices, since there is no point in making the block
> backend write a bunch of zeros just so that we can memcmp them
> immediately afterwards.  Host devices can't benefit from this because
> there is no way to differentiate between a sector being unallocated
> because it was never written, or because it was written with all zeros
> and then made a trip through qemu-img convert.
> 
> Finally, there is an unrelated fix for a typo in the error message
> printed if the destination device does not support ->bdrv_create.
> 
> Signed-off-by: Nolan Leake <nolan <at> sigbus.net>
> 
> Index: block-raw-posix.c
> ===================================================================
> --- block-raw-posix.c	(revision 6963)
> +++ block-raw-posix.c	(working copy)
> @@ -1378,11 +1378,47 @@
>  }
>  #endif /* !linux && !FreeBSD */
>  
> +#if defined(__linux__) || defined(__FreeBSD__)
> +static int hdev_create(const char *filename, int64_t total_size,
> +                       const char *backing_file, int flags)
> +{
> +    int fd;
> +    int ret = 0;
> +    struct stat stat_buf;
> +
> +    if (flags || backing_file)
> +        return -ENOTSUP;
> +
> +    fd = open(filename, O_WRONLY | O_BINARY);
> +    if (fd < 0)
> +        return -EIO;
> +
> +    if (fstat(fd, &stat_buf) < 0)
> +        ret = -EIO;
> +    else if (!S_ISBLK(stat_buf.st_mode))
> +        ret = -EIO;
> +    else if (lseek(fd, 0, SEEK_END) < total_size * 512)
> +        ret = -ENOSPC;

This leaks fescriptors doesn't it, even if only user is qemu-img which
just exits on errors this still looks fragile.

> +
> +    close(fd);
> +    return ret;
> +}
> +
> +#else  /* !(linux || freebsd) */
> +
> +static int hdev_create(const char *filename, int64_t total_size,
> +                       const char *backing_file, int flags)
> +{
> +    return -ENOTSUP;
> +}
> +#endif
> +
>  BlockDriver bdrv_host_device = {
>      .format_name	= "host_device",
>      .instance_size	= sizeof(BDRVRawState),
>      .bdrv_open		= hdev_open,
>      .bdrv_close		= raw_close,
> +    .bdrv_create        = hdev_create,
>      .bdrv_flush		= raw_flush,
>  
>  #ifdef CONFIG_AIO
> Index: qemu-img.c
> ===================================================================
> --- qemu-img.c	(revision 6963)
> +++ qemu-img.c	(working copy)
> @@ -493,7 +493,7 @@
>      ret = bdrv_create(drv, out_filename, total_sectors, out_baseimg, flags);
>      if (ret < 0) {
>          if (ret == -ENOTSUP) {
> -            error("Formatting not supported for file format '%s'", fmt);
> +            error("Formatting not supported for file format '%s'", out_fmt);
>          } else {
>              error("Error while formatting '%s'", out_filename);
>          }
> @@ -592,18 +592,17 @@
>              if (n > bs_offset + bs_sectors - sector_num)
>                  n = bs_offset + bs_sectors - sector_num;
>  
> -            /* If the output image is being created as a copy on write image,
> -               assume that sectors which are unallocated in the input image
> -               are present in both the output's and input's base images (no
> -               need to copy them). */
> -            if (out_baseimg) {
> -               if (!bdrv_is_allocated(bs[bs_i], sector_num - bs_offset, n, &n1)) {
> -                  sector_num += n1;
> -                  continue;
> -               }
> -               /* The next 'n1' sectors are allocated in the input image. Copy
> -                  only those as they may be followed by unallocated sectors. */
> -               n = n1;
> +            if (drv != &bdrv_host_device) {
> +                if (!bdrv_is_allocated(bs[bs_i], sector_num - bs_offset,
> +                                       n, &n1)) {
> +                    sector_num += n1;
> +                    continue;
> +                }
> +                /* The next 'n1' sectors are allocated in the input image. Copy
> +                   only those as they may be followed by unallocated sectors. */
> +                n = n1;
> +            } else {
> +                n1 = n;
>              }
>  
>              if (bdrv_read(bs[bs_i], sector_num - bs_offset, buf, n) < 0) 
> @@ -615,8 +614,13 @@
>              while (n > 0) {
>                  /* If the output image is being created as a copy on write image,
>                     copy all sectors even the ones containing only NUL bytes,
> -                   because they may differ from the sectors in the base image. */
> -                if (out_baseimg || is_allocated_sectors(buf1, n, &n1)) {
> +                   because they may differ from the sectors in the base image.
> +
> +                   If the output is to a host device, we also write out
> +                   sectors that are entirely 0, since whatever data was
> +                   already there is garbage, not 0s. */
> +                if (drv == &bdrv_host_device || out_baseimg ||
> +                    is_allocated_sectors(buf1, n, &n1)) {
>                      if (bdrv_write(out_bs, sector_num, buf1, n1) < 0)
>                          error("error while writing");
>                  }
> 
> 
> 
> 

-- 
mailto:av1474@comtv.ru

[Qemu-devel] Re: [PATCH] [UPDATED] Add host_device support to qemu-img.

[Nolan]

malc <av1474 <at> comtv.ru> writes:
> On Wed, 1 Apr 2009, Nolan wrote:

<... snip ...>

> > +    fd = open(filename, O_WRONLY | O_BINARY);
> > +    if (fd < 0)
> > +        return -EIO;
> > +
> > +    if (fstat(fd, &stat_buf) < 0)
> > +        ret = -EIO;
> > +    else if (!S_ISBLK(stat_buf.st_mode))
> > +        ret = -EIO;
> > +    else if (lseek(fd, 0, SEEK_END) < total_size * 512)
> > +        ret = -ENOSPC;
> 
> This leaks fescriptors doesn't it, even if only user is qemu-img which
> just exits on errors this still looks fragile.

I don't see how.

Once a valid fd is allocated, all paths out of this function pass through the
close (now below this comment) right before the "return ret;".

> > +
> > +    close(fd);
> > +    return ret;
> > +}

<... snip ...>

Re: [Qemu-devel] Re: [PATCH] [UPDATED] Add host_device support to qemu-img.

[malc]

On Thu, 2 Apr 2009, Nolan wrote:

> malc <av1474 <at> comtv.ru> writes:
> > On Wed, 1 Apr 2009, Nolan wrote:
> 
> <... snip ...>
> 
> > > +    fd = open(filename, O_WRONLY | O_BINARY);
> > > +    if (fd < 0)
> > > +        return -EIO;
> > > +
> > > +    if (fstat(fd, &stat_buf) < 0)
> > > +        ret = -EIO;
> > > +    else if (!S_ISBLK(stat_buf.st_mode))
> > > +        ret = -EIO;
> > > +    else if (lseek(fd, 0, SEEK_END) < total_size * 512)
> > > +        ret = -ENOSPC;
> > 
> > This leaks fescriptors doesn't it, even if only user is qemu-img which
> > just exits on errors this still looks fragile.
> 
> I don't see how.

I should cease trying to read patches at 5:20 in the morning it seems,
you are right ofcourse, sorry for the noise.
 
That said, if fstat/lseek fail the real reason/errno will never be
communicated to the user not directly nor in form of a warning message.

[..snip..]

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] [PATCH] [UPDATED] Add host_device support to qemu-img.

[Anthony Liguori]

Nolan wrote:
> This patch allows the use a host_device as the destination for "qemu-img
> convert".
>
> I added a ->bdrv_create function host_device.  It merely verifies that
> the device exists and is large enough.
>
> A check is needed in the qemu-img convert loop to ensure that we write
> out all 0 sectors to the host_device.  Otherwise they end up with stale
> garbage where all zero sectors were expected.
>
> I also made the check against bdrv_is_allocated enabled for everything
> _except_ host devices, since there is no point in making the block
> backend write a bunch of zeros just so that we can memcmp them
> immediately afterwards.  Host devices can't benefit from this because
> there is no way to differentiate between a sector being unallocated
> because it was never written, or because it was written with all zeros
> and then made a trip through qemu-img convert.
>
> Finally, there is an unrelated fix for a typo in the error message
> printed if the destination device does not support ->bdrv_create.
>
> Signed-off-by: Nolan Leake <nolan <at> sigbus.net>
>   
Applied.  Thanks.

Regards,

Anthony Liguori