[libvirt] [PATCH v2 0/4] attach-interface: Learn net type='direct'
v2 Michal Privoznik (4): libvirt_private.syms: Expose virDomainNetTypeFromString virsh attach-interface: Use enum instead of arbitrary integers virsh attach-interface: Use virDomainNetType{From,To}String() virsh attach-interface: Allow macvtap hotplug src/libvirt_private.syms | 1 + tools/virsh-domain.c | 36 ++++++++++++++++++++++++++---------- tools/virsh.pod | 14 ++++++++------ 3 files changed, 35 insertions(+), 16 deletions(-) -- 2.0.5
The enum converters are defined in the domain_conf.h (so accessible widely across the code), but on the symbol layer, only virDomainNetTypeToString was exposed. However, FromString variant is going to be needed shortly. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 376c69b..9cb81dd 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -343,6 +343,7 @@ virDomainNetGetActualVlan; virDomainNetInsert; virDomainNetRemove; virDomainNetRemoveHostdev; +virDomainNetTypeFromString; virDomainNetTypeToString; virDomainNostateReasonTypeFromString; virDomainNostateReasonTypeToString; -- 2.0.5
The type of interface to attach is held in the variable 'typ'. Depending on interface type selected by user, the variable is set either to 1 (network), or 2 (bridge). Lets use already existing enum from domain_conf.h instead: virDomainNetType. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/virsh-domain.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 358d61c..201a1b1 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -906,7 +906,7 @@ cmdAttachInterface(vshControl *ctl, const vshCmd *cmd) *type = NULL, *source = NULL, *model = NULL, *inboundStr = NULL, *outboundStr = NULL; virNetDevBandwidthRate inbound, outbound; - int typ; + virDomainNetType typ; int ret; bool functionReturn = false; virBuffer buf = VIR_BUFFER_INITIALIZER; @@ -946,9 +946,9 @@ cmdAttachInterface(vshControl *ctl, const vshCmd *cmd) /* check interface type */ if (STREQ(type, "network")) { - typ = 1; + typ = VIR_DOMAIN_NET_TYPE_NETWORK; } else if (STREQ(type, "bridge")) { - typ = 2; + typ = VIR_DOMAIN_NET_TYPE_BRIDGE; } else { vshError(ctl, _("No support for %s in command 'attach-interface'"), type); @@ -982,9 +982,9 @@ cmdAttachInterface(vshControl *ctl, const vshCmd *cmd) virBufferAsprintf(&buf, "<interface type='%s'>\n", type); virBufferAdjustIndent(&buf, 2); - if (typ == 1) + if (typ == VIR_DOMAIN_NET_TYPE_NETWORK) virBufferAsprintf(&buf, "<source network='%s'/>\n", source); - else if (typ == 2) + else if (typ == VIR_DOMAIN_NET_TYPE_BRIDGE) virBufferAsprintf(&buf, "<source bridge='%s'/>\n", source); if (target != NULL) -- 2.0.5
Instead of verbose string to enum conversion (if STREQ() else if STREQ() else if STREQ() ...) lets use virDomainNetType{From,To}String. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/virsh-domain.c | 32 +++++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 201a1b1..117c52b 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -945,11 +945,7 @@ cmdAttachInterface(vshControl *ctl, const vshCmd *cmd) goto cleanup; /* check interface type */ - if (STREQ(type, "network")) { - typ = VIR_DOMAIN_NET_TYPE_NETWORK; - } else if (STREQ(type, "bridge")) { - typ = VIR_DOMAIN_NET_TYPE_BRIDGE; - } else { + if ((int)(typ = virDomainNetTypeFromString(type)) < 0) { vshError(ctl, _("No support for %s in command 'attach-interface'"), type); goto cleanup; @@ -982,10 +978,28 @@ cmdAttachInterface(vshControl *ctl, const vshCmd *cmd) virBufferAsprintf(&buf, "<interface type='%s'>\n", type); virBufferAdjustIndent(&buf, 2); - if (typ == VIR_DOMAIN_NET_TYPE_NETWORK) - virBufferAsprintf(&buf, "<source network='%s'/>\n", source); - else if (typ == VIR_DOMAIN_NET_TYPE_BRIDGE) - virBufferAsprintf(&buf, "<source bridge='%s'/>\n", source); + switch (typ) { + case VIR_DOMAIN_NET_TYPE_NETWORK: + case VIR_DOMAIN_NET_TYPE_BRIDGE: + virBufferAsprintf(&buf, "<source %s='%s'/>\n", + virDomainNetTypeToString(typ), source); + break; + + case VIR_DOMAIN_NET_TYPE_USER: + case VIR_DOMAIN_NET_TYPE_ETHERNET: + case VIR_DOMAIN_NET_TYPE_VHOSTUSER: + case VIR_DOMAIN_NET_TYPE_SERVER: + case VIR_DOMAIN_NET_TYPE_CLIENT: + case VIR_DOMAIN_NET_TYPE_MCAST: + case VIR_DOMAIN_NET_TYPE_INTERNAL: + case VIR_DOMAIN_NET_TYPE_DIRECT: + case VIR_DOMAIN_NET_TYPE_HOSTDEV: + case VIR_DOMAIN_NET_TYPE_LAST: + vshError(ctl, _("No support for %s in command 'attach-interface'"), + type); + goto cleanup; + break; + } if (target != NULL) virBufferAsprintf(&buf, "<target dev='%s'/>\n", target); -- 2.0.5
Our hotplug code supports macvtap insertion to guests. However, we somehow forgot about 'attach-interface' (which tries to build XML from passed arguments and use virDomainAttachDeviceFlags()). New type is accessible under 'direct' type, to keep the same type as used in domain XML. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/virsh-domain.c | 4 +++- tools/virsh.pod | 14 ++++++++------ 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 117c52b..13c0b83 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -984,6 +984,9 @@ cmdAttachInterface(vshControl *ctl, const vshCmd *cmd) virBufferAsprintf(&buf, "<source %s='%s'/>\n", virDomainNetTypeToString(typ), source); break; + case VIR_DOMAIN_NET_TYPE_DIRECT: + virBufferAsprintf(&buf, "<source dev='%s'/>\n", source); + break; case VIR_DOMAIN_NET_TYPE_USER: case VIR_DOMAIN_NET_TYPE_ETHERNET: @@ -992,7 +995,6 @@ cmdAttachInterface(vshControl *ctl, const vshCmd *cmd) case VIR_DOMAIN_NET_TYPE_CLIENT: case VIR_DOMAIN_NET_TYPE_MCAST: case VIR_DOMAIN_NET_TYPE_INTERNAL: - case VIR_DOMAIN_NET_TYPE_DIRECT: case VIR_DOMAIN_NET_TYPE_HOSTDEV: case VIR_DOMAIN_NET_TYPE_LAST: vshError(ctl, _("No support for %s in command 'attach-interface'"), diff --git a/tools/virsh.pod b/tools/virsh.pod index a3f527f..50de32c 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -2391,12 +2391,14 @@ Likewise, I<--shareable> is an alias for I<--mode shareable>. [I<--target target>] [I<--mac mac>] [I<--script script>] [I<--model model>] [I<--config>] [I<--inbound average,peak,burst>] [I<--outbound average,peak,burst>] -Attach a new network interface to the domain. I<type> can be either -I<network> to indicate connection via a libvirt virtual network or -I<bridge> to indicate connection via a bridge device on the host. -I<source> indicates the source of the connection (either the name of a -network, or of a bridge device). I<target> is used to specify the -tap/macvtap device to be used to connect the domain to the +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 -- 2.0.5
On 02/09/2015 10:20 AM, Michal Privoznik wrote:
v2
Michal Privoznik (4): libvirt_private.syms: Expose virDomainNetTypeFromString virsh attach-interface: Use enum instead of arbitrary integers virsh attach-interface: Use virDomainNetType{From,To}String() virsh attach-interface: Allow macvtap hotplug
src/libvirt_private.syms | 1 + tools/virsh-domain.c | 36 ++++++++++++++++++++++++++---------- tools/virsh.pod | 14 ++++++++------ 3 files changed, 35 insertions(+), 16 deletions(-)
ACK series... If you want to merge 1-3 into just one patch that'd be fine too... At the very least 1&3 are related since the From API isn't used until 3. John
On 02/12/2015 04:50 PM, John Ferlan wrote:
On 02/09/2015 10:20 AM, Michal Privoznik wrote:
v2
Michal Privoznik (4): libvirt_private.syms: Expose virDomainNetTypeFromString virsh attach-interface: Use enum instead of arbitrary integers virsh attach-interface: Use virDomainNetType{From,To}String() virsh attach-interface: Allow macvtap hotplug
src/libvirt_private.syms | 1 + tools/virsh-domain.c | 36 ++++++++++++++++++++++++++---------- tools/virsh.pod | 14 ++++++++------ 3 files changed, 35 insertions(+), 16 deletions(-)
ACK series...
I had thought that during V1 of this patch series there had been enough sentiment about not pulling in private parts of libvirt internals that this V2 (posted 2 minutes before the last comment in V1, so very literally crossing in the inter-tubes) would be discarded in favor of something that only used public APIs - the last word on the subject was this: https://www.redhat.com/archives/libvir-list/2015-February/msg00284.html (Yes, I know there are other places where it's already done, but it seems like a better idea to clean those up than to compound the pollution.)
On Fri, Feb 13, 2015 at 07:55:48AM -0500, Laine Stump wrote:
On 02/12/2015 04:50 PM, John Ferlan wrote:
On 02/09/2015 10:20 AM, Michal Privoznik wrote:
v2
Michal Privoznik (4): libvirt_private.syms: Expose virDomainNetTypeFromString virsh attach-interface: Use enum instead of arbitrary integers virsh attach-interface: Use virDomainNetType{From,To}String() virsh attach-interface: Allow macvtap hotplug
src/libvirt_private.syms | 1 + tools/virsh-domain.c | 36 ++++++++++++++++++++++++++---------- tools/virsh.pod | 14 ++++++++------ 3 files changed, 35 insertions(+), 16 deletions(-)
ACK series...
I had thought that during V1 of this patch series there had been enough sentiment about not pulling in private parts of libvirt internals that this V2 (posted 2 minutes before the last comment in V1, so very literally crossing in the inter-tubes) would be discarded in favor of something that only used public APIs - the last word on the subject was this:
https://www.redhat.com/archives/libvir-list/2015-February/msg00284.html
(Yes, I know there are other places where it's already done, but it seems like a better idea to clean those up than to compound the pollution.)
Yeah, I'd prefer that in general, but this particular patch is only using the trivial VIR_ENUM string/int conversion, so I'm willing to overlook it for this. If the internal API being used were non-trivial then I'd say we should look at the public API support. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (4)
-
Daniel P. Berrange -
John Ferlan -
Laine Stump -
Michal Privoznik