Re: [Qemu-block] [PATCH v2] live-block-ops.txt: Rename, rewrite, and improve it

[Kashyap Chamarthy]

On Fri, Jun 16, 2017 at 04:41:20PM +0100, Stephen Finucane wrote:
> On Fri, 2017-06-16 at 16:51 +0200, Kashyap Chamarthy wrote:

[...]

> As requested, a couple of rST pointers below that will help you if/when you
> switch to Sphinx. I've only focused on the design aspect, not the content.

Thanks for the Sphinx / rST review.  Eric Blake is reviewing the
content, and probably others will chime in, too.

[...]

> > +Copyright (C) 2017 Red Hat Inc.
> > +
> > +This work is licensed under the terms of the GNU GPL, version 2 or
> > +later.  See the COPYING file in the top-level directory.
> > +
> > +---
> > +
> 
> This information doesn't need to be output in the web version, IMO. If write 
> it
> like a comment, it will only be visible in the source. See what we do in OVS
> docs [1] for an example.
> 
> [1] 
> https://raw.githubusercontent.com/openvswitch/ovs/master/Documentation/index.rst

Yep, that's useful.  Fixed in v3.

[...]

> > +NB: The file ``qapi/block-core.json`` in the QEMU source tree has the
> > +canonical QEMU API (QAPI) schema documentation for the QMP primitives
> > +discussed here.
> > +
> 
> You might consider using admonitions here and elsewhere. This would make sense
> as a 'note' or 'important' directive:
> 
>   .. note::
> 
>       The file ``qapi/block-core.json`` ...

Yes, done.

> > +
> > +.. contents::
> 
> This can probably go if/when Sphinx is integrated - Sphinx includes a ToC in
> the sidebar by default. Perhaps include a TODO to remove this?
> 
>   .. TODO(kashyap): Remove this when Sphinx is integrated

Useful, done.

[...]

> > +(1) Directional: 'base' and 'top'.  Given the simple disk image chain
> > +    above, image [A] can be referred to as 'base', and image [B] as
> > +    'top'.  (This terminology can be seen in in QAPI schema file,
> > +    block-core.json.)
> 
> This looks really like a definition list, which is rST are written like so:
> 
>   term
> 
>     Detailed description of the term here...
> 
> So this would become:
> 
>   Directional
> 
>     'base' and 'top'. Given...

Yeah, I tried it, but it's just creating needless blank lines for me in
the HTML rendering.  So I stuck with using numbers.

[...]

> > +NB: The base disk image can be raw format; however, all the overlay
> > +files must be of QCOW2 format.
> 
> .. important::

Yes, noted.

[...]

> > +Brief overview of live block QMP primitives
> > +-------------------------------------------

[...]

> Definition list?

Not quite.  It's more a quick overview.
 
> > +
> > +
> > +.. _`Interacting with a QEMU instance`:
> 
> If you're not linking to this, you don't need to include this. The 'contents'
> directive will automatically insert an anchor for each heading.

Yeah, I actually did link to to further below, hence I retained it.

[...]

> > +The ``-blockdev`` command-line option, used above, is available from
> > +QEMU 2.9 onwards.  In the above invocation, notice the 'node-name'
> 
> ``node-name``?

Yes.

> 
> > +parameter that is used to refer to the disk image a.qcow2 ('node-A') --
> 
> ``a.qcow2``?

Perhaps.

[...]

> > +NB: In the event we have to repeat a certain QMP command, we will: for
> > +the first occurrence of it, show the the ``qmp-shell`` invocation,
> > +*and* the corresponding raw JSON QMP syntax; but for subsequent
> > +invocations, present just the ``qmp-shell`` syntax, and omit the
> > +equivalent JSON output.
> 
> .. important::

Yeah, it's more of a ".. note::", will fix.

[...]

> > +Here, "node-A" is the name QEMU internally uses to refer to the base
> > +image [A] -- it is the backing file, based on which the overlay image,
> > +[B], is created.
> 
> I guess you should probably use ``[A]`` here to preserve formatting

Hmm, noted.

[...]

> > +
> > +A note on points-in-time vs file names
> > +--------------------------------------
> > +
> > +In our disk disk image chain:
> > +
> > +::
> 
> repeated word and no need for ':\n\n::' - you can just use '::'.
> 
>   In our disk image chain::
> 
> ditto for the rest of the file

Ah, indeed.  I recall using it in the past, but forgot.  Avoids needless
blank lines.  Will fix throughout.

[...]

> > +
> > +
> > +Live block streaming --- ``block-stream``
> > +-----------------------------------------
> > +
> > +The ``block-stream`` command allows you to do live copy data from backing
> > +files into overlay images.
> > +
> > +Given our original example disk image chain from earlier:
> > +
> > +::
> > +
> > +    [A] <-- [B] <-- [C] <-- [D]
> > +
> > +The disk image chain can be shortened in one of the following different
> > +ways (not an exhaustive list).
> > +
> 
> Maybe you should include an anchor here, so you can link to it below.

Yes, makes sense.  I know by "link to it below", you mean link to it in
the next where I refer to these.

[...]

> > +[The invocation for rest of the cases, discussed in the previous
> > +section, is omitted for brevity.]
> 
> This looks like a:
> 
>   .. note::

Yes; will fix.

[...]

-- 
/kashyap