On 08/09/2012 09:20 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange@xxxxxxxxxx>
>
> Add two new APIs virLockSpaceNewPostExecRestart and
> virLockSpacePreExecRestart which allow a virLockSpacePtr
> object to be created from a JSON object and saved to a
> JSON object, for the purposes of re-exec'ing a process.
>
> As well as saving the state in JSON format, the second
> method will disable the O_CLOEXEC flag so that the open
> file descriptors are preserved across the process re-exec()
>
> Signed-off-by: Daniel P. Berrange <berrange@xxxxxxxxxx>
> ---
Is virLockSpacePreExecRestart called in the parent prior to forking
(mostly good, with one caveat) or in the child between fork and exec
(bad, since you malloc and do a lot of other non-async-safe stuff)? If
the latter, we risk deadlock; if the former, then you have at least one
bug...
> +
> +virLockSpacePtr virLockSpaceNewPostExecRestart(virJSONValuePtr object)
> +{
> + virLockSpacePtr lockspace;
> + virJSONValuePtr resources;
> + int n;
> + size_t i;
> +
> + VIR_DEBUG("object=%p", object);
Would it be any better to pretty-print the contents of object instead of
just listing its address?
> + if (virJSONValueObjectGetNumberInt(child, "fd", &res->fd) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Missing resource fd in JSON document"));
> + virLockSpaceResourceFree(res);
> + goto error;
> + }
> + if (virSetInherit(res->fd, false) < 0) {
> + virReportSystemError(errno, "%s",
> + _("Cannot enable close-on-exec flag"));
> + goto error;
Is the reparse loop called while the exec'd process is still
single-threaded, so as to avoid any races with the fds that are not yet
re-marked CLOEXEC?
> +
> +virJSONValuePtr virLockSpacePreExecRestart(virLockSpacePtr lockspace)
> +{
> +
> + if (virSetInherit(res->fd, true) < 0) {
> + virReportSystemError(errno, "%s",
> + _("Cannot disable close-on-exec flag"));
> + goto error;
> + }
...clearing O_CLOEXEC in the parent is wrong. You cannot clear
O_CLOEXEC until you are in the child (so as not to leak into any other
child process spawned by another thread), which really means that you
have to use virCommandPreserveFD instead of virSetInherit (virCommand
will then call virSetInherit in the child, between fork and exec, on
your behalf). But that makes it sound like you need to pass a
virCommandPtr in to this function, in addition to the lockspace.
Overall, looks like a mostly sane serialization - simpler than coding it
up in XML, and still robust to pass between parent and child, but you do
need to solve the CLOEXEC safety issues.
--
Eric Blake eblake@xxxxxxxxxx +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