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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
