Re: [PATCH] Only advertise aio=io_uring if support is actually available

[Daniel P . Berrangé]

On Tue, Apr 19, 2022 at 07:19:31PM +0200, Dirk Müller wrote:
> This allows $qemu --help runtime configure checks for detecting
> the host support.

Note: detecting features by parsing --help output is something that
is explicitly a non-goal for QEMU. The only supported interface for
detecting features is QMP. We can't actively prevent people writing
code that parses --help, but if such parsing breaks on arrival of
new QEMU releases that is not considered a bug on the QEMU side.

That all said, the patch itself is OK, because for human targetted
interactive usage, it is desirable for --help to be representative
of what's available.

IOW, I'm just complaining about the commit message justification
here & warning against writing tools to use --help :-)

> Signed-off-by: Dirk Müller <dmueller@suse.de>
> ---
>  block/file-posix.c | 4 ++++
>  qemu-nbd.c         | 4 ++++
>  qemu-options.hx    | 6 +++++-
>  3 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/block/file-posix.c b/block/file-posix.c
> index 39a3d6dbe6..aec4763862 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -544,7 +544,11 @@ static QemuOptsList raw_runtime_opts = {
>          {
>              .name = "aio",
>              .type = QEMU_OPT_STRING,
> +#ifdef CONFIG_LINUX_IO_URING
>              .help = "host AIO implementation (threads, native, io_uring)",
> +#else
> +            .help = "host AIO implementation (threads, native)",
> +#endif

If we're going to conditionalize this, then we really ought to be
address it fully, because 'native' is also platform specific.

IOW, we would end up needing something more like this:

           .help = "host AIO implementation (threads"
 #if defined(WIN32) || defined(CONFIG_LINUX_AIO)
                   ", native"
 #endif
 #if defined(CONFIG_LINUX_IO_URING)
                   ", io_uring"
 #else
                   "),"

admittedly pretty ugly

>          },
>          {
>              .name = "aio-max-batch",
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index 713e7557a9..4634a0fc42 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -147,7 +147,11 @@ static void usage(const char *name)
>  "      --cache=MODE          set cache mode used to access the disk image, 
> the\n"
>  "                            valid options are: 'none', 'writeback' 
> (default),\n"
>  "                            'writethrough', 'directsync' and 'unsafe'\n"
> +#ifdef CONFIG_LINUX_IO_URING
>  "      --aio=MODE            set AIO mode (native, io_uring or threads)\n"
> +#else
> +"      --aio=MODE            set AIO mode (native or threads)\n"
> +#endif
>  "      --discard=MODE        set discard mode (ignore, unmap)\n"
>  "      --detect-zeroes=MODE  set detect-zeroes mode (off, on, unmap)\n"
>  "      --image-opts          treat FILE as a full set of image options\n"
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 34e9b32a5c..973125cfca 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -1338,7 +1338,11 @@ DEF("drive", HAS_ARG, QEMU_OPTION_drive,
>      "       
> [,cache=writethrough|writeback|none|directsync|unsafe][,format=f]\n"
>      "       [,snapshot=on|off][,rerror=ignore|stop|report]\n"
>      "       [,werror=ignore|stop|report|enospc][,id=name]\n"
> -    "       [,aio=threads|native|io_uring]\n"
> +    "       [,aio=threads|native"
> +#if defined(CONFIG_LINUX_IO_URING)
> +    "|io_uring"
> +#endif
> +    "]\n"
>      "       [,readonly=on|off][,copy-on-read=on|off]\n"
>      "       [,discard=ignore|unmap][,detect-zeroes=on|off|unmap]\n"
>      "       [[,bps=b]|[[,bps_rd=r][,bps_wr=w]]]\n"
> -- 
> 2.35.3
> 
> 

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 :|