5:53 p.m.
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>
Reviewed-by: Boris Fiuczynski <fiuczy(a)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