[PATCH 1/3] Improve blockpull man entry

[Sebastian Mitterle]

> So bandwidth indeed is a positional argument. Since all of the manpage
> uses the syntax we've had here originally fixing just this place would
> be wrong. The only fix that should be done here is to group the --bytes
> flag under bandwidth as specifying --bytes is mandatory.

I think there's misunderstanding:

# virsh blockpull fedora vda vda[1]
error: Scaled numeric value 'vda[1]' for <--bandwidth> option is
malformed or out of range

# virsh blockpull fedora vda 1024 vda[1]
Block Pull started

I'll change to [bandwidth [--bytes] [base]]



On Fri, Apr 24, 2020 at 9:35 AM Peter Krempa <pkrempa at redhat.com> wrote:
>
> On Wed, Apr 15, 2020 at 11:34:04 +0000, Sebastian Mitterle wrote:
> > 1. Mention usage of `--base` and `--bandwidth` and fix cmd syntax.
> > 2. Explain valid arguments for `base`.
> > 3. Move explanation for `--keep-relative` to end considering it
> >    less frequent use case because libvirt doesn't create relative
> >    backing chain names.
> > 4. Add reference to documentation for relative paths in backing chains.
> >
> > Signed-off-by: Sebastian Mitterle <smitterl at redhat.com>
> > ---
> >  docs/manpages/virsh.rst | 19 +++++++++++++------
> >  1 file changed, 13 insertions(+), 6 deletions(-)
>
> Given new information about the virsh argument parser I've figured out
> I'll re-review this patch:
>
> > diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst
> > index dc404ddfe8..27ecc53d56 100644
> > --- a/docs/manpages/virsh.rst
> > +++ b/docs/manpages/virsh.rst
> > @@ -1345,7 +1345,7 @@ blockpull
> >
> >  .. code-block::
> >
> > -   blockpull domain path [bandwidth] [--bytes] [base]
> > +   blockpull domain path { [bandwidth [--bytes] [base]] | [--bandwidth [--bytes]] [--base] }
>
> So bandwidth indeed is a positional argument. Since all of the manpage
> uses the syntax we've had here originally fixing just this place would
> be wrong. The only fix that should be done here is to group the --bytes
> flag under bandwidth as specifying --bytes is mandatory.
>
> >        [--wait [--verbose] [--timeout seconds] [--async]]
> >        [--keep-relative]
> >
> > @@ -1353,7 +1353,7 @@ Populate a disk from its backing image chain. By default, this command
> >  flattens the entire chain; but if *base* is specified, containing the
> >  name of one of the backing files in the chain, then that file becomes
> >  the new backing file and only the intermediate portion of the chain is
> > -pulled.  Once all requested data from the backing image chain has been
> > +pulled. Once all requested data from the backing image chain has been
> >  pulled, the disk no longer depends on that portion of the backing chain.
>
> As mentioned previously some of the docs use two spaces between
> sentences. If it should be fixed then all of it should be fixed in one
> patch.
>
> Please drop this hunk.
>
> >
> >  By default, this command returns as soon as possible, and data for
> > @@ -1367,16 +1367,23 @@ is triggered, *--async* will return control to the user as fast as
> >  possible, otherwise the command may continue to block a little while
> >  longer until the job is done cleaning up.
> >
> > -Using the *--keep-relative* flag will keep the backing chain names
> > -relative.
> > -
> >  *path* specifies fully-qualified path of the disk; it corresponds
> >  to a unique target name (<target dev='name'/>) or source file (<source
> >  file='name'/>) for one of the disk devices attached to *domain* (see
> >  also ``domblklist`` for listing these names).
> > +
> >  *bandwidth* specifies copying bandwidth limit in MiB/s. For further information
> >  on the *bandwidth* argument see the corresponding section for the ``blockjob``
> > -command.
> > +command. Using *--bytes* flag indicates the value in *bandwidth* is given in
> > +bytes.
> > +
> > +*base* specifies fully-qualified path of the backing file; it corresponds
> > +to a unique indexed target name 'name[i]' (<target dev='name'/>..
> > +<backingStore index='i'/>) or source file 'name' (<source file='name'/>).
>
> I'd move this hunk under the first paragraph in section about blockpull
> since it explains what 'base' actually is along with some rewording:
>
> Something like
>
> *base* can eithr be specified as indexed target name 'name[i]'
> where 'name corresponds to the disk target name (<target dev='name'/>)
> and 'i' corresponds to the 'index' of the '<backingStore>', or the file
> name of backing file (<source file='name'/>).
> > +
> > +Using the *--keep-relative* flag will keep the backing chain names
> > +relative (details on `https://www.libvirt.org/kbase/backing_chains.html
> > +<https://www.libvirt.org/kbase/backing_chains.html>`__).
> >
> >
> >  blockresize
> > --
> > 2.25.2
> >
>


--
Best,
Sebastian