[PATCH 0/4] docs: add qemu-storage-daemon documentation

[PATCH 0/4] docs: add qemu-storage-daemon documentation

[Stefan Hajnoczi]

Add documentation for the qemu-storage-daemon program and its QMP commands.

The man page looks like this:

QEMU-STORAGE-DAEMON(1)       QEMU                    QEMU-STORAGE-DAEMON(1)

NAME
       qemu-storage-daemon - 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 operations, and
       perform other disk-related operations. The daemon is controlled via a
       QMP monitor socket and initial configuration from the command-line.

       The daemon offers the following subset of QEMU features:

       =C2=B7 Blockdev nodes

       =C2=B7 Block jobs

       =C2=B7 NBD server

       =C2=B7 Character devices

       =C2=B7 Crypto and secrets

       =C2=B7 QMP

       Commands can be sent over a QEMU Monitor Protocol (QMP) connection. See
       the 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 queryin=
g an
       image that is being modified by another process may encounter
       inconsistent state.

OPTIONS
       Standard options:

       -h, --help
              Display this help and exit

       -V, --version
              Display version information and exit

       -T, --trace [[enable=3D]PATTERN][,events=3DFILE][,file=3DFILE]
              Specify tracing options.

              [enable=3D]PATTERN
                     Immediately enable events matching PATTERN (either event=
 name or a globbing pattern).  This option is only available if QEMU has been=
 compiled with the simple,  log
                     or ftrace tracing backend.  To specify multiple events o=
r patterns, specify the -trace option multiple times.

                     Use -trace help to print a list of names of trace points.

              events=3DFILE
                     Immediately  enable  events  listed in FILE.  The file m=
ust contain one event name (as listed in the trace-events-all file) per line;=
 globbing patterns are accepted
                     too.  This option is only available if QEMU has been com=
piled with the simple, log or ftrace tracing backend.

              file=3DFILE
                     Log output traces to FILE.  This option is only availabl=
e if QEMU has been compiled with the simple tracing backend.

       --blockdev BLOCKDEVDEF
              is a blockdev node definition. See the qemu(1) manual page for =
a description of blockdev node properties and the qemu-block-drivers(7) manua=
l page  for  a  description  of
              driver-specific parameters.

       --chardev CHARDEVDEF
              is  a  character  device  definition.  See the qemu(1) manual p=
age for a description of character device properties. A common character devi=
ce definition configures a UNIX
              domain socket:

                 --chardev socket,id=3Dchar1,path=3D/tmp/qmp.sock,server,nowa=
it

       --monitor MONITORDEF
              is a QMP monitor definition. See the qemu(1) manual page for a =
description of QMP monitor properties. A common QMP monitor definition config=
ures  a  monitor  on  character
              device char1:

                 --monitor chardev=3Dchar1

       --nbd-server addr.type=3Dinet,addr.host=3D<host>,addr.port=3D<port>[,t=
ls-creds=3D<id>][,tls-authz=3D<id>]

       --nbd-server addr.type=3Dunix,addr.path=3D<path>[,tls-creds=3D<id>][,t=
ls-authz=3D<id>]
              is a NBD server definition. Both TCP and UNIX domain sockets ar=
e 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=3Dunix,addr.path=3D/tmp/nbd.sock

       --object help

       --object <type>,help

       --object <type>[,<property>=3D<value>...]
              is  a  QEMU  user  creatable object definition. List object typ=
es with help.  List object properties with <type>,help. See the qemu(1) manua=
l page for a description of the
              object properties. The most common object type is a secret, whi=
ch is used to supply passwords and/or encryption keys.

SEE ALSO
       qemu(1), qemu-block-drivers(7), qemu-storage-daemon-qmp-ref(7)

COPYRIGHT
       2020, The QEMU Project Developers

5.1.50                            Sep 08, 2020        QEMU-STORAGE-DAEMON(1)

Stefan Hajnoczi (4):
  docs: lift block-core.json rST header into parents
  docs: generate qemu-storage-daemon-qmp-ref(7) man page
  docs: add qemu-storage-daemon(1) man page
  MAINTAINERS: add Kevin Wolf as storage daemon maintainer

 MAINTAINERS                                   |   9 ++
 docs/interop/firmware.json                    |   4 +
 qapi/block-core.json                          |   4 -
 qapi/block.json                               |   1 +
 docs/interop/qemu-storage-daemon-qmp-ref.texi |  80 +++++++++++++
 docs/tools/conf.py                            |   2 +
 docs/tools/index.rst                          |   1 +
 docs/tools/qemu-storage-daemon.rst            | 105 ++++++++++++++++++
 meson.build                                   |   9 ++
 storage-daemon/qapi/meson.build               |   2 +
 10 files changed, 213 insertions(+), 4 deletions(-)
 create mode 100644 docs/interop/qemu-storage-daemon-qmp-ref.texi
 create mode 100644 docs/tools/qemu-storage-daemon.rst

--=20
2.26.2

[PATCH 1/4] docs: lift block-core.json rST header into parents

[Stefan Hajnoczi]

block-core.json is included from several places. It has no way of
knowing what header level (h1, h2, ...) is appropriate. Sphinx reports
errors when it encounters an h2 header where it expects an h1 header.
This issue prevents the next patch from generating documentation for
qemu-storage-daemon QMP commands.

Move the header into parents so that the correct header level can be
used. Note that transaction.json is not updated since it doesn't seem to
need a header.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 docs/interop/firmware.json | 4 ++++
 qapi/block-core.json       | 4 ----
 qapi/block.json            | 1 +
 3 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/docs/interop/firmware.json b/docs/interop/firmware.json
index 989f10b626..48af327f98 100644
--- a/docs/interop/firmware.json
+++ b/docs/interop/firmware.json
@@ -15,6 +15,10 @@
 ##
 
 { 'include' : 'machine.json' }
+
+##
+# == Block devices
+##
 { 'include' : 'block-core.json' }
 
 ##
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 55b58ba892..e986341997 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1,10 +1,6 @@
 # -*- Mode: Python -*-
 # vim: filetype=python
 
-##
-# == Block core (VM unrelated)
-##
-
 { 'include': 'common.json' }
 { 'include': 'crypto.json' }
 { 'include': 'job.json' }
diff --git a/qapi/block.json b/qapi/block.json
index c54a393cf3..473b294a3b 100644
--- a/qapi/block.json
+++ b/qapi/block.json
@@ -3,6 +3,7 @@
 
 ##
 # = Block devices
+# == Block core (VM unrelated)
 ##
 
 { 'include': 'block-core.json' }
-- 
2.26.2

[PATCH 2/4] docs: generate qemu-storage-daemon-qmp-ref(7) man page

[Stefan Hajnoczi]

Although qemu-storage-daemon QMP commands are identical to QEMU QMP
commands they are a subset. Generate a manual page of just the commands
supported by qemu-storage-daemon so that users know exactly what is
available in qemu-storage-daemon.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 docs/interop/qemu-storage-daemon-qmp-ref.texi | 80 +++++++++++++++++++
 meson.build                                   |  9 +++
 storage-daemon/qapi/meson.build               |  2 +
 3 files changed, 91 insertions(+)
 create mode 100644 docs/interop/qemu-storage-daemon-qmp-ref.texi

diff --git a/docs/interop/qemu-storage-daemon-qmp-ref.texi b/docs/interop/qemu-storage-daemon-qmp-ref.texi
new file mode 100644
index 0000000000..a6a70c9674
--- /dev/null
+++ b/docs/interop/qemu-storage-daemon-qmp-ref.texi
@@ -0,0 +1,80 @@
+\input texinfo
+@setfilename qemu-storage-daemon-qmp-ref.info
+
+@include version.texi
+
+@exampleindent 0
+@paragraphindent 0
+
+@settitle QEMU Storage Daemon QMP Reference Manual
+
+@iftex
+@center @image{docs/qemu_logo}
+@end iftex
+
+@copying
+This is the QEMU Storage Daemon QMP reference manual.
+
+Copyright @copyright{} 2020 The QEMU Project developers
+
+@quotation
+This manual is free documentation: you can redistribute it and/or
+modify it under the terms of the GNU General Public License as
+published by the Free Software Foundation, either version 2 of the
+License, or (at your option) any later version.
+
+This manual is distributed in the hope that it will be useful, but
+WITHOUT ANY WARRANTY; without even the implied warranty of
+MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+General Public License for more details.
+
+You should have received a copy of the GNU General Public License
+along with this manual.  If not, see http://www.gnu.org/licenses/.
+@end quotation
+@end copying
+
+@dircategory QEMU
+@direntry
+* QEMU-Storage-Daemon-QMP-Ref: (qemu-storage-daemon-qmp-ref). QEMU Storage Daemon QMP Reference Manual
+@end direntry
+
+@titlepage
+@title QEMU Storage Daemon QMP Reference Manual
+@subtitle QEMU version @value{VERSION}
+@page
+@vskip 0pt plus 1filll
+@insertcopying
+@end titlepage
+
+@contents
+
+@ifnottex
+@node Top
+@top QEMU Storage Daemon QMP reference
+@end ifnottex
+
+@menu
+* API Reference::
+* Commands and Events Index::
+* Data Types Index::
+@end menu
+
+@node API Reference
+@chapter API Reference
+
+@c for texi2pod:
+@c man begin DESCRIPTION
+
+@include storage-daemon/qapi/qapi-doc.texi
+
+@c man end
+
+@node Commands and Events Index
+@unnumbered Commands and Events Index
+@printindex fn
+
+@node Data Types Index
+@unnumbered Data Types Index
+@printindex tp
+
+@bye
diff --git a/meson.build b/meson.build
index 5aaa364730..0ff19ce699 100644
--- a/meson.build
+++ b/meson.build
@@ -1162,6 +1162,15 @@ if build_docs
   if 'CONFIG_GUEST_AGENT' in config_host
     texi += {'qemu-ga-ref': ['docs/interop/qemu-ga-ref.texi', qga_qapi_doc_texi, version_texi]}
   endif
+  if have_tools
+    texi += {
+      'qemu-storage-daemon-qmp-ref': [
+	'docs/interop/qemu-storage-daemon-qmp-ref.texi',
+	qsd_qapi_doc_texi,
+	version_texi
+      ]
+    }
+  endif
 
   if makeinfo.found()
     cmd = [
diff --git a/storage-daemon/qapi/meson.build b/storage-daemon/qapi/meson.build
index cea618bec0..7c48a388d4 100644
--- a/storage-daemon/qapi/meson.build
+++ b/storage-daemon/qapi/meson.build
@@ -4,4 +4,6 @@ qsd_qapi_files = custom_target('QAPI files for qemu-storage-daemon',
                                command: [ qapi_gen, '-o', 'storage-daemon/qapi', '@INPUT@' ],
                                depend_files: [ qapi_inputs, qapi_gen_depends ])
 
+qsd_qapi_doc_texi = qsd_qapi_files[-1]
+
 qsd_ss.add(qsd_qapi_files.to_list())
-- 
2.26.2

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

[Stefan Hajnoczi]

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 | 105 +++++++++++++++++++++++++++++
 3 files changed, 108 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),
     ('qemu-trace-stap', 'qemu-trace-stap', u'QEMU SystemTap trace tool',
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..729a5e7248
--- /dev/null
+++ b/docs/tools/qemu-storage-daemon.rst
@@ -0,0 +1,105 @@
+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
+operations, and perform other disk-related operations. The daemon is controlled
+via a QMP monitor socket and initial configuration from the command-line.
+
+The daemon offers the following subset of QEMU features:
+
+* Blockdev nodes
+* Block jobs
+* NBD server
+* Character devices
+* Crypto and secrets
+* QMP
+
+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
+
+.. 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.
+
+.. 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.
+
+See also
+--------
+
+:manpage:`qemu(1)`, :manpage:`qemu-block-drivers(7)`, :manpage:`qemu-storage-daemon-qmp-ref(7)`
-- 
2.26.2

[PATCH 4/4] MAINTAINERS: add Kevin Wolf as storage daemon maintainer

[Stefan Hajnoczi]

The MAINTAINERS file was not updated when the storage daemon was merged.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 MAINTAINERS | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index b233da2a73..3e8bfde1e3 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2099,6 +2099,15 @@ F: qobject/block-qdict.c
 F: tests/check-block-qdict.c
 T: git https://repo.or.cz/qemu/kevin.git block
 
+Storage daemon
+M: Kevin Wolf <kwolf@redhat.com>
+L: qemu-block@nongnu.org
+S: Supported
+F: storage-daemon/
+F: docs/interop/qemu-storage-daemon-qmp-ref.texi
+F: docs/tools/qemu-storage-daemon.rst
+T: git https://repo.or.cz/qemu/kevin.git block
+
 Block I/O path
 M: Stefan Hajnoczi <stefanha@redhat.com>
 M: Fam Zheng <fam@euphon.net>
-- 
2.26.2

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

[Kashyap Chamarthy]

On Tue, Sep 08, 2020 at 10:31:12AM +0100, Stefan Hajnoczi wrote:
> 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 | 105 +++++++++++++++++++++++++++++
>  3 files changed, 108 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),
>      ('qemu-trace-stap', 'qemu-trace-stap', u'QEMU SystemTap trace tool',
> 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..729a5e7248
> --- /dev/null
> +++ b/docs/tools/qemu-storage-daemon.rst
> @@ -0,0 +1,105 @@
> +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
> +operations, and perform other disk-related operations. The daemon is controlled
> +via a QMP monitor socket and initial configuration from the command-line.
> +
> +The daemon offers the following subset of QEMU features:
> +
> +* Blockdev nodes
> +* Block jobs
> +* NBD server
> +* Character devices
> +* Crypto and secrets
> +* QMP
> +
> +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.

I wonder if it's appropriate to mention libguestfs for safe, read-only
access to disk images (via `guestfish -ro -i -a disk.qcow2`).  I say
this because, the above warning tells what _not_ to do; a pointer on
what to do could be useful.  I let you decide on this.

The rest looks good to me; I couldn't even spot a typo.


Reviewed-by: Kashyap Chamarthy <kchamart@redhat.com>

[...]

-- 
/kashyap

Re: [PATCH 1/4] docs: lift block-core.json rST header into parents

[Laszlo Ersek]

Hi Stefan,

On 09/08/20 11:31, Stefan Hajnoczi wrote:
> block-core.json is included from several places. It has no way of
> knowing what header level (h1, h2, ...) is appropriate. Sphinx reports
> errors when it encounters an h2 header where it expects an h1 header.
> This issue prevents the next patch from generating documentation for
> qemu-storage-daemon QMP commands.
> 
> Move the header into parents so that the correct header level can be
> used. Note that transaction.json is not updated since it doesn't seem to
> need a header.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  docs/interop/firmware.json | 4 ++++
>  qapi/block-core.json       | 4 ----
>  qapi/block.json            | 1 +
>  3 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/docs/interop/firmware.json b/docs/interop/firmware.json
> index 989f10b626..48af327f98 100644
> --- a/docs/interop/firmware.json
> +++ b/docs/interop/firmware.json
> @@ -15,6 +15,10 @@
>  ##
>  
>  { 'include' : 'machine.json' }
> +
> +##
> +# == Block devices
> +##
>  { 'include' : 'block-core.json' }
>  
>  ##

I think "docs/interop/firmware.json" deserves the same treatment as
"transaction.json".

It's been a long time since I last looked at a rendered view of
"docs/interop/firmware.json", but it only includes "block-core.json" so
it can refer to some block-related types (@BlockdevDriver seems like the
main, or only, one).

I wouldn't expect the rendered view of "firmware.json" to have a section
header saying "Block devices".

I think it should be fine to drop this hunk (and my CC along with it ;))

Thanks!
Laszlo

> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 55b58ba892..e986341997 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1,10 +1,6 @@
>  # -*- Mode: Python -*-
>  # vim: filetype=python
>  
> -##
> -# == Block core (VM unrelated)
> -##
> -
>  { 'include': 'common.json' }
>  { 'include': 'crypto.json' }
>  { 'include': 'job.json' }
> diff --git a/qapi/block.json b/qapi/block.json
> index c54a393cf3..473b294a3b 100644
> --- a/qapi/block.json
> +++ b/qapi/block.json
> @@ -3,6 +3,7 @@
>  
>  ##
>  # = Block devices
> +# == Block core (VM unrelated)
>  ##
>  
>  { 'include': 'block-core.json' }
> 

Re: [PATCH 1/4] docs: lift block-core.json rST header into parents

[Kevin Wolf]

Am 08.09.2020 um 14:03 hat Laszlo Ersek geschrieben:
> Hi Stefan,
> 
> On 09/08/20 11:31, Stefan Hajnoczi wrote:
> > block-core.json is included from several places. It has no way of
> > knowing what header level (h1, h2, ...) is appropriate. Sphinx reports
> > errors when it encounters an h2 header where it expects an h1 header.
> > This issue prevents the next patch from generating documentation for
> > qemu-storage-daemon QMP commands.
> > 
> > Move the header into parents so that the correct header level can be
> > used. Note that transaction.json is not updated since it doesn't seem to
> > need a header.
> > 
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  docs/interop/firmware.json | 4 ++++
> >  qapi/block-core.json       | 4 ----
> >  qapi/block.json            | 1 +
> >  3 files changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/docs/interop/firmware.json b/docs/interop/firmware.json
> > index 989f10b626..48af327f98 100644
> > --- a/docs/interop/firmware.json
> > +++ b/docs/interop/firmware.json
> > @@ -15,6 +15,10 @@
> >  ##
> >  
> >  { 'include' : 'machine.json' }
> > +
> > +##
> > +# == Block devices
> > +##
> >  { 'include' : 'block-core.json' }
> >  
> >  ##
> 
> I think "docs/interop/firmware.json" deserves the same treatment as
> "transaction.json".
> 
> It's been a long time since I last looked at a rendered view of
> "docs/interop/firmware.json", but it only includes "block-core.json" so
> it can refer to some block-related types (@BlockdevDriver seems like the
> main, or only, one).
> 
> I wouldn't expect the rendered view of "firmware.json" to have a section
> header saying "Block devices".
> 
> I think it should be fine to drop this hunk (and my CC along with it ;))

I think this is actually a more general problem with the way we generate
the documentation. For example, the "Background jobs" documentation ends
up under "Block Devices" just because qapi-schema.json includes
block-core.json before job.json and block-core.json includes job.json to
be able to access some types.

Maybe we should always look for the least nested include directive to
figure out where the documentation should be placed. Then things
directly referenced by qapi-schema.json would always be on the top
level.

Possibly we would then want to remove some includes from
qapi-schema.json and include them only from some other file to group
documentation sections that actually make sense to be grouped together.

Kevin

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

[Kevin Wolf]

Am 08.09.2020 um 13:42 hat Kashyap Chamarthy geschrieben:
> On Tue, Sep 08, 2020 at 10:31:12AM +0100, Stefan Hajnoczi wrote:
> > 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>

Looks good to me.

If you have to respin, maybe an example section with some full command
lines for common cases would be nice. Maybe one for exporting a qcow2
image via NBD, and another one for attaching a host_device and having a
QMP monitor, or something like this.

> > +**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.
> 
> I wonder if it's appropriate to mention libguestfs for safe, read-only
> access to disk images (via `guestfish -ro -i -a disk.qcow2`).  I say
> this because, the above warning tells what _not_ to do; a pointer on
> what to do could be useful.  I let you decide on this.

libguestfs may expose exactly the inconsistent state that the above
warning is about. Maybe it breaks not very often in practice, but it's
fundamentally unsafe and if it breaks, you get to keep both pieces.

The safe way is to access it from the guest.

Kevin

Re: [PATCH 1/4] docs: lift block-core.json rST header into parents

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 08.09.2020 um 14:03 hat Laszlo Ersek geschrieben:
>> Hi Stefan,
>> 
>> On 09/08/20 11:31, Stefan Hajnoczi wrote:
>> > block-core.json is included from several places. It has no way of
>> > knowing what header level (h1, h2, ...) is appropriate. Sphinx reports
>> > errors when it encounters an h2 header where it expects an h1 header.
>> > This issue prevents the next patch from generating documentation for
>> > qemu-storage-daemon QMP commands.
>> > 
>> > Move the header into parents so that the correct header level can be
>> > used. Note that transaction.json is not updated since it doesn't seem to
>> > need a header.
>> > 
>> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>> > ---
>> >  docs/interop/firmware.json | 4 ++++
>> >  qapi/block-core.json       | 4 ----
>> >  qapi/block.json            | 1 +
>> >  3 files changed, 5 insertions(+), 4 deletions(-)
>> > 
>> > diff --git a/docs/interop/firmware.json b/docs/interop/firmware.json
>> > index 989f10b626..48af327f98 100644
>> > --- a/docs/interop/firmware.json
>> > +++ b/docs/interop/firmware.json
>> > @@ -15,6 +15,10 @@
>> >  ##
>> >  
>> >  { 'include' : 'machine.json' }
>> > +
>> > +##
>> > +# == Block devices
>> > +##
>> >  { 'include' : 'block-core.json' }
>> >  
>> >  ##
>> 
>> I think "docs/interop/firmware.json" deserves the same treatment as
>> "transaction.json".
>> 
>> It's been a long time since I last looked at a rendered view of
>> "docs/interop/firmware.json", but it only includes "block-core.json" so
>> it can refer to some block-related types (@BlockdevDriver seems like the
>> main, or only, one).
>> 
>> I wouldn't expect the rendered view of "firmware.json" to have a section
>> header saying "Block devices".
>> 
>> I think it should be fine to drop this hunk (and my CC along with it ;))
>
> I think this is actually a more general problem with the way we generate
> the documentation. For example, the "Background jobs" documentation ends
> up under "Block Devices" just because qapi-schema.json includes
> block-core.json before job.json and block-core.json includes job.json to
> be able to access some types.

The doc generator is stupid and greedy (which also makes it
predictable): a module's documentation is emitted where it is first
included.

For full control of the order, have the main module include all
sub-modules in the order you want.

Alternatively, add just enough includes to get the order you want.

> Maybe we should always look for the least nested include directive to
> figure out where the documentation should be placed. Then things
> directly referenced by qapi-schema.json would always be on the top
> level.
>
> Possibly we would then want to remove some includes from
> qapi-schema.json and include them only from some other file to group
> documentation sections that actually make sense to be grouped together.

I doubt implementing this feature would pay back the invested time.
Manually controlling the order like I described above is not much of a
burden, isn't it?

Re: [PATCH 1/4] docs: lift block-core.json rST header into parents

[Kevin Wolf]

Am 09.09.2020 um 09:38 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > Am 08.09.2020 um 14:03 hat Laszlo Ersek geschrieben:
> >> Hi Stefan,
> >> 
> >> On 09/08/20 11:31, Stefan Hajnoczi wrote:
> >> > block-core.json is included from several places. It has no way of
> >> > knowing what header level (h1, h2, ...) is appropriate. Sphinx reports
> >> > errors when it encounters an h2 header where it expects an h1 header.
> >> > This issue prevents the next patch from generating documentation for
> >> > qemu-storage-daemon QMP commands.
> >> > 
> >> > Move the header into parents so that the correct header level can be
> >> > used. Note that transaction.json is not updated since it doesn't seem to
> >> > need a header.
> >> > 
> >> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> >> > ---
> >> >  docs/interop/firmware.json | 4 ++++
> >> >  qapi/block-core.json       | 4 ----
> >> >  qapi/block.json            | 1 +
> >> >  3 files changed, 5 insertions(+), 4 deletions(-)
> >> > 
> >> > diff --git a/docs/interop/firmware.json b/docs/interop/firmware.json
> >> > index 989f10b626..48af327f98 100644
> >> > --- a/docs/interop/firmware.json
> >> > +++ b/docs/interop/firmware.json
> >> > @@ -15,6 +15,10 @@
> >> >  ##
> >> >  
> >> >  { 'include' : 'machine.json' }
> >> > +
> >> > +##
> >> > +# == Block devices
> >> > +##
> >> >  { 'include' : 'block-core.json' }
> >> >  
> >> >  ##
> >> 
> >> I think "docs/interop/firmware.json" deserves the same treatment as
> >> "transaction.json".
> >> 
> >> It's been a long time since I last looked at a rendered view of
> >> "docs/interop/firmware.json", but it only includes "block-core.json" so
> >> it can refer to some block-related types (@BlockdevDriver seems like the
> >> main, or only, one).
> >> 
> >> I wouldn't expect the rendered view of "firmware.json" to have a section
> >> header saying "Block devices".
> >> 
> >> I think it should be fine to drop this hunk (and my CC along with it ;))
> >
> > I think this is actually a more general problem with the way we generate
> > the documentation. For example, the "Background jobs" documentation ends
> > up under "Block Devices" just because qapi-schema.json includes
> > block-core.json before job.json and block-core.json includes job.json to
> > be able to access some types.
> 
> The doc generator is stupid and greedy (which also makes it
> predictable): a module's documentation is emitted where it is first
> included.
> 
> For full control of the order, have the main module include all
> sub-modules in the order you want.

This works as long as the order that we want is consistent with the
requirement that every file must be included by qapi-schea.json before
it is included by any other file (essentially making the additional
includes in other files useless).

Is this the order that we want?

If so, we could support following the rule consistently by making double
include of a file an error.

> Alternatively, add just enough includes to get the order you want.

There are orders that I can't get this way. For example, if I want to
have "Block devices" documented before "Background jobs", there is no
way to add includes to actually get this order without having
"Background jobs" as a subsection in "Block devices".

> > Maybe we should always look for the least nested include directive to
> > figure out where the documentation should be placed. Then things
> > directly referenced by qapi-schema.json would always be on the top
> > level.
> >
> > Possibly we would then want to remove some includes from
> > qapi-schema.json and include them only from some other file to group
> > documentation sections that actually make sense to be grouped together.
> 
> I doubt implementing this feature would pay back the invested time.
> Manually controlling the order like I described above is not much of a
> burden, isn't it?

Depends on whether we are okay with the limitations of the tool
dictating the order of sections in our documentation.

Kevin

Re: [PATCH 1/4] docs: lift block-core.json rST header into parents

[Kevin Wolf]

Am 08.09.2020 um 11:31 hat Stefan Hajnoczi geschrieben:
> block-core.json is included from several places. It has no way of
> knowing what header level (h1, h2, ...) is appropriate. Sphinx reports
> errors when it encounters an h2 header where it expects an h1 header.
> This issue prevents the next patch from generating documentation for
> qemu-storage-daemon QMP commands.
> 
> Move the header into parents so that the correct header level can be
> used. Note that transaction.json is not updated since it doesn't seem to
> need a header.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  docs/interop/firmware.json | 4 ++++
>  qapi/block-core.json       | 4 ----
>  qapi/block.json            | 1 +
>  3 files changed, 5 insertions(+), 4 deletions(-)

storage-daemon/qapi/qapi-schema.json needs an update, too. With the
series as it is, the block-core.json definitions don't get any headline
at all and look as if they were part of the previous section.

Maybe a nicer solution would be to keep the second-level heading where
it is, but to just add a first-level one to the storage daemon
qapi-schema.json. It makes sense to group block-core and block-export
together even without the system emulator part, so the top-level
section wouldn't be arbitrary either, but we would add a second
subsection soon.

Kevin

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

[Stefan Hajnoczi]

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

On Tue, Sep 08, 2020 at 04:33:47PM +0200, Kevin Wolf wrote:
> Am 08.09.2020 um 13:42 hat Kashyap Chamarthy geschrieben:
> > On Tue, Sep 08, 2020 at 10:31:12AM +0100, Stefan Hajnoczi wrote:
> > > 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>
> 
> Looks good to me.
> 
> If you have to respin, maybe an example section with some full command
> lines for common cases would be nice. Maybe one for exporting a qcow2
> image via NBD, and another one for attaching a host_device and having a
> QMP monitor, or something like this.

Good idea. Will fix in v2.

> > > +**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.
> > 
> > I wonder if it's appropriate to mention libguestfs for safe, read-only
> > access to disk images (via `guestfish -ro -i -a disk.qcow2`).  I say
> > this because, the above warning tells what _not_ to do; a pointer on
> > what to do could be useful.  I let you decide on this.
> 
> libguestfs may expose exactly the inconsistent state that the above
> warning is about. Maybe it breaks not very often in practice, but it's
> fundamentally unsafe and if it breaks, you get to keep both pieces.
> 
> The safe way is to access it from the guest.

I agree. If a guest has disk.qcow2 open read/write then libguestfs
cannot open it (even just for reading) and expect a consistent view of
the disk.

Stefan

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

Re: [PATCH 1/4] docs: lift block-core.json rST header into parents

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 09.09.2020 um 09:38 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>> 
>> > Am 08.09.2020 um 14:03 hat Laszlo Ersek geschrieben:
>> >> Hi Stefan,
>> >> 
>> >> On 09/08/20 11:31, Stefan Hajnoczi wrote:
>> >> > block-core.json is included from several places. It has no way of
>> >> > knowing what header level (h1, h2, ...) is appropriate. Sphinx reports
>> >> > errors when it encounters an h2 header where it expects an h1 header.
>> >> > This issue prevents the next patch from generating documentation for
>> >> > qemu-storage-daemon QMP commands.
>> >> > 
>> >> > Move the header into parents so that the correct header level can be
>> >> > used. Note that transaction.json is not updated since it doesn't seem to
>> >> > need a header.
>> >> > 
>> >> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>> >> > ---
>> >> >  docs/interop/firmware.json | 4 ++++
>> >> >  qapi/block-core.json       | 4 ----
>> >> >  qapi/block.json            | 1 +
>> >> >  3 files changed, 5 insertions(+), 4 deletions(-)
>> >> > 
>> >> > diff --git a/docs/interop/firmware.json b/docs/interop/firmware.json
>> >> > index 989f10b626..48af327f98 100644
>> >> > --- a/docs/interop/firmware.json
>> >> > +++ b/docs/interop/firmware.json
>> >> > @@ -15,6 +15,10 @@
>> >> >  ##
>> >> >  
>> >> >  { 'include' : 'machine.json' }
>> >> > +
>> >> > +##
>> >> > +# == Block devices
>> >> > +##
>> >> >  { 'include' : 'block-core.json' }
>> >> >  
>> >> >  ##
>> >> 
>> >> I think "docs/interop/firmware.json" deserves the same treatment as
>> >> "transaction.json".
>> >> 
>> >> It's been a long time since I last looked at a rendered view of
>> >> "docs/interop/firmware.json", but it only includes "block-core.json" so
>> >> it can refer to some block-related types (@BlockdevDriver seems like the
>> >> main, or only, one).
>> >> 
>> >> I wouldn't expect the rendered view of "firmware.json" to have a section
>> >> header saying "Block devices".
>> >> 
>> >> I think it should be fine to drop this hunk (and my CC along with it ;))
>> >
>> > I think this is actually a more general problem with the way we generate
>> > the documentation. For example, the "Background jobs" documentation ends
>> > up under "Block Devices" just because qapi-schema.json includes
>> > block-core.json before job.json and block-core.json includes job.json to
>> > be able to access some types.
>> 
>> The doc generator is stupid and greedy (which also makes it
>> predictable): a module's documentation is emitted where it is first
>> included.
>> 
>> For full control of the order, have the main module include all
>> sub-modules in the order you want.
>
> This works as long as the order that we want is consistent with the
> requirement that every file must be included by qapi-schea.json before
> it is included by any other file (essentially making the additional
> includes in other files useless).

These other includes are not useless: they are essential for generating
self-contained headers.

When MOD.json includes SUBMOD.json, then the generated qapi-FOO-MOD.h
include qapi-FOO-SUBMOD.h.  When every module pulls in the modules it
requires, so do the generated headers.  When a module doesn't, its
generated headers won't compile unless you manually include the missing
generated headers it requires.

> Is this the order that we want?
>
> If so, we could support following the rule consistently by making double
> include of a file an error.

Breaks our simple & stupid way to generate self-contained headers.

>> Alternatively, add just enough includes to get the order you want.
>
> There are orders that I can't get this way.

You're right, ordering by first include is not completely general.

>                                             For example, if I want to
> have "Block devices" documented before "Background jobs", there is no
> way to add includes to actually get this order without having
> "Background jobs" as a subsection in "Block devices".

"Background jobs" is job.json.

"Block devices" is block.json, which includes block-core.json, which
includes job.json.

To be able to put "Block devices" first, you'd have to break the chain
from block.json to job.json.  Which might even be an improvement:

$ wc -l qapi/*.json | awk '$1 >= 1000 { print }'
  5527 qapi/block-core.json
  1722 qapi/migration.json
  1617 qapi/misc.json
  1180 qapi/ui.json
 17407 total

Could we split block-core.json into several cohesive parts?

>> > Maybe we should always look for the least nested include directive to
>> > figure out where the documentation should be placed. Then things
>> > directly referenced by qapi-schema.json would always be on the top
>> > level.
>> >
>> > Possibly we would then want to remove some includes from
>> > qapi-schema.json and include them only from some other file to group
>> > documentation sections that actually make sense to be grouped together.
>> 
>> I doubt implementing this feature would pay back the invested time.
>> Manually controlling the order like I described above is not much of a
>> burden, isn't it?
>
> Depends on whether we are okay with the limitations of the tool
> dictating the order of sections in our documentation.

Fair enough.

Re: [PATCH 1/4] docs: lift block-core.json rST header into parents

[Kevin Wolf]

Am 09.09.2020 um 14:10 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > Am 09.09.2020 um 09:38 hat Markus Armbruster geschrieben:
> >> Kevin Wolf <kwolf@redhat.com> writes:
> >> 
> >> > Am 08.09.2020 um 14:03 hat Laszlo Ersek geschrieben:
> >> >> Hi Stefan,
> >> >> 
> >> >> On 09/08/20 11:31, Stefan Hajnoczi wrote:
> >> >> > block-core.json is included from several places. It has no way of
> >> >> > knowing what header level (h1, h2, ...) is appropriate. Sphinx reports
> >> >> > errors when it encounters an h2 header where it expects an h1 header.
> >> >> > This issue prevents the next patch from generating documentation for
> >> >> > qemu-storage-daemon QMP commands.
> >> >> > 
> >> >> > Move the header into parents so that the correct header level can be
> >> >> > used. Note that transaction.json is not updated since it doesn't seem to
> >> >> > need a header.
> >> >> > 
> >> >> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> >> >> > ---
> >> >> >  docs/interop/firmware.json | 4 ++++
> >> >> >  qapi/block-core.json       | 4 ----
> >> >> >  qapi/block.json            | 1 +
> >> >> >  3 files changed, 5 insertions(+), 4 deletions(-)
> >> >> > 
> >> >> > diff --git a/docs/interop/firmware.json b/docs/interop/firmware.json
> >> >> > index 989f10b626..48af327f98 100644
> >> >> > --- a/docs/interop/firmware.json
> >> >> > +++ b/docs/interop/firmware.json
> >> >> > @@ -15,6 +15,10 @@
> >> >> >  ##
> >> >> >  
> >> >> >  { 'include' : 'machine.json' }
> >> >> > +
> >> >> > +##
> >> >> > +# == Block devices
> >> >> > +##
> >> >> >  { 'include' : 'block-core.json' }
> >> >> >  
> >> >> >  ##
> >> >> 
> >> >> I think "docs/interop/firmware.json" deserves the same treatment as
> >> >> "transaction.json".
> >> >> 
> >> >> It's been a long time since I last looked at a rendered view of
> >> >> "docs/interop/firmware.json", but it only includes "block-core.json" so
> >> >> it can refer to some block-related types (@BlockdevDriver seems like the
> >> >> main, or only, one).
> >> >> 
> >> >> I wouldn't expect the rendered view of "firmware.json" to have a section
> >> >> header saying "Block devices".
> >> >> 
> >> >> I think it should be fine to drop this hunk (and my CC along with it ;))
> >> >
> >> > I think this is actually a more general problem with the way we generate
> >> > the documentation. For example, the "Background jobs" documentation ends
> >> > up under "Block Devices" just because qapi-schema.json includes
> >> > block-core.json before job.json and block-core.json includes job.json to
> >> > be able to access some types.
> >> 
> >> The doc generator is stupid and greedy (which also makes it
> >> predictable): a module's documentation is emitted where it is first
> >> included.
> >> 
> >> For full control of the order, have the main module include all
> >> sub-modules in the order you want.
> >
> > This works as long as the order that we want is consistent with the
> > requirement that every file must be included by qapi-schea.json before
> > it is included by any other file (essentially making the additional
> > includes in other files useless).
> 
> These other includes are not useless: they are essential for generating
> self-contained headers.
> 
> When MOD.json includes SUBMOD.json, then the generated qapi-FOO-MOD.h
> include qapi-FOO-SUBMOD.h.  When every module pulls in the modules it
> requires, so do the generated headers.  When a module doesn't, its
> generated headers won't compile unless you manually include the missing
> generated headers it requires.

Hm, right. So we're using includes for two different purposes, but just
from looking at the include line, you can't know which one it is.

> > Is this the order that we want?
> >
> > If so, we could support following the rule consistently by making double
> > include of a file an error.
> 
> Breaks our simple & stupid way to generate self-contained headers.
> 
> >> Alternatively, add just enough includes to get the order you want.
> >
> > There are orders that I can't get this way.
> 
> You're right, ordering by first include is not completely general.
> 
> >                                             For example, if I want to
> > have "Block devices" documented before "Background jobs", there is no
> > way to add includes to actually get this order without having
> > "Background jobs" as a subsection in "Block devices".
> 
> "Background jobs" is job.json.
> 
> "Block devices" is block.json, which includes block-core.json, which
> includes job.json.
> 
> To be able to put "Block devices" first, you'd have to break the chain
> from block.json to job.json.  Which might even be an improvement:
> 
> $ wc -l qapi/*.json | awk '$1 >= 1000 { print }'
>   5527 qapi/block-core.json
>   1722 qapi/migration.json
>   1617 qapi/misc.json
>   1180 qapi/ui.json
>  17407 total
> 
> Could we split block-core.json into several cohesive parts?

Possibly. However, while it would be an improvement generally speaking,
how does this change the specific problem? All of the smaller files will
be included by block.json (or whatever file provides the "Block devices"
chapter in the documentation) and at least one of them will still have
to include job.json.

(As it happens, the block export series splits off a block-export QAPI
module, but it's probably small enough to be barely noticable in this
comparison.)

Kevin

Re: [PATCH 1/4] docs: lift block-core.json rST header into parents

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 09.09.2020 um 14:10 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>> 
>> > Am 09.09.2020 um 09:38 hat Markus Armbruster geschrieben:
>> >> Kevin Wolf <kwolf@redhat.com> writes:
>> >> 
>> >> > Am 08.09.2020 um 14:03 hat Laszlo Ersek geschrieben:
>> >> >> Hi Stefan,
>> >> >> 
>> >> >> On 09/08/20 11:31, Stefan Hajnoczi wrote:
>> >> >> > block-core.json is included from several places. It has no way of
>> >> >> > knowing what header level (h1, h2, ...) is appropriate. Sphinx reports
>> >> >> > errors when it encounters an h2 header where it expects an h1 header.
>> >> >> > This issue prevents the next patch from generating documentation for
>> >> >> > qemu-storage-daemon QMP commands.
>> >> >> > 
>> >> >> > Move the header into parents so that the correct header level can be
>> >> >> > used. Note that transaction.json is not updated since it doesn't seem to
>> >> >> > need a header.
>> >> >> > 
>> >> >> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>> >> >> > ---
>> >> >> >  docs/interop/firmware.json | 4 ++++
>> >> >> >  qapi/block-core.json       | 4 ----
>> >> >> >  qapi/block.json            | 1 +
>> >> >> >  3 files changed, 5 insertions(+), 4 deletions(-)
>> >> >> > 
>> >> >> > diff --git a/docs/interop/firmware.json b/docs/interop/firmware.json
>> >> >> > index 989f10b626..48af327f98 100644
>> >> >> > --- a/docs/interop/firmware.json
>> >> >> > +++ b/docs/interop/firmware.json
>> >> >> > @@ -15,6 +15,10 @@
>> >> >> >  ##
>> >> >> >  
>> >> >> >  { 'include' : 'machine.json' }
>> >> >> > +
>> >> >> > +##
>> >> >> > +# == Block devices
>> >> >> > +##
>> >> >> >  { 'include' : 'block-core.json' }
>> >> >> >  
>> >> >> >  ##
>> >> >> 
>> >> >> I think "docs/interop/firmware.json" deserves the same treatment as
>> >> >> "transaction.json".
>> >> >> 
>> >> >> It's been a long time since I last looked at a rendered view of
>> >> >> "docs/interop/firmware.json", but it only includes "block-core.json" so
>> >> >> it can refer to some block-related types (@BlockdevDriver seems like the
>> >> >> main, or only, one).
>> >> >> 
>> >> >> I wouldn't expect the rendered view of "firmware.json" to have a section
>> >> >> header saying "Block devices".
>> >> >> 
>> >> >> I think it should be fine to drop this hunk (and my CC along with it ;))
>> >> >
>> >> > I think this is actually a more general problem with the way we generate
>> >> > the documentation. For example, the "Background jobs" documentation ends
>> >> > up under "Block Devices" just because qapi-schema.json includes
>> >> > block-core.json before job.json and block-core.json includes job.json to
>> >> > be able to access some types.
>> >> 
>> >> The doc generator is stupid and greedy (which also makes it
>> >> predictable): a module's documentation is emitted where it is first
>> >> included.
>> >> 
>> >> For full control of the order, have the main module include all
>> >> sub-modules in the order you want.
>> >
>> > This works as long as the order that we want is consistent with the
>> > requirement that every file must be included by qapi-schea.json before
>> > it is included by any other file (essentially making the additional
>> > includes in other files useless).
>> 
>> These other includes are not useless: they are essential for generating
>> self-contained headers.
>> 
>> When MOD.json includes SUBMOD.json, then the generated qapi-FOO-MOD.h
>> include qapi-FOO-SUBMOD.h.  When every module pulls in the modules it
>> requires, so do the generated headers.  When a module doesn't, its
>> generated headers won't compile unless you manually include the missing
>> generated headers it requires.
>
> Hm, right. So we're using includes for two different purposes, but just
> from looking at the include line, you can't know which one it is.

Correct.  The use for controlling doc order is a bit of a hack.

>> > Is this the order that we want?
>> >
>> > If so, we could support following the rule consistently by making double
>> > include of a file an error.
>> 
>> Breaks our simple & stupid way to generate self-contained headers.
>> 
>> >> Alternatively, add just enough includes to get the order you want.
>> >
>> > There are orders that I can't get this way.
>> 
>> You're right, ordering by first include is not completely general.
>> 
>> >                                             For example, if I want to
>> > have "Block devices" documented before "Background jobs", there is no
>> > way to add includes to actually get this order without having
>> > "Background jobs" as a subsection in "Block devices".
>> 
>> "Background jobs" is job.json.
>> 
>> "Block devices" is block.json, which includes block-core.json, which
>> includes job.json.
>> 
>> To be able to put "Block devices" first, you'd have to break the chain
>> from block.json to job.json.  Which might even be an improvement:
>> 
>> $ wc -l qapi/*.json | awk '$1 >= 1000 { print }'
>>   5527 qapi/block-core.json
>>   1722 qapi/migration.json
>>   1617 qapi/misc.json
>>   1180 qapi/ui.json
>>  17407 total
>> 
>> Could we split block-core.json into several cohesive parts?
>
> Possibly. However, while it would be an improvement generally speaking,
> how does this change the specific problem? All of the smaller files will
> be included by block.json (or whatever file provides the "Block devices"
> chapter in the documentation) and at least one of them will still have
> to include job.json.

Splitting block-core.json can help only if other modules then use less
than all the parts.  In particular, as long as block.json includes the
same stuff, it'll surely still include jobs.json.  Could it include
less?

> (As it happens, the block export series splits off a block-export QAPI
> module, but it's probably small enough to be barely noticable in this
> comparison.)
>
> Kevin

Re: [PATCH 1/4] docs: lift block-core.json rST header into parents

[Kevin Wolf]

Am 09.09.2020 um 17:28 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > Am 09.09.2020 um 14:10 hat Markus Armbruster geschrieben:
> >> Kevin Wolf <kwolf@redhat.com> writes:
> >> 
> >> > Am 09.09.2020 um 09:38 hat Markus Armbruster geschrieben:
> >> >> Kevin Wolf <kwolf@redhat.com> writes:
> >> >> 
> >> >> > Am 08.09.2020 um 14:03 hat Laszlo Ersek geschrieben:
> >> >> >> Hi Stefan,
> >> >> >> 
> >> >> >> On 09/08/20 11:31, Stefan Hajnoczi wrote:
> >> >> >> > block-core.json is included from several places. It has no way of
> >> >> >> > knowing what header level (h1, h2, ...) is appropriate. Sphinx reports
> >> >> >> > errors when it encounters an h2 header where it expects an h1 header.
> >> >> >> > This issue prevents the next patch from generating documentation for
> >> >> >> > qemu-storage-daemon QMP commands.
> >> >> >> > 
> >> >> >> > Move the header into parents so that the correct header level can be
> >> >> >> > used. Note that transaction.json is not updated since it doesn't seem to
> >> >> >> > need a header.
> >> >> >> > 
> >> >> >> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> >> >> >> > ---
> >> >> >> >  docs/interop/firmware.json | 4 ++++
> >> >> >> >  qapi/block-core.json       | 4 ----
> >> >> >> >  qapi/block.json            | 1 +
> >> >> >> >  3 files changed, 5 insertions(+), 4 deletions(-)
> >> >> >> > 
> >> >> >> > diff --git a/docs/interop/firmware.json b/docs/interop/firmware.json
> >> >> >> > index 989f10b626..48af327f98 100644
> >> >> >> > --- a/docs/interop/firmware.json
> >> >> >> > +++ b/docs/interop/firmware.json
> >> >> >> > @@ -15,6 +15,10 @@
> >> >> >> >  ##
> >> >> >> >  
> >> >> >> >  { 'include' : 'machine.json' }
> >> >> >> > +
> >> >> >> > +##
> >> >> >> > +# == Block devices
> >> >> >> > +##
> >> >> >> >  { 'include' : 'block-core.json' }
> >> >> >> >  
> >> >> >> >  ##
> >> >> >> 
> >> >> >> I think "docs/interop/firmware.json" deserves the same treatment as
> >> >> >> "transaction.json".
> >> >> >> 
> >> >> >> It's been a long time since I last looked at a rendered view of
> >> >> >> "docs/interop/firmware.json", but it only includes "block-core.json" so
> >> >> >> it can refer to some block-related types (@BlockdevDriver seems like the
> >> >> >> main, or only, one).
> >> >> >> 
> >> >> >> I wouldn't expect the rendered view of "firmware.json" to have a section
> >> >> >> header saying "Block devices".
> >> >> >> 
> >> >> >> I think it should be fine to drop this hunk (and my CC along with it ;))
> >> >> >
> >> >> > I think this is actually a more general problem with the way we generate
> >> >> > the documentation. For example, the "Background jobs" documentation ends
> >> >> > up under "Block Devices" just because qapi-schema.json includes
> >> >> > block-core.json before job.json and block-core.json includes job.json to
> >> >> > be able to access some types.
> >> >> 
> >> >> The doc generator is stupid and greedy (which also makes it
> >> >> predictable): a module's documentation is emitted where it is first
> >> >> included.
> >> >> 
> >> >> For full control of the order, have the main module include all
> >> >> sub-modules in the order you want.
> >> >
> >> > This works as long as the order that we want is consistent with the
> >> > requirement that every file must be included by qapi-schea.json before
> >> > it is included by any other file (essentially making the additional
> >> > includes in other files useless).
> >> 
> >> These other includes are not useless: they are essential for generating
> >> self-contained headers.
> >> 
> >> When MOD.json includes SUBMOD.json, then the generated qapi-FOO-MOD.h
> >> include qapi-FOO-SUBMOD.h.  When every module pulls in the modules it
> >> requires, so do the generated headers.  When a module doesn't, its
> >> generated headers won't compile unless you manually include the missing
> >> generated headers it requires.
> >
> > Hm, right. So we're using includes for two different purposes, but just
> > from looking at the include line, you can't know which one it is.
> 
> Correct.  The use for controlling doc order is a bit of a hack.
> 
> >> > Is this the order that we want?
> >> >
> >> > If so, we could support following the rule consistently by making double
> >> > include of a file an error.
> >> 
> >> Breaks our simple & stupid way to generate self-contained headers.
> >> 
> >> >> Alternatively, add just enough includes to get the order you want.
> >> >
> >> > There are orders that I can't get this way.
> >> 
> >> You're right, ordering by first include is not completely general.
> >> 
> >> >                                             For example, if I want to
> >> > have "Block devices" documented before "Background jobs", there is no
> >> > way to add includes to actually get this order without having
> >> > "Background jobs" as a subsection in "Block devices".
> >> 
> >> "Background jobs" is job.json.
> >> 
> >> "Block devices" is block.json, which includes block-core.json, which
> >> includes job.json.
> >> 
> >> To be able to put "Block devices" first, you'd have to break the chain
> >> from block.json to job.json.  Which might even be an improvement:
> >> 
> >> $ wc -l qapi/*.json | awk '$1 >= 1000 { print }'
> >>   5527 qapi/block-core.json
> >>   1722 qapi/migration.json
> >>   1617 qapi/misc.json
> >>   1180 qapi/ui.json
> >>  17407 total
> >> 
> >> Could we split block-core.json into several cohesive parts?
> >
> > Possibly. However, while it would be an improvement generally speaking,
> > how does this change the specific problem? All of the smaller files will
> > be included by block.json (or whatever file provides the "Block devices"
> > chapter in the documentation) and at least one of them will still have
> > to include job.json.
> 
> Splitting block-core.json can help only if other modules then use less
> than all the parts.  In particular, as long as block.json includes the
> same stuff, it'll surely still include jobs.json.  Could it include
> less?

Not if the documentation wants to have a single chapter for the block
layer instead of many small block related top-level chapters.

Otherwise we could include some things directly from qapi-schema.json,
but obviously, that would still have to be after job.json for some
parts.

Kevin

Re: [PATCH 1/4] docs: lift block-core.json rST header into parents

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 09.09.2020 um 17:28 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>> 
>> > Am 09.09.2020 um 14:10 hat Markus Armbruster geschrieben:
>> >> Kevin Wolf <kwolf@redhat.com> writes:
[...]
>> >> > There are orders that I can't get this way.
>> >> 
>> >> You're right, ordering by first include is not completely general.
>> >> 
>> >> >                                             For example, if I want to
>> >> > have "Block devices" documented before "Background jobs", there is no
>> >> > way to add includes to actually get this order without having
>> >> > "Background jobs" as a subsection in "Block devices".
>> >> 
>> >> "Background jobs" is job.json.
>> >> 
>> >> "Block devices" is block.json, which includes block-core.json, which
>> >> includes job.json.
>> >> 
>> >> To be able to put "Block devices" first, you'd have to break the chain
>> >> from block.json to job.json.  Which might even be an improvement:
>> >> 
>> >> $ wc -l qapi/*.json | awk '$1 >= 1000 { print }'
>> >>   5527 qapi/block-core.json
>> >>   1722 qapi/migration.json
>> >>   1617 qapi/misc.json
>> >>   1180 qapi/ui.json
>> >>  17407 total
>> >> 
>> >> Could we split block-core.json into several cohesive parts?
>> >
>> > Possibly. However, while it would be an improvement generally speaking,
>> > how does this change the specific problem? All of the smaller files will
>> > be included by block.json (or whatever file provides the "Block devices"
>> > chapter in the documentation) and at least one of them will still have
>> > to include job.json.
>> 
>> Splitting block-core.json can help only if other modules then use less
>> than all the parts.  In particular, as long as block.json includes the
>> same stuff, it'll surely still include jobs.json.  Could it include
>> less?
>
> Not if the documentation wants to have a single chapter for the block
> layer instead of many small block related top-level chapters.
>
> Otherwise we could include some things directly from qapi-schema.json,
> but obviously, that would still have to be after job.json for some
> parts.

You're right.

Being unable to talk about something before you define it may not be all
bad, though :)

Re: [PATCH 1/4] docs: lift block-core.json rST header into parents

[Kevin Wolf]

Am 10.09.2020 um 07:18 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > Am 09.09.2020 um 17:28 hat Markus Armbruster geschrieben:
> >> Kevin Wolf <kwolf@redhat.com> writes:
> >> 
> >> > Am 09.09.2020 um 14:10 hat Markus Armbruster geschrieben:
> >> >> Kevin Wolf <kwolf@redhat.com> writes:
> [...]
> >> >> > There are orders that I can't get this way.
> >> >> 
> >> >> You're right, ordering by first include is not completely general.
> >> >> 
> >> >> >                                             For example, if I want to
> >> >> > have "Block devices" documented before "Background jobs", there is no
> >> >> > way to add includes to actually get this order without having
> >> >> > "Background jobs" as a subsection in "Block devices".
> >> >> 
> >> >> "Background jobs" is job.json.
> >> >> 
> >> >> "Block devices" is block.json, which includes block-core.json, which
> >> >> includes job.json.
> >> >> 
> >> >> To be able to put "Block devices" first, you'd have to break the chain
> >> >> from block.json to job.json.  Which might even be an improvement:
> >> >> 
> >> >> $ wc -l qapi/*.json | awk '$1 >= 1000 { print }'
> >> >>   5527 qapi/block-core.json
> >> >>   1722 qapi/migration.json
> >> >>   1617 qapi/misc.json
> >> >>   1180 qapi/ui.json
> >> >>  17407 total
> >> >> 
> >> >> Could we split block-core.json into several cohesive parts?
> >> >
> >> > Possibly. However, while it would be an improvement generally speaking,
> >> > how does this change the specific problem? All of the smaller files will
> >> > be included by block.json (or whatever file provides the "Block devices"
> >> > chapter in the documentation) and at least one of them will still have
> >> > to include job.json.
> >> 
> >> Splitting block-core.json can help only if other modules then use less
> >> than all the parts.  In particular, as long as block.json includes the
> >> same stuff, it'll surely still include jobs.json.  Could it include
> >> less?
> >
> > Not if the documentation wants to have a single chapter for the block
> > layer instead of many small block related top-level chapters.
> >
> > Otherwise we could include some things directly from qapi-schema.json,
> > but obviously, that would still have to be after job.json for some
> > parts.
> 
> You're right.
> 
> Being unable to talk about something before you define it may not be all
> bad, though :)

Yes, as long as the resulting order is what we want anyway, this is not
a problem.

I'm just not sure if we will always be aware of the include structure
without tool support so that we would always declare the dependencies
first. Nobody checks the headlines in the documentation after making
schema changes.

Kevin