Re: [PATCH libvirt v1 2/3] Improve `virsh start --console` behavior

2023-10-24 Thread Michal Prívozník
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

2023-10-23 Thread Thomas Huth

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