Re: [PATCH v5 1/1] util/async-teardown: wire up query-command-line-options

[Paolo Bonzini <pbonzini@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 6593 bytes --]

I am honestly not a fan of adding a more complex option,.just because
query-command-line-options only returns the square holes whereas here we
got a round one.

Can we imagine another functionality that would be added to -teardown? If
not, it's not a good design. If it works, I would add a completely dummy
(no suboptions) group "async-teardown" and not modify the parsing at all.

Paolo

Il lun 27 mar 2023, 15:35 Claudio Imbrenda <imbrenda@linux.ibm.com> ha
scritto:

> Add new -teardown option with an async=on|off parameter. It is visible
> in the output of query-command-line-options QMP command, so it can be
> discovered and used by libvirt.
>
> The option -async-teardown is now redundant. We'd normally deprecate it
> and remove it after a grace period, but it was introduced only in the
> previous version and it had no users, since it was not visible in the
> query-command-line-options QMP command. Drop it.
>
> Reported-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
> Fixes: c891c24b1a ("os-posix: asynchronous teardown for shutdown on Linux")
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> ---
>  docs/about/removed-features.rst |  5 +++++
>  os-posix.c                      | 15 +++++++++++++--
>  qemu-options.hx                 | 33 +++++++++++++++++++--------------
>  util/async-teardown.c           | 21 +++++++++++++++++++++
>  4 files changed, 58 insertions(+), 16 deletions(-)
>
> diff --git a/docs/about/removed-features.rst
> b/docs/about/removed-features.rst
> index 5b258b446b..6d89f69be9 100644
> --- a/docs/about/removed-features.rst
> +++ b/docs/about/removed-features.rst
> @@ -416,6 +416,11 @@ Input parameters that take a size value should only
> use a size suffix
>  the value is hexadecimal.  That is, '0x20M' should be written either as
>  '32M' or as '0x2000000'.
>
> +``-async-teardown`` (removed in 8.0)
> +,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,
> +
> +Use ``-teardown async=on`` instead.
> +
>  ``-chardev`` backend aliases ``tty`` and ``parport`` (removed in 8.0)
>  '''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
>
> diff --git a/os-posix.c b/os-posix.c
> index 5adc69f560..c1ca7b1cb3 100644
> --- a/os-posix.c
> +++ b/os-posix.c
> @@ -36,6 +36,8 @@
>  #include "qemu/log.h"
>  #include "sysemu/runstate.h"
>  #include "qemu/cutils.h"
> +#include "qemu/config-file.h"
> +#include "qemu/option.h"
>
>  #ifdef CONFIG_LINUX
>  #include <sys/prctl.h>
> @@ -132,6 +134,8 @@ static bool os_parse_runas_uid_gid(const char *optarg)
>   */
>  int os_parse_cmd_args(int index, const char *optarg)
>  {
> +    QemuOpts *opts;
> +
>      switch (index) {
>      case QEMU_OPTION_runas:
>          user_pwd = getpwnam(optarg);
> @@ -152,8 +156,15 @@ int os_parse_cmd_args(int index, const char *optarg)
>          daemonize = 1;
>          break;
>  #if defined(CONFIG_LINUX)
> -    case QEMU_OPTION_asyncteardown:
> -        init_async_teardown();
> +    case QEMU_OPTION_teardown:
> +        opts = qemu_opts_parse_noisily(qemu_find_opts("teardown"),
> +                                       optarg, false);
> +        if (!opts) {
> +            exit(1);
> +        }
> +        if (qemu_opt_get_bool(opts, "async", false)) {
> +            init_async_teardown();
> +        }
>          break;
>  #endif
>      default:
> diff --git a/qemu-options.hx b/qemu-options.hx
> index d42f60fb91..6a69b84f3c 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -4763,23 +4763,28 @@ DEF("qtest", HAS_ARG, QEMU_OPTION_qtest, "",
> QEMU_ARCH_ALL)
>  DEF("qtest-log", HAS_ARG, QEMU_OPTION_qtest_log, "", QEMU_ARCH_ALL)
>
>  #ifdef __linux__
> -DEF("async-teardown", 0, QEMU_OPTION_asyncteardown,
> -    "-async-teardown enable asynchronous teardown\n",
> +DEF("teardown", HAS_ARG, QEMU_OPTION_teardown,
> +    "-teardown async[=on|off]\n"
> +    "                process teardown options\n"
> +    "                async=on enables asynchronous teardown\n"
> +   ,
>      QEMU_ARCH_ALL)
> -#endif
>  SRST
> -``-async-teardown``
> -    Enable asynchronous teardown. A new process called
> "cleanup/<QEMU_PID>"
> -    will be created at startup sharing the address space with the main
> qemu
> -    process, using clone. It will wait for the main qemu process to
> -    terminate completely, and then exit.
> -    This allows qemu to terminate very quickly even if the guest was
> -    huge, leaving the teardown of the address space to the cleanup
> -    process. Since the cleanup process shares the same cgroups as the
> -    main qemu process, accounting is performed correctly. This only
> -    works if the cleanup process is not forcefully killed with SIGKILL
> -    before the main qemu process has terminated completely.
> +``-teardown``
> +    Set process teardown options.
> +
> +    ``async=on`` enables asynchronous teardown. A new process called
> +    "cleanup/<QEMU_PID>" will be created at startup sharing the address
> +    space with the main QEMU process, using clone. It will wait for the
> +    main QEMU process to terminate completely, and then exit. This allows
> +    QEMU to terminate very quickly even if the guest was huge, leaving the
> +    teardown of the address space to the cleanup process. Since the
> cleanup
> +    process shares the same cgroups as the main QEMU process, accounting
> is
> +    performed correctly. This only works if the cleanup process is not
> +    forcefully killed with SIGKILL before the main QEMU process has
> +    terminated completely.
>  ERST
> +#endif
>
>  DEF("msg", HAS_ARG, QEMU_OPTION_msg,
>      "-msg [timestamp[=on|off]][,guest-name=[on|off]]\n"
> diff --git a/util/async-teardown.c b/util/async-teardown.c
> index 62cdeb0f20..4a5dbce958 100644
> --- a/util/async-teardown.c
> +++ b/util/async-teardown.c
> @@ -12,6 +12,9 @@
>   */
>
>  #include "qemu/osdep.h"
> +#include "qemu/config-file.h"
> +#include "qemu/option.h"
> +#include "qemu/module.h"
>  #include <dirent.h>
>  #include <sys/prctl.h>
>  #include <sched.h>
> @@ -144,3 +147,21 @@ void init_async_teardown(void)
>      clone(async_teardown_fn, new_stack_for_clone(), CLONE_VM, NULL);
>      sigprocmask(SIG_SETMASK, &old_signals, NULL);
>  }
> +
> +static QemuOptsList qemu_teardown_opts = {
> +    .name = "teardown",
> +    .head = QTAILQ_HEAD_INITIALIZER(qemu_teardown_opts.head),
> +    .desc = {
> +        {
> +            .name = "async",
> +            .type = QEMU_OPT_BOOL,
> +        },
> +        { /* end of list */ }
> +    },
> +};
> +
> +static void register_teardown(void)
> +{
> +    qemu_add_opts(&qemu_teardown_opts);
> +}
> +opts_init(register_teardown);
> --
> 2.39.2
>
>

[-- Attachment #2: Type: text/html, Size: 8496 bytes --]