[libvirt] [PATCH 3/5] virDomainNetDefParseXML: suppress generation of MAC when VIR_DOMAIN_PARSE_NO_GENERATE is set

[Daniel P. Berrange]

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