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