Re: [PATCH] Only advertise aio=io_uring if support is actually available
On Mittwoch, 20. April 2022 10:33:38 CEST Daniel P. Berrangé wrote:
Hi Daniel,
> If we're going to conditionalize this, then we really ought to be
> address it fully, because 'native' is also platform specific.
Good point. I can do that as well.
> 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
the only other option I came up with so far is something like this:
+#if defined(CONFIG_LINUX_AIO)
+#define IF_CONFIG_LINUX_AIO(X) X
+#else
+#define IF_CONFIG_LINUX_AIO(X)
+#endif
+
+#if defined(CONFIG_LINUX_IO_URING)
+#define IF_CONFIG_LINUX_IO_URING(X) X
+#else
+#define IF_CONFIG_LINUX_IO_URING(X)
+#endif
+
and then use it like
-.help = "host AIO implementation (threads"
-#ifdef CONFIG_LINUX_AIO
-", native"
-#endif
-#ifdef CONFIG_LINUX_IO_URING
-", io_uring"
-#endif
-")",
+.help = "host AIO implementation (threads" IF_CONFIG_LINUX_AIO(",
native") IF_CONFIG_LINUX_IO_URING(", io_uring") ")",
WDYT, is that worth it?
Thanks,
Dirk
Re: [PATCH] Only advertise aio=io_uring if support is actually available
On Mittwoch, 20. April 2022 10:33:38 CEST Daniel P. Berrangé wrote: Hi Daniel, > 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 :-) understood, thank you for the good feedback, I've sent a v2 with a reworded commit message. Greetings, Dirk
Re: [PATCH] Only advertise aio=io_uring if support is actually available
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
> ---
> 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=MODEset AIO mode (native, io_uring or threads)\n"
> +#else
> +" --aio=MODEset AIO mode (native or threads)\n"
> +#endif
> " --discard=MODEset 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 :|
Re: [PATCH] Only advertise aio=io_uring if support is actually available
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. --help is human-parseable, and thus not stable text. Relying on it for detecting host support is not always ideal, but anything is better than nothing, so I'm not opposed to this patch. Reviewed-by: Eric Blake Since qemu-nbd is affected, I'm happy to queue this through my NBD tree now that 7.1 development has opened, if no one else queues it elsewhere first. > > Signed-off-by: Dirk Müller > --- > block/file-posix.c | 4 > qemu-nbd.c | 4 > qemu-options.hx| 6 +- > 3 files changed, 13 insertions(+), 1 deletion(-) > -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
[PATCH] Only advertise aio=io_uring if support is actually available
This allows $qemu --help runtime configure checks for detecting
the host support.
Signed-off-by: Dirk Müller
---
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
},
{
.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=MODEset AIO mode (native, io_uring or threads)\n"
+#else
+" --aio=MODEset AIO mode (native or threads)\n"
+#endif
" --discard=MODEset 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
