[PATCH] docs: show how to spawn qemu-storage-daemon with fd passing

[PATCH] docs: show how to spawn qemu-storage-daemon with fd passing

[Stefan Hajnoczi]

The QMP monitor, NBD server, and vhost-user-blk export all support file
descriptor passing. This is a useful technique because it allows the
parent process to spawn and wait for qemu-storage-daemon without busy
waiting, which may delay startup due to arbitrary sleep() calls.

This Python example is inspired by the test case written for libnbd by
Richard W.M. Jones <rjones@redhat.com>:
https://gitlab.com/nbdkit/libnbd/-/commit/89113f484effb0e6c322314ba75c1cbe07a04543

Thanks to Daniel P. Berrangé <berrange@redhat.com> for suggestions on
how to get this working. Now let's document it!

Reported-by: Richard W.M. Jones <rjones@redhat.com>
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 docs/tools/qemu-storage-daemon.rst | 38 ++++++++++++++++++++++++++++--
 1 file changed, 36 insertions(+), 2 deletions(-)

diff --git a/docs/tools/qemu-storage-daemon.rst b/docs/tools/qemu-storage-daemon.rst
index f63627eaf6..45854c131e 100644
--- a/docs/tools/qemu-storage-daemon.rst
+++ b/docs/tools/qemu-storage-daemon.rst
@@ -101,10 +101,12 @@ Standard options:
 
 .. option:: --nbd-server addr.type=inet,addr.host=<host>,addr.port=<port>[,tls-creds=<id>][,tls-authz=<id>][,max-connections=<n>]
   --nbd-server addr.type=unix,addr.path=<path>[,tls-creds=<id>][,tls-authz=<id>][,max-connections=<n>]
+  --nbd-server addr.type=fd,addr.str=<fd>[,tls-creds=<id>][,tls-authz=<id>][,max-connections=<n>]
 
   is a server for NBD exports. Both TCP and UNIX domain sockets are supported.
-  TLS encryption can be configured using ``--object`` tls-creds-* and authz-*
-  secrets (see below).
+  A listen socket can be provided via file descriptor passing (see Examples
+  below). TLS encryption can be configured using ``--object`` tls-creds-* and
+  authz-* secrets (see below).
 
   To configure an NBD server on UNIX domain socket path ``/tmp/nbd.sock``::
 
@@ -127,6 +129,38 @@ QMP commands::
       --chardev socket,path=qmp.sock,server,nowait,id=char1 \
       --monitor chardev=char1
 
+Launch the daemon from Python with a QMP monitor socket using file descriptor
+passing so there is no need to busy wait for the QMP monitor to become
+available::
+
+  #!/usr/bin/env python3
+  import os
+  import subprocess
+  import socket
+
+  sock_path = '/tmp/qmp-{}.sock'.format(os.getpid())
+
+  with socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) as listen_sock:
+      listen_sock.bind(sock_path)
+      listen_sock.listen()
+
+      fd = listen_sock.fileno()
+
+      subprocess.Popen(
+          ['qemu-storage-daemon',
+           '--chardev', f'socket,fd={fd},server=on,id=char1',
+           '--monitor', 'chardev=char1'],
+          pass_fds=[fd],
+      )
+
+  qmp_sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
+  qmp_sock.connect(sock_path)
+  ...QMP interaction...
+
+The same socket spawning approach also works with the ``--nbd-server
+addr.type=fd,addr.str=<fd>`` and ``--export
+type=vhost-user-blk,addr.type=fd,addr.str=<fd>`` options.
+
 Export raw image file ``disk.img`` over NBD UNIX domain socket ``nbd.sock``::
 
   $ qemu-storage-daemon \
-- 
2.29.2

Re: [PATCH] docs: show how to spawn qemu-storage-daemon with fd passing

[Richard W.M. Jones]

On Mon, Mar 01, 2021 at 03:31:59PM +0000, Stefan Hajnoczi wrote:
> The QMP monitor, NBD server, and vhost-user-blk export all support file
> descriptor passing. This is a useful technique because it allows the
> parent process to spawn and wait for qemu-storage-daemon without busy
> waiting, which may delay startup due to arbitrary sleep() calls.
> 
> This Python example is inspired by the test case written for libnbd by
> Richard W.M. Jones <rjones@redhat.com>:
> https://gitlab.com/nbdkit/libnbd/-/commit/89113f484effb0e6c322314ba75c1cbe07a04543
> 
> Thanks to Daniel P. Berrangé <berrange@redhat.com> for suggestions on
> how to get this working. Now let's document it!
> 
> Reported-by: Richard W.M. Jones <rjones@redhat.com>
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  docs/tools/qemu-storage-daemon.rst | 38 ++++++++++++++++++++++++++++--
>  1 file changed, 36 insertions(+), 2 deletions(-)
> 
> diff --git a/docs/tools/qemu-storage-daemon.rst b/docs/tools/qemu-storage-daemon.rst
> index f63627eaf6..45854c131e 100644
> --- a/docs/tools/qemu-storage-daemon.rst
> +++ b/docs/tools/qemu-storage-daemon.rst
> @@ -101,10 +101,12 @@ Standard options:
>  
>  .. option:: --nbd-server addr.type=inet,addr.host=<host>,addr.port=<port>[,tls-creds=<id>][,tls-authz=<id>][,max-connections=<n>]
>    --nbd-server addr.type=unix,addr.path=<path>[,tls-creds=<id>][,tls-authz=<id>][,max-connections=<n>]
> +  --nbd-server addr.type=fd,addr.str=<fd>[,tls-creds=<id>][,tls-authz=<id>][,max-connections=<n>]
>  
>    is a server for NBD exports. Both TCP and UNIX domain sockets are supported.
> -  TLS encryption can be configured using ``--object`` tls-creds-* and authz-*
> -  secrets (see below).
> +  A listen socket can be provided via file descriptor passing (see Examples
> +  below). TLS encryption can be configured using ``--object`` tls-creds-* and
> +  authz-* secrets (see below).
>  
>    To configure an NBD server on UNIX domain socket path ``/tmp/nbd.sock``::
>  
> @@ -127,6 +129,38 @@ QMP commands::
>        --chardev socket,path=qmp.sock,server,nowait,id=char1 \
>        --monitor chardev=char1
>  
> +Launch the daemon from Python with a QMP monitor socket using file descriptor
> +passing so there is no need to busy wait for the QMP monitor to become
> +available::
> +
> +  #!/usr/bin/env python3
> +  import os
> +  import subprocess
> +  import socket
> +
> +  sock_path = '/tmp/qmp-{}.sock'.format(os.getpid())

Not sure how much you worry about the insecure / easily guessable tmp
path here.  I notice that there's already one in the surrounding
documentation (/tmp/nbd.sock) so maybe it's not a problem :-)

> +  with socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) as listen_sock:
> +      listen_sock.bind(sock_path)
> +      listen_sock.listen()
> +
> +      fd = listen_sock.fileno()
> +
> +      subprocess.Popen(
> +          ['qemu-storage-daemon',
> +           '--chardev', f'socket,fd={fd},server=on,id=char1',
> +           '--monitor', 'chardev=char1'],
> +          pass_fds=[fd],
> +      )
> +
> +  qmp_sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
> +  qmp_sock.connect(sock_path)

A note that the order of opening the sockets is slightly different
from how I did it in the interop test.  But I believe it makes no
difference, as long as you don't connect to the socket until it's in
the listening state, which is what you're doing here.  So it should be
fine.

> +  ...QMP interaction...
> +
> +The same socket spawning approach also works with the ``--nbd-server
> +addr.type=fd,addr.str=<fd>`` and ``--export
> +type=vhost-user-blk,addr.type=fd,addr.str=<fd>`` options.
> +
>  Export raw image file ``disk.img`` over NBD UNIX domain socket ``nbd.sock``::

The patch looks fine in general:

Reviewed-by: Richard W.M. Jones <rjones@redhat.com>

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

Re: [PATCH] docs: show how to spawn qemu-storage-daemon with fd passing

[Daniel P. Berrangé]

On Mon, Mar 01, 2021 at 03:31:59PM +0000, Stefan Hajnoczi wrote:
> The QMP monitor, NBD server, and vhost-user-blk export all support file
> descriptor passing. This is a useful technique because it allows the
> parent process to spawn and wait for qemu-storage-daemon without busy
> waiting, which may delay startup due to arbitrary sleep() calls.
> 
> This Python example is inspired by the test case written for libnbd by
> Richard W.M. Jones <rjones@redhat.com>:
> https://gitlab.com/nbdkit/libnbd/-/commit/89113f484effb0e6c322314ba75c1cbe07a04543
> 
> Thanks to Daniel P. Berrangé <berrange@redhat.com> for suggestions on
> how to get this working. Now let's document it!
> 
> Reported-by: Richard W.M. Jones <rjones@redhat.com>
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  docs/tools/qemu-storage-daemon.rst | 38 ++++++++++++++++++++++++++++--
>  1 file changed, 36 insertions(+), 2 deletions(-)
> 
> diff --git a/docs/tools/qemu-storage-daemon.rst b/docs/tools/qemu-storage-daemon.rst
> index f63627eaf6..45854c131e 100644
> --- a/docs/tools/qemu-storage-daemon.rst
> +++ b/docs/tools/qemu-storage-daemon.rst
> @@ -101,10 +101,12 @@ Standard options:
>  
>  .. option:: --nbd-server addr.type=inet,addr.host=<host>,addr.port=<port>[,tls-creds=<id>][,tls-authz=<id>][,max-connections=<n>]
>    --nbd-server addr.type=unix,addr.path=<path>[,tls-creds=<id>][,tls-authz=<id>][,max-connections=<n>]
> +  --nbd-server addr.type=fd,addr.str=<fd>[,tls-creds=<id>][,tls-authz=<id>][,max-connections=<n>]
>  
>    is a server for NBD exports. Both TCP and UNIX domain sockets are supported.
> -  TLS encryption can be configured using ``--object`` tls-creds-* and authz-*
> -  secrets (see below).
> +  A listen socket can be provided via file descriptor passing (see Examples
> +  below). TLS encryption can be configured using ``--object`` tls-creds-* and
> +  authz-* secrets (see below).
>  
>    To configure an NBD server on UNIX domain socket path ``/tmp/nbd.sock``::
>  
> @@ -127,6 +129,38 @@ QMP commands::
>        --chardev socket,path=qmp.sock,server,nowait,id=char1 \
>        --monitor chardev=char1
>  
> +Launch the daemon from Python with a QMP monitor socket using file descriptor
> +passing so there is no need to busy wait for the QMP monitor to become
> +available::
> +
> +  #!/usr/bin/env python3
> +  import os
> +  import subprocess
> +  import socket
> +
> +  sock_path = '/tmp/qmp-{}.sock'.format(os.getpid())

Example code inevitably gets cut+paste into real world apps, and this
example is a tmpfile CVE flaw. At least put it in $CWD instead.

> +
> +  with socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) as listen_sock:
> +      listen_sock.bind(sock_path)
> +      listen_sock.listen()
> +
> +      fd = listen_sock.fileno()
> +
> +      subprocess.Popen(
> +          ['qemu-storage-daemon',
> +           '--chardev', f'socket,fd={fd},server=on,id=char1',
> +           '--monitor', 'chardev=char1'],
> +          pass_fds=[fd],
> +      )
> +
> +  qmp_sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
> +  qmp_sock.connect(sock_path)
> +  ...QMP interaction...
> +
> +The same socket spawning approach also works with the ``--nbd-server
> +addr.type=fd,addr.str=<fd>`` and ``--export
> +type=vhost-user-blk,addr.type=fd,addr.str=<fd>`` options.
> +
>  Export raw image file ``disk.img`` over NBD UNIX domain socket ``nbd.sock``::
>  
>    $ qemu-storage-daemon \
> -- 
> 2.29.2
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH] docs: show how to spawn qemu-storage-daemon with fd passing

[Daniel P. Berrangé]

On Mon, Mar 01, 2021 at 03:39:06PM +0000, Richard W.M. Jones wrote:
> On Mon, Mar 01, 2021 at 03:31:59PM +0000, Stefan Hajnoczi wrote:
> > The QMP monitor, NBD server, and vhost-user-blk export all support file
> > descriptor passing. This is a useful technique because it allows the
> > parent process to spawn and wait for qemu-storage-daemon without busy
> > waiting, which may delay startup due to arbitrary sleep() calls.
> > 
> > This Python example is inspired by the test case written for libnbd by
> > Richard W.M. Jones <rjones@redhat.com>:
> > https://gitlab.com/nbdkit/libnbd/-/commit/89113f484effb0e6c322314ba75c1cbe07a04543
> > 
> > Thanks to Daniel P. Berrangé <berrange@redhat.com> for suggestions on
> > how to get this working. Now let's document it!
> > 
> > Reported-by: Richard W.M. Jones <rjones@redhat.com>
> > Cc: Kevin Wolf <kwolf@redhat.com>
> > Cc: Daniel P. Berrangé <berrange@redhat.com>
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  docs/tools/qemu-storage-daemon.rst | 38 ++++++++++++++++++++++++++++--
> >  1 file changed, 36 insertions(+), 2 deletions(-)
> > 
> > diff --git a/docs/tools/qemu-storage-daemon.rst b/docs/tools/qemu-storage-daemon.rst
> > index f63627eaf6..45854c131e 100644
> > --- a/docs/tools/qemu-storage-daemon.rst
> > +++ b/docs/tools/qemu-storage-daemon.rst
> > @@ -101,10 +101,12 @@ Standard options:
> >  
> >  .. option:: --nbd-server addr.type=inet,addr.host=<host>,addr.port=<port>[,tls-creds=<id>][,tls-authz=<id>][,max-connections=<n>]
> >    --nbd-server addr.type=unix,addr.path=<path>[,tls-creds=<id>][,tls-authz=<id>][,max-connections=<n>]
> > +  --nbd-server addr.type=fd,addr.str=<fd>[,tls-creds=<id>][,tls-authz=<id>][,max-connections=<n>]
> >  
> >    is a server for NBD exports. Both TCP and UNIX domain sockets are supported.
> > -  TLS encryption can be configured using ``--object`` tls-creds-* and authz-*
> > -  secrets (see below).
> > +  A listen socket can be provided via file descriptor passing (see Examples
> > +  below). TLS encryption can be configured using ``--object`` tls-creds-* and
> > +  authz-* secrets (see below).
> >  
> >    To configure an NBD server on UNIX domain socket path ``/tmp/nbd.sock``::
> >  
> > @@ -127,6 +129,38 @@ QMP commands::
> >        --chardev socket,path=qmp.sock,server,nowait,id=char1 \
> >        --monitor chardev=char1
> >  
> > +Launch the daemon from Python with a QMP monitor socket using file descriptor
> > +passing so there is no need to busy wait for the QMP monitor to become
> > +available::
> > +
> > +  #!/usr/bin/env python3
> > +  import os
> > +  import subprocess
> > +  import socket
> > +
> > +  sock_path = '/tmp/qmp-{}.sock'.format(os.getpid())
> 
> Not sure how much you worry about the insecure / easily guessable tmp
> path here.  I notice that there's already one in the surrounding
> documentation (/tmp/nbd.sock) so maybe it's not a problem :-)
> 
> > +  with socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) as listen_sock:
> > +      listen_sock.bind(sock_path)
> > +      listen_sock.listen()
> > +
> > +      fd = listen_sock.fileno()
> > +
> > +      subprocess.Popen(
> > +          ['qemu-storage-daemon',
> > +           '--chardev', f'socket,fd={fd},server=on,id=char1',
> > +           '--monitor', 'chardev=char1'],
> > +          pass_fds=[fd],
> > +      )
> > +
> > +  qmp_sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
> > +  qmp_sock.connect(sock_path)
> 
> A note that the order of opening the sockets is slightly different
> from how I did it in the interop test.  But I believe it makes no
> difference, as long as you don't connect to the socket until it's in
> the listening state, which is what you're doing here.  So it should be
> fine.

Nothing here is closing listen_sock in the parent though.

The trick of passing the listener FD into the child relies on the
listener being closed in the parent, so that the parent can get
a socket error if the child exits abnormally during startup. Keeping
the listen socket open means the parent will wait forever for an
accept() that never comes.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH] docs: show how to spawn qemu-storage-daemon with fd passing

[Eric Blake]

On 3/1/21 9:41 AM, Daniel P. Berrangé wrote:
> On Mon, Mar 01, 2021 at 03:31:59PM +0000, Stefan Hajnoczi wrote:
>> The QMP monitor, NBD server, and vhost-user-blk export all support file
>> descriptor passing. This is a useful technique because it allows the
>> parent process to spawn and wait for qemu-storage-daemon without busy
>> waiting, which may delay startup due to arbitrary sleep() calls.
>>
>> This Python example is inspired by the test case written for libnbd by
>> Richard W.M. Jones <rjones@redhat.com>:
>> https://gitlab.com/nbdkit/libnbd/-/commit/89113f484effb0e6c322314ba75c1cbe07a04543
>>
>> Thanks to Daniel P. Berrangé <berrange@redhat.com> for suggestions on
>> how to get this working. Now let's document it!
>>

>> +  sock_path = '/tmp/qmp-{}.sock'.format(os.getpid())
> 
> Example code inevitably gets cut+paste into real world apps, and this
> example is a tmpfile CVE flaw. At least put it in $CWD instead.

Except $CWD may be too long for a sock file name to be created.
Creating the sock in a securely-created subdirectory of /tmp is more
reliable.

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

Re: [PATCH] docs: show how to spawn qemu-storage-daemon with fd passing

[Daniel P. Berrangé]

On Mon, Mar 01, 2021 at 09:49:21AM -0600, Eric Blake wrote:
> On 3/1/21 9:41 AM, Daniel P. Berrangé wrote:
> > On Mon, Mar 01, 2021 at 03:31:59PM +0000, Stefan Hajnoczi wrote:
> >> The QMP monitor, NBD server, and vhost-user-blk export all support file
> >> descriptor passing. This is a useful technique because it allows the
> >> parent process to spawn and wait for qemu-storage-daemon without busy
> >> waiting, which may delay startup due to arbitrary sleep() calls.
> >>
> >> This Python example is inspired by the test case written for libnbd by
> >> Richard W.M. Jones <rjones@redhat.com>:
> >> https://gitlab.com/nbdkit/libnbd/-/commit/89113f484effb0e6c322314ba75c1cbe07a04543
> >>
> >> Thanks to Daniel P. Berrangé <berrange@redhat.com> for suggestions on
> >> how to get this working. Now let's document it!
> >>
> 
> >> +  sock_path = '/tmp/qmp-{}.sock'.format(os.getpid())
> > 
> > Example code inevitably gets cut+paste into real world apps, and this
> > example is a tmpfile CVE flaw. At least put it in $CWD instead.
> 
> Except $CWD may be too long for a sock file name to be created.
> Creating the sock in a securely-created subdirectory of /tmp is more
> reliable.

$XDG_RUNTIME_DIR then, which is /run/user/$UID, so safely per user on all
modern OS.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH] docs: show how to spawn qemu-storage-daemon with fd passing

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 4741 bytes --]

On Mon, Mar 01, 2021 at 03:44:42PM +0000, Daniel P. Berrangé wrote:
> On Mon, Mar 01, 2021 at 03:39:06PM +0000, Richard W.M. Jones wrote:
> > On Mon, Mar 01, 2021 at 03:31:59PM +0000, Stefan Hajnoczi wrote:
> > > The QMP monitor, NBD server, and vhost-user-blk export all support file
> > > descriptor passing. This is a useful technique because it allows the
> > > parent process to spawn and wait for qemu-storage-daemon without busy
> > > waiting, which may delay startup due to arbitrary sleep() calls.
> > > 
> > > This Python example is inspired by the test case written for libnbd by
> > > Richard W.M. Jones <rjones@redhat.com>:
> > > https://gitlab.com/nbdkit/libnbd/-/commit/89113f484effb0e6c322314ba75c1cbe07a04543
> > > 
> > > Thanks to Daniel P. Berrangé <berrange@redhat.com> for suggestions on
> > > how to get this working. Now let's document it!
> > > 
> > > Reported-by: Richard W.M. Jones <rjones@redhat.com>
> > > Cc: Kevin Wolf <kwolf@redhat.com>
> > > Cc: Daniel P. Berrangé <berrange@redhat.com>
> > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > > ---
> > >  docs/tools/qemu-storage-daemon.rst | 38 ++++++++++++++++++++++++++++--
> > >  1 file changed, 36 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/docs/tools/qemu-storage-daemon.rst b/docs/tools/qemu-storage-daemon.rst
> > > index f63627eaf6..45854c131e 100644
> > > --- a/docs/tools/qemu-storage-daemon.rst
> > > +++ b/docs/tools/qemu-storage-daemon.rst
> > > @@ -101,10 +101,12 @@ Standard options:
> > >  
> > >  .. option:: --nbd-server addr.type=inet,addr.host=<host>,addr.port=<port>[,tls-creds=<id>][,tls-authz=<id>][,max-connections=<n>]
> > >    --nbd-server addr.type=unix,addr.path=<path>[,tls-creds=<id>][,tls-authz=<id>][,max-connections=<n>]
> > > +  --nbd-server addr.type=fd,addr.str=<fd>[,tls-creds=<id>][,tls-authz=<id>][,max-connections=<n>]
> > >  
> > >    is a server for NBD exports. Both TCP and UNIX domain sockets are supported.
> > > -  TLS encryption can be configured using ``--object`` tls-creds-* and authz-*
> > > -  secrets (see below).
> > > +  A listen socket can be provided via file descriptor passing (see Examples
> > > +  below). TLS encryption can be configured using ``--object`` tls-creds-* and
> > > +  authz-* secrets (see below).
> > >  
> > >    To configure an NBD server on UNIX domain socket path ``/tmp/nbd.sock``::
> > >  
> > > @@ -127,6 +129,38 @@ QMP commands::
> > >        --chardev socket,path=qmp.sock,server,nowait,id=char1 \
> > >        --monitor chardev=char1
> > >  
> > > +Launch the daemon from Python with a QMP monitor socket using file descriptor
> > > +passing so there is no need to busy wait for the QMP monitor to become
> > > +available::
> > > +
> > > +  #!/usr/bin/env python3
> > > +  import os
> > > +  import subprocess
> > > +  import socket
> > > +
> > > +  sock_path = '/tmp/qmp-{}.sock'.format(os.getpid())
> > 
> > Not sure how much you worry about the insecure / easily guessable tmp
> > path here.  I notice that there's already one in the surrounding
> > documentation (/tmp/nbd.sock) so maybe it's not a problem :-)
> > 
> > > +  with socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) as listen_sock:
> > > +      listen_sock.bind(sock_path)
> > > +      listen_sock.listen()
> > > +
> > > +      fd = listen_sock.fileno()
> > > +
> > > +      subprocess.Popen(
> > > +          ['qemu-storage-daemon',
> > > +           '--chardev', f'socket,fd={fd},server=on,id=char1',
> > > +           '--monitor', 'chardev=char1'],
> > > +          pass_fds=[fd],
> > > +      )
> > > +
> > > +  qmp_sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
> > > +  qmp_sock.connect(sock_path)
> > 
> > A note that the order of opening the sockets is slightly different
> > from how I did it in the interop test.  But I believe it makes no
> > difference, as long as you don't connect to the socket until it's in
> > the listening state, which is what you're doing here.  So it should be
> > fine.
> 
> Nothing here is closing listen_sock in the parent though.
> 
> The trick of passing the listener FD into the child relies on the
> listener being closed in the parent, so that the parent can get
> a socket error if the child exits abnormally during startup. Keeping
> the listen socket open means the parent will wait forever for an
> accept() that never comes.

The listen socket is closed by the context manager at the end of the
'with' statement. This is the modern Python approach for resource
acquisition that also handles exceptions automatically. It's like RAII
in C++.

https://www.python.org/dev/peps/pep-0343/#specification-the-with-statement

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] docs: show how to spawn qemu-storage-daemon with fd passing

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 3195 bytes --]

On Mon, Mar 01, 2021 at 03:39:06PM +0000, Richard W.M. Jones wrote:
> On Mon, Mar 01, 2021 at 03:31:59PM +0000, Stefan Hajnoczi wrote:
> > The QMP monitor, NBD server, and vhost-user-blk export all support file
> > descriptor passing. This is a useful technique because it allows the
> > parent process to spawn and wait for qemu-storage-daemon without busy
> > waiting, which may delay startup due to arbitrary sleep() calls.
> > 
> > This Python example is inspired by the test case written for libnbd by
> > Richard W.M. Jones <rjones@redhat.com>:
> > https://gitlab.com/nbdkit/libnbd/-/commit/89113f484effb0e6c322314ba75c1cbe07a04543
> > 
> > Thanks to Daniel P. Berrangé <berrange@redhat.com> for suggestions on
> > how to get this working. Now let's document it!
> > 
> > Reported-by: Richard W.M. Jones <rjones@redhat.com>
> > Cc: Kevin Wolf <kwolf@redhat.com>
> > Cc: Daniel P. Berrangé <berrange@redhat.com>
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  docs/tools/qemu-storage-daemon.rst | 38 ++++++++++++++++++++++++++++--
> >  1 file changed, 36 insertions(+), 2 deletions(-)
> > 
> > diff --git a/docs/tools/qemu-storage-daemon.rst b/docs/tools/qemu-storage-daemon.rst
> > index f63627eaf6..45854c131e 100644
> > --- a/docs/tools/qemu-storage-daemon.rst
> > +++ b/docs/tools/qemu-storage-daemon.rst
> > @@ -101,10 +101,12 @@ Standard options:
> >  
> >  .. option:: --nbd-server addr.type=inet,addr.host=<host>,addr.port=<port>[,tls-creds=<id>][,tls-authz=<id>][,max-connections=<n>]
> >    --nbd-server addr.type=unix,addr.path=<path>[,tls-creds=<id>][,tls-authz=<id>][,max-connections=<n>]
> > +  --nbd-server addr.type=fd,addr.str=<fd>[,tls-creds=<id>][,tls-authz=<id>][,max-connections=<n>]
> >  
> >    is a server for NBD exports. Both TCP and UNIX domain sockets are supported.
> > -  TLS encryption can be configured using ``--object`` tls-creds-* and authz-*
> > -  secrets (see below).
> > +  A listen socket can be provided via file descriptor passing (see Examples
> > +  below). TLS encryption can be configured using ``--object`` tls-creds-* and
> > +  authz-* secrets (see below).
> >  
> >    To configure an NBD server on UNIX domain socket path ``/tmp/nbd.sock``::
> >  
> > @@ -127,6 +129,38 @@ QMP commands::
> >        --chardev socket,path=qmp.sock,server,nowait,id=char1 \
> >        --monitor chardev=char1
> >  
> > +Launch the daemon from Python with a QMP monitor socket using file descriptor
> > +passing so there is no need to busy wait for the QMP monitor to become
> > +available::
> > +
> > +  #!/usr/bin/env python3
> > +  import os
> > +  import subprocess
> > +  import socket
> > +
> > +  sock_path = '/tmp/qmp-{}.sock'.format(os.getpid())
> 
> Not sure how much you worry about the insecure / easily guessable tmp
> path here.  I notice that there's already one in the surrounding
> documentation (/tmp/nbd.sock) so maybe it's not a problem :-)

Yes, the documentation doesn't address those issues. If I respin I'll
change the path to something that's less likely to be a globally
writeable directory (/var/run/... where the pid files usually go).

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] docs: show how to spawn qemu-storage-daemon with fd passing

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1572 bytes --]

On Mon, Mar 01, 2021 at 04:06:47PM +0000, Daniel P. Berrangé wrote:
> On Mon, Mar 01, 2021 at 09:49:21AM -0600, Eric Blake wrote:
> > On 3/1/21 9:41 AM, Daniel P. Berrangé wrote:
> > > On Mon, Mar 01, 2021 at 03:31:59PM +0000, Stefan Hajnoczi wrote:
> > >> The QMP monitor, NBD server, and vhost-user-blk export all support file
> > >> descriptor passing. This is a useful technique because it allows the
> > >> parent process to spawn and wait for qemu-storage-daemon without busy
> > >> waiting, which may delay startup due to arbitrary sleep() calls.
> > >>
> > >> This Python example is inspired by the test case written for libnbd by
> > >> Richard W.M. Jones <rjones@redhat.com>:
> > >> https://gitlab.com/nbdkit/libnbd/-/commit/89113f484effb0e6c322314ba75c1cbe07a04543
> > >>
> > >> Thanks to Daniel P. Berrangé <berrange@redhat.com> for suggestions on
> > >> how to get this working. Now let's document it!
> > >>
> > 
> > >> +  sock_path = '/tmp/qmp-{}.sock'.format(os.getpid())
> > > 
> > > Example code inevitably gets cut+paste into real world apps, and this
> > > example is a tmpfile CVE flaw. At least put it in $CWD instead.
> > 
> > Except $CWD may be too long for a sock file name to be created.
> > Creating the sock in a securely-created subdirectory of /tmp is more
> > reliable.
> 
> $XDG_RUNTIME_DIR then, which is /run/user/$UID, so safely per user on all
> modern OS.

Both you and Rich mentioned this. I'll change it to /var/run/qsd.pid.

I'll also update the /tmp/nbd.sock example in the documentation.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] docs: show how to spawn qemu-storage-daemon with fd passing

[Daniel P. Berrangé]

On Mon, Mar 01, 2021 at 04:50:14PM +0000, Stefan Hajnoczi wrote:
> On Mon, Mar 01, 2021 at 03:44:42PM +0000, Daniel P. Berrangé wrote:
> > On Mon, Mar 01, 2021 at 03:39:06PM +0000, Richard W.M. Jones wrote:
> > > On Mon, Mar 01, 2021 at 03:31:59PM +0000, Stefan Hajnoczi wrote:
> > > > The QMP monitor, NBD server, and vhost-user-blk export all support file
> > > > descriptor passing. This is a useful technique because it allows the
> > > > parent process to spawn and wait for qemu-storage-daemon without busy
> > > > waiting, which may delay startup due to arbitrary sleep() calls.
> > > > 
> > > > This Python example is inspired by the test case written for libnbd by
> > > > Richard W.M. Jones <rjones@redhat.com>:
> > > > https://gitlab.com/nbdkit/libnbd/-/commit/89113f484effb0e6c322314ba75c1cbe07a04543
> > > > 
> > > > Thanks to Daniel P. Berrangé <berrange@redhat.com> for suggestions on
> > > > how to get this working. Now let's document it!
> > > > 
> > > > Reported-by: Richard W.M. Jones <rjones@redhat.com>
> > > > Cc: Kevin Wolf <kwolf@redhat.com>
> > > > Cc: Daniel P. Berrangé <berrange@redhat.com>
> > > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > > > ---
> > > >  docs/tools/qemu-storage-daemon.rst | 38 ++++++++++++++++++++++++++++--
> > > >  1 file changed, 36 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/docs/tools/qemu-storage-daemon.rst b/docs/tools/qemu-storage-daemon.rst
> > > > index f63627eaf6..45854c131e 100644
> > > > --- a/docs/tools/qemu-storage-daemon.rst
> > > > +++ b/docs/tools/qemu-storage-daemon.rst
> > > > @@ -101,10 +101,12 @@ Standard options:
> > > >  
> > > >  .. option:: --nbd-server addr.type=inet,addr.host=<host>,addr.port=<port>[,tls-creds=<id>][,tls-authz=<id>][,max-connections=<n>]
> > > >    --nbd-server addr.type=unix,addr.path=<path>[,tls-creds=<id>][,tls-authz=<id>][,max-connections=<n>]
> > > > +  --nbd-server addr.type=fd,addr.str=<fd>[,tls-creds=<id>][,tls-authz=<id>][,max-connections=<n>]
> > > >  
> > > >    is a server for NBD exports. Both TCP and UNIX domain sockets are supported.
> > > > -  TLS encryption can be configured using ``--object`` tls-creds-* and authz-*
> > > > -  secrets (see below).
> > > > +  A listen socket can be provided via file descriptor passing (see Examples
> > > > +  below). TLS encryption can be configured using ``--object`` tls-creds-* and
> > > > +  authz-* secrets (see below).
> > > >  
> > > >    To configure an NBD server on UNIX domain socket path ``/tmp/nbd.sock``::
> > > >  
> > > > @@ -127,6 +129,38 @@ QMP commands::
> > > >        --chardev socket,path=qmp.sock,server,nowait,id=char1 \
> > > >        --monitor chardev=char1
> > > >  
> > > > +Launch the daemon from Python with a QMP monitor socket using file descriptor
> > > > +passing so there is no need to busy wait for the QMP monitor to become
> > > > +available::
> > > > +
> > > > +  #!/usr/bin/env python3
> > > > +  import os
> > > > +  import subprocess
> > > > +  import socket
> > > > +
> > > > +  sock_path = '/tmp/qmp-{}.sock'.format(os.getpid())
> > > 
> > > Not sure how much you worry about the insecure / easily guessable tmp
> > > path here.  I notice that there's already one in the surrounding
> > > documentation (/tmp/nbd.sock) so maybe it's not a problem :-)
> > > 
> > > > +  with socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) as listen_sock:
> > > > +      listen_sock.bind(sock_path)
> > > > +      listen_sock.listen()
> > > > +
> > > > +      fd = listen_sock.fileno()
> > > > +
> > > > +      subprocess.Popen(
> > > > +          ['qemu-storage-daemon',
> > > > +           '--chardev', f'socket,fd={fd},server=on,id=char1',
> > > > +           '--monitor', 'chardev=char1'],
> > > > +          pass_fds=[fd],
> > > > +      )
> > > > +
> > > > +  qmp_sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
> > > > +  qmp_sock.connect(sock_path)
> > > 
> > > A note that the order of opening the sockets is slightly different
> > > from how I did it in the interop test.  But I believe it makes no
> > > difference, as long as you don't connect to the socket until it's in
> > > the listening state, which is what you're doing here.  So it should be
> > > fine.
> > 
> > Nothing here is closing listen_sock in the parent though.
> > 
> > The trick of passing the listener FD into the child relies on the
> > listener being closed in the parent, so that the parent can get
> > a socket error if the child exits abnormally during startup. Keeping
> > the listen socket open means the parent will wait forever for an
> > accept() that never comes.
> 
> The listen socket is closed by the context manager at the end of the
> 'with' statement. This is the modern Python approach for resource
> acquisition that also handles exceptions automatically. It's like RAII
> in C++.

Hmm, yes, I didn't remember that at first. I'm not sure that is a good
idea as an example code, because people mapping this example into other
languages are likely to miss that critical detail.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH] docs: show how to spawn qemu-storage-daemon with fd passing

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 5460 bytes --]

On Mon, Mar 01, 2021 at 04:56:17PM +0000, Daniel P. Berrangé wrote:
> On Mon, Mar 01, 2021 at 04:50:14PM +0000, Stefan Hajnoczi wrote:
> > On Mon, Mar 01, 2021 at 03:44:42PM +0000, Daniel P. Berrangé wrote:
> > > On Mon, Mar 01, 2021 at 03:39:06PM +0000, Richard W.M. Jones wrote:
> > > > On Mon, Mar 01, 2021 at 03:31:59PM +0000, Stefan Hajnoczi wrote:
> > > > > The QMP monitor, NBD server, and vhost-user-blk export all support file
> > > > > descriptor passing. This is a useful technique because it allows the
> > > > > parent process to spawn and wait for qemu-storage-daemon without busy
> > > > > waiting, which may delay startup due to arbitrary sleep() calls.
> > > > > 
> > > > > This Python example is inspired by the test case written for libnbd by
> > > > > Richard W.M. Jones <rjones@redhat.com>:
> > > > > https://gitlab.com/nbdkit/libnbd/-/commit/89113f484effb0e6c322314ba75c1cbe07a04543
> > > > > 
> > > > > Thanks to Daniel P. Berrangé <berrange@redhat.com> for suggestions on
> > > > > how to get this working. Now let's document it!
> > > > > 
> > > > > Reported-by: Richard W.M. Jones <rjones@redhat.com>
> > > > > Cc: Kevin Wolf <kwolf@redhat.com>
> > > > > Cc: Daniel P. Berrangé <berrange@redhat.com>
> > > > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > > > > ---
> > > > >  docs/tools/qemu-storage-daemon.rst | 38 ++++++++++++++++++++++++++++--
> > > > >  1 file changed, 36 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/docs/tools/qemu-storage-daemon.rst b/docs/tools/qemu-storage-daemon.rst
> > > > > index f63627eaf6..45854c131e 100644
> > > > > --- a/docs/tools/qemu-storage-daemon.rst
> > > > > +++ b/docs/tools/qemu-storage-daemon.rst
> > > > > @@ -101,10 +101,12 @@ Standard options:
> > > > >  
> > > > >  .. option:: --nbd-server addr.type=inet,addr.host=<host>,addr.port=<port>[,tls-creds=<id>][,tls-authz=<id>][,max-connections=<n>]
> > > > >    --nbd-server addr.type=unix,addr.path=<path>[,tls-creds=<id>][,tls-authz=<id>][,max-connections=<n>]
> > > > > +  --nbd-server addr.type=fd,addr.str=<fd>[,tls-creds=<id>][,tls-authz=<id>][,max-connections=<n>]
> > > > >  
> > > > >    is a server for NBD exports. Both TCP and UNIX domain sockets are supported.
> > > > > -  TLS encryption can be configured using ``--object`` tls-creds-* and authz-*
> > > > > -  secrets (see below).
> > > > > +  A listen socket can be provided via file descriptor passing (see Examples
> > > > > +  below). TLS encryption can be configured using ``--object`` tls-creds-* and
> > > > > +  authz-* secrets (see below).
> > > > >  
> > > > >    To configure an NBD server on UNIX domain socket path ``/tmp/nbd.sock``::
> > > > >  
> > > > > @@ -127,6 +129,38 @@ QMP commands::
> > > > >        --chardev socket,path=qmp.sock,server,nowait,id=char1 \
> > > > >        --monitor chardev=char1
> > > > >  
> > > > > +Launch the daemon from Python with a QMP monitor socket using file descriptor
> > > > > +passing so there is no need to busy wait for the QMP monitor to become
> > > > > +available::
> > > > > +
> > > > > +  #!/usr/bin/env python3
> > > > > +  import os
> > > > > +  import subprocess
> > > > > +  import socket
> > > > > +
> > > > > +  sock_path = '/tmp/qmp-{}.sock'.format(os.getpid())
> > > > 
> > > > Not sure how much you worry about the insecure / easily guessable tmp
> > > > path here.  I notice that there's already one in the surrounding
> > > > documentation (/tmp/nbd.sock) so maybe it's not a problem :-)
> > > > 
> > > > > +  with socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) as listen_sock:
> > > > > +      listen_sock.bind(sock_path)
> > > > > +      listen_sock.listen()
> > > > > +
> > > > > +      fd = listen_sock.fileno()
> > > > > +
> > > > > +      subprocess.Popen(
> > > > > +          ['qemu-storage-daemon',
> > > > > +           '--chardev', f'socket,fd={fd},server=on,id=char1',
> > > > > +           '--monitor', 'chardev=char1'],
> > > > > +          pass_fds=[fd],
> > > > > +      )
> > > > > +
> > > > > +  qmp_sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
> > > > > +  qmp_sock.connect(sock_path)
> > > > 
> > > > A note that the order of opening the sockets is slightly different
> > > > from how I did it in the interop test.  But I believe it makes no
> > > > difference, as long as you don't connect to the socket until it's in
> > > > the listening state, which is what you're doing here.  So it should be
> > > > fine.
> > > 
> > > Nothing here is closing listen_sock in the parent though.
> > > 
> > > The trick of passing the listener FD into the child relies on the
> > > listener being closed in the parent, so that the parent can get
> > > a socket error if the child exits abnormally during startup. Keeping
> > > the listen socket open means the parent will wait forever for an
> > > accept() that never comes.
> > 
> > The listen socket is closed by the context manager at the end of the
> > 'with' statement. This is the modern Python approach for resource
> > acquisition that also handles exceptions automatically. It's like RAII
> > in C++.
> 
> Hmm, yes, I didn't remember that at first. I'm not sure that is a good
> idea as an example code, because people mapping this example into other
> languages are likely to miss that critical detail.

Let's keep the Pythonic code but add a comment that makes this clear.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] docs: show how to spawn qemu-storage-daemon with fd passing

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Mon, Mar 01, 2021 at 09:49:21AM -0600, Eric Blake wrote:
>> On 3/1/21 9:41 AM, Daniel P. Berrangé wrote:
>> > On Mon, Mar 01, 2021 at 03:31:59PM +0000, Stefan Hajnoczi wrote:
>> >> The QMP monitor, NBD server, and vhost-user-blk export all support file
>> >> descriptor passing. This is a useful technique because it allows the
>> >> parent process to spawn and wait for qemu-storage-daemon without busy
>> >> waiting, which may delay startup due to arbitrary sleep() calls.
>> >>
>> >> This Python example is inspired by the test case written for libnbd by
>> >> Richard W.M. Jones <rjones@redhat.com>:
>> >> https://gitlab.com/nbdkit/libnbd/-/commit/89113f484effb0e6c322314ba75c1cbe07a04543
>> >>
>> >> Thanks to Daniel P. Berrangé <berrange@redhat.com> for suggestions on
>> >> how to get this working. Now let's document it!
>> >>
>> 
>> >> +  sock_path = '/tmp/qmp-{}.sock'.format(os.getpid())
>> > 
>> > Example code inevitably gets cut+paste into real world apps, and this
>> > example is a tmpfile CVE flaw. At least put it in $CWD instead.
>> 
>> Except $CWD may be too long for a sock file name to be created.
>> Creating the sock in a securely-created subdirectory of /tmp is more
>> reliable.
>
> $XDG_RUNTIME_DIR then, which is /run/user/$UID, so safely per user on all
> modern OS.

Reach under your pillow and check the standard library:

    import tempfile

    with tempfile.TemporaryDirectory() as tmpdirname:
        print('created temporary directory', tmpdirname)

https://docs.python.org/3.6/library/tempfile.html#tempfile.TemporaryDirectory