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

[Michal Prívozník]

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