Libguestfs - Feb 2020 - [nbdkit PATCH] server: Correct logic when filter fails .prepare

If .prepare fails, we do not want to call .finalize (similar to how if
.open fails, we do not want to call .close).  However, the logic in
backend_finalize was checking on the wrong condition ('was the
connection opened' rather than 'was the connection prepared'), which
led to an assertion failure.

Simple reproducer:

$ nbdkit -U - -f --filter cache --run 'qemu-io -r -c quit $nbd' sh -
<<\EOF
> case $1 in get_size) echo oops >&2; exit 1 ;; *) exit 2 ;; esac
> EOF
nbdkit: sh[1]: error: /tmp/nbdkitshYvAQbz/inline-script.sh: oops
nbdkit: backend.c:206: backend_finalize: Assertion `h->state &
HANDLE_CONNECTED' failed.
qemu-io: can't open device nbd:unix:/tmp/nbdkit60FUTw/socket: Failed to read
option reply: Unexpected end-of-file before all bytes were read
nbdkit: nbdkit command was killed by signal 6

With this patch, the command now fails gracefully:
nbdkit: sh[1]: error: /tmp/nbdkitshie18Lp/inline-script.sh: oops
qemu-io: can't open device nbd:unix:/tmp/nbdkitqvAOlr/socket: Requested
export not available

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

Pushing this to master and stable-1.16

 server/backend.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/server/backend.c b/server/backend.c
index 8bfa8525..753f5cca 100644
--- a/server/backend.c
+++ b/server/backend.c
@@ -221,15 +221,13 @@ backend_finalize (struct backend *b)
   if (h->state & HANDLE_FAILED)
     return -1;

-  if (h->handle) {
-    assert (h->state & HANDLE_CONNECTED);
+  assert (h->state & HANDLE_OPEN);
+  if (h->state & HANDLE_CONNECTED) {
     if (b->finalize (b, h->handle) == -1) {
       h->state |= HANDLE_FAILED;
       return -1;
     }
   }
-  else
-    assert (! (h->state & HANDLE_CONNECTED));

   if (b->i)
     return backend_finalize (b->next);
-- 
2.24.1

On 2/11/20 7:24 PM, Eric Blake wrote:
> If .prepare fails, we do not want to call .finalize (similar to how if
> .open fails, we do not want to call .close).  However, the logic in
> backend_finalize was checking on the wrong condition ('was the
> connection opened' rather than 'was the connection prepared'),
which
> led to an assertion failure.
> 
> Simple reproducer:
> 
> $ nbdkit -U - -f --filter cache --run 'qemu-io -r -c quit $nbd' sh
- <<\EOF
>> case $1 in get_size) echo oops >&2; exit 1 ;; *) exit 2 ;; esac
>> EOF
> nbdkit: sh[1]: error: /tmp/nbdkitshYvAQbz/inline-script.sh: oops
> nbdkit: backend.c:206: backend_finalize: Assertion `h->state &
HANDLE_CONNECTED' failed.
> qemu-io: can't open device nbd:unix:/tmp/nbdkit60FUTw/socket: Failed to
read option reply: Unexpected end-of-file before all bytes were read
> nbdkit: nbdkit command was killed by signal 6
> 
> With this patch, the command now fails gracefully:
> nbdkit: sh[1]: error: /tmp/nbdkitshie18Lp/inline-script.sh: oops
> qemu-io: can't open device nbd:unix:/tmp/nbdkitqvAOlr/socket: Requested
export not available
> 
> Fixes: ffa98c8d
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> 
> Pushing this to master and stable-1.16
I need to quit pushing things before letting 'make check' finish; this 
change now makes test-reopen-retry-fail.sh hang with a different assertion:
> 
>   server/backend.c | 6 ++----
>   1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/server/backend.c b/server/backend.c
> index 8bfa8525..753f5cca 100644
> --- a/server/backend.c
> +++ b/server/backend.c
> @@ -221,15 +221,13 @@ backend_finalize (struct backend *b)
>     if (h->state & HANDLE_FAILED)
>       return -1;
> 
> -  if (h->handle) {
> -    assert (h->state & HANDLE_CONNECTED);
> +  assert (h->state & HANDLE_OPEN);
this code is not always true.  I'm pushing the fixup as well.
> +  if (h->state & HANDLE_CONNECTED) {
>       if (b->finalize (b, h->handle) == -1) {
>         h->state |= HANDLE_FAILED;
>         return -1;
>       }
>     }
> -  else
> -    assert (! (h->state & HANDLE_CONNECTED));
> 
>     if (b->i)
>       return backend_finalize (b->next);
> 
-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On Tue, Feb 11, 2020 at 07:24:43PM -0600, Eric Blake
wrote:
> If .prepare fails, we do not want to call .finalize (similar to how if
> .open fails, we do not want to call .close).  However, the logic in
> backend_finalize was checking on the wrong condition ('was the
> connection opened' rather than 'was the connection prepared'),
which
> led to an assertion failure.
> 
> Simple reproducer:
> 
> $ nbdkit -U - -f --filter cache --run 'qemu-io -r -c quit $nbd' sh
- <<\EOF
> > case $1 in get_size) echo oops >&2; exit 1 ;; *) exit 2 ;; esac
> > EOF
> nbdkit: sh[1]: error: /tmp/nbdkitshYvAQbz/inline-script.sh: oops
> nbdkit: backend.c:206: backend_finalize: Assertion `h->state &
HANDLE_CONNECTED' failed.
> qemu-io: can't open device nbd:unix:/tmp/nbdkit60FUTw/socket: Failed to
read option reply: Unexpected end-of-file before all bytes were read
> nbdkit: nbdkit command was killed by signal 6
> 
> With this patch, the command now fails gracefully:
> nbdkit: sh[1]: error: /tmp/nbdkitshie18Lp/inline-script.sh: oops
> qemu-io: can't open device nbd:unix:/tmp/nbdkitqvAOlr/socket: Requested
export not available
> 
> Fixes: ffa98c8d
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> 
> Pushing this to master and stable-1.16
> 
>  server/backend.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/server/backend.c b/server/backend.c
> index 8bfa8525..753f5cca 100644
> --- a/server/backend.c
> +++ b/server/backend.c
> @@ -221,15 +221,13 @@ backend_finalize (struct backend *b)
>    if (h->state & HANDLE_FAILED)
>      return -1;
> 
> -  if (h->handle) {
> -    assert (h->state & HANDLE_CONNECTED);
> +  assert (h->state & HANDLE_OPEN);
> +  if (h->state & HANDLE_CONNECTED) {
>      if (b->finalize (b, h->handle) == -1) {
>        h->state |= HANDLE_FAILED;
>        return -1;
>      }
>    }
> -  else
> -    assert (! (h->state & HANDLE_CONNECTED));
> 
>    if (b->i)
>      return backend_finalize (b->next);
ACK

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