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

On 02/14/2013 05:00 AM, Stefan Berger wrote: 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. 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