Libguestfs - Apr 2019 - Re: [nbdkit PATCH 0/3] Fix data integrity in vddk plugin

Couple of fixes to return correct data and one nice-to-have clean-up which is
not needed.  I just find it nicer to read.

Martin Kletzander (3):
  vddk: Use a separate handle for single-link=true
  vddk: Do not report hole extents to be zero with single-link=true
  vddk: Eliminate one needless goto

 plugins/vddk/vddk.c | 48 +++++++++++++++++++++++++++++++++------------
 1 file changed, 36 insertions(+), 12 deletions(-)

-- 
2.21.0

When using VIXDISKLIB_FLAG_OPEN_SINGLE_LINK, parent images are not taken into
account.  However, the data we get from VixDiskLib_QueryAllocatedBlocks() has
such granularity that it might return incorrect information.  For example when
the top image has only one block allocated, we get information about the whole
chunk being allocated.  This results in the consequent read to return invalid
data - the rest of the chunk is just zeros.  To eliminate losing data integrity
this patch makes the plugin hold two separate handles, one opened with the
VIXDISKLIB_FLAG_OPEN_SINGLE_LINK flag and one without it.  The top image without
the parent chain is then used only for returning information about extents and
the original handle is used for the rest of the operations.  Even though this
way we report more data in the top image, the resulting data is correct.

Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
---
 plugins/vddk/vddk.c | 28 +++++++++++++++++++++++-----
 1 file changed, 23 insertions(+), 5 deletions(-)

diff --git a/plugins/vddk/vddk.c b/plugins/vddk/vddk.c
index ca6ae6d6ff63..8cf54f6e2b96 100644
--- a/plugins/vddk/vddk.c
+++ b/plugins/vddk/vddk.c
@@ -428,6 +428,7 @@ struct vddk_handle {
   VixDiskLibConnectParams *params; /* connection parameters */
   VixDiskLibConnection connection; /* connection */
   VixDiskLibHandle handle;         /* disk handle */
+  VixDiskLibHandle handle_single_link; /* disk handle for single_link=true */
 };
 
 static inline VixDiskLibConnectParams *
@@ -522,8 +523,6 @@ vddk_open (int readonly)
   flags = 0;
   if (readonly)
     flags |= VIXDISKLIB_FLAG_OPEN_READ_ONLY;
-  if (single_link)
-    flags |= VIXDISKLIB_FLAG_OPEN_SINGLE_LINK;
   if (unbuffered)
     flags |= VIXDISKLIB_FLAG_OPEN_UNBUFFERED;
 
@@ -538,6 +537,21 @@ vddk_open (int readonly)
   nbdkit_debug ("transport mode: %s",
                 VixDiskLib_GetTransportMode (h->handle));
 
+  if (single_link) {
+      flags |= VIXDISKLIB_FLAG_OPEN_SINGLE_LINK;
+
+      DEBUG_CALL ("VixDiskLib_Open",
+                  "connection, %s, %d, &handle_single_link",
filename, flags);
+      err = VixDiskLib_Open (h->connection, filename, flags,
&h->handle_single_link);
+      if (err != VIX_OK) {
+        VDDK_ERROR (err, "VixDiskLib_Open: %s", filename);
+        goto err2;
+      }
+
+      nbdkit_debug ("transport mode for single_link: %s",
+                    VixDiskLib_GetTransportMode (h->handle_single_link));
+  }
+
   return h;
 
  err2:
@@ -559,6 +573,10 @@ vddk_close (void *handle)
   free_connect_params (h->params);
   DEBUG_CALL ("VixDiskLib_Close", "handle");
   VixDiskLib_Close (h->handle);
+  if (single_link) {
+    DEBUG_CALL ("VixDiskLib_Close", "handle_single_link");
+    VixDiskLib_Close (h->handle_single_link);
+  }
   DEBUG_CALL ("VixDiskLib_Disconnect", "connection");
   VixDiskLib_Disconnect (h->connection);
   free (h);
@@ -698,7 +716,7 @@ vddk_flush (void *handle, uint32_t flags)
     return 0;
 
   DEBUG_CALL ("VixDiskLib_Flush", "handle");
-  err = VixDiskLib_Flush (h->handle);
+  err = VixDiskLib_Flush (single_link ? h->handle_single_link :
h->handle);
   if (err != VIX_OK) {
     VDDK_ERROR (err, "VixDiskLib_Flush");
     return -1;
@@ -730,7 +748,7 @@ vddk_can_extents (void *handle)
   DEBUG_CALL ("VixDiskLib_QueryAllocatedBlocks",
               "handle, 0, %d sectors, %d sectors",
               VIXDISKLIB_MIN_CHUNK_SIZE, VIXDISKLIB_MIN_CHUNK_SIZE);
-  err = VixDiskLib_QueryAllocatedBlocks (h->handle,
+  err = VixDiskLib_QueryAllocatedBlocks (single_link ? h->handle_single_link
: h->handle,
                                          0, VIXDISKLIB_MIN_CHUNK_SIZE,
                                          VIXDISKLIB_MIN_CHUNK_SIZE,
                                          &block_list);
@@ -809,7 +827,7 @@ vddk_extents (void *handle, uint32_t count, uint64_t offset,
uint32_t flags,
                 "handle, %" PRIu64 " sectors, %" PRIu64
" sectors, "
                 "%d sectors",
                 start_sector, nr_sectors, VIXDISKLIB_MIN_CHUNK_SIZE);
-    err = VixDiskLib_QueryAllocatedBlocks (h->handle,
+    err = VixDiskLib_QueryAllocatedBlocks (single_link ?
h->handle_single_link : h->handle,
                                            start_sector, nr_sectors,
                                            VIXDISKLIB_MIN_CHUNK_SIZE,
                                            &block_list);
-- 
2.21.0

the data in holes is actually not zeros as there might be a parent image having
other data.  Properly reporting (non-)zero hole extents allows clients to have
information about whether the data in the image is unallocated, but the read
will return zero blocks (HOLE+ZERO) or whether the data is just not in this
later of the image (only HOLE).

Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
---
 plugins/vddk/vddk.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/plugins/vddk/vddk.c b/plugins/vddk/vddk.c
index 8cf54f6e2b96..cda670325b85 100644
--- a/plugins/vddk/vddk.c
+++ b/plugins/vddk/vddk.c
@@ -773,9 +773,17 @@ static int
 add_extent (struct nbdkit_extents *extents,
             uint64_t *position, uint64_t next_position, bool is_hole)
 {
-  const uint32_t type = is_hole ? NBDKIT_EXTENT_HOLE|NBDKIT_EXTENT_ZERO : 0;
+  uint32_t type = 0;
   const uint64_t length = next_position - *position;
 
+  if (is_hole) {
+    type = NBDKIT_EXTENT_HOLE;
+    /* Images opened as single link might be backed by another file in the
+       chain, so the holes are not guaranteed to be zeros. */
+    if (!single_link)
+      type |= NBDKIT_EXTENT_ZERO;
+  }
+
   assert (*position <= next_position);
   if (*position == next_position)
     return 0;
-- 
2.21.0

It just reads a bit better when the goto does not jump between blocks of code
with different scope (rather then just being used for retries or clean-ups).

Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
---
 plugins/vddk/vddk.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/plugins/vddk/vddk.c b/plugins/vddk/vddk.c
index cda670325b85..528a5896aad6 100644
--- a/plugins/vddk/vddk.c
+++ b/plugins/vddk/vddk.c
@@ -853,12 +853,10 @@ vddk_extents (void *handle, uint32_t count, uint64_t
offset, uint32_t flags,
       /* The query returns allocated blocks.  We must insert holes
        * between the blocks as necessary.
        */
-      if (position < blk_offset &&
-          add_extent (extents, &position, blk_offset, true) == -1)
-        goto error_in_add;
-      if (add_extent (extents,
-                      &position, blk_offset + blk_length, false) == -1) {
-      error_in_add:
+      if ((position < blk_offset &&
+           add_extent (extents, &position, blk_offset, true) == -1) ||
+          (add_extent (extents,
+                       &position, blk_offset + blk_length, false) == -1)) {
         DEBUG_CALL ("VixDiskLib_FreeBlockList",
"block_list");
         VixDiskLib_FreeBlockList (block_list);
         return -1;
-- 
2.21.0

I pushed 2 & 3, thanks.

I'm not overjoyed about patch 1 however.  It seems to me that this is
solved better by having two NBD connections open, one with the
single-link=true flag and one without, and then reading extents from
one and data from the other.  Admittedly his pushes the complexity to
the client and prevents you using qemu-img, but it seems better
architecturally than making an odd change to the VDDK plugin for this
one application.

(I'm about to post some thoughts on an NBD client, will CC you on it)

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

On Tue, Apr 30, 2019 at 01:03:28PM +0100, Richard W.M. Jones
wrote:
>I pushed 2 & 3, thanks.
>
>I'm not overjoyed about patch 1 however.  It seems to me that this is
>solved better by having two NBD connections open, one with the
>single-link=true flag and one without, and then reading extents from
>one and data from the other.  Admittedly his pushes the complexity to
>the client and prevents you using qemu-img, but it seems better
>architecturally than making an odd change to the VDDK plugin for this
>one application.
>
I actually thought of this from a more generic point of view, not just my use
case.  What VDDK exposes is really not something that is really unusable from
any point of view.  Be it qemu-img, any other client or users.  The resulting
data makes single-link=true completely unusable.  Well, unless the extent
granularity on the source block device and the file format is exactly equal to
the CHUNK_MIN_SIZE, but nobody can ever guarantee that.

I also thought of a way how to have nbdkit handle "backing chains" (it
would
basically have a list of plug-ins and the first one that has data available
would be used), but that seemed too much of a hammer.  It could also be handled
by a specialized filter where the filter would provide extents from single-link
and plugin would do the rest.  But, honestly, that sounds worse to me than what
I posted in the first patch.  Maybe the filter could just take a plugin as a
parameter, e.g. "intercept extents and use this plugin for those
calls", or even
generalize it to a specified list of APIs that it should intercept.

Both above approaches would need the user to explicitly specify what needs to be
done and they would really need to know how all the details fit together just to
be able to use it, even though there is a possible solve-all solution (without
which the single-link does not really has a standalone use case).
>(I'm about to post some thoughts on an NBD client, will CC you on it)
>
>Rich.
>
>-- 
>Richard Jones, Virtualization Group, Red Hat
http://people.redhat.com/~rjones
>Read my programming and virtualization blog: http://rwmj.wordpress.com
>virt-builder quickly builds VMs from scratch
>http://libguestfs.org/virt-builder.1.html
>
>_______________________________________________
>Libguestfs mailing list
>Libguestfs@redhat.com
>https://www.redhat.com/mailman/listinfo/libguestfs