[libvirt] [PATCH 1/3] Introduce flag representing if MAC address of interface was generated or not.

[Michal Prívozník]

On 02/25/2011 05:42 PM, Eric Blake wrote:
> On 02/24/2011 07:56 AM, Michal Privoznik wrote:
>> ---
>>   src/conf/domain_conf.c |    2 ++
>>   src/conf/domain_conf.h |    1 +
>>   2 files changed, 3 insertions(+), 0 deletions(-)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index b97c1f0..454f631 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -2593,8 +2593,10 @@ virDomainNetDefParseXML(virCapsPtr caps,
>>                                    (const char *)macaddr);
>>               goto error;
>>           }
>> +        def->mac_generated = false;
>>       } else {
>>           virCapabilitiesGenerateMac(caps, def->mac);
>> +        def->mac_generated = true;
>>       }
>
> Rather than sticking the flag into the domain definition, I'd rather
> pass it via the flags argument to
> virDomainDeviceDefParse/virDomainNetDefParseXML - that is, most callers
> will want:
>
> virDomainDeviceDefParse(..., 0)
>
> but qemuDomainDetachDevice (in qemu_driver.c) will want:
>
> virDomainDeviceDefParse(..., VIR_DOMAIN_PARSE_NO_GENERATE)
>
> (or maybe VIR_DOMAIN_XML_INACTIVE vs.
> VIR_DOMAIN_XML_INACTIVE|VIR_DOMAIN_PARSE_NO_GENERATE)
>
> That is, it's more than just detaching an interface mac addresses that
> has an issue.  It's anywhere that parsing a device XML snippet to be
> used to match an existing device that we want to suppress generation of
> additional items in the parse (this includes generating a random mac for
> an interface, but may include other generated items).  So adding a new
> flag that can be used to tell ALL parse routines to avoid generating
> extra data is more useful than changing the domain definition to say
> whether one piece of information was generated.
>

Yes, i firstly thought in similar way. But I don't really need to 
suppress generation of random values. I need information whether user 
gave say mac in input XML or not. Same apply for pci address (by the 
way, we don't support detaching by it now). The idea behind is:
if (<mac was generated> && <pci addr was generated>) {
    if (domain->nnets == 1)
        device_clone_mac(domain->nnet[0], device)
    else
        error("don't know which detach")
} else {
    search for device
}

If i suppress generation, I wouldn't know if it was generated anyway, 
because mac is stored in statically allocated array. One (tricky) way 
out of this is to set mac before to some special mac (e.g. all 0's) and 
then check for change. But I'd rather avoid it. Changing array to 
dynamically allocated is huge amount of work to be done, because it is 
hard to find pieces of code touching it, and omitting even one line may 
lead into sigegv.

But I agree, sticking the flag into domain definition is not very clean 
either.

One way out of this is adding pointer to flag as (another) function 
parameter. Would this be acceptable?

Michal