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@redhat.com>
---
 docs/manpages/virsh.rst | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

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] }
       [--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.
 
 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'/>).
+
+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

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@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
> 

> 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@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@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

On 4/28/20 9:19 AM, Sebastian Mitterle wrote:
>> 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]

We should really encourage users to properly quote their command line to 
avoid unintentional globbing:

# virsh blockpull fedora vda "vda[1]"

(Otherwise, if I 'touch vda1', the unquoted command will see the 
argument 'vda1')

> 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]]
> 


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

> We should really encourage users to properly quote their command line to
avoid unintentional globbing:

Not sure I understand, is this a request to use '"name[i]"' with
single and double quotes in the manpage?

Please, note that the line you quote is not part of the patch but
sample invocations to demonstrate behavior of blockpull command
regarding mandatory positional arguments to justify v2 of this patch.

On Tue, Apr 28, 2020 at 4:33 PM Eric Blake <eblake@redhat.com> wrote:
>
> On 4/28/20 9:19 AM, Sebastian Mitterle wrote:
> >> 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]
>
> We should really encourage users to properly quote their command line to
> avoid unintentional globbing:
>
> # virsh blockpull fedora vda "vda[1]"
>
> (Otherwise, if I 'touch vda1', the unquoted command will see the
> argument 'vda1')
>
> > 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]]
> >
>
>
> --
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3226
> Virtualization:  qemu.org | libvirt.org
>


-- 
Best,
Sebastian

On 4/29/20 8:11 AM, Sebastian Mitterle wrote:
>> We should really encourage users to properly quote their command line to
> avoid unintentional globbing:
> 
> Not sure I understand, is this a request to use '"name[i]"' with
> single and double quotes in the manpage?

One or the other, but not both.

Buggy:
# virsh blockpull fedora vda vda[1]

Ok:
# virsh blockpull fedora vda 'vda[1]'
# virsh blockpull fedora vda "vda[1]"
# virsh blockpull fedora vda vda\[1]

the point is that any shell example that can misbehave due to globbing 
if underquoted should instead be properly quoted.
> 
> Please, note that the line you quote is not part of the patch but
> sample invocations to demonstrate behavior of blockpull command
> regarding mandatory positional arguments to justify v2 of this patch.
> 

If the example is not something you plan on putting in the man page, 
then that's okay.  I was just pointing out that anything mentioned in 
the man page should also mention the need for proper shell quoting.



-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

> If the example is not something you plan on putting in the man page,
> then that's okay.  I was just pointing out that anything mentioned in
> the man page should also mention the need for proper shell quoting.

Got it, thank you. I'm unsure. No, I wasn't planning on adding an
example. Peter, you initially reviewed this patch, what do you think?
(I checked and I don't see many examples on the manpage in general.)
Personally, I like examples but am unsure about the scope, like how
many examples, which options should be exemplified, etc.


On Wed, Apr 29, 2020 at 4:16 PM Eric Blake <eblake@redhat.com> wrote:
>
> On 4/29/20 8:11 AM, Sebastian Mitterle wrote:
> >> We should really encourage users to properly quote their command line to
> > avoid unintentional globbing:
> >
> > Not sure I understand, is this a request to use '"name[i]"' with
> > single and double quotes in the manpage?
>
> One or the other, but not both.
>
> Buggy:
> # virsh blockpull fedora vda vda[1]
>
> Ok:
> # virsh blockpull fedora vda 'vda[1]'
> # virsh blockpull fedora vda "vda[1]"
> # virsh blockpull fedora vda vda\[1]
>
> the point is that any shell example that can misbehave due to globbing
> if underquoted should instead be properly quoted.
> >
> > Please, note that the line you quote is not part of the patch but
> > sample invocations to demonstrate behavior of blockpull command
> > regarding mandatory positional arguments to justify v2 of this patch.
> >
>
> If the example is not something you plan on putting in the man page,
> then that's okay.  I was just pointing out that anything mentioned in
> the man page should also mention the need for proper shell quoting.
>
>
>
> --
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3226
> Virtualization:  qemu.org | libvirt.org
>


-- 
Best,
Sebastian

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@redhat.com>
> ---
>  docs/manpages/virsh.rst | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
> 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] }

For any argument where the '--' can be skipped we use the non-dash
syntax. Additionally your version is missing the argument placeholder.

>        [--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.

This is somewhat common in our docs. (two spaces between sentences)

>  
>  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'/>).

Well, it's either a fully qualified path, or preferrably the 'name[i]'
thing. but 'name[i]' is not a fully qualified path.

> +
> +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>`__).

I don't think this document contains information on relative file paths.

On Fri, Apr 17, 2020 at 14:52:06 +0200, Peter Krempa 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@redhat.com>
> > ---
> >  docs/manpages/virsh.rst | 19 +++++++++++++------
> >  1 file changed, 13 insertions(+), 6 deletions(-)

[...]

> > +
> > +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>`__).
> 
> I don't think this document contains information on relative file paths.

It actually does. Unfortunately it doesn't specify how to take a
snapshot with relative paths thoug. But this is probably okay then.