Next patch will use those function to collect NodeDevice list and find a specific device. Make functions virshNodeDeviceListCollect() and virshNodeDeviceListFree() together with struct virshNodeDeviceList available to reuse existing code. Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; --- tools/virsh-nodedev.c | 16 +++++----------- tools/virsh-nodedev.h | 11 +++++++++++ 2 files changed, 16 insertions(+), 11 deletions(-) diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c index cc359e2..26f2c7b 100644 --- a/tools/virsh-nodedev.c +++ b/tools/virsh-nodedev.c @@ -194,13 +194,7 @@ virshNodeDeviceSorter(const void *a, const void *b) virNodeDeviceGetName(*nb)); } -struct virshNodeDeviceList { - virNodeDevicePtr *devices; - size_t ndevices; -}; -typedef struct virshNodeDeviceList *virshNodeDeviceListPtr; - -static void +void virshNodeDeviceListFree(virshNodeDeviceListPtr list) { size_t i; @@ -215,11 +209,11 @@ virshNodeDeviceListFree(virshNodeDeviceListPtr list) VIR_FREE(list); } -static virshNodeDeviceListPtr +virshNodeDeviceListPtr virshNodeDeviceListCollect(vshControl *ctl, - char **capnames, - int ncapnames, - unsigned int flags) + char **capnames, + int ncapnames, + unsigned int flags) { virshNodeDeviceListPtr list = vshMalloc(ctl, sizeof(*list)); size_t i; diff --git a/tools/virsh-nodedev.h b/tools/virsh-nodedev.h index c64f7df..1d2337b 100644 --- a/tools/virsh-nodedev.h +++ b/tools/virsh-nodedev.h @@ -30,4 +30,15 @@ extern const vshCmdDef nodedevCmds[]; +struct virshNodeDeviceList { + virNodeDevicePtr *devices; + size_t ndevices; +}; +typedef struct virshNodeDeviceList *virshNodeDeviceListPtr; + +virshNodeDeviceListPtr virshNodeDeviceListCollect(vshControl *ctl, + char **capnames, + int ncapnames, + unsigned int flags); +void virshNodeDeviceListFree(virshNodeDeviceListPtr list); #endif /* VIRSH_NODEDEV_H */ -- 2.6.2

On 10/27/2015 11:01 AM, Pavel Hrdina wrote: What happens if someone provides --managed with some other 'typ'? IOW: If it's only being supported by <hostdev>, then after the switch, a check should probably occur for "if (managed && typ != VIR_DOMAIN_NET_TYPE_HOSTDEV), message, and fail. I'm not fully clear after reading the bz and the previous review whether this patch resolves the bz - something to be worked out in the bz for history's sake though I think with the adjustment to check whether managed is supplied for non hostdev, then you have an ACK for what's changed here. John

Rewrite the attach-interface section in man page to be more readable and add the new hostdev functionality. Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; --- tools/virsh.pod | 82 +++++++++++++++++++++++++++++++++++---------------------- 1 file changed, 50 insertions(+), 32 deletions(-) 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. + +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. + +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 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 +type I<bridge> and only for Xen domains. + +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> 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 +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> -- 2.6.2

On Tue, Oct 27, 2015 at 06:53:55PM -0400, John Ferlan wrote: 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. 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. 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 ...". Good point, I'll rework it to not use the '--script' again, it's clear also without mentioning it again. 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. I'll try it and see, how it looks like. Thanks, Pavel

On Thu, Oct 29, 2015 at 10:52:00AM -0400, John Ferlan wrote: It's not that hard to split the patch, so I'll do it. I agree, the man page is a mess and really hard to read and understand. Pavel