mbox series

[v2,0/5] Exposing backing-chain allocation over NBD

Message ID 20200930121105.667049-1-eblake@redhat.com
Headers show
Series Exposing backing-chain allocation over NBD | expand

Message

Eric Blake Sept. 30, 2020, 12:11 p.m. UTC 
v1 was here: https://lists.gnu.org/archive/html/qemu-devel/2020-09/msg09623.htm
l

Based-on: <20200924152717.287415-1-kwolf@redhat.com>
(block/export: Add infrastructure and QAPI for block exports)

Also available at:
https://repo.or.cz/qemu/ericb.git/shortlog/refs/tags/nbd-alloc-depth-v2

Since then:
- rebase on Kevin's work
- add new patch to fix qemu-nbd SIGINT (conflicts with Stefan's work,
we can either rebase his on mine or drop mine if his goes in first)
- split out fix for handling NUL bytes from client [Vladimir]
- further cleanups of query parsing [Vladimir]
- more documentation of how we could also expose actual depth in
remaining bits of the context reply [Rich]

001/5:[down] 'qemu-nbd: Honor SIGINT and SIGHUP'
002/5:[down] 'nbd/server: Reject embedded NUL in NBD strings'
003/5:[0139] [FC] 'nbd: Simplify meta-context parsing'
004/5:[0035] [FC] 'nbd: Add new qemu:allocation-depth metacontext'
005/5:[0038] [FC] 'nbd: Add 'qemu-nbd -A' to expose allocation depth'

Eric Blake (5):
  qemu-nbd: Honor SIGINT and SIGHUP
  nbd/server: Reject embedded NUL in NBD strings
  nbd: Simplify meta-context parsing
  nbd: Add new qemu:allocation-depth metacontext
  nbd: Add 'qemu-nbd -A' to expose allocation depth

 docs/interop/nbd.txt       |  22 ++-
 docs/tools/qemu-nbd.rst    |   6 +
 qapi/block-core.json       |   7 +-
 qapi/block-export.json     |   6 +-
 include/block/nbd.h        |   8 +-
 blockdev-nbd.c             |   2 +
 nbd/server.c               | 324 +++++++++++++++++++++----------------
 qemu-nbd.c                 |  20 ++-
 tests/qemu-iotests/309     |  73 +++++++++
 tests/qemu-iotests/309.out |  22 +++
 tests/qemu-iotests/group   |   1 +
 11 files changed, 342 insertions(+), 149 deletions(-)
 create mode 100755 tests/qemu-iotests/309
 create mode 100644 tests/qemu-iotests/309.out

Comments

Vladimir Sementsov-Ogievskiy Oct. 7, 2020, 10:32 a.m. UTC | #1 
30.09.2020 15:11, Eric Blake wrote:
> Honoring just SIGTERM on Linux is too weak; we also want to handle
> other common signals, and do so even on BSD.  Why?  Because at least
> 'qemu-nbd -B bitmap' needs a chance to clean up the in-use bit on
> bitmaps when the server is shut down via a signal.

Probably not bad to update a comment [*] if you have a good wording in mind.

> 
> See also: http://bugzilla.redhat.com/1883608
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

> ---
>   qemu-nbd.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index bacb69b0898b..e7520261134f 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -581,7 +581,7 @@ int main(int argc, char **argv)
>       const char *pid_file_name = NULL;
>       BlockExportOptions *export_opts;
> 
> -#if HAVE_NBD_DEVICE
> +#ifdef CONFIG_POSIX
>       /* The client thread uses SIGTERM to interrupt the server.  A signal
>        * handler ensures that "qemu-nbd -v -c" exits with a nice status code.

[*]

>        */
> @@ -589,9 +589,9 @@ int main(int argc, char **argv)
>       memset(&sa_sigterm, 0, sizeof(sa_sigterm));
>       sa_sigterm.sa_handler = termsig_handler;
>       sigaction(SIGTERM, &sa_sigterm, NULL);
> -#endif /* HAVE_NBD_DEVICE */
> +    sigaction(SIGINT, &sa_sigterm, NULL);
> +    sigaction(SIGHUP, &sa_sigterm, NULL);
> 
> -#ifdef CONFIG_POSIX
>       signal(SIGPIPE, SIG_IGN);
>   #endif
>
Eric Blake Oct. 7, 2020, 9:13 p.m. UTC | #2 
On 10/7/20 5:32 AM, Vladimir Sementsov-Ogievskiy wrote:
> 30.09.2020 15:11, Eric Blake wrote:

>> Honoring just SIGTERM on Linux is too weak; we also want to handle

>> other common signals, and do so even on BSD.  Why?  Because at least

>> 'qemu-nbd -B bitmap' needs a chance to clean up the in-use bit on

>> bitmaps when the server is shut down via a signal.

> 

> Probably not bad to update a comment [*] if you have a good wording in

> mind.

> 

>>

>> See also: http://bugzilla.redhat.com/1883608

>>

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

> 

> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

> 

>> ---

>>   qemu-nbd.c | 6 +++---

>>   1 file changed, 3 insertions(+), 3 deletions(-)

>>

>> diff --git a/qemu-nbd.c b/qemu-nbd.c

>> index bacb69b0898b..e7520261134f 100644

>> --- a/qemu-nbd.c

>> +++ b/qemu-nbd.c

>> @@ -581,7 +581,7 @@ int main(int argc, char **argv)

>>       const char *pid_file_name = NULL;

>>       BlockExportOptions *export_opts;

>>

>> -#if HAVE_NBD_DEVICE

>> +#ifdef CONFIG_POSIX

>>       /* The client thread uses SIGTERM to interrupt the server.  A

>> signal

>>        * handler ensures that "qemu-nbd -v -c" exits with a nice

>> status code.

> 

> [*]

> 


Sure, I went with:

diff --git i/qemu-nbd.c w/qemu-nbd.c
index e7520261134f..c731dda04ec0 100644
--- i/qemu-nbd.c
+++ w/qemu-nbd.c
@@ -582,8 +582,9 @@ int main(int argc, char **argv)
     BlockExportOptions *export_opts;

 #ifdef CONFIG_POSIX
-    /* The client thread uses SIGTERM to interrupt the server.  A signal
-     * handler ensures that "qemu-nbd -v -c" exits with a nice status code.
+    /*
+     * Exit gracefully on various signals, which includes SIGTERM used
+     * by 'qemu-nbd -v -c'.
      */
     struct sigaction sa_sigterm;
     memset(&sa_sigterm, 0, sizeof(sa_sigterm));

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org
Vladimir Sementsov-Ogievskiy Oct. 8, 2020, 8:27 a.m. UTC | #3 
08.10.2020 00:13, Eric Blake wrote:
> On 10/7/20 5:32 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 30.09.2020 15:11, Eric Blake wrote:
>>> Honoring just SIGTERM on Linux is too weak; we also want to handle
>>> other common signals, and do so even on BSD.  Why?  Because at least
>>> 'qemu-nbd -B bitmap' needs a chance to clean up the in-use bit on
>>> bitmaps when the server is shut down via a signal.
>>
>> Probably not bad to update a comment [*] if you have a good wording in
>> mind.
>>
>>>
>>> See also: http://bugzilla.redhat.com/1883608
>>>
>>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>
>>> ---
>>>    qemu-nbd.c | 6 +++---
>>>    1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/qemu-nbd.c b/qemu-nbd.c
>>> index bacb69b0898b..e7520261134f 100644
>>> --- a/qemu-nbd.c
>>> +++ b/qemu-nbd.c
>>> @@ -581,7 +581,7 @@ int main(int argc, char **argv)
>>>        const char *pid_file_name = NULL;
>>>        BlockExportOptions *export_opts;
>>>
>>> -#if HAVE_NBD_DEVICE
>>> +#ifdef CONFIG_POSIX
>>>        /* The client thread uses SIGTERM to interrupt the server.  A
>>> signal
>>>         * handler ensures that "qemu-nbd -v -c" exits with a nice
>>> status code.
>>
>> [*]
>>
> 
> Sure, I went with:
> 
> diff --git i/qemu-nbd.c w/qemu-nbd.c
> index e7520261134f..c731dda04ec0 100644
> --- i/qemu-nbd.c
> +++ w/qemu-nbd.c
> @@ -582,8 +582,9 @@ int main(int argc, char **argv)
>       BlockExportOptions *export_opts;
> 
>   #ifdef CONFIG_POSIX
> -    /* The client thread uses SIGTERM to interrupt the server.  A signal
> -     * handler ensures that "qemu-nbd -v -c" exits with a nice status code.
> +    /*
> +     * Exit gracefully on various signals, which includes SIGTERM used
> +     * by 'qemu-nbd -v -c'.
>        */
>       struct sigaction sa_sigterm;
>       memset(&sa_sigterm, 0, sizeof(sa_sigterm));
> 

Thanks!