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

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(a)linux.vnet.ibm.com&gt; > Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com&gt; > --- > 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).