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

[Eric Blake]

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?
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().


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;

> +++ 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.

> +                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.

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20110708/bda62d3c/attachment-0001.sig>