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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 10/29/2015 04:42 AM, Pavel Hrdina wrote:
> 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@xxxxxxxxxx>
>>> ---
>>>  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.
> 

Sure that works too - although I would think mostly unnecessary, but I
know that's the norm to separate rewrite from feature/bug fix.

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

I contend virsh.pod is a conglomeration of writing and formatting
styles. I like your use of B<> instead of I<>, but that is "different".
I think separating switches and better/longer descriptions are better,
but that's not always the case. The whole file could use some amount of
adjustment for consistency in format/style.

>> +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 ...".
> 

See this is one of those damned if you do and damned if you don't.  One
doesn't have to provide "--type" as long as the order is correct.
However, providing --type can allow for a different argument order. I
don't believe it's ever really described that ARGs can either be
specified in the order for each COMMAND, but if not supplied in order,
then the --ARG must be used.  It's one of those things you just get used
to while using virsh.

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

Yes - being consistent is difficult. I know some like ".  Next" while
others prefer ". Next". I sometimes find myself going back and removing
the extra space, while other times I just don't bother. I think my
fingers have been trained through the years of doing this to add two
spaces after a period. See I removed them here because I was thinking
about it, but before I wasn't so they crept in!

John
>>
>>>  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@xxxxxxxxxx
>> https://www.redhat.com/mailman/listinfo/libvir-list

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]