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@xxxxxxxxxxxxxxxxxx>
> Reviewed-by: Boris Fiuczynski <fiuczy@xxxxxxxxxxxxx>
> ---
> 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
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
