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@xxxxxxxxxxxxxxxxxx>
>
> ---
> 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
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list