thr3ads.net - Libguestfs - Re: [Libguestfs] [nbdkit PATCH] nbd: Another libnbd version bump [Aug 2019]

If this information is useful, please help other people find it:
Share via:

Eric Blake

2019-Aug-15 14:27 UTC

[Libguestfs] [nbdkit PATCH] nbd: Another libnbd version bump

The 0.9.8 release breaks API, requiring a number of changes:
- Use symbolic constants instead of magic numbers/open-coded strings
  (well, the string for "base:allocation" was present before this
  libnbd bump)
- Change callbacks to drop the valid_flag parameter
- Add _is to nbd_read_only call
- Drop the _callback suffix on nbd_aio_FOO calls
- Add a struct for managing callback/user_data at once

Signed-off-by: Eric Blake <eblake@redhat.com>
---

Pushing, since we're releasing libnbd 0.9.8 today

 configure.ac      |  4 +--
 plugins/nbd/nbd.c | 85 +++++++++++++++++++++--------------------------
 README            |  2 +-
 3 files changed, 41 insertions(+), 50 deletions(-)

diff --git a/configure.ac b/configure.ac
index ee14516d..0b54c00a 100644
--- a/configure.ac
+++ b/configure.ac
@@ -721,12 +721,12 @@ AC_ARG_WITH([libnbd],
     [],
     [with_libnbd=check])
 AS_IF([test "$with_libnbd" != "no"],[
-    PKG_CHECK_MODULES([LIBNBD], [libnbd >= 0.9.6],[
+    PKG_CHECK_MODULES([LIBNBD], [libnbd >= 0.9.8],[
         AC_SUBST([LIBNBD_CFLAGS])
         AC_SUBST([LIBNBD_LIBS])
         AC_DEFINE([HAVE_LIBNBD],[1],[libnbd found at compile time.])
     ],
-    [AC_MSG_WARN([libnbd >= 0.9.6 not found, nbd plugin will be crippled])])
+    [AC_MSG_WARN([libnbd >= 0.9.8 not found, nbd plugin will be crippled])])
 ])
 AM_CONDITIONAL([HAVE_LIBNBD], [test "x$LIBNBD_LIBS" !=
"x"])

diff --git a/plugins/nbd/nbd.c b/plugins/nbd/nbd.c
index f11e54d5..09c8891e 100644
--- a/plugins/nbd/nbd.c
+++ b/plugins/nbd/nbd.c
@@ -62,7 +62,7 @@ struct transaction {
   sem_t sem;
   uint32_t early_err;
   uint32_t err;
-  struct nbdkit_extents *extents;
+  nbd_completion_callback cb;
 };

 /* The per-connection handle */
@@ -160,11 +160,12 @@ nbdplug_config (const char *key, const char *value)
     if (strcasecmp (value, "require") == 0 ||
         strcasecmp (value, "required") == 0 ||
         strcasecmp (value, "force") == 0)
-      tls = 2;
+      tls = LIBNBD_TLS_REQUIRE;
     else {
-      tls = nbdkit_parse_bool (value);
-      if (tls == -1)
+      r = nbdkit_parse_bool (value);
+      if (r == -1)
         exit (EXIT_FAILURE);
+      tls = r ? LIBNBD_TLS_ALLOW : LIBNBD_TLS_DISABLE;
     }
   }
   else if (strcmp (key, "tls-certificates") == 0) {
@@ -245,8 +246,9 @@ nbdplug_config_complete (void)
     export = "";

   if (tls == -1)
-    tls = tls_certificates || tls_verify >= 0 || tls_username || tls_psk;
-  if (tls > 0) {
+    tls = (tls_certificates || tls_verify >= 0 || tls_username || tls_psk)
+      ? LIBNBD_TLS_ALLOW : LIBNBD_TLS_DISABLE;
+  if (tls != LIBNBD_TLS_DISABLE) {
     struct nbd_handle *nbd = nbd_create ();

     if (!nbd) {
@@ -345,23 +347,12 @@ nbdplug_reader (void *handle)
   return NULL;
 }

-/* Prepare for a transaction. */
-static void
-nbdplug_prepare (struct transaction *trans)
-{
-  memset (trans, 0, sizeof *trans);
-  if (sem_init (&trans->sem, 0, 0))
-    assert (false);
-}
-
+/* Callback used at end of a transaction. */
 static int
-nbdplug_notify (unsigned valid_flag, void *opaque, int *error)
+nbdplug_notify (void *opaque, int *error)
 {
   struct transaction *trans = opaque;

-  if (!(valid_flag & LIBNBD_CALLBACK_VALID))
-    return 0;
-
   /* There's a possible race here where trans->cookie has not yet been
    * updated by nbdplug_register, but it's only an informational
    * message.
@@ -376,6 +367,17 @@ nbdplug_notify (unsigned valid_flag, void *opaque, int
*error)
   return 1;
 }

+/* Prepare for a transaction. */
+static void
+nbdplug_prepare (struct transaction *trans)
+{
+  memset (trans, 0, sizeof *trans);
+  if (sem_init (&trans->sem, 0, 0))
+    assert (false);
+  trans->cb.callback = nbdplug_notify;
+  trans->cb.user_data = trans;
+}
+
 /* Register a cookie and kick the I/O thread. */
 static void
 nbdplug_register (struct handle *h, struct transaction *trans, int64_t cookie)
@@ -466,7 +468,7 @@ nbdplug_open_handle (int readonly)
     goto err;
   if (nbd_set_export_name (h->nbd, export) == -1)
     goto err;
-  if (nbd_add_meta_context (h->nbd, "base:allocation") == -1)
+  if (nbd_add_meta_context (h->nbd, LIBNBD_CONTEXT_BASE_ALLOCATION) == -1)
     goto err;
   if (nbd_set_tls (h->nbd, tls) == -1)
     goto err;
@@ -570,7 +572,7 @@ static int
 nbdplug_can_write (void *handle)
 {
   struct handle *h = handle;
-  int i = nbd_read_only (h->nbd);
+  int i = nbd_is_read_only (h->nbd);

   if (i == -1) {
     nbdkit_error ("failure to check readonly flag: %s", nbd_get_error
());
@@ -674,7 +676,7 @@ static int
 nbdplug_can_extents (void *handle)
 {
   struct handle *h = handle;
-  int i = nbd_can_meta_context (h->nbd, "base:allocation");
+  int i = nbd_can_meta_context (h->nbd, LIBNBD_CONTEXT_BASE_ALLOCATION);

   if (i == -1) {
     nbdkit_error ("failure to check extents ability: %s",
nbd_get_error ());
@@ -693,8 +695,8 @@ nbdplug_pread (void *handle, void *buf, uint32_t count,
uint64_t offset,

   assert (!flags);
   nbdplug_prepare (&s);
-  nbdplug_register (h, &s, nbd_aio_pread_callback (h->nbd, buf, count,
offset,
-                                                   nbdplug_notify, &s, 0));
+  nbdplug_register (h, &s, nbd_aio_pread (h->nbd, buf, count, offset,
+                                          s.cb, 0));
   return nbdplug_reply (h, &s);
 }

@@ -709,8 +711,8 @@ nbdplug_pwrite (void *handle, const void *buf, uint32_t
count, uint64_t offset,

   assert (!(flags & ~NBDKIT_FLAG_FUA));
   nbdplug_prepare (&s);
-  nbdplug_register (h, &s, nbd_aio_pwrite_callback (h->nbd, buf, count,
offset,
-                                                    nbdplug_notify, &s,
f));
+  nbdplug_register (h, &s, nbd_aio_pwrite (h->nbd, buf, count, offset,
+                                           s.cb, f));
   return nbdplug_reply (h, &s);
 }

@@ -729,8 +731,7 @@ nbdplug_zero (void *handle, uint32_t count, uint64_t offset,
uint32_t flags)
   if (flags & NBDKIT_FLAG_FUA)
     f |= LIBNBD_CMD_FLAG_FUA;
   nbdplug_prepare (&s);
-  nbdplug_register (h, &s, nbd_aio_zero_callback (h->nbd, count, offset,
-                                                  nbdplug_notify, &s, f));
+  nbdplug_register (h, &s, nbd_aio_zero (h->nbd, count, offset, s.cb,
f));
   return nbdplug_reply (h, &s);
 }

@@ -744,8 +745,7 @@ nbdplug_trim (void *handle, uint32_t count, uint64_t offset,
uint32_t flags)

   assert (!(flags & ~NBDKIT_FLAG_FUA));
   nbdplug_prepare (&s);
-  nbdplug_register (h, &s, nbd_aio_trim_callback (h->nbd, count, offset,
-                                                  nbdplug_notify, &s, f));
+  nbdplug_register (h, &s, nbd_aio_trim (h->nbd, count, offset, s.cb,
f));
   return nbdplug_reply (h, &s);
 }

@@ -758,23 +758,17 @@ nbdplug_flush (void *handle, uint32_t flags)

   assert (!flags);
   nbdplug_prepare (&s);
-  nbdplug_register (h, &s, nbd_aio_flush_callback (h->nbd,
-                                                   nbdplug_notify, &s, 0));
+  nbdplug_register (h, &s, nbd_aio_flush (h->nbd, s.cb, 0));
   return nbdplug_reply (h, &s);
 }

 static int
-nbdplug_extent (unsigned valid_flag, void *opaque,
-                const char *metacontext, uint64_t offset,
+nbdplug_extent (void *opaque, const char *metacontext, uint64_t offset,
                 uint32_t *entries, size_t nr_entries, int *error)
 {
-  struct transaction *trans = opaque;
-  struct nbdkit_extents *extents = trans->extents;
+  struct nbdkit_extents *extents = opaque;

-  if (!(valid_flag & LIBNBD_CALLBACK_VALID))
-    return 0;
-
-  assert (strcmp (metacontext, "base:allocation") == 0);
+  assert (strcmp (metacontext, LIBNBD_CONTEXT_BASE_ALLOCATION) == 0);
   assert (nr_entries % 2 == 0);
   while (nr_entries) {
     /* We rely on the fact that NBDKIT_EXTENT_* match NBD_STATE_* */
@@ -797,14 +791,12 @@ nbdplug_extents (void *handle, uint32_t count, uint64_t
offset,
   struct handle *h = handle;
   struct transaction s;
   uint32_t f = flags & NBDKIT_FLAG_REQ_ONE ? LIBNBD_CMD_FLAG_REQ_ONE : 0;
+  nbd_extent_callback extcb = { nbdplug_extent, extents };

   assert (!(flags & ~NBDKIT_FLAG_REQ_ONE));
   nbdplug_prepare (&s);
-  s.extents = extents;
-  nbdplug_register (h, &s, nbd_aio_block_status_callback (h->nbd, count,
offset,
-                                                          nbdplug_extent,
&s,
-                                                          nbdplug_notify,
&s,
-                                                          f));
+  nbdplug_register (h, &s, nbd_aio_block_status (h->nbd, count, offset,
+                                                 extcb, s.cb, f));
   return nbdplug_reply (h, &s);
 }

@@ -817,8 +809,7 @@ nbdplug_cache (void *handle, uint32_t count, uint64_t
offset, uint32_t flags)

   assert (!flags);
   nbdplug_prepare (&s);
-  nbdplug_register (h, &s, nbd_aio_cache_callback (h->nbd, count,
offset,
-                                                   nbdplug_notify, &s, 0));
+  nbdplug_register (h, &s, nbd_aio_cache (h->nbd, count, offset, s.cb,
0));
   return nbdplug_reply (h, &s);
 }

diff --git a/README b/README
index 06c16dd1..b78f490a 100644
--- a/README
+++ b/README
@@ -113,7 +113,7 @@ For the linuxdisk plugin:

 For the nbd plugin, to get URI and TLS support:

- - libnbd >= 0.9.6
+ - libnbd >= 0.9.8

 For the Perl, example4 and tar plugins:

-- 
2.20.1

Richard W.M. Jones

2019-Aug-15 15:37 UTC

head link

Re: [Libguestfs] [nbdkit PATCH] nbd: Another libnbd version bump

On Thu, Aug 15, 2019 at 09:27:14AM -0500, Eric Blake
wrote:> The 0.9.8 release breaks API, requiring a number of changes:
> - Use symbolic constants instead of magic numbers/open-coded strings
>   (well, the string for "base:allocation" was present before this
>   libnbd bump)
> - Change callbacks to drop the valid_flag parameter
> - Add _is to nbd_read_only call
> - Drop the _callback suffix on nbd_aio_FOO calls
> - Add a struct for managing callback/user_data at once
Seems reasonable.
> @@ -160,11 +160,12 @@ nbdplug_config (const char *key, const char *value)
>      if (strcasecmp (value, "require") == 0 ||
>          strcasecmp (value, "required") == 0 ||
>          strcasecmp (value, "force") == 0)
> -      tls = 2;
> +      tls = LIBNBD_TLS_REQUIRE;
>      else {
> -      tls = nbdkit_parse_bool (value);
> -      if (tls == -1)
> +      r = nbdkit_parse_bool (value);
> +      if (r == -1)
>          exit (EXIT_FAILURE);
> +      tls = r ? LIBNBD_TLS_ALLOW : LIBNBD_TLS_DISABLE;
Our feedback was the LIBNBD_TLS_ALLOW was really bad (I'm unconvinced
because I prefer my stuff to work and TLS very often doesn't).  Do you
think we should use REQUIRE here as well?

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

Richard W.M. Jones

2019-Aug-15 15:37 UTC

head link

Re: [Libguestfs] [nbdkit PATCH] nbd: Another libnbd version bump

Oh that was an ACK whatever you decide :-)

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

Eric Blake

2019-Aug-15 16:14 UTC

head link

Re: [Libguestfs] [nbdkit PATCH] nbd: Another libnbd version bump

On 8/15/19 10:37 AM, Richard W.M. Jones wrote:> On Thu, Aug 15, 2019 at 09:27:14AM -0500, Eric Blake wrote:
>> The 0.9.8 release breaks API, requiring a number of changes:
>> - Use symbolic constants instead of magic numbers/open-coded strings
>>   (well, the string for "base:allocation" was present before
this
>>   libnbd bump)
>> - Change callbacks to drop the valid_flag parameter
>> - Add _is to nbd_read_only call
>> - Drop the _callback suffix on nbd_aio_FOO calls
>> - Add a struct for managing callback/user_data at once
> 
> Seems reasonable.
> 
>> @@ -160,11 +160,12 @@ nbdplug_config (const char *key, const char
*value)
>>      if (strcasecmp (value, "require") == 0 ||
>>          strcasecmp (value, "required") == 0 ||
>>          strcasecmp (value, "force") == 0)
>> -      tls = 2;
>> +      tls = LIBNBD_TLS_REQUIRE;
>>      else {
>> -      tls = nbdkit_parse_bool (value);
>> -      if (tls == -1)
>> +      r = nbdkit_parse_bool (value);
>> +      if (r == -1)
>>          exit (EXIT_FAILURE);
>> +      tls = r ? LIBNBD_TLS_ALLOW : LIBNBD_TLS_DISABLE;
For this patch, it was straight conversion.  If we change things, that
belongs in a separate patch.
> 
> Our feedback was the LIBNBD_TLS_ALLOW was really bad (I'm unconvinced
> because I prefer my stuff to work and TLS very often doesn't).  Do you
> think we should use REQUIRE here as well?
The existing behavior of 'nbd tls=...' matches the 'nbdkit
--tls=...'
behavior. Whether to keep both options as tri-state, or whether to make
'--tls=on'/'--tls=yes' default to strict, and add yet another
spelling
for '--tls=optional' as something that is harder to spell for the
specific case when you are willing to open yourself to
man-in-the-middle, may have merit.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Reasonably Related Threads

Search for more seemingly similar threads

Libguestfs - Aug 2019 - Re: [nbdkit PATCH] nbd: Another libnbd version bump

[Libguestfs] [nbdkit PATCH] nbd: Another libnbd version bump

Re: [Libguestfs] [nbdkit PATCH] nbd: Another libnbd version bump

Re: [Libguestfs] [nbdkit PATCH] nbd: Another libnbd version bump

Re: [Libguestfs] [nbdkit PATCH] nbd: Another libnbd version bump

Reasonably Related Threads