[libvirt] [PATCH 09/10] qemu: use virDomainNetGetActual*() functions where appropriate

[Laine Stump]

On 07/08/2011 04:18 PM, Eric Blake wrote:
> On 07/05/2011 01:45 AM, Laine Stump wrote:
>> The qemu driver accesses fields in the virDomainNetDef directly, but
>> with the advent of the virDomainActualNetDef, some pieces of
>> information may be found in a different place (the ActualNetDef) if
>> the network connection is of type='network' and that network is of
>> forward type='bridge|private|vepa|passthrough'. The previous patch
>> added functions to mask this difference from callers - they hide the
>> decision making process and just pick the value from the proper place.
>>
>> This patch uses those functions in the qemu driver as a first step in
>> making qemu work with the new network types. At this point, it's
>> assumed that the virDomainActualNetDef is already properly initialized
>> (it isn't yet).
> Is this going to bite anyone during bisection of this patch series?

No. I misused the word "initialized" there. I probably should have just 
said "set". The virDomainActualNetDef *is* "properly initialized" to 
NULL, and when it's null, all behavior with this patch in place is 
identical to what it would have been without the patch. What is being 
assumed here is that an ActualNetDef might really be present, but one 
never is, that's all.


> Hopefully not, so I'm not sure how much you want to rework this while
> rebasing, which means you can probably keep the code as-is.  But my
> approach would have been:
>
> patch 1 - introduce wrapper functions that make no semantic change,
> while updating all callers to use wrapper functions.  Something like:
>
> int
> virDomainNetGetActualType(virDomainNetDefPtr iface)
> {
>      return iface->type;
> }
>
> and replace all uses of iface->type with virDomainNetGetActualType().


We can't replace *all* uses. There are times when we want to know the 
original type.


> patch 2 - enhance wrapper functions to actually look into
> virDomainActualNetDef, preferably while guaranteeing that it is
> initialized correctly
>
> at this stage, you fix the body of virDomainNetGetActualType to:
>
> if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK ||
>      !iface->data.network.actual)
>      return iface->type;
> return iface->data.network.actual->type;


I don't really see this two stage process (well, it was already 2, and 
this turns it into 3)  as necessary,

because 1) the code that added the ActualNetDef also ensured that it was 
always initialized to NULL, 2) there was no place in the code that 
changed the ActualNetDef, and 3) if the ActualNetDef is NULL, 
virDomainNetGetActualType will *always* return iface->type.



>> +++ b/src/qemu/qemu_driver.c
>> @@ -3935,9 +3935,35 @@ static char *qemuDomainXMLToNative(virConnectPtr conn,
>>       for (i = 0 ; i<  def->nnets ; i++) {
>>           virDomainNetDefPtr net = def->nets[i];
>>           int bootIndex = net->bootIndex;
>> -        if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK ||
>> -            net->type == VIR_DOMAIN_NET_TYPE_DIRECT) {
>> +        if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) {
>> +            int actualType = virDomainNetGetActualType(net);
>>               VIR_FREE(net->data.network.name);
>> +            VIR_FREE(net->data.network.portgroup);
>> +            if (actualType == VIR_DOMAIN_NET_TYPE_BRIDGE) {
> And here is an instance where you are refactoring existing code
> (converting net->type to virDomainNetGetActualType(net)) as well as
> adding new code (taking action if actualType is TYPE_BRIDGE).
> Separating the refactoring from the introduction of new features can
> make review a bit easier.


Okay, I can agree with that. I can put that in a different patch.


>> +                char *brname = strdup(virDomainNetGetActualBridgeName(net));
>> +                virDomainActualNetDefFree(net->data.network.actual);
>> +
>> +                memset(net, 0, sizeof *net);
>> +
>> +                net->type = VIR_DOMAIN_NET_TYPE_ETHERNET;
>> +                net->data.ethernet.dev = brname;
> Need to check for strdup failure, rather than setting dev to NULL.
>


Yep. I missed that one.