Re: [libvirt] [PATCH 09/10] qemu: use virDomainNetGetActual*() functions where appropriate
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.