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

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.

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.