9:19 a.m.
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(a)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(a)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