[libvirt] [PATCH 0/5] Improve detach-device intelligence for net interfaces
Until now it was possible to device-detach network device by mac address only. This patch improves libvirt user-friendliness so it is possible to detach also via PCI address, and in case when there is exactly one network interface even without specifying redundant addresses (MAC/PCI). Michal Privoznik (5): conf: Change statically allocated MAC to dynamic DomainXMLFlags: Introduce flag allowing suppress of generation of additional items virDomainNetDefParseXML: suppress generation of MAC when VIR_DOMAIN_PARSE_NO_GENERATE is set qemu: Allow network device-detach by PCI address and/or MAC xen: Allow network device-detach by PCI address and/or MAC include/libvirt/libvirt.h.in | 7 ++-- src/conf/domain_conf.c | 9 +++++- src/conf/domain_conf.h | 2 +- src/openvz/openvz_conf.c | 3 +- src/qemu/qemu_command.c | 3 +- src/qemu/qemu_driver.c | 3 +- src/qemu/qemu_hotplug.c | 46 +++++++++++++++++++++------- src/vbox/vbox_tmpl.c | 4 ++- src/vmx/vmx.c | 3 +- src/xen/xm_internal.c | 66 +++++++++++++++++++++++++++++++++-------- src/xenxs/xen_sxpr.c | 3 +- src/xenxs/xen_xm.c | 3 +- 12 files changed, 115 insertions(+), 37 deletions(-) -- 1.7.4
We need this so we can later suppress generation and know whether user supplied one or not in XML. --- src/conf/domain_conf.c | 4 +++- src/conf/domain_conf.h | 2 +- src/openvz/openvz_conf.c | 3 ++- src/qemu/qemu_command.c | 3 ++- src/qemu/qemu_hotplug.c | 2 +- src/vbox/vbox_tmpl.c | 4 +++- src/vmx/vmx.c | 3 ++- src/xen/xm_internal.c | 5 ++--- src/xenxs/xen_sxpr.c | 3 ++- src/xenxs/xen_xm.c | 3 ++- 10 files changed, 20 insertions(+), 12 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 16e1291..c2c7057 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -594,6 +594,7 @@ void virDomainNetDefFree(virDomainNetDefPtr def) if (!def) return; + VIR_FREE(def->mac); VIR_FREE(def->model); switch (def->type) { @@ -2490,7 +2491,8 @@ virDomainNetDefParseXML(virCapsPtr caps, xmlNodePtr oldnode = ctxt->node; int ret; - if (VIR_ALLOC(def) < 0) { + if ((VIR_ALLOC(def) < 0) || + (VIR_ALLOC_N(def->mac,VIR_MAC_BUFLEN) < 0)) { virReportOOMError(); return NULL; } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 30aeccc..476e122 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -345,7 +345,7 @@ typedef struct _virDomainNetDef virDomainNetDef; typedef virDomainNetDef *virDomainNetDefPtr; struct _virDomainNetDef { enum virDomainNetType type; - unsigned char mac[VIR_MAC_BUFLEN]; + unsigned char *mac; char *model; union { struct { diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c index 0eb5ab3..458f107 100644 --- a/src/openvz/openvz_conf.c +++ b/src/openvz/openvz_conf.c @@ -236,7 +236,8 @@ openvzReadNetworkConf(virDomainDefPtr def, token = strtok_r(temp, ";", &saveptr); while (token != NULL) { /*add new device to list*/ - if (VIR_ALLOC(net) < 0) + if ((VIR_ALLOC(net) < 0) || + (VIR_ALLOC_N(net->mac, VIR_MAC_BUFLEN) < 0)) goto no_memory; net->type = VIR_DOMAIN_NET_TYPE_BRIDGE; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index c9b9850..35d695d 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5017,7 +5017,8 @@ qemuParseCommandLineNet(virCapsPtr caps, nkeywords = 0; } - if (VIR_ALLOC(def) < 0) { + if ((VIR_ALLOC(def) < 0) || + (VIR_ALLOC_N(def->mac, VIR_MAC_BUFLEN) < 0)) { virReportOOMError(); goto cleanup; } diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index e4ba526..d8a8e5d 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1479,7 +1479,7 @@ int qemuDomainDetachNetDevice(struct qemud_driver *driver, for (i = 0 ; i < vm->def->nnets ; i++) { virDomainNetDefPtr net = vm->def->nets[i]; - if (!memcmp(net->mac, dev->data.net->mac, sizeof(net->mac))) { + if (!memcmp(net->mac, dev->data.net->mac, VIR_MAC_BUFLEN)) { detach = net; break; } diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index e8ac48f..f964e4b 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -2709,7 +2709,9 @@ static char *vboxDomainDumpXML(virDomainPtr dom, int flags) { /* Allocate memory for the networkcards which are enabled */ if ((def->nnets > 0) && (VIR_ALLOC_N(def->nets, def->nnets) >= 0)) { for (i = 0; i < def->nnets; i++) { - if (VIR_ALLOC(def->nets[i]) >= 0) { + if ((VIR_ALLOC(def->nets[i]) >= 0) || + (VIR_ALLOC_N(def->nets[i]->mac, + VIR_MAC_BUFLEN) >= 0)) { } else virReportOOMError(); } diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c index 9f4d5fb..77f18c2 100644 --- a/src/vmx/vmx.c +++ b/src/vmx/vmx.c @@ -2271,7 +2271,8 @@ virVMXParseEthernet(virConfPtr conf, int controller, virDomainNetDefPtr *def) return -1; } - if (VIR_ALLOC(*def) < 0) { + if ((VIR_ALLOC(*def) < 0) || + (VIR_ALLOC_N((*def)->mac, VIR_MAC_BUFLEN) < 0)) { virReportOOMError(); return -1; } diff --git a/src/xen/xm_internal.c b/src/xen/xm_internal.c index 7f73588..9a9fa0c 100644 --- a/src/xen/xm_internal.c +++ b/src/xen/xm_internal.c @@ -1501,9 +1501,8 @@ xenXMDomainDetachDeviceFlags(virDomainPtr domain, const char *xml, case VIR_DOMAIN_DEVICE_NET: { for (i = 0 ; i < def->nnets ; i++) { - if (!memcmp(def->nets[i]->mac, - dev->data.net->mac, - sizeof(def->nets[i]->mac))) { + if (!memcmp(def->nets[i]->mac, dev->data.net->mac, + VIR_MAC_BUFLEN)) { virDomainNetDefFree(def->nets[i]); if (i < (def->nnets - 1)) memmove(def->nets + i, diff --git a/src/xenxs/xen_sxpr.c b/src/xenxs/xen_sxpr.c index 3a412a6..e918801 100644 --- a/src/xenxs/xen_sxpr.c +++ b/src/xenxs/xen_sxpr.c @@ -518,7 +518,8 @@ xenParseSxprNets(virDomainDefPtr def, model = sexpr_node(node, "device/vif/model"); type = sexpr_node(node, "device/vif/type"); - if (VIR_ALLOC(net) < 0) + if ((VIR_ALLOC(net) < 0) || + (VIR_ALLOC_N(net->mac, VIR_MAC_BUFLEN) < 0)) goto no_memory; if (tmp != NULL || diff --git a/src/xenxs/xen_xm.c b/src/xenxs/xen_xm.c index 0acd120..6a94b09 100644 --- a/src/xenxs/xen_xm.c +++ b/src/xenxs/xen_xm.c @@ -647,7 +647,8 @@ xenParseXM(virConfPtr conf, int xendConfigVersion, key = nextkey; } - if (VIR_ALLOC(net) < 0) + if ((VIR_ALLOC(net) < 0) || + (VIR_ALLOC_N(net->mac, VIR_MAC_BUFLEN) < 0)) goto no_memory; if (mac[0]) { -- 1.7.4
--- include/libvirt/libvirt.h.in | 7 ++++--- 1 files changed, 4 insertions(+), 3 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index eaeccd6..596f3b3 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -886,9 +886,10 @@ int virDomainGetSecurityLabel (virDomainPtr domain, */ typedef enum { - VIR_DOMAIN_XML_SECURE = (1 << 0), /* dump security sensitive information too */ - VIR_DOMAIN_XML_INACTIVE = (1 << 1), /* dump inactive domain information */ - VIR_DOMAIN_XML_UPDATE_CPU = (1 << 2), /* update guest CPU requirements according to host CPU */ + VIR_DOMAIN_XML_SECURE = (1 << 0), /* dump security sensitive information too */ + VIR_DOMAIN_XML_INACTIVE = (1 << 1), /* dump inactive domain information */ + VIR_DOMAIN_XML_UPDATE_CPU = (1 << 2), /* update guest CPU requirements according to host CPU */ + VIR_DOMAIN_PARSE_NO_GENERATE = (1 << 3), /* suppress generation of additional items in the parse */ } virDomainXMLFlags; char * virDomainGetXMLDesc (virDomainPtr domain, -- 1.7.4
--- src/conf/domain_conf.c | 11 ++++++++--- 1 files changed, 8 insertions(+), 3 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index c2c7057..7f9c178 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2491,8 +2491,7 @@ virDomainNetDefParseXML(virCapsPtr caps, xmlNodePtr oldnode = ctxt->node; int ret; - if ((VIR_ALLOC(def) < 0) || - (VIR_ALLOC_N(def->mac,VIR_MAC_BUFLEN) < 0)) { + if (VIR_ALLOC(def) < 0) { virReportOOMError(); return NULL; } @@ -2588,6 +2587,12 @@ virDomainNetDefParseXML(virCapsPtr caps, cur = cur->next; } + if ((macaddr || !(flags & VIR_DOMAIN_PARSE_NO_GENERATE)) && + (VIR_ALLOC_N(def->mac, VIR_MAC_BUFLEN) < 0)) { + virReportOOMError(); + goto error; + } + if (macaddr) { if (virParseMacAddr((const char *)macaddr, def->mac) < 0) { virDomainReportError(VIR_ERR_INTERNAL_ERROR, @@ -2595,7 +2600,7 @@ virDomainNetDefParseXML(virCapsPtr caps, (const char *)macaddr); goto error; } - } else { + } else if (!(flags & VIR_DOMAIN_PARSE_NO_GENERATE)) { virCapabilitiesGenerateMac(caps, def->mac); } -- 1.7.4
On 03/17/2011 10:38 AM, Michal Privoznik wrote:
--- src/conf/domain_conf.c | 11 ++++++++--- 1 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index c2c7057..7f9c178 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2491,8 +2491,7 @@ virDomainNetDefParseXML(virCapsPtr caps, xmlNodePtr oldnode = ctxt->node; int ret;
- if ((VIR_ALLOC(def)< 0) || - (VIR_ALLOC_N(def->mac,VIR_MAC_BUFLEN)< 0)) { + if (VIR_ALLOC(def)< 0) { virReportOOMError(); return NULL; } @@ -2588,6 +2587,12 @@ virDomainNetDefParseXML(virCapsPtr caps, cur = cur->next; }
+ if ((macaddr || !(flags& VIR_DOMAIN_PARSE_NO_GENERATE))&& + (VIR_ALLOC_N(def->mac, VIR_MAC_BUFLEN)< 0)) { + virReportOOMError(); + goto error; + } + if (macaddr) { if (virParseMacAddr((const char *)macaddr, def->mac)< 0) { virDomainReportError(VIR_ERR_INTERNAL_ERROR, @@ -2595,7 +2600,7 @@ virDomainNetDefParseXML(virCapsPtr caps, (const char *)macaddr); goto error; } - } else { + } else if (!(flags& VIR_DOMAIN_PARSE_NO_GENERATE)) { virCapabilitiesGenerateMac(caps, def->mac);
I'm going to voice my dislike of side-effects in Parse functions one last time (I think that a function called "...Parse" should interpret the data it's given, not add new data) and suggest that the Mac generation should always be done in the caller when needed rather than passing a strange flag down to the parser. (I know the parser was already generating the MAC; I think it should be changed to *not* do that) (or at the very least the flag shouldn't be "on == don't do something extra", it should be "on == do something extra"). However, if nobody else expresses agreement with me on that, I'll consider myself outvoted, and keep quiet during the next version of the patch (based on danpb's review of PATCH 4/5, it seems there is going to be another round anyway :-)
}
On Thu, Mar 17, 2011 at 12:55:50PM -0400, Laine Stump wrote:
On 03/17/2011 10:38 AM, Michal Privoznik wrote:
--- src/conf/domain_conf.c | 11 ++++++++--- 1 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index c2c7057..7f9c178 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2491,8 +2491,7 @@ virDomainNetDefParseXML(virCapsPtr caps, xmlNodePtr oldnode = ctxt->node; int ret;
- if ((VIR_ALLOC(def)< 0) || - (VIR_ALLOC_N(def->mac,VIR_MAC_BUFLEN)< 0)) { + if (VIR_ALLOC(def)< 0) { virReportOOMError(); return NULL; } @@ -2588,6 +2587,12 @@ virDomainNetDefParseXML(virCapsPtr caps, cur = cur->next; }
+ if ((macaddr || !(flags& VIR_DOMAIN_PARSE_NO_GENERATE))&& + (VIR_ALLOC_N(def->mac, VIR_MAC_BUFLEN)< 0)) { + virReportOOMError(); + goto error; + } + if (macaddr) { if (virParseMacAddr((const char *)macaddr, def->mac)< 0) { virDomainReportError(VIR_ERR_INTERNAL_ERROR, @@ -2595,7 +2600,7 @@ virDomainNetDefParseXML(virCapsPtr caps, (const char *)macaddr); goto error; } - } else { + } else if (!(flags& VIR_DOMAIN_PARSE_NO_GENERATE)) { virCapabilitiesGenerateMac(caps, def->mac);
I'm going to voice my dislike of side-effects in Parse functions one last time (I think that a function called "...Parse" should interpret the data it's given, not add new data) and suggest that the Mac generation should always be done in the caller when needed rather than passing a strange flag down to the parser. (I know the parser was already generating the MAC; I think it should be changed to *not* do that) (or at the very least the flag shouldn't be "on == don't do something extra", it should be "on == do something extra").
I understand your point, but the main issue with this is that there a very many places where virDomainDefParseXML is called, and nearly all of them require automatic MAC generation if omitted. If we pushed that into the callers, then we'd trivally miss places where MAC generation was required, and never notice until someone wonders why a VM is getting a NIC with mac of all-zeroes. In this specific case, rather than turning off MAC generation, what we actually want to do is *mandate* that the MAC is always present in the XML we're given. When detaching a NIC, it is never acceptable to leave out the MAC addr. So I'd have a flag VIR_DOMAIN_PARSE_REQUIRE_MAC and thus raise a fatal if omitted from the XML 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 :|
On 03/17/2011 06:07 PM, Daniel P. Berrange wrote:
On Thu, Mar 17, 2011 at 12:55:50PM -0400, Laine Stump wrote:
On 03/17/2011 10:38 AM, Michal Privoznik wrote:
--- src/conf/domain_conf.c | 11 ++++++++--- 1 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index c2c7057..7f9c178 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2491,8 +2491,7 @@ virDomainNetDefParseXML(virCapsPtr caps, xmlNodePtr oldnode = ctxt->node; int ret;
- if ((VIR_ALLOC(def)< 0) || - (VIR_ALLOC_N(def->mac,VIR_MAC_BUFLEN)< 0)) { + if (VIR_ALLOC(def)< 0) { virReportOOMError(); return NULL; } @@ -2588,6 +2587,12 @@ virDomainNetDefParseXML(virCapsPtr caps, cur = cur->next; }
+ if ((macaddr || !(flags& VIR_DOMAIN_PARSE_NO_GENERATE))&& + (VIR_ALLOC_N(def->mac, VIR_MAC_BUFLEN)< 0)) { + virReportOOMError(); + goto error; + } + if (macaddr) { if (virParseMacAddr((const char *)macaddr, def->mac)< 0) { virDomainReportError(VIR_ERR_INTERNAL_ERROR, @@ -2595,7 +2600,7 @@ virDomainNetDefParseXML(virCapsPtr caps, (const char *)macaddr); goto error; } - } else { + } else if (!(flags& VIR_DOMAIN_PARSE_NO_GENERATE)) { virCapabilitiesGenerateMac(caps, def->mac);
I'm going to voice my dislike of side-effects in Parse functions one last time (I think that a function called "...Parse" should interpret the data it's given, not add new data) and suggest that the Mac generation should always be done in the caller when needed rather than passing a strange flag down to the parser. (I know the parser was already generating the MAC; I think it should be changed to *not* do that) (or at the very least the flag shouldn't be "on == don't do something extra", it should be "on == do something extra").
I understand your point, but the main issue with this is that there a very many places where virDomainDefParseXML is called, and nearly all of them require automatic MAC generation if omitted. If we pushed that into the callers, then we'd trivally miss places where MAC generation was required, and never notice until someone wonders why a VM is getting a NIC with mac of all-zeroes.
In this specific case, rather than turning off MAC generation, what we actually want to do is *mandate* that the MAC is always present in the XML we're given. When detaching a NIC, it is never acceptable to leave out the MAC addr. So I'd have a flag VIR_DOMAIN_PARSE_REQUIRE_MAC and thus raise a fatal if omitted from the XML
Regards, Daniel
I am not completely convinced this is what we want. If domain has exactly one NIC, device-detach semantic is clear. Or if we want to allow detaching interface by PCI address - MAC shouldn't be required, because it is redundant. Anyway - what is the best solution to this issue? N.B.: MAC itself actually is not unique identifier - one can attach as many NICs with the same MAC address as he wants. The only true unique ID is PCI address. Regards Michal
On Fri, Mar 18, 2011 at 04:45:17PM +0100, Michal Prívozník wrote:
On 03/17/2011 06:07 PM, Daniel P. Berrange wrote:
On Thu, Mar 17, 2011 at 12:55:50PM -0400, Laine Stump wrote:
On 03/17/2011 10:38 AM, Michal Privoznik wrote:
--- src/conf/domain_conf.c | 11 ++++++++--- 1 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index c2c7057..7f9c178 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2491,8 +2491,7 @@ virDomainNetDefParseXML(virCapsPtr caps, xmlNodePtr oldnode = ctxt->node; int ret;
- if ((VIR_ALLOC(def)< 0) || - (VIR_ALLOC_N(def->mac,VIR_MAC_BUFLEN)< 0)) { + if (VIR_ALLOC(def)< 0) { virReportOOMError(); return NULL; } @@ -2588,6 +2587,12 @@ virDomainNetDefParseXML(virCapsPtr caps, cur = cur->next; }
+ if ((macaddr || !(flags& VIR_DOMAIN_PARSE_NO_GENERATE))&& + (VIR_ALLOC_N(def->mac, VIR_MAC_BUFLEN)< 0)) { + virReportOOMError(); + goto error; + } + if (macaddr) { if (virParseMacAddr((const char *)macaddr, def->mac)< 0) { virDomainReportError(VIR_ERR_INTERNAL_ERROR, @@ -2595,7 +2600,7 @@ virDomainNetDefParseXML(virCapsPtr caps, (const char *)macaddr); goto error; } - } else { + } else if (!(flags& VIR_DOMAIN_PARSE_NO_GENERATE)) { virCapabilitiesGenerateMac(caps, def->mac);
I'm going to voice my dislike of side-effects in Parse functions one last time (I think that a function called "...Parse" should interpret the data it's given, not add new data) and suggest that the Mac generation should always be done in the caller when needed rather than passing a strange flag down to the parser. (I know the parser was already generating the MAC; I think it should be changed to *not* do that) (or at the very least the flag shouldn't be "on == don't do something extra", it should be "on == do something extra").
I understand your point, but the main issue with this is that there a very many places where virDomainDefParseXML is called, and nearly all of them require automatic MAC generation if omitted. If we pushed that into the callers, then we'd trivally miss places where MAC generation was required, and never notice until someone wonders why a VM is getting a NIC with mac of all-zeroes.
In this specific case, rather than turning off MAC generation, what we actually want to do is *mandate* that the MAC is always present in the XML we're given. When detaching a NIC, it is never acceptable to leave out the MAC addr. So I'd have a flag VIR_DOMAIN_PARSE_REQUIRE_MAC and thus raise a fatal if omitted from the XML
I am not completely convinced this is what we want. If domain has exactly one NIC, device-detach semantic is clear. Or if we want to allow detaching interface by PCI address - MAC shouldn't be required, because it is redundant.
You're missing my point here - virsh is what is broken. It should *not* be trying to guess what the unique attribute the HV driver wants. Each HV may have different decision about this. Applications using the method virDomainDetachDevice, are required to pass the full XML description for the device to be detached as it is currently shown in the guest XML config. virsh really needs to call virDomainDumpXML and then extract the <inteface> element that it wishes to detach, rather than trying to craft some partial XML description on what it thinks the HV might want.
N.B.: MAC itself actually is not unique identifier - one can attach as many NICs with the same MAC address as he wants. The only true unique ID is PCI address.
That is a also bug. The QEMU driver should be validating the MAC address *is* unique for each of the guests' NICs. PCI address is not suitable because not all NICs have PCI addresses. eg USB NIC, ISA NICs, or s/390 doesn't use PCI at all. 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 :|
On 03/18/2011 10:13 AM, Daniel P. Berrange wrote:
I am not completely convinced this is what we want. If domain has exactly one NIC, device-detach semantic is clear. Or if we want to allow detaching interface by PCI address - MAC shouldn't be required, because it is redundant.
You're missing my point here - virsh is what is broken. It should *not* be trying to guess what the unique attribute the HV driver wants. Each HV may have different decision about this. Applications using the method virDomainDetachDevice, are required to pass the full XML description for the device to be detached as it is currently shown in the guest XML config.
virsh really needs to call virDomainDumpXML and then extract the <inteface> element that it wishes to detach, rather than trying to craft some partial XML description on what it thinks the HV might want.
If I'm understanding correctly: In other words, we can still do some smarts where partial info from the user is turned into the right interface, but the smarts should live in virsh not the drivers. That is, virsh is the one that should say dumpxml only gave one device so that's the one to delete, vs. dumpxml gave two devices, so some additional argument (be it --mac, --address, or something else) better be present and unambiguously match one of the two devices. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Fri, Mar 18, 2011 at 10:28:34AM -0600, Eric Blake wrote:
On 03/18/2011 10:13 AM, Daniel P. Berrange wrote:
I am not completely convinced this is what we want. If domain has exactly one NIC, device-detach semantic is clear. Or if we want to allow detaching interface by PCI address - MAC shouldn't be required, because it is redundant.
You're missing my point here - virsh is what is broken. It should *not* be trying to guess what the unique attribute the HV driver wants. Each HV may have different decision about this. Applications using the method virDomainDetachDevice, are required to pass the full XML description for the device to be detached as it is currently shown in the guest XML config.
virsh really needs to call virDomainDumpXML and then extract the <inteface> element that it wishes to detach, rather than trying to craft some partial XML description on what it thinks the HV might want.
If I'm understanding correctly: In other words, we can still do some smarts where partial info from the user is turned into the right interface, but the smarts should live in virsh not the drivers. That is, virsh is the one that should say dumpxml only gave one device so that's the one to delete, vs. dumpxml gave two devices, so some additional argument (be it --mac, --address, or something else) better be present and unambiguously match one of the two devices.
Yes, that's pretty much what I meant. 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 :|
Until now, users could detach interface by MAC address, which is not sufficient in cases when MAC is not unique. Therefore we need to support detaching via PCI address as well. Or in case when domain has exactly one interface users does not need to specify any MAC/PCI address at all. --- src/qemu/qemu_driver.c | 3 ++- src/qemu/qemu_hotplug.c | 46 ++++++++++++++++++++++++++++++++++------------ 2 files changed, 36 insertions(+), 13 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index dac2bf2..3bf5d2e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4293,7 +4293,8 @@ static int qemudDomainDetachDevice(virDomainPtr dom, } dev = virDomainDeviceDefParse(driver->caps, vm->def, xml, - VIR_DOMAIN_XML_INACTIVE); + VIR_DOMAIN_XML_INACTIVE | + VIR_DOMAIN_PARSE_NO_GENERATE); if (dev == NULL) goto endjob; diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index d8a8e5d..4175d2c 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1470,7 +1470,7 @@ int qemuDomainDetachNetDevice(struct qemud_driver *driver, virDomainDeviceDefPtr dev, virBitmapPtr qemuCaps) { - int i, ret = -1; + int i, interface_num = 0, ret = -1; virDomainNetDefPtr detach = NULL; qemuDomainObjPrivatePtr priv = vm->privateData; int vlan; @@ -1478,19 +1478,41 @@ int qemuDomainDetachNetDevice(struct qemud_driver *driver, for (i = 0 ; i < vm->def->nnets ; i++) { virDomainNetDefPtr net = vm->def->nets[i]; - - if (!memcmp(net->mac, dev->data.net->mac, VIR_MAC_BUFLEN)) { + virDomainDevicePCIAddress pci[2] = {net->info.addr.pci, + dev->data.net->info.addr.pci}; + + if ( ((dev->data.net->mac != NULL) && + !memcmp(net->mac, dev->data.net->mac, VIR_MAC_BUFLEN)) || + ((dev->data.net->mac == NULL) && (vm->def->nnets == 1)) || + ((pci[0].domain == pci[1].domain) && + (pci[0].bus == pci[1].bus) && + (pci[0].slot == pci[1].slot) && + (pci[0].function == pci[1].function)) ) { + if (detach) { + qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("ambiguous device specification. " + "Use <mac> and/or <address>")); + goto cleanup; + } detach = net; - break; + interface_num = i; } } if (!detach) { - qemuReportError(VIR_ERR_OPERATION_FAILED, - _("network device %02x:%02x:%02x:%02x:%02x:%02x not found"), - dev->data.net->mac[0], dev->data.net->mac[1], - dev->data.net->mac[2], dev->data.net->mac[3], - dev->data.net->mac[4], dev->data.net->mac[5]); + if (vm->def->nnets == 0) { + qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("domain has no device to detach")); + } else if (dev->data.net->mac) { + qemuReportError(VIR_ERR_OPERATION_FAILED, + _("network device %02x:%02x:%02x:%02x:%02x:%02x not found"), + dev->data.net->mac[0], dev->data.net->mac[1], + dev->data.net->mac[2], dev->data.net->mac[3], + dev->data.net->mac[4], dev->data.net->mac[5]); + } else { + qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("network device not found. Use <mac> and/or <address>")); + } goto cleanup; } @@ -1571,10 +1593,10 @@ int qemuDomainDetachNetDevice(struct qemud_driver *driver, } if (vm->def->nnets > 1) { - memmove(vm->def->nets + i, - vm->def->nets + i + 1, + memmove(vm->def->nets + interface_num, + vm->def->nets + interface_num + 1, sizeof(*vm->def->nets) * - (vm->def->nnets - (i + 1))); + (vm->def->nnets - (interface_num + 1))); vm->def->nnets--; if (VIR_REALLOC_N(vm->def->nets, vm->def->nnets) < 0) { /* ignore, harmless */ -- 1.7.4
On Thu, Mar 17, 2011 at 03:38:48PM +0100, Michal Privoznik wrote:
Until now, users could detach interface by MAC address, which is not sufficient in cases when MAC is not unique. Therefore we need to support detaching via PCI address as well. Or in case when domain has exactly one interface users does not need to specify any MAC/PCI address at all. --- src/qemu/qemu_driver.c | 3 ++- src/qemu/qemu_hotplug.c | 46 ++++++++++++++++++++++++++++++++++------------ 2 files changed, 36 insertions(+), 13 deletions(-)
The semantics of the virDomainDetachDevice method are that you should pass the complete XML for the device, as obtained from the current domain XML dump. So as far as virDomainDetachDevice is concerned all XML passed in should have the MAC address already. The problem is really with the way virsh is calling this. It should really be querying the XML of the guest & then extracting the device it wants to detach & passing that data as is, rather than manually building up XML. So IMHO this patch is really not addressing the real problem. Just hacking around it in one hypervisor driver, is not solving it for all the other drivers, so NACK for now. 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 :|
--- src/xen/xm_internal.c | 65 ++++++++++++++++++++++++++++++++++++++++--------- 1 files changed, 53 insertions(+), 12 deletions(-) diff --git a/src/xen/xm_internal.c b/src/xen/xm_internal.c index 9a9fa0c..5b074b3 100644 --- a/src/xen/xm_internal.c +++ b/src/xen/xm_internal.c @@ -1445,7 +1445,7 @@ xenXMDomainDetachDeviceFlags(virDomainPtr domain, const char *xml, virDomainDeviceDefPtr dev = NULL; virDomainDefPtr def; int ret = -1; - int i; + int i, interface_num = -1; xenUnifiedPrivatePtr priv; if ((!domain) || (!domain->conn) || (!domain->name) || (!xml)) { @@ -1475,7 +1475,8 @@ xenXMDomainDetachDeviceFlags(virDomainPtr domain, const char *xml, if (!(dev = virDomainDeviceDefParse(priv->caps, entry->def, - xml, VIR_DOMAIN_XML_INACTIVE))) + xml, VIR_DOMAIN_XML_INACTIVE | + VIR_DOMAIN_PARSE_NO_GENERATE))) goto cleanup; switch (dev->type) { @@ -1501,18 +1502,58 @@ xenXMDomainDetachDeviceFlags(virDomainPtr domain, const char *xml, case VIR_DOMAIN_DEVICE_NET: { for (i = 0 ; i < def->nnets ; i++) { - if (!memcmp(def->nets[i]->mac, dev->data.net->mac, - VIR_MAC_BUFLEN)) { - virDomainNetDefFree(def->nets[i]); - if (i < (def->nnets - 1)) - memmove(def->nets + i, - def->nets + i + 1, - sizeof(*def->nets) * - (def->nnets - (i + 1))); - def->nnets--; - break; + virDomainDevicePCIAddress pci[2] = {def->nets[i]->info.addr.pci, + dev->data.net->info.addr.pci}; + + if ( ((dev->data.net->mac != NULL) && + !memcmp(def->nets[i]->mac, dev->data.net->mac, VIR_MAC_BUFLEN)) || + ((dev->data.net->mac == NULL) && (def->nnets == 1)) || + ((pci[0].domain == pci[1].domain) && + (pci[0].bus == pci[1].bus) && + (pci[0].slot == pci[1].slot) && + (pci[0].function == pci[1].function)) ) { + if (interface_num >= 0) { + xenXMError(VIR_ERR_OPERATION_FAILED, "%s", + _("ambiguous device specification. " + "Use <mac> and/or <address>")); + goto cleanup; + } + interface_num = i; + } + } + + if (interface_num < 0) { + if (def->nnets == 0) { + xenXMError(VIR_ERR_OPERATION_FAILED, "%s", + _("domain has no device to detach")); + } else if (dev->data.net->mac) { + xenXMError(VIR_ERR_OPERATION_FAILED, _("network device " + "%02x:%02x:%02x:%02x:%02x:%02x not found"), + dev->data.net->mac[0], dev->data.net->mac[1], + dev->data.net->mac[2], dev->data.net->mac[3], + dev->data.net->mac[4], dev->data.net->mac[5]); + } else { + xenXMError(VIR_ERR_OPERATION_FAILED, "%s", _("network " + "device not found. Use <mac> and/or <address>")); } + goto cleanup; } + + if (def->nnets > 1) { + memmove(def->nets + interface_num, + def->nets + interface_num + 1, + sizeof(*def->nets) * + (def->nnets - (interface_num + 1))); + def->nnets--; + if (VIR_REALLOC_N(def->nets, def->nnets) < 0) { + /* ingore, harmless */ + } + } else { + VIR_FREE(def->nets); + def->nnets = 0; + } + + virDomainNetDefFree(def->nets[interface_num]); break; } default: -- 1.7.4
participants (5)
-
Daniel P. Berrange -
Eric Blake -
Laine Stump -
Michal Privoznik -
Michal Prívozník