[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH V6 2/3] Introduce file descriptor set for QEMU domains



On 02/14/2013 05:00 AM, Stefan Berger wrote:
> Extend the QEMU private domain structure with virFdSet.
> Persist the virFdSet using XML and parse its XML.
> Reset the FdSet upon domain stop.
> 
> Stefan Berger <stefanb linux vnet ibm com>
> 
> ---
>  v5->v6:
>   - change found in patch 3 moved to this patch
> 
> @@ -470,6 +480,9 @@ static int qemuDomainObjPrivateXMLParse(
>  
>      priv->fakeReboot = virXPathBoolean("boolean(./fakereboot)", ctxt) == 1;
>  
> +    if (virFdSetParseXML(priv->fdset, "./fdset/entry", ctxt) < 0)
> +        goto error;

Does this work on the upgrade path?  If older libvirt did not use
fdsets, then the XML element will not be present, but in patch 1, you had:

+    if ((n = virXPathNodeSet(xPath, ctxt, &nodes)) < 0) {
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       "%s", _("failed to parse qemu file descriptor
sets"));
+        goto error;
+    }

which implies an error if there was nothing to parse.

But in reality, nothing to parse should be treated the same as success -
there's no set to reinstate, because it was from an older libvirt that
didn't use a set.

/me rereads patch 1...

Ewww - this works, but only by accident.  In patch 1, you initialized
int ret = 0, therefore, the 'error:' label is reached with ret still at
0, and the function returns success.  Typically, we name a label
'cleanup:' rather than 'error:' when the label can be used on the
success path; also, we tend to initialize ret to -1 and then flip it to
0 just before cleanup, instead of your approach of initializing to 0 and
flipping to -1 on all call sites that goto error because of a real problem.

> +++ libvirt/src/qemu/qemu_process.c
> @@ -4255,6 +4255,8 @@ void qemuProcessStop(virQEMUDriverPtr dr
>          priv->monConfig = NULL;
>      }
>  
> +    virFdSetReset(priv->fdset);

An offline domain doesn't need an fdset.  I think that rather than
pre-allocate the set when creating the vm, then resetting it, that
instead we create it just before starting a domain, and free the set
when stopping the domain:

virObjectUnref(priv->fdset);
priv->fdset = NULL;

so that we aren't wasting memory for a set with offline domains, since
offline domains also have no aliases that would live in an fdset.

And if we do that, then my complaint on 1/3 about not needing a public
virFdSetReset() is valid, as this appears to be the only place you do a
reset.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]