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

Re: [libvirt] [PATCH 3/3] Add support for shutdown / reboot APIs in LXC driver



> Add support for doing controlled shutdown / reboot in the LXC
> driver. The default behaviour is to try talking to /dev/initctl
> inside the container's virtual root (/proc/$INITPID/root). This
> works with sysvinit or systemd. If that file does not exist
> then send SIGTERM (for shutdown) or SIGHUP (for reboot). These
> signals are not any kind of particular standard for shutdown
> or reboot, just something apps can choose to handle. The new
> virDomainSendProcessSignal allows for sending custom signals.
> 
> We might allow the choice of SIGTERM/HUP to be configured for
> LXC containers via the XML in the future.

Maybe even via new attributes or sub-elements of the existing
<on_reboot> XML.

> +
> +    if (flags == 0 ||
> +        (flags & VIR_DOMAIN_SHUTDOWN_INITCTL)) {
> +        if ((rc =
> virInitctlSetRunLevel(VIR_INITCTL_RUNLEVEL_POWEROFF,
> +                                        vroot)) < 0) {
> +            goto cleanup;
> +        }
> +        if (rc == 0 && flags != 0 &&

Based on libvirt.c (well, if you follow my advice in 2/3 about
enforcing the mutual exclusion between the four explicit
shutdown methods), if 'flags != 0', then we are guaranteed
that 'flags == VIR_DOMAIN_SHUTDOWN_INITCTL)' at this point...

> +            ((flags & ~VIR_DOMAIN_SHUTDOWN_INITCTL) == 0)) {

...which means this leg of the conditional is always true.

Or is your intent to allow the user to specify multiple flags
rather than enforcing mutual exclusion between the flags?  And
if so, you need to fix libvirt.c to drop the mutual exclusion,
as well as to document that when multiple flags are specified,
it is up to the hypervisor which method is attempted first.

> +static int
> +lxcDomainReboot(virDomainPtr dom,
> +                unsigned int flags)
> +{
> +    virLXCDriverPtr driver = dom->conn->privateData;
> +    virLXCDomainObjPrivatePtr priv;
> +    virDomainObjPtr vm;
> +    char *vroot = NULL;
> +    int ret = -1;
> +    int rc;
> +
> +    virCheckFlags(VIR_DOMAIN_REBOOT_INITCTL |
> +                  VIR_DOMAIN_REBOOT_SIGNAL, -1);

This code is very similar to the shutdown; is it worth factoring
out a helper method that takes as additional arguments the choice
of initctl message (_POWEROFF vs. _REBOOT) and signal (SIGKILL
vs. SIGHUP) to use?

> +    if (flags == 0 ||
> +        (flags & VIR_DOMAIN_REBOOT_INITCTL)) {
> +        if ((rc = virInitctlSetRunLevel(VIR_INITCTL_RUNLEVEL_REBOOT,
> +                                        vroot)) < 0) {
> +            goto cleanup;
> +        }
> +        if (rc == 0 && flags != 0 &&
> +            ((flags & ~VIR_DOMAIN_SHUTDOWN_INITCTL) == 0)) {

Same review comments apply regarding whether we enforce mutual
exclusion of flags at the API level.


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