Libguestfs - Mar 2020 - Re: [nbdkit PATCH 2/4] nbd: Normalize return values of can_*

On Mon, Mar 16, 2020 at 10:36:15PM -0500, Eric Blake
wrote:
> Although nbdkit documents that any positive value should be treated as
> success to the .can_* callbacks, we had a window of releases where
> anything other than 1 could trigger an assertion failure, fixed in the
> previous patch.  Update what we return to avoid tripping the bug in
> broken nbdkit.
> 
> Our return value has been latent since 0e8e8eb11d (v1.1.17), but the
> assertion failures did not occur until c306fa93ab and neighboring
> commits (v1.15.1).  As v1.13.6 was the point where we started
> preferring builds against libnbd, where we always returned 1 on
> success, the problem was not detected until now; but it is still in
> the wild for any user mixing old plugins with new libnbd.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  plugins/nbd/nbd-standalone.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/plugins/nbd/nbd-standalone.c b/plugins/nbd/nbd-standalone.c
> index cbf9dae7..4230ecd9 100644
> --- a/plugins/nbd/nbd-standalone.c
> +++ b/plugins/nbd/nbd-standalone.c
> @@ -1,5 +1,5 @@
>  /* nbdkit
> - * Copyright (C) 2017-2019 Red Hat Inc.
> + * Copyright (C) 2017-2020 Red Hat Inc.
>   *
>   * Redistribution and use in source and binary forms, with or without
>   * modification, are permitted provided that the following conditions are
> @@ -1175,7 +1175,7 @@ nbd_can_flush (void *handle)
>  {
>    struct handle *h = handle;
> 
> -  return h->flags & NBD_FLAG_SEND_FLUSH;
> +  return !!(h->flags & NBD_FLAG_SEND_FLUSH);
>  }
> 
>  static int
> @@ -1183,7 +1183,7 @@ nbd_is_rotational (void *handle)
>  {
>    struct handle *h = handle;
> 
> -  return h->flags & NBD_FLAG_ROTATIONAL;
> +  return !!(h->flags & NBD_FLAG_ROTATIONAL);
>  }
> 
>  static int
> @@ -1191,7 +1191,7 @@ nbd_can_trim (void *handle)
>  {
>    struct handle *h = handle;
> 
> -  return h->flags & NBD_FLAG_SEND_TRIM;
> +  return !!(h->flags & NBD_FLAG_SEND_TRIM);
>  }
> 
>  static int
> @@ -1199,7 +1199,7 @@ nbd_can_zero (void *handle)
>  {
>    struct handle *h = handle;
> 
> -  return h->flags & NBD_FLAG_SEND_WRITE_ZEROES;
> +  return !!(h->flags & NBD_FLAG_SEND_WRITE_ZEROES);
>  }
> 
>  static int
> @@ -1215,7 +1215,7 @@ nbd_can_multi_conn (void *handle)
>  {
>    struct handle *h = handle;
> 
> -  return h->flags & NBD_FLAG_CAN_MULTI_CONN;
> +  return !!(h->flags & NBD_FLAG_CAN_MULTI_CONN);
>  }
> 
>  static int
While I don't like the idea of modifying plugins to fix broken nbdkit
behaviour, you make a good case for this being safer, so:

ACK

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

On 3/17/20 3:12 AM, Richard W.M. Jones wrote:
> On Mon, Mar 16, 2020 at 10:36:15PM -0500, Eric Blake wrote:
>> Although nbdkit documents that any positive value should be treated as
>> success to the .can_* callbacks, we had a window of releases where
>> anything other than 1 could trigger an assertion failure, fixed in the
>> previous patch.  Update what we return to avoid tripping the bug in
>> broken nbdkit.
>>
>> Our return value has been latent since 0e8e8eb11d (v1.1.17), but the
>> assertion failures did not occur until c306fa93ab and neighboring
>> commits (v1.15.1).  As v1.13.6 was the point where we started
>> preferring builds against libnbd, where we always returned 1 on
>> success, the problem was not detected until now; but it is still in
>> the wild for any user mixing old plugins with new libnbd.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---
>>   plugins/nbd/nbd-standalone.c | 12 ++++++------
>>   1 file changed, 6 insertions(+), 6 deletions(-)
> While I don't like the idea of modifying plugins to fix broken nbdkit
> behaviour, you make a good case for this being safer, so:
> 
> ACK
Think of it as working around broken nbdkit behavior, rather than fixing 
it (as the fix is in patch 1/4).  How many stable branches should we 
backport this to?  Going all the way back to stable-1.2 seems like it 
might be overkill; maybe back to stable-1.14 (where nbd-standalone was 
first split off) or stable-1.12 (the last release that did not use 
libnbd) is sufficient?

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

On Tue, Mar 17, 2020 at 06:59:09AM -0500, Eric Blake
wrote:
> On 3/17/20 3:12 AM, Richard W.M. Jones wrote:
> >On Mon, Mar 16, 2020 at 10:36:15PM -0500, Eric Blake wrote:
> >>Although nbdkit documents that any positive value should be treated
as
> >>success to the .can_* callbacks, we had a window of releases where
> >>anything other than 1 could trigger an assertion failure, fixed in
the
> >>previous patch.  Update what we return to avoid tripping the bug in
> >>broken nbdkit.
> >>
> >>Our return value has been latent since 0e8e8eb11d (v1.1.17), but
the
> >>assertion failures did not occur until c306fa93ab and neighboring
> >>commits (v1.15.1).  As v1.13.6 was the point where we started
> >>preferring builds against libnbd, where we always returned 1 on
> >>success, the problem was not detected until now; but it is still in
> >>the wild for any user mixing old plugins with new libnbd.
> >>
> >>Signed-off-by: Eric Blake <eblake@redhat.com>
> >>---
> >>  plugins/nbd/nbd-standalone.c | 12 ++++++------
> >>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> >While I don't like the idea of modifying plugins to fix broken
nbdkit
> >behaviour, you make a good case for this being safer, so:
> >
> >ACK
> 
> Think of it as working around broken nbdkit behavior, rather than
> fixing it (as the fix is in patch 1/4).  How many stable branches
> should we backport this to?  Going all the way back to stable-1.2
> seems like it might be overkill; maybe back to stable-1.14 (where
> nbd-standalone was first split off) or stable-1.12 (the last release
> that did not use libnbd) is sufficient?
I'm routinely backporting only to 1.16 and 1.18.

And then backporting to other branches only as-needed.  For example I
backported something to 1.4 yesterday, but that was only because I was
trying to check for a bug which had been reported against 1.4 and I
needed the backport to make the branch compile on a modern distro.

By the way I'm happy to do these cherry picks (just for 1.16 and 1.18)
because I find it helps me to keep the commits in their original order
so I know which ones have been applied and which ones have been
skipped.  Unless there is tooling which does this, which I've
occasionally looked for but never found.

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