Re: [PATCH libvirt v1 2/3] Improve `virsh start --console` behavior
On 9/28/23 17:37, Marc Hartmayer wrote:
> When starting a guest via libvirt (`virsh start --console`), early
> console output was missed because the guest was started first and then
> the console was attached. This patch changes this to the following
> sequence:
>
> 1. create a paused guest
> 2. attach the console
> 3. resume the guest
>
> Reviewed-by: Boris Fiuczynski
> Signed-off-by: Marc Hartmayer
> ---
> tools/virsh-domain.c | 50 +++-
> 1 file changed, 35 insertions(+), 15 deletions(-)
>
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index 5c3c6d18aebf..36670039444c 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -4059,12 +4059,27 @@ static const vshCmdOptDef opts_start[] = {
> {.name = NULL}
> };
>
> +static int
> +virDomainCreateHelper(virDomainPtr dom, unsigned int nfds, int *fds,
> + unsigned int flags)
To make naming less confusing, we prefer 'virsh' prefix in virsh instead
of 'vir'. It makes it obvious whether a function is a public libvirt API
(virDomainCreate...) or a virsh helper (virshDomainCreate...)
> +{
> +/* Prefer older API unless we have to pass a flag. */
> +if (nfds > 0) {
> +return virDomainCreateWithFiles(dom, nfds, fds, flags);
> +} else if (flags != 0) {
> +return virDomainCreateWithFlags(dom, flags);
> +} else {
> +return virDomainCreate(dom);
> +}
> +}
> +
> static bool
> cmdStart(vshControl *ctl, const vshCmd *cmd)
> {
> g_autoptr(virshDomain) dom = NULL;
> #ifndef WIN32
> bool console = vshCommandOptBool(cmd, "console");
> +bool resume_domain = false;
> #endif
> unsigned int flags = VIR_DOMAIN_NONE;
> int rc;
> @@ -4083,8 +4098,14 @@ cmdStart(vshControl *ctl, const vshCmd *cmd)
> if (virshFetchPassFdsList(ctl, cmd, &nfds, &fds) < 0)
> return false;
>
> -if (vshCommandOptBool(cmd, "paused"))
> +if (vshCommandOptBool(cmd, "paused")) {
> flags |= VIR_DOMAIN_START_PAUSED;
> +#ifndef WIN32
> +} else if (console) {
> +flags |= VIR_DOMAIN_START_PAUSED;
> +resume_domain = true;
> +#endif
> +}
> if (vshCommandOptBool(cmd, "autodestroy"))
> flags |= VIR_DOMAIN_START_AUTODESTROY;
> if (vshCommandOptBool(cmd, "bypass-cache"))
> @@ -4096,12 +4117,7 @@ cmdStart(vshControl *ctl, const vshCmd *cmd)
>
> /* We can emulate force boot, even for older servers that reject it. */
> if (flags & VIR_DOMAIN_START_FORCE_BOOT) {
> -if (nfds > 0) {
> -rc = virDomainCreateWithFiles(dom, nfds, fds, flags);
> -} else {
> -rc = virDomainCreateWithFlags(dom, flags);
> -}
> -
> +rc = virDomainCreateHelper(dom, nfds, fds, flags);
> if (rc == 0)
> goto started;
>
> @@ -4124,14 +4140,18 @@ cmdStart(vshControl *ctl, const vshCmd *cmd)
> flags &= ~VIR_DOMAIN_START_FORCE_BOOT;
> }
>
> -/* Prefer older API unless we have to pass a flag. */
> -if (nfds > 0) {
> -rc = virDomainCreateWithFiles(dom, nfds, fds, flags);
> -} else if (flags != 0) {
> -rc = virDomainCreateWithFlags(dom, flags);
> -} else {
> -rc = virDomainCreate(dom);
> +rc = virDomainCreateHelper(dom, nfds, fds, flags);
> +#ifndef WIN32
> +/* If the driver does not support the paused flag, let's fallback to the
> old
> + * behavior without the flag. */
> +if (rc < 0 && resume_domain && last_error && last_error->code ==
> VIR_ERR_INVALID_ARG) {
Long line.
Michal
Re: [PATCH libvirt v1 2/3] Improve `virsh start --console` behavior
On 28/09/2023 17.37, Marc Hartmayer wrote: When starting a guest via libvirt (`virsh start --console`), early console output was missed because the guest was started first and then the console was attached. This patch changes this to the following sequence: 1. create a paused guest 2. attach the console 3. resume the guest Reviewed-by: Boris Fiuczynski Signed-off-by: Marc Hartmayer --- tools/virsh-domain.c | 50 +++- 1 file changed, 35 insertions(+), 15 deletions(-) Reviewed-by: Thomas Huth
[PATCH libvirt v1 2/3] Improve `virsh start --console` behavior
When starting a guest via libvirt (`virsh start --console`), early
console output was missed because the guest was started first and then
the console was attached. This patch changes this to the following
sequence:
1. create a paused guest
2. attach the console
3. resume the guest
Reviewed-by: Boris Fiuczynski
Signed-off-by: Marc Hartmayer
---
tools/virsh-domain.c | 50 +++-
1 file changed, 35 insertions(+), 15 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 5c3c6d18aebf..36670039444c 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -4059,12 +4059,27 @@ static const vshCmdOptDef opts_start[] = {
{.name = NULL}
};
+static int
+virDomainCreateHelper(virDomainPtr dom, unsigned int nfds, int *fds,
+ unsigned int flags)
+{
+/* Prefer older API unless we have to pass a flag. */
+if (nfds > 0) {
+return virDomainCreateWithFiles(dom, nfds, fds, flags);
+} else if (flags != 0) {
+return virDomainCreateWithFlags(dom, flags);
+} else {
+return virDomainCreate(dom);
+}
+}
+
static bool
cmdStart(vshControl *ctl, const vshCmd *cmd)
{
g_autoptr(virshDomain) dom = NULL;
#ifndef WIN32
bool console = vshCommandOptBool(cmd, "console");
+bool resume_domain = false;
#endif
unsigned int flags = VIR_DOMAIN_NONE;
int rc;
@@ -4083,8 +4098,14 @@ cmdStart(vshControl *ctl, const vshCmd *cmd)
if (virshFetchPassFdsList(ctl, cmd, &nfds, &fds) < 0)
return false;
-if (vshCommandOptBool(cmd, "paused"))
+if (vshCommandOptBool(cmd, "paused")) {
flags |= VIR_DOMAIN_START_PAUSED;
+#ifndef WIN32
+} else if (console) {
+flags |= VIR_DOMAIN_START_PAUSED;
+resume_domain = true;
+#endif
+}
if (vshCommandOptBool(cmd, "autodestroy"))
flags |= VIR_DOMAIN_START_AUTODESTROY;
if (vshCommandOptBool(cmd, "bypass-cache"))
@@ -4096,12 +4117,7 @@ cmdStart(vshControl *ctl, const vshCmd *cmd)
/* We can emulate force boot, even for older servers that reject it. */
if (flags & VIR_DOMAIN_START_FORCE_BOOT) {
-if (nfds > 0) {
-rc = virDomainCreateWithFiles(dom, nfds, fds, flags);
-} else {
-rc = virDomainCreateWithFlags(dom, flags);
-}
-
+rc = virDomainCreateHelper(dom, nfds, fds, flags);
if (rc == 0)
goto started;
@@ -4124,14 +4140,18 @@ cmdStart(vshControl *ctl, const vshCmd *cmd)
flags &= ~VIR_DOMAIN_START_FORCE_BOOT;
}
-/* Prefer older API unless we have to pass a flag. */
-if (nfds > 0) {
-rc = virDomainCreateWithFiles(dom, nfds, fds, flags);
-} else if (flags != 0) {
-rc = virDomainCreateWithFlags(dom, flags);
-} else {
-rc = virDomainCreate(dom);
+rc = virDomainCreateHelper(dom, nfds, fds, flags);
+#ifndef WIN32
+/* If the driver does not support the paused flag, let's fallback to the
old
+ * behavior without the flag. */
+if (rc < 0 && resume_domain && last_error && last_error->code ==
VIR_ERR_INVALID_ARG) {
+vshResetLibvirtError();
+
+flags &= ~VIR_DOMAIN_START_PAUSED;
+resume_domain = false;
+rc = virDomainCreateHelper(dom, nfds, fds, flags);
}
+#endif
if (rc < 0) {
vshError(ctl, _("Failed to start domain '%1$s'"),
virDomainGetName(dom));
@@ -4142,7 +4162,7 @@ cmdStart(vshControl *ctl, const vshCmd *cmd)
vshPrintExtra(ctl, _("Domain '%1$s' started\n"),
virDomainGetName(dom));
#ifndef WIN32
-if (console && !cmdRunConsole(ctl, dom, NULL, false, 0))
+if (console && !cmdRunConsole(ctl, dom, NULL, resume_domain, 0))
return false;
#endif
--
2.34.1
Re: [RFC PATCH libvirt v1 2/3] Improve `virsh start --console` behavior
On Tue, Sep 26, 2023 at 02:11:37PM +0200, Marc Hartmayer wrote:
> On Mon, Sep 25, 2023 at 04:15 PM +0100, Daniel P. Berrangé
> wrote:
> > On Mon, Sep 25, 2023 at 03:39:09PM +0200, Marc Hartmayer wrote:
> >> When starting a guest via libvirt (`virsh start --console`), early
> >> console output was missed because the guest was started first and then
> >> the console was attached. This patch changes this to the following
> >> sequence:
> >>
> >> 1. create a paused guest
> >> 2. attach the console
> >> 3. resume the guest
> >>
> >> Reviewed-by: Boris Fiuczynski
> >> Signed-off-by: Marc Hartmayer
> >> ---
> >> tools/virsh-domain.c | 11 +--
> >> 1 file changed, 9 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> >> index 5c3c6d18aebf..3581161c6f53 100644
> >> --- a/tools/virsh-domain.c
> >> +++ b/tools/virsh-domain.c
> >> @@ -4065,6 +4065,7 @@ cmdStart(vshControl *ctl, const vshCmd *cmd)
> >> g_autoptr(virshDomain) dom = NULL;
> >> #ifndef WIN32
> >> bool console = vshCommandOptBool(cmd, "console");
> >> +bool resume_domain = false;
> >> #endif
> >> unsigned int flags = VIR_DOMAIN_NONE;
> >> int rc;
> >> @@ -4083,8 +4084,14 @@ cmdStart(vshControl *ctl, const vshCmd *cmd)
> >> if (virshFetchPassFdsList(ctl, cmd, &nfds, &fds) < 0)
> >> return false;
> >>
> >> -if (vshCommandOptBool(cmd, "paused"))
> >> +if (vshCommandOptBool(cmd, "paused")) {
> >> flags |= VIR_DOMAIN_START_PAUSED;
> >> +#ifndef WIN32
> >> +} else if (console) {
> >> +flags |= VIR_DOMAIN_START_PAUSED;
> >> +resume_domain = true;
> >> +#endif
> >
> > Hypervisor drivers are not required to support VIR_DOMAIN_START_PAUSED.
> >
> > So we need to detect the error code, and retry without that flag
> > set as a fallback. Same in next patch.
>
> Yep, makes sense - will change. Thanks for the feedback.
It'll be VIR_ERR_INVALID_ARG btw for an unsupported flag.
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [RFC PATCH libvirt v1 2/3] Improve `virsh start --console` behavior
On Mon, Sep 25, 2023 at 04:15 PM +0100, Daniel P. Berrangé
wrote:
> On Mon, Sep 25, 2023 at 03:39:09PM +0200, Marc Hartmayer wrote:
>> When starting a guest via libvirt (`virsh start --console`), early
>> console output was missed because the guest was started first and then
>> the console was attached. This patch changes this to the following
>> sequence:
>>
>> 1. create a paused guest
>> 2. attach the console
>> 3. resume the guest
>>
>> Reviewed-by: Boris Fiuczynski
>> Signed-off-by: Marc Hartmayer
>> ---
>> tools/virsh-domain.c | 11 +--
>> 1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
>> index 5c3c6d18aebf..3581161c6f53 100644
>> --- a/tools/virsh-domain.c
>> +++ b/tools/virsh-domain.c
>> @@ -4065,6 +4065,7 @@ cmdStart(vshControl *ctl, const vshCmd *cmd)
>> g_autoptr(virshDomain) dom = NULL;
>> #ifndef WIN32
>> bool console = vshCommandOptBool(cmd, "console");
>> +bool resume_domain = false;
>> #endif
>> unsigned int flags = VIR_DOMAIN_NONE;
>> int rc;
>> @@ -4083,8 +4084,14 @@ cmdStart(vshControl *ctl, const vshCmd *cmd)
>> if (virshFetchPassFdsList(ctl, cmd, &nfds, &fds) < 0)
>> return false;
>>
>> -if (vshCommandOptBool(cmd, "paused"))
>> +if (vshCommandOptBool(cmd, "paused")) {
>> flags |= VIR_DOMAIN_START_PAUSED;
>> +#ifndef WIN32
>> +} else if (console) {
>> +flags |= VIR_DOMAIN_START_PAUSED;
>> +resume_domain = true;
>> +#endif
>
> Hypervisor drivers are not required to support VIR_DOMAIN_START_PAUSED.
>
> So we need to detect the error code, and retry without that flag
> set as a fallback. Same in next patch.
Yep, makes sense - will change. Thanks for the feedback.
[…snip]
--
Kind regards / Beste Grüße
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: David Faller
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
Re: [RFC PATCH libvirt v1 2/3] Improve `virsh start --console` behavior
On Mon, Sep 25, 2023 at 03:39:09PM +0200, Marc Hartmayer wrote:
> When starting a guest via libvirt (`virsh start --console`), early
> console output was missed because the guest was started first and then
> the console was attached. This patch changes this to the following
> sequence:
>
> 1. create a paused guest
> 2. attach the console
> 3. resume the guest
>
> Reviewed-by: Boris Fiuczynski
> Signed-off-by: Marc Hartmayer
> ---
> tools/virsh-domain.c | 11 +--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index 5c3c6d18aebf..3581161c6f53 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -4065,6 +4065,7 @@ cmdStart(vshControl *ctl, const vshCmd *cmd)
> g_autoptr(virshDomain) dom = NULL;
> #ifndef WIN32
> bool console = vshCommandOptBool(cmd, "console");
> +bool resume_domain = false;
> #endif
> unsigned int flags = VIR_DOMAIN_NONE;
> int rc;
> @@ -4083,8 +4084,14 @@ cmdStart(vshControl *ctl, const vshCmd *cmd)
> if (virshFetchPassFdsList(ctl, cmd, &nfds, &fds) < 0)
> return false;
>
> -if (vshCommandOptBool(cmd, "paused"))
> +if (vshCommandOptBool(cmd, "paused")) {
> flags |= VIR_DOMAIN_START_PAUSED;
> +#ifndef WIN32
> +} else if (console) {
> +flags |= VIR_DOMAIN_START_PAUSED;
> +resume_domain = true;
> +#endif
Hypervisor drivers are not required to support VIR_DOMAIN_START_PAUSED.
So we need to detect the error code, and retry without that flag
set as a fallback. Same in next patch.
> +}
> if (vshCommandOptBool(cmd, "autodestroy"))
> flags |= VIR_DOMAIN_START_AUTODESTROY;
> if (vshCommandOptBool(cmd, "bypass-cache"))
> @@ -4142,7 +4149,7 @@ cmdStart(vshControl *ctl, const vshCmd *cmd)
> vshPrintExtra(ctl, _("Domain '%1$s' started\n"),
>virDomainGetName(dom));
> #ifndef WIN32
> -if (console && !cmdRunConsole(ctl, dom, NULL, false, 0))
> +if (console && !cmdRunConsole(ctl, dom, NULL, resume_domain, 0))
> return false;
> #endif
>
> --
> 2.34.1
>
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
[RFC PATCH libvirt v1 2/3] Improve `virsh start --console` behavior
When starting a guest via libvirt (`virsh start --console`), early
console output was missed because the guest was started first and then
the console was attached. This patch changes this to the following
sequence:
1. create a paused guest
2. attach the console
3. resume the guest
Reviewed-by: Boris Fiuczynski
Signed-off-by: Marc Hartmayer
---
tools/virsh-domain.c | 11 +--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 5c3c6d18aebf..3581161c6f53 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -4065,6 +4065,7 @@ cmdStart(vshControl *ctl, const vshCmd *cmd)
g_autoptr(virshDomain) dom = NULL;
#ifndef WIN32
bool console = vshCommandOptBool(cmd, "console");
+bool resume_domain = false;
#endif
unsigned int flags = VIR_DOMAIN_NONE;
int rc;
@@ -4083,8 +4084,14 @@ cmdStart(vshControl *ctl, const vshCmd *cmd)
if (virshFetchPassFdsList(ctl, cmd, &nfds, &fds) < 0)
return false;
-if (vshCommandOptBool(cmd, "paused"))
+if (vshCommandOptBool(cmd, "paused")) {
flags |= VIR_DOMAIN_START_PAUSED;
+#ifndef WIN32
+} else if (console) {
+flags |= VIR_DOMAIN_START_PAUSED;
+resume_domain = true;
+#endif
+}
if (vshCommandOptBool(cmd, "autodestroy"))
flags |= VIR_DOMAIN_START_AUTODESTROY;
if (vshCommandOptBool(cmd, "bypass-cache"))
@@ -4142,7 +4149,7 @@ cmdStart(vshControl *ctl, const vshCmd *cmd)
vshPrintExtra(ctl, _("Domain '%1$s' started\n"),
virDomainGetName(dom));
#ifndef WIN32
-if (console && !cmdRunConsole(ctl, dom, NULL, false, 0))
+if (console && !cmdRunConsole(ctl, dom, NULL, resume_domain, 0))
return false;
#endif
--
2.34.1
