[libvirt] [PATCH v2 3/3] virsh.pod: update and improve a attach-interface section

Thu Oct 29 08:42:38 UTC 2015

On Tue, Oct 27, 2015 at 06:53:55PM -0400, John Ferlan wrote:
> 
> 
> On 10/27/2015 11:01 AM, Pavel Hrdina wrote:
> > Rewrite the attach-interface section in man page to be more readable and
> > add the new hostdev functionality.
> > 
> > Signed-off-by: Pavel Hrdina <phrdina at redhat.com>
> > ---
> >  tools/virsh.pod | 82 +++++++++++++++++++++++++++++++++++----------------------
> >  1 file changed, 50 insertions(+), 32 deletions(-)
> > 
> 
> Why a separate patch? It's related to the previous one and if anyone
> ever (ahem) backed ported the other one, they could miss this one... No
> strong feeling either way - just curious.

It's a rewrite of the attach-interface part of the man page, so I thought, that
would be better to do it in a separate patch.  Maybe I can at first do the
rewrite without adding anything about the new feature and than have the update
of man page together with previous patch.

> 
> > diff --git a/tools/virsh.pod b/tools/virsh.pod
> > index 0212e7a..843c293 100644
> > --- a/tools/virsh.pod
> > +++ b/tools/virsh.pod
> > @@ -2507,51 +2507,69 @@ Likewise, I<--shareable> is an alias for I<--mode shareable>.
> >  [[[I<--live>] [I<--config>] | [I<--current>]] | [I<--persistent>]]
> >  [I<--target target>] [I<--mac mac>] [I<--script script>] [I<--model model>]
> >  [I<--inbound average,peak,burst,floor>] [I<--outbound average,peak,burst>]
> > -[I<--print-xml>]
> > -
> > -Attach a new network interface to the domain.  I<type> can be
> > -I<network> to indicate connection via a libvirt virtual network, or
> > -I<bridge> to indicate connection via a bridge device on the host, or
> > -I<direct> to indicate connection directly to one of the host's network
> > -interfaces or bridges.  I<source> indicates the source of the
> > -connection (the name of a network, or of a bridge device, or the
> > -host's network interfaces or bridges).  I<target> is used to specify
> > -the tap/macvtap device to be used to connect the domain to the
> > -source. Names starting with 'vnet' are considered as auto-generated
> > -and are blanked out/regenerated each time the interface is attached.
> > -I<mac> specifies the MAC address of the network interface; if a MAC
> > +[I<--managed>] [I<--print-xml>]
> > +
> > +Attach a new network interface to the domain.
> > +
> > +B<--type> can be one of the: I<network> to indicate connection via a libvirt
> > +virtual network, I<bridge> to indicate connection via a bridge device
> > +on the host, I<direct> to indicate connection directly to one of the host's
> > +network interfaces or bridges, I<hostdev> to indicate connection using a
> > +passthrough of PCI device on the host.
> 
> Using a ':' I think is unnecessary unless you somehow generate a real
> list such as via "=item * xxxx" entries.  In that case you'd have the
> following:
> 

I've considered this format before writing the patch and it used a lot of space.
I agree, that it would be better arranged.  I'll update it.

> +B<--type> can be one of the following:
> +
> +=over 4
> +
> +=item * Use I<network> to indicate connection via a libvirt virtual network
> +
> +=item * Use I<bridge> to indicate connection via a bridge device on the
> host
> +
> +
> +=item * Use I<direct> to indicate connection directly to one of the host's
> +network interfaces or bridges
> +
> +=item * Use I<hostdev> to indicate connection using a passthrough of
> PCI device
> +on the host.
> +
> +=back
> 
> NB: Whether the '--' is required on the type is perhaps a matter of
> opinion... Since the command line shown doesn't list --type shouldn't
> this still be an 'B<type>'?

Oh, You're right, there is no '--type' in the man page.  In that case, it should
be also referenced the same way.  I've used it probably because you can write
something like this "attach-interface --domain test --type hostdev ...".

> 
> > +
> > +B<--source> indicates the source of the connection.  The source depends
> > +on the type of the interface: I<network> name of the virtual network,
> > +I<bridge> the name of the bridge device, I<direct> the name of the host's
> > +interface or bridge, I<hostdev> the PCI address of the host's interface
> > +formatted as domain:bus:slot.function.
> 
> Similar comment/construct here and same comment about '--' for source.
> 
> > +
> > +B<--target> is used to specify the tap/macvtap device to be used to
> > +connect the domain to the source.  Names starting with 'vnet' are
> > +considered as auto-generated and are blanked out/regenerated each
> > +time the interface is attached.
> > +
> > +B<--mac> specifies the MAC address of the network interface; if a MAC
> 
> B<--target> and B<--mac> seem OK...
> 
> >  address is not given, a new address will be automatically generated
> >  (and stored in the persistent configuration if "--config" is given on
> > -the commandline).  I<script> is used to specify a path to a custom
> > -script to be called while attaching to a bridge - this will be called
> > -instead of the default script not in addition to it; --script is valid
> > -only for interfaces of type I<bridge> and only for Xen domains.
> > -I<model> specifies the network device model to be presented to the
> > -domain.  I<inbound> and I<outbound> control the bandwidth of the
> > -interface. At least one from the I<average>, I<floor> pair must be
> > -specified. The other two I<peak> and I<burst> are optional, so
> > +the command line).
> > +
> > +B<--script> is used to specify a path to a custom script to be called
> > +while attaching to a bridge - this will be called instead of the default
> > +script not in addition to it;  --script is valid only for interfaces of
> 
> Existing, but since you're adjusting - should that be I<--script>?  a

Good point, I'll rework it to not use the '--script' again, it's clear also
without mentioning it again.

> 
> > +type I<bridge> and only for Xen domains.
> 
> similarly perhaps "B<type> I<bridge>
> 
> > +
> > +B<--model> specifies the network device model to be presented to the
> > +domain.
> > +
> > +B<--inbound> and B<--outbound> control the bandwidth of the
> > +interface.  At least one from the I<average>, I<floor> pair must be
> > +specified.  The other two I<peak> and I<burst> are optional, so
> >  "average,peak", "average,,burst", "average,,,floor", "average" and
> > -",,,floor" are also legal. Values for I<average>, I<floor> and I<peak>
> > +",,,floor" are also legal.  Values for I<average>, I<floor> and I<peak>
> 
> Not sure the extra space between '.' and start of next sentance is
> consistent.  You changed it here, but not everywhere. There are those
> that don't like the extra space and those that do. Just be consistent.

We are not consistent through the man page or the source code about those two vs
one space between sentences.  I've tried to follow the two space rule for this
whole command and in my opinion, it's more readable and makes a good separation
between sentences.

>
> >  are expressed in kilobytes per second, while I<burst> is expressed in
> >  kilobytes in a single burst at I<peak> speed as described in the
> >  Network XML documentation at
> >  L<http://libvirt.org/formatnetwork.html#elementQoS>.
> >  
> > -If I<--print-xml> is specified, then the XML of the interface that would be
> > +B<--managed> is usable only for I<hostdev> type and tells libvirt
> 
> for B<type> I<hostdev> ??

I'll try it and see, how it looks like.

Thanks,

Pavel

> 
> An ACK with some adjustments.
> 
> John
> 
> > +that the interface should be managed, which means detached and reattached
> > +from/to the host by libvirt.
> > +
> > +If B<--print-xml> is specified, then the XML of the interface that would be
> >  attached is printed instead.
> >  
> > -If I<--live> is specified, affect a running domain.
> > -If I<--config> is specified, affect the next startup of a persistent domain.
> > -If I<--current> is specified, affect the current domain state.
> > -Both I<--live> and I<--config> flags may be given, but I<--current> is
> > -exclusive. When no flag is specified legacy API is used whose behavior depends
> > -on the hypervisor driver.
> > +If B<--live> is specified, affect a running domain.
> > +If B<--config> is specified, affect the next startup of a persistent domain.
> > +If B<--current> is specified, affect the current domain state.
> > +Both B<--live> and B<--config> flags may be given, but B<--current> is
> > +exclusive.  When no flag is specified legacy API is used whose behavior
> > +depends on the hypervisor driver.
> >  
> > -For compatibility purposes, I<--persistent> behaves like I<--config> for
> > -an offline domain, and like I<--live> I<--config> for a running domain.
> > +For compatibility purposes, B<--persistent> behaves like B<--config> for
> > +an offline domain, and like B<--live> B<--config> for a running domain.
> >  
> >  B<Note>: the optional target value is the name of a device to be created
> > -as the back-end on the node. If not provided a device named "vnetN" or "vifN"
> > +as the back-end on the node.  If not provided a device named "vnetN" or "vifN"
> >  will be created automatically.
> >  
> >  =item B<detach-device> I<domain> I<FILE>
> > 
> 
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list