[libvirt] [PATCH 2/2] Implementation deficiency in virInitctlSetRunLevel v4

[Eric Blake]

On 12/20/2013 09:24 AM, Reco wrote:
> Use helper virProcessRunInMountNamespace in lxcDomainShutdownFlags and
> lxcDomainReboot.
> 

This fails to compile, due to [1] below:

  CCLD     libvirt_lxc
lxc/lxc_driver.c:2783:1: error: return type defaults to 'int' [-Werror]
 virDomainRebootCallback(pid_t pid ATTRIBUTE_UNUSED,
 ^
lxc/lxc_driver.c:2783:1: error: no previous prototype for
'virDomainRebootCallback' [-Werror=missing-prototypes]
cc1: all warnings being treated as errors

> ---
>  src/lxc/lxc_driver.c |   44 ++++++++++++++++++++++++++++----------------
>  1 file changed, 28 insertions(+), 16 deletions(-)
> 

>  
>  static int
> +virDomainShutdownCallback(pid_t pid ATTRIBUTE_UNUSED,
> +                          void *opaque ATTRIBUTE_UNUSED)
> +{
> +    int rc;
> +    rc = virInitctlSetRunLevel(VIR_INITCTL_RUNLEVEL_POWEROFF, NULL);
> +    return rc;

Hmm.  The callback's return value being used directly as the _exit()
value in patch 1 is not a good idea; either this function must convert
-1 to EXIT_FAILURE, or we should fix the framework to turn our normal -1
conventions into the correct exit code (I'm opting for the latter).
Also, you are now invoking code in between the fork/exec boundary off of
a multithreaded parent, which means we must now audit that all calls in
this code path are async-signal-safe (if not, the child will deadlock if
the fork happened in the parent while some other thread held a lock that
the child wants to obtain).

Alas, virInitctlSetRunLevel() calls virAsprintf() calls malloc(), which
is not async safe :(  [we've gotten away with it in the past; LXC is a
Linux-only technology, and glibc's malloc is relatively robust enough
that I've never seen it fail multithreaded fork/exec setups, whereas I
HAVE seen malloc deadlocks on other systems like Solaris].  It also
calls virReportSystemError() on failure, but in a child process, this
may not get logged the same was as an error message in the parent.  More
robust would be having the child be silent and just use distinct error
codes, then let the parent convert error codes into actual error
messages to be logged.  But I think we can wait until an actual report
of a deadlock before going to that effort.

Aha - you are passing NULL for vroot, which means we could get rid of
the malloc() issue.  In fact, we could get rid of the vroot argument
altogether, since all callers now pass NULL, and since we don't want to
be tempted to use the unsafe construct.

>      if (flags == 0 ||
>          (flags & VIR_DOMAIN_SHUTDOWN_INITCTL)) {
> -        if ((rc = virInitctlSetRunLevel(VIR_INITCTL_RUNLEVEL_POWEROFF,
> -                                        vroot)) < 0) {
> +        rc = virProcessRunInMountNamespace(priv->initpid,
> +                                           virDomainShutdownCallback,
> +                                           NULL);

We could use the opaque argument to pass our desired runlevel
constant... [2]

>  
> +
> +virDomainRebootCallback(pid_t pid ATTRIBUTE_UNUSED,
> +                        void *opaque ATTRIBUTE_UNUSED)

[1] you missed the 'static int' line here.

> +{
> +    int rc;
> +    rc = virInitctlSetRunLevel(VIR_INITCTL_RUNLEVEL_REBOOT, NULL);
> +    return rc;
> +}

[2] ... and have only one static callback helper instead of 2, since
they differ only by the constant used.

Good - I have enough cleanup to do that I can feel good about claiming
authorship of the final version (thus bypassing the full name question)
while still giving you credit for the find and initial attempts at the
fix.  v5 coming up soon!

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 621 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20131220/356d9e25/attachment-0001.sig>