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@linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
---
tools/virsh-domain.c | 52 ++++++++++++++++++++++++++++------------------------
1 file changed, 28 insertions(+), 24 deletions(-)
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. */
- 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;
- }
- 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);
+ } 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) {
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
--
2.13.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, May 09, 2018 at 04:56 PM +0200, Marc Hartmayer <mhartmay@linux.vnet.ibm.com> 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@linux.vnet.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
> ---
> tools/virsh-domain.c | 52 ++++++++++++++++++++++++++++------------------------
> 1 file changed, 28 insertions(+), 24 deletions(-)
>
> 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. */
> - 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;
> - }
> - 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);
> + } 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) {
> 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
> --
> 2.13.4
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
Since this patch is not directly related to the proposed API it may be
still useful => polite ping :)
Beste Grüße / Kind regards
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@linux.vnet.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy@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
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Jun 13, 2018 at 12:53 AM +0200, John Ferlan <jferlan@redhat.com> wrote:
> 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@linux.vnet.ibm.com>
>> Reviewed-by: Boris Fiuczynski <fiuczy@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.
Okay. I haven’t thought that a newer API wouldn’t support this flag.
>
> 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).
Anyway, thanks for the review!
[…snip]
--
Beste Grüße / Kind regards
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.