Re: [PATCH v2 2/3] docs: add qemu-storage-daemon(1) man page

[Kevin Wolf]

Am 10.09.2020 um 16:43 hat Stefan Hajnoczi geschrieben:
> Document the qemu-storage-daemon tool. Most of the command-line options
> are identical to their QEMU counterparts. Perhaps Sphinx hxtool
> integration could be extended to extract documentation for individual
> command-line options so they can be shared. For now the
> qemu-storage-daemon simply refers to the qemu(1) man page where the
> command-line options are identical.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  docs/tools/conf.py                 |   2 +
>  docs/tools/index.rst               |   1 +
>  docs/tools/qemu-storage-daemon.rst | 121 +++++++++++++++++++++++++++++
>  3 files changed, 124 insertions(+)
>  create mode 100644 docs/tools/qemu-storage-daemon.rst
> 
> diff --git a/docs/tools/conf.py b/docs/tools/conf.py
> index 9052d17d6d..c16290e716 100644
> --- a/docs/tools/conf.py
> +++ b/docs/tools/conf.py
> @@ -20,6 +20,8 @@ html_theme_options['description'] = \
>  man_pages = [
>      ('qemu-img', 'qemu-img', u'QEMU disk image utility',
>       ['Fabrice Bellard'], 1),
> +    ('qemu-storage-daemon', 'qemu-storage-daemon', u'QEMU storage daemon',
> +     [], 1),
>      ('qemu-nbd', 'qemu-nbd', u'QEMU Disk Network Block Device Server',
>       ['Anthony Liguori <anthony@codemonkey.ws>'], 8),

> diff --git a/docs/tools/index.rst b/docs/tools/index.rst
> index 232ce9f3e4..9b076adb62 100644
> --- a/docs/tools/index.rst
> +++ b/docs/tools/index.rst
> @@ -11,6 +11,7 @@ Contents:
>     :maxdepth: 2
>  
>     qemu-img
> +   qemu-storage-daemon
>     qemu-nbd
>     qemu-trace-stap
>     virtfs-proxy-helper
> diff --git a/docs/tools/qemu-storage-daemon.rst 
> b/docs/tools/qemu-storage-daemon.rst
> new file mode 100644
> index 0000000000..a0b8b1a2bf
> --- /dev/null
> +++ b/docs/tools/qemu-storage-daemon.rst
> @@ -0,0 +1,121 @@
> +QEMU Storage Daemon
> +===================
> +
> +Synopsis
> +--------
> +
> +**qemu-storage-daemon** [options]
> +
> +Description
> +-----------
> +
> +qemu-storage-daemon provides disk image functionality from QEMU, qemu-img, 
> and
> +qemu-nbd in a long-running process controlled via QMP commands without 
> running
> +a virtual machine. It can export disk images over NBD, run block job

While this is true today, I think we should phrase it in a more generic
way so that we don't have to modify it each time we add something.

Specifically, I wouldn't mention NBD here, but just say "can export disk
images". If you want, you can add something like "(e.g. over NBD)" to
make clear that this won't be the only option.

> +operations, and perform other disk-related operations. The daemon is 
> controlled
> +via a QMP monitor socket and initial configuration from the command-line.

Likewise I would s/socket// because you can have the QMP monitor on any
chardev. It could be a socket, it could be stdio, maybe there is even a
third option that makes sense.

> +The daemon offers the following subset of QEMU features:
> +
> +* Blockdev nodes

I think we call this "Block nodes" usually in documentation.

> +* Block jobs
> +* NBD server

Let's make this "Block exports"

> +* Character devices
> +* Crypto and secrets
> +* QMP

If we mention crypto/secrets, should we also mention iothreads
explicitly? Or throttle groups?

Or again more generically something like "User-creatable QOM objects
such as crypto secrets and iothread"?

> +Commands can be sent over a QEMU Monitor Protocol (QMP) connection. See the
> +:manpage:`qemu-storage-daemon-qmp-ref(7)` manual page for a description of 
> the
> +commands.
> +
> +The daemon runs until it is stopped using the ``quit`` QMP command or
> +SIGINT/SIGHUP/SIGTERM.
> +
> +**Warning:** Never modify images in use by a running virtual machine or any
> +other process; this may destroy the image. Also, be aware that querying an
> +image that is being modified by another process may encounter inconsistent
> +state.
> +
> +Options
> +-------
> +
> +.. program:: qemu-storage-daemon
> +
> +Standard options:
> +
> +.. option:: -h, --help
> +
> +  Display this help and exit

It's not "this" help in the man page.

> +.. option:: -V, --version
> +
> +  Display version information and exit
> +
> +.. option:: -T, --trace [[enable=]PATTERN][,events=FILE][,file=FILE]
> +
> +  .. include:: ../qemu-option-trace.rst.inc
> +
> +.. option:: --blockdev BLOCKDEVDEF
> +
> +  is a blockdev node definition. See the :manpage:`qemu(1)` manual page for a
> +  description of blockdev node properties and the
> +  :manpage:`qemu-block-drivers(7)` manual page for a description of
> +  driver-specific parameters.

s/blockdev/block/g

qemu-block-drivers(7) was originally a list of options for 'qemu-img
create'. I see that it now describes runtime options for some drivers (in
particular the network drivers), but not the image format drivers.

I guess this needs some cleanup so that create options and runtime
options are clearly separated, and that runtime options for format
drivers are documented.

It doesn't have to stop this patch, though the reference could be
confusing when the other man page starts with create options.

> +.. option:: --chardev CHARDEVDEF
> +
> +  is a character device definition. See the :manpage:`qemu(1)` manual page 
> for
> +  a description of character device properties. A common character device
> +  definition configures a UNIX domain socket::
> +
> +  --chardev socket,id=char1,path=/tmp/qmp.sock,server,nowait
> +
> +.. option:: --monitor MONITORDEF
> +
> +  is a QMP monitor definition. See the :manpage:`qemu(1)` manual page for
> +  a description of QMP monitor properties. A common QMP monitor definition
> +  configures a monitor on character device ``char1``::
> +
> +  --monitor chardev=char1
> +
> +.. option:: --nbd-server 
> addr.type=inet,addr.host=<host>,addr.port=<port>[,tls-creds=<id>][,tls-authz=<id>]
> +  --nbd-server 
> addr.type=unix,addr.path=<path>[,tls-creds=<id>][,tls-authz=<id>]
> +
> +  is a NBD server definition. Both TCP and UNIX domain sockets are supported.
> +  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``::
> +
> +  --nbd-server addr.type=unix,addr.path=/tmp/nbd.sock
> +
> +.. option:: --object help
> +  --object <type>,help
> +  --object <type>[,<property>=<value>...]
> +
> +  is a QEMU user creatable object definition. List object types with 
> ``help``.
> +  List object properties with ``<type>,help``. See the :manpage:`qemu(1)`
> +  manual page for a description of the object properties. The most common
> +  object type is a ``secret``, which is used to supply passwords and/or
> +  encryption keys.

Isn't iothread more common actually?

> +Examples
> +--------
> +Launch the daemon with QMP monitor socket ``qmp.sock`` so clients can execute
> +QMP commands::
> +
> +  $ qemu-storage-daemon \
> +      --chardev socket,path=qmp.sock,server,nowait,id=char1 \
> +      --monitor chardev=char1
> +
> +Export raw image file ``disk.img`` over NBD UNIX domain socket ``nbd.sock``::
> +
> +  $ qemu-storage-daemon \
> +      --blockdev driver=file,node-name=disk,filename=disk.img \
> +      --nbd-server addr.type=unix,addr.path=nbd.sock \
> +      --export type=nbd,device=disk,writable=on

The --export syntax for NBD has changed with the block export series.
I think that you need s/device/node-name/ and that it requires an id=...
option now, too.

> +See also
> +--------
> +
> +:manpage:`qemu(1)`, :manpage:`qemu-block-drivers(7)`, 
> :manpage:`qemu-storage-daemon-qmp-ref(7)`
> -- 
> 2.26.2

Kevin