[libvirt] [PATCH 01/12] virsh: Force boot emulation is only required for virDomainCreateWithFlags

[John Ferlan]

On 05/09/2018 10:56 AM, Marc Hartmayer wrote:
> The force boot emulation is only required for virDomainCreateWithFlags
> as the flag VIR_DOMAIN_START_FORCE_BOOT was introduced with the commit
> 27c85260532f879be5674a4eed0811c21fd34f94 (2011). But
> virDomainCreateXMLWithFiles is newer and therefore already had support
> for VIR_DOMAIN_START_FORCE_BOOT. This means there is now no second
> call with VIR_DOMAIN_START_FORCE_BOOT removed from flags for
> virDomainCreateXMLWithFiles in case the first call
> fails. virDomainCreateXMLWithFiles was introduced with commit
> d76227bea35cc49374a94414f6d46e3493ac2a52 (2013).
> 
> This patch takes this into account and simplifies the function. In
> addition, it's now easier to extend the function.
> 
> Signed-off-by: Marc Hartmayer <mhartmay at linux.vnet.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy at linux.ibm.com>
> ---
>  tools/virsh-domain.c | 52 ++++++++++++++++++++++++++++------------------------
>  1 file changed, 28 insertions(+), 24 deletions(-)
> 

OK so I took the bait ;-)...  I agree 110% what you've changed to is
easier to read; however, I think there's a subtle difference with your
changes...  I'm not sure this new flow will work quite right "if" for
some reason someone passes the --force-boot flag to/for a startup that
uses CreateWithFiles, such as lxc.

> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index 598d2fa4a4bd..7cf8373f05bc 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -4038,40 +4038,44 @@ cmdStart(vshControl *ctl, const vshCmd *cmd)
>      if (vshCommandOptBool(cmd, "force-boot"))
>          flags |= VIR_DOMAIN_START_FORCE_BOOT;>
> -    /* We can emulate force boot, even for older servers that reject it.  */

FWIW: I see that this hunk was added by commit id 691ec08b shortly after
adding support for force_boot via commit id 27c85260.

> -    if (flags & VIR_DOMAIN_START_FORCE_BOOT) {
> -        if ((nfds ?
> -             virDomainCreateWithFiles(dom, nfds, fds, flags) :
> -             virDomainCreateWithFlags(dom, flags)) == 0)
> -            goto started;
> -        if (last_error->code != VIR_ERR_NO_SUPPORT &&
> -            last_error->code != VIR_ERR_INVALID_ARG) {
> -            vshReportError(ctl);
> -            goto cleanup;
> -        }

Previously if flags & VIR_DOMAIN_START_FORCE_BOOT, we'd try w/
CreateWithFiles without changing @flags.

For lxc, eventually we'd end up in lxcDomainCreateWithFiles which would
do a virCheckFlags and "fail" with VIR_ERR_INVALID_ARG because the flag
VIR_DOMAIN_START_FORCE_BOOT is not supported.

That would fall through this removed code and remove the FORCE_BOOT
flag. Then we'd again call virDomainCreateWithFiles w/ @flags adjusted
to remove VIR_DOMAIN_START_FORCE_BOOT

> -        vshResetLibvirtError();
> -        rc = virDomainHasManagedSaveImage(dom, 0);
> -        if (rc < 0) {
> -            /* No managed save image to remove */
> -            vshResetLibvirtError();
> -        } else if (rc > 0) {
> -            if (virDomainManagedSaveRemove(dom, 0) < 0) {
> +    /* Prefer older API unless we have to pass extra parameters */
> +    if (nfds) {
> +        rc = virDomainCreateWithFiles(dom, nfds, fds, flags);

With this new code if @flags has VIR_DOMAIN_START_FORCE_BOOT, then
there's no fallback and we fail.

> +    } else if (flags) {
> +        rc = virDomainCreateWithFlags(dom, flags);
> +        /* We can emulate force boot, even for older servers that
> +         * reject it. */
> +        if (rc < 0 && flags & VIR_DOMAIN_START_FORCE_BOOT) {
> +            if (last_error->code != VIR_ERR_NO_SUPPORT &&
> +                last_error->code != VIR_ERR_INVALID_ARG) {

The CreateWithFlags code could "fail" w/ INVALID_ARG if some hypervisor
didn't support some flag, but even calling it again without the
FORCE_BOOT may not change the result.

This (existing, I know) code assumes the failure is from the lack of a
FORCE_BOOT flag as added via commit 691ec08b.  Having commit id afb50d79
further complicate the logic especially w/r/t FORCE_BOOT (which I think
makes no sense for a container hypervisor, but then again I'm not the
expert there ;-)).  Still since it's in the API and documented, it's not
like we could remove it (whether or not it was an unintentional
cut-copy-paste, change API name).

John


>                  vshReportError(ctl);
>                  goto cleanup;
>              }
> +            vshResetLibvirtError();
> +            rc = virDomainHasManagedSaveImage(dom, 0);
> +            if (rc < 0) {
> +                /* No managed save image to remove */
> +                vshResetLibvirtError();
> +            } else if (rc > 0) {
> +                if (virDomainManagedSaveRemove(dom, 0) < 0) {
> +                    vshReportError(ctl);
> +                    goto cleanup;
> +                }
> +            }
> +
> +            /* now try it again without the force boot flag */
> +            flags &= ~VIR_DOMAIN_START_FORCE_BOOT;
> +            rc = virDomainCreateWithFlags(dom, flags);
>          }
> -        flags &= ~VIR_DOMAIN_START_FORCE_BOOT;
> +    } else {
> +        rc = virDomainCreate(dom);
>      }
>  
> -    /* Prefer older API unless we have to pass a flag.  */
> -    if ((nfds ? virDomainCreateWithFiles(dom, nfds, fds, flags) :
> -         (flags ? virDomainCreateWithFlags(dom, flags)
> -          : virDomainCreate(dom))) < 0) {
> +    if (rc < 0) {
>          vshError(ctl, _("Failed to start domain %s"), virDomainGetName(dom));
>          goto cleanup;
>      }
>  
> - started:
>      vshPrintExtra(ctl, _("Domain %s started\n"),
>                    virDomainGetName(dom));
>  #ifndef WIN32
>