Re: [PATCH v6 05/11] docs/deprecated: move QMP events bellow QMP command section
In the subject: s/bellow/below/ On Fri, May 26, 2023 at 05:53:55PM +0100, Alex Bennée wrote: > Also rename the section to make the fact this is part of the > management protocol even clearer. > > Suggested-by: Markus Armbruster > Signed-off-by: Alex Bennée > --- > docs/about/deprecated.rst | 18 +- > 1 file changed, 9 insertions(+), 9 deletions(-) Otherwise, Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 2/3] ui: Switch "-display sdl" to use the QAPI parser
On Wed, May 18, 2022 at 03:44:45PM +0200, Thomas Huth wrote: > The "-display sdl" option still uses a hand-crafted parser for its > parameters since we didn't want to drag an interface we considered > somewhat flawed into the QAPI schema. Since the flaws are gone now, > it's time to QAPIfy. > > This introduces the new "DisplaySDL" QAPI struct that is used to hold > the parameters that are unique to the SDL display. The only specific > parameter is currently "grab-mod" that is used to specify the required > modifier keys to escape from the mouse grabbing mode. > > Signed-off-by: Thomas Huth > --- > +++ b/qapi/ui.json > @@ -1295,6 +1295,30 @@ >'*swap-opt-cmd': 'bool' >} } > > +## > +# @GrabMod: > +# > +# Set of modifier keys that need to be hold for shortcut key actions. s/hold/held/ -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 8/8] softmmu: remove is_daemonized() method
On Fri, Mar 04, 2022 at 06:56:20PM +, Daniel P. Berrangé wrote: > There are no longer any users of this method, so it can be removed to > prevent future accidental (mis)use. > > Signed-off-by: Daniel P. Berrangé > --- > include/sysemu/os-posix.h | 2 -- > include/sysemu/os-win32.h | 5 - > os-posix.c| 5 - > stubs/is-daemonized.c | 9 - > stubs/meson.build | 1 - > 5 files changed, 22 deletions(-) > delete mode 100644 stubs/is-daemonized.c Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 7/8] softmmu: move parsing of -runas, -chroot and -daemonize code
On Fri, Mar 04, 2022 at 06:56:19PM +, Daniel P. Berrangé wrote:
> With the future intent to try to move to a fully QAPI driven
> configuration system, we want to have any current command
> parsing well isolated from logic that applies the resulting
> configuration.
>
> We also don't want os-posix.c to contain code that is specific
> to the system emulators, as this file is linked to other binaries
> too.
>
> To satisfy these goals, we move parsing of the -runas, -chroot and
> -daemonize code out of the os-posix.c helper code, and pass the
> parsed data into APIs in os-posix.c.
>
> As a side benefit the 'os_daemonize()' function now lives up to
> its name and will always daemonize, instead of using global state
> to decide to be a no-op sometimes.
>
> Signed-off-by: Daniel P. Berrangé
> ---
> include/sysemu/os-posix.h | 4 +-
> include/sysemu/os-win32.h | 1 -
> os-posix.c| 148 +++---
> os-win32.c| 9 ---
> softmmu/vl.c | 86 ++
> 5 files changed, 117 insertions(+), 131 deletions(-)
> @@ -2780,6 +2811,14 @@ void qemu_init(int argc, char **argv, char **envp)
> MachineClass *machine_class;
> bool userconfig = true;
> FILE *vmstate_dump_file = NULL;
> +bool daemonize = false;
Given this declaration,...
> +#ifndef WIN32
> +struct passwd *pwd;
> +uid_t runas_uid = -1;
> +gid_t runas_gid = -1;
> +g_autofree char *runas_name = NULL;
> +const char *chroot_dir = NULL;
> +#endif /* !WIN32 */
>
> qemu_add_opts(_drive_opts);
> qemu_add_drive_opts(_legacy_drive_opts);
> @@ -3661,15 +3700,34 @@ void qemu_init(int argc, char **argv, char **envp)
> case QEMU_OPTION_nouserconfig:
> /* Nothing to be parsed here. Especially, do not error out
> below. */
> break;
> -default:
> -if (os_parse_cmd_args(popt->index, optarg)) {
> -error_report("Option not supported in this build");
> +#ifndef WIN32
> +case QEMU_OPTION_runas:
> +pwd = getpwnam(optarg);
> +if (pwd) {
> +runas_uid = pwd->pw_uid;
> +runas_gid = pwd->pw_gid;
> +runas_name = g_strdup(pwd->pw_name);
> +} else if (!os_parse_runas_uid_gid(optarg,
> + _uid,
> + _gid)) {
> +error_report("User \"%s\" doesn't exist"
> + " (and is not :)",
> + optarg);
> exit(1);
> }
> -if (is_daemonized()) {
> -qemu_log_stdio_disable();
> -qemu_chr_stdio_disable();
> -}
> + break;
> + case QEMU_OPTION_chroot:
> +chroot_dir = optarg;
> +break;
> +case QEMU_OPTION_daemonize:
> +daemonize = 1;
...this should s/1/true/. With that tweak,
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 6/8] chardev: add API to block use of the stdio implementation
On Fri, Mar 04, 2022 at 06:56:18PM +, Daniel P. Berrangé wrote: > When daemonizing QEMU it is not possible to use the stdio chardev > backend because the file descriptors are connected to /dev/null. > Currently the chardev checks for this scenario directly, but to > decouple it from the system emulator daemonizing code, we reverse > the relationship. Now the system emulator calls a helper to > explicitly disable use of the stdio backend. > > Signed-off-by: Daniel P. Berrangé > --- > chardev/char-stdio.c | 12 ++-- > include/chardev/char-stdio.h | 29 + > softmmu/vl.c | 2 ++ > 3 files changed, 41 insertions(+), 2 deletions(-) > create mode 100644 include/chardev/char-stdio.h > Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 5/8] softmmu: refactor use of is_daemonized() method
On Fri, Mar 04, 2022 at 06:56:17PM +, Daniel P. Berrangé wrote: > Use of the is_daemonized() method is isolated to allow it to be > more easily eliminated in a future change. > > Signed-off-by: Daniel P. Berrangé > --- > softmmu/vl.c | 13 +++-- > 1 file changed, 7 insertions(+), 6 deletions(-) Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 4/8] util: remove use of is_daemonized flag from logging code
On Fri, Mar 04, 2022 at 06:56:16PM +, Daniel P. Berrangé wrote: > We want to decouple knowledge of daemonization from other code. What the > logging code really wants to know is whether it can use stdio or not. > Add an API to let the logging code be informed of this fact explicitly. > > Signed-off-by: Daniel P. Berrangé > --- > include/qemu/log.h | 1 + > softmmu/vl.c | 3 +++ > util/log.c | 12 +--- > 3 files changed, 13 insertions(+), 3 deletions(-) > Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [PATCH 4/4] softmmu: move parsing of -runas, -chroot and -daemonize code
On Fri, Mar 04, 2022 at 02:54:54PM +, Daniel P. Berrangé wrote: > On Fri, Mar 04, 2022 at 11:56:57AM +, Daniel P. Berrangé wrote: > > With the future intent to try to move to a fully QAPI driven > > configuration system, we want to have any current command > > parsing well isolated from logic that applies the resulting > > configuration. > > > > We also don't want os-posix.c to contain code that is specific > > to the system emulators, as this file is linked to other binaries > > too. > > > > To satisfy these goals, we move parsing of the -runas, -chroot and > > -daemonize code out of the os-posix.c helper code, and pass the > > parsed data into APIs in os-posix.c. > > > > As a side benefit the 'os_daemonize()' function now lives upto to up to > > its name and will always daemonize, instead of using global state > > to decide to be a no-op sometimes. Yay. > > @@ -3683,7 +3743,7 @@ void qemu_init(int argc, char **argv, char **envp) > > qemu_process_early_options(); > > > > qemu_process_help_options(); > > -qemu_maybe_daemonize(pid_file); > > +qemu_maybe_daemonize(daemonize, pid_file); > > This commit is a bit flawed, because we're until we call the > os_daemonize() method, the is_daemonized() method won't return > true. Unfortunately some callers rely is_daemonized() returning > true merely for the request, even though we've not yet put it > into action. ie the method would have been better called > is_daemonize_requested() Eww, indeed. > > The upshot is that we fail to properly close stderr. > > I'll send a v2 that handles this by fully removing the > is_daemonize() method. Looking forward to it. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [PATCH 2/4] os-posix: refactor code handling the -runas argument
On Fri, Mar 04, 2022 at 11:56:55AM +, Daniel P. Berrangé wrote: > Change the change_process_uid() function so that it takes its input as > parameters instead of relying on static global variables. > > Signed-off-by: Daniel P. Berrangé > --- > os-posix.c | 83 +- > 1 file changed, 39 insertions(+), 44 deletions(-) > Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [PATCH 1/4] softmmu: remove deprecated --enable-fips option
On Fri, Mar 04, 2022 at 11:56:54AM +, Daniel P. Berrangé wrote: > Users requiring FIPS support must build QEMU with either the libgcrypt > or gnutls libraries for as the crytography backend. s/for // > > Signed-off-by: Daniel P. Berrangé > --- > docs/about/deprecated.rst | 12 > docs/about/removed-features.rst | 11 +++ > include/qemu/osdep.h| 3 --- > os-posix.c | 8 > qemu-options.hx | 10 -- > ui/vnc.c| 7 --- > util/osdep.c| 28 > 7 files changed, 11 insertions(+), 68 deletions(-) -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [PATCH v4 3/3] qapi: deprecate drive-backup
On Thu, Nov 04, 2021 at 09:58:11AM +0100, Markus Armbruster wrote: > From: Vladimir Sementsov-Ogievskiy > > Modern way is using blockdev-add + blockdev-backup, which provides a > lot more control on how target is opened. > > As example of drive-backup problems consider the following: > > User of drive-backup expects that target will be opened in the same > cache and aio mode as source. Corresponding logic is in > drive_backup_prepare(), where we take bs->open_flags of source. > > It works rather bad if source was added by blockdev-add. Assume source > is qcow2 image. On blockdev-add we should specify aio and cache options > for file child of qcow2 node. What happens next: > > drive_backup_prepare() looks at bs->open_flags of qcow2 source node. > But there no BDRV_O_NOCAHE neither BDRV_O_NATIVE_AIO: BDRV_O_NOCAHE is > places in bs->file->bs->open_flags, and BDRV_O_NATIVE_AIO is nowhere, > as file-posix parse options and simply set s->use_linux_aio. > > The documentation is updated in a minimal way, so that drive-backup is > noted only as a deprecated command, and blockdev-backup used in most of > places. > > Signed-off-by: Vladimir Sementsov-Ogievskiy > Reviewed-by: Kashyap Chamarthy > Signed-off-by: Markus Armbruster > --- Reviewed-by: Eric Blake I think this is appropriate for inclusion in 6.2, even if it did not quite make soft freeze, so that we aren't delaying the deprecation for an entire cycle. > docs/about/deprecated.rst | 11 ++ > docs/interop/live-block-operations.rst | 47 +- > qapi/block-core.json | 5 ++- > qapi/transaction.json | 6 +++- > 4 files changed, 51 insertions(+), 18 deletions(-) > -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [PATCH v3 3/3] qapi: deprecate drive-backup
On Wed, Nov 03, 2021 at 02:29:12PM +0100, Vladimir Sementsov-Ogievskiy wrote: > Modern way is using blockdev-add + blockdev-backup, which provides a > lot more control on how target is opened. > > > Signed-off-by: Vladimir Sementsov-Ogievskiy > Reviewed-by: Kashyap Chamarthy > --- > docs/about/deprecated.rst | 11 ++ > docs/interop/live-block-operations.rst | 47 +- > qapi/block-core.json | 5 ++- > 3 files changed, 46 insertions(+), 17 deletions(-) > > diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst > index 25b7ec8d92..4a4910143f 100644 > --- a/docs/about/deprecated.rst > +++ b/docs/about/deprecated.rst > @@ -234,6 +234,17 @@ single ``bitmap``, the new ``block-export-add`` uses a > list of ``bitmaps``. > Member ``values`` in return value elements with meta-type ``enum`` is > deprecated. Use ``members`` instead. > > +``drive-backup`` (since 6.2) > + Correct here,... > +++ b/docs/interop/live-block-operations.rst > > QMP invocation for ``drive-backup`` > ~~~ > > +Note that ``drive-backup`` command is deprecated since QEMU 6.1 and > +will be removed in future. ...but stale here (this patch has been around for a while). But even though we're in soft freeze, I lean toward this going in rather than wait yet another release to do the deprecation. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 6/9] qapi: Generalize command policy checking
On Thu, Oct 28, 2021 at 12:25:17PM +0200, Markus Armbruster wrote:
> The code to check command policy can see special feature flag
> 'deprecated' as command flag QCO_DEPRECATED. I want to make feature
> flag 'unstable' visible there as well, so I can add policy for it.
>
> To let me make it visible, add member @special_features (a bitset of
> QapiSpecialFeature) to QmpCommand, and adjust the generator to pass it
> through qmp_register_command(). Then replace "QCO_DEPRECATED in
> @flags" by QAPI_DEPRECATED in @special_features", and drop
> QCO_DEPRECATED.
>
> Signed-off-by: Markus Armbruster
> Reviewed-by: Philippe Mathieu-Daudé
> Acked-by: John Snow
> ---
> +++ b/qapi/qmp-dispatch.c
> @@ -176,7 +176,7 @@ QDict *qmp_dispatch(const QmpCommandList *cmds, QObject
> *request,
>"The command %s has not been found", command);
> goto out;
> }
> -if (cmd->options & QCO_DEPRECATED) {
> +if (cmd->special_features & 1u << QAPI_DEPRECATED) {
I admit having to check the C operator precedence table when reading
this (<< is higher than &); if writing it myself, I would probably
have used explicit () to avoid reviewer confusion, but what you have
is correct. (After grepping for ' & 1.*<<' and ' & (1.*<<', it looks
like authors using explicit precedence happens more often, but that
there are other instances in the code base relying on implicit
precedence.)
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 5/9] qapi: Generalize struct member policy checking
On Thu, Oct 28, 2021 at 12:25:16PM +0200, Markus Armbruster wrote:
> The generated visitor functions call visit_deprecated_accept() and
> visit_deprecated() when visiting a struct member with special feature
> flag 'deprecated'. This makes the feature flag visible to the actual
> visitors. I want to make feature flag 'unstable' visible there as
> well, so I can add policy for it.
>
> To let me make it visible, replace these functions by
> visit_policy_reject() and visit_policy_skip(), which take the member's
> special features as an argument. Note that the new functions have the
> opposite sense, i.e. the return value flips.
>
> Signed-off-by: Markus Armbruster
> ---
> +++ b/qapi/qapi-forward-visitor.c
> @@ -246,25 +246,27 @@ static void forward_field_optional(Visitor *v, const
> char *name, bool *present)
> visit_optional(ffv->target, name, present);
> }
>
> -static bool forward_field_deprecated_accept(Visitor *v, const char *name,
> -Error **errp)
> +static bool forward_field_policy_reject(Visitor *v, const char *name,
> +unsigned special_features,
> +Error **errp)
> {
> ForwardFieldVisitor *ffv = to_ffv(v);
>
> if (!forward_field_translate_name(ffv, , errp)) {
> return false;
Should this return value be flipped?
> }
> -return visit_deprecated_accept(ffv->target, name, errp);
> +return visit_policy_reject(ffv->target, name, special_features, errp);
> }
>
> -static bool forward_field_deprecated(Visitor *v, const char *name)
> +static bool forward_field_policy_skip(Visitor *v, const char *name,
> + unsigned special_features)
> {
> ForwardFieldVisitor *ffv = to_ffv(v);
>
> if (!forward_field_translate_name(ffv, , NULL)) {
> return false;
and here too?
> }
> -return visit_deprecated(ffv->target, name);
> +return visit_policy_skip(ffv->target, name, special_features);
> }
>
Otherwise, the rest of the logic changes for flipped sense look right.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 9/9] qapi: Extend -compat to set policy for unstable interfaces
On Thu, Oct 28, 2021 at 12:25:20PM +0200, Markus Armbruster wrote:
> New option parameters unstable-input and unstable-output set policy
> for unstable interfaces just like deprecated-input and
> deprecated-output set policy for deprecated interfaces (see commit
> 6dd75472d5 "qemu-options: New -compat to set policy for deprecated
> interfaces"). This is intended for testing users of the management
> interfaces. It is experimental.
>
> For now, this covers only syntactic aspects of QMP, i.e. stuff tagged
> with feature 'unstable'. We may want to extend it to cover semantic
> aspects, or the command line.
>
> Note that there is no good way for management application to detect
> presence of these new option parameters: they are not visible output
> of query-qmp-schema or query-command-line-options. Tolerable, because
> it's meant for testing. If running with -compat fails, skip the test.
Not to mention, once we finish QAPIfying the command line, we could
make sure it is visible through introspection at that time (it may
require tagging the command line option with a feature, if nothing
else makes it pop through).
>
> Signed-off-by: Markus Armbruster
> Acked-by: John Snow
> ---
> qapi/compat.json | 6 +-
> include/qapi/util.h | 1 +
> qapi/qmp-dispatch.c | 6 ++
> qapi/qobject-output-visitor.c | 8 ++--
> qemu-options.hx | 20 +++-
> scripts/qapi/events.py| 10 ++
> scripts/qapi/schema.py| 10 ++
> 7 files changed, 49 insertions(+), 12 deletions(-)
>
> diff --git a/qapi/compat.json b/qapi/compat.json
> index 74a8493d3d..9bc9804abb 100644
> --- a/qapi/compat.json
> +++ b/qapi/compat.json
> @@ -47,9 +47,13 @@
> #
> # @deprecated-input: how to handle deprecated input (default 'accept')
> # @deprecated-output: how to handle deprecated output (default 'accept')
> +# @unstable-input: how to handle unstable input (default 'accept')
> +# @unstable-output: how to handle unstable output (default 'accept')
Missing '(since 6.2)' doc tags on the two new policies.
> #
> # Since: 6.0
> ##
> { 'struct': 'CompatPolicy',
>'data': { '*deprecated-input': 'CompatPolicyInput',
> -'*deprecated-output': 'CompatPolicyOutput' } }
> +'*deprecated-output': 'CompatPolicyOutput',
> +'*unstable-input': 'CompatPolicyInput',
> +'*unstable-output': 'CompatPolicyOutput' } }
> diff --git a/include/qapi/util.h b/include/qapi/util.h
> index 0cc98db9f9..81a2b13a33 100644
> --- a/include/qapi/util.h
> +++ b/include/qapi/util.h
> @@ -13,6 +13,7 @@
>
> typedef enum {
> QAPI_DEPRECATED,
> +QAPI_UNSTABLE,
> } QapiSpecialFeature;
> +++ b/qemu-options.hx
> @@ -3641,7 +3641,9 @@ DEFHEADING(Debug/Expert options:)
>
> DEF("compat", HAS_ARG, QEMU_OPTION_compat,
> "-compat
> [deprecated-input=accept|reject|crash][,deprecated-output=accept|hide]\n"
> -"Policy for handling deprecated management interfaces\n",
> +"Policy for handling deprecated management interfaces\n"
> +"-compat
> [unstable-input=accept|reject|crash][,unstable-output=accept|hide]\n"
> +"Policy for handling unstable management interfaces\n",
It may not be machine-introspectible, but at least we can grep --help
output to see when the policy is usable for testing.
> QEMU_ARCH_ALL)
> SRST
> ``-compat
> [deprecated-input=@var{input-policy}][,deprecated-output=@var{output-policy}]``
> @@ -3659,6 +3661,22 @@ SRST
> Suppress deprecated command results and events
>
> Limitation: covers only syntactic aspects of QMP.
> +
> +``-compat
> [unstable-input=@var{input-policy}][,unstable-output=@var{output-policy}]``
> +Set policy for handling unstable management interfaces (experimental):
Once we QAPIfy the command line, this says we would add the 'unstable'
feature flag to '-compat unstable-input'. How meta ;) And goes along
with your comments earlier in the series about how we may use the
'unstable' feature even without the 'x-' naming prefix, once it is
machine-detectible.
With the doc tweak,
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: [PATCH 2/9] qapi: Mark unstable QMP parts with feature 'unstable'
On Mon, Oct 25, 2021 at 07:25:25AM +0200, Markus Armbruster wrote:
> Add special feature 'unstable' everywhere the name starts with 'x-',
> except for InputBarrierProperties member x-origin and
> MemoryBackendProperties member x-use-canonical-path-for-ramblock-id,
> because these two are actually stable.
>
> Signed-off-by: Markus Armbruster
> ---
> @@ -2495,27 +2508,57 @@
> #
> # Properties for throttle-group objects.
> #
> -# The options starting with x- are aliases for the same key without x- in
> -# the @limits object. As indicated by the x- prefix, this is not a stable
> -# interface and may be removed or changed incompatibly in the future. Use
> -# @limits for a supported stable interface.
> -#
> # @limits: limits to apply for this throttle group
> #
> +# Features:
> +# @unstable: All members starting with x- are aliases for the same key
> +#without x- in the @limits object. This is not a stable
> +#interface and may be removed or changed incompatibly in
> +#the future. Use @limits for a supported stable
> +#interface.
> +#
> # Since: 2.11
> ##
> { 'struct': 'ThrottleGroupProperties',
>'data': { '*limits': 'ThrottleLimits',
> -'*x-iops-total' : 'int', '*x-iops-total-max' : 'int',
> +'*x-iops-total': { 'type': 'int',
> + 'features': [ 'unstable' ] },
This struct has been around since 381bd74 (v6.0); but was not listed
as deprecated at the time. Do we still need it in 6.2, or have we
gone enough release cycles with the saner naming without x- that we
could drop this? But that is a question independent of this patch.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 09/15] softmmu/qdev-monitor: add error handling in qdev_set_id
On Wed, Oct 13, 2021 at 03:10:38PM +0200, Damien Hedde wrote:
> > > @@ -691,7 +703,13 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error
> > > **errp)
> > > }
> > > }
> > > -qdev_set_id(dev, g_strdup(qemu_opts_id(opts)));
> > > +/*
> > > + * set dev's parent and register its id.
> > > + * If it fails it means the id is already taken.
> > > + */
> > > +if (!qdev_set_id(dev, g_strdup(qemu_opts_id(opts)), errp)) {
> > > +goto err_del_dev;
> >
> > ...nor on this, which means the g_strdup() leaks on failure.
> >
>
> Since we strdup the id just before calling qdev_set_id.
> Maybe we should do the the strdup in qdev_set_id (and free things on error
> there too). It seems simplier than freeing things on the callee side just in
> case of an error.
Indeed. If we expected qdev_set_id() to be passed something that it
can later free, we would have used 'char *'; but because we used
'const char *' for that parameter, it really does make more sense for
the callers to pass in any string and for qdev_set_id() to do the
necessary strdup()ing, as well as clean up on error.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 09/15] softmmu/qdev-monitor: add error handling in qdev_set_id
On Fri, Oct 08, 2021 at 03:34:36PM +0200, Kevin Wolf wrote:
> From: Damien Hedde
>
> qdev_set_id() is mostly used when the user adds a device (using
> -device cli option or device_add qmp command). This commit adds
> an error parameter to handle the case where the given id is
> already taken.
>
> Also document the function and add a return value in order to
> be able to capture success/failure: the function now returns the
> id in case of success, or NULL in case of failure.
>
> The commit modifies the 2 calling places (qdev-monitor and
> xen-legacy-backend) to add the error object parameter.
>
> Note that the id is, right now, guaranteed to be unique because
> all ids came from the "device" QemuOptsList where the id is used
> as key. This addition is a preparation for a future commit which
> will relax the uniqueness.
>
> Signed-off-by: Damien Hedde
> Signed-off-by: Kevin Wolf
> ---
> +++ b/softmmu/qdev-monitor.c
> @@ -593,22 +593,34 @@ static BusState *qbus_find(const char *path, Error
> **errp)
> }
>
> /* Takes ownership of @id, will be freed when deleting the device */
> -void qdev_set_id(DeviceState *dev, char *id)
> +const char *qdev_set_id(DeviceState *dev, char *id, Error **errp)
> {
> -if (id) {
> -dev->id = id;
> -}
> +ObjectProperty *prop;
>
> -if (dev->id) {
> -object_property_add_child(qdev_get_peripheral(), dev->id,
> - OBJECT(dev));
> +assert(!dev->id && !dev->realized);
> +
> +/*
> + * object_property_[try_]add_child() below will assert the device
> + * has no parent
> + */
> +if (id) {
> +prop = object_property_try_add_child(qdev_get_peripheral(), id,
> + OBJECT(dev), NULL);
> +if (prop) {
> +dev->id = id;
> +} else {
> +error_setg(errp, "Duplicate device ID '%s'", id);
> +return NULL;
id is not freed on this error path...
> +}
> } else {
> static int anon_count;
> gchar *name = g_strdup_printf("device[%d]", anon_count++);
> -object_property_add_child(qdev_get_peripheral_anon(), name,
> - OBJECT(dev));
> +prop = object_property_add_child(qdev_get_peripheral_anon(), name,
> + OBJECT(dev));
> g_free(name);
> }
> +
> +return prop->name;
> }
>
> DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
> @@ -691,7 +703,13 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error
> **errp)
> }
> }
>
> -qdev_set_id(dev, g_strdup(qemu_opts_id(opts)));
> +/*
> + * set dev's parent and register its id.
> + * If it fails it means the id is already taken.
> + */
> +if (!qdev_set_id(dev, g_strdup(qemu_opts_id(opts)), errp)) {
> +goto err_del_dev;
...nor on this, which means the g_strdup() leaks on failure.
> +}
>
> /* set properties */
> if (qemu_opt_foreach(opts, set_property, dev, errp)) {
> --
> 2.31.1
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: [PATCH RFC v2 5/5] block: Deprecate transaction type drive-backup
On Sat, Oct 09, 2021 at 02:09:44PM +0200, Markus Armbruster wrote:
> Several moons ago, Vladimir posted
>
> Subject: [PATCH v2 3/3] qapi: deprecate drive-backup
> Date: Wed, 5 May 2021 16:58:03 +0300
> Message-Id: <20210505135803.67896-4-vsement...@virtuozzo.com>
> https://lists.gnu.org/archive/html/qemu-devel/2021-05/msg01394.html
>
> with this
>
> TODO: We also need to deprecate drive-backup transaction action..
> But union members in QAPI doesn't support 'deprecated' feature. I tried
> to dig a bit, but failed :/ Markus, could you please help with it? At
> least by advice?
>
> This is one way to resolve it. Sorry it took so long.
>
> John explored another way, namely adding feature flags to union
> branches. Could also be useful, say to add different features to
> branches in multiple unions sharing the same tag enum.
>
> Signed-off-by: Markus Armbruster
> ---
> qapi/transaction.json | 5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/qapi/transaction.json b/qapi/transaction.json
> index d175b5f863..0564a893b3 100644
> --- a/qapi/transaction.json
> +++ b/qapi/transaction.json
> @@ -54,6 +54,9 @@
> # @blockdev-snapshot-sync: since 1.1
> # @drive-backup: Since 1.6
> #
> +# Features:
> +# @deprecated: Member @drive-backup is deprecated. Use FIXME instead.
Obviously, we'd need to flesh this out ("'blockdev-backup' with proper
node names"? something else?) before dropping RFC on this patch.
And we'd want to edit docs/about/deprecated.rst to match.
> +#
> # Since: 1.1
> ##
> { 'enum': 'TransactionActionKind',
> @@ -62,7 +65,7 @@
> 'block-dirty-bitmap-disable', 'block-dirty-bitmap-merge',
> 'blockdev-backup', 'blockdev-snapshot',
> 'blockdev-snapshot-internal-sync', 'blockdev-snapshot-sync',
> -'drive-backup' ] }
> +{ 'name': 'drive-backup', 'features': [ 'deprecated' ] } ] }
>
> ##
> # @AbortWrapper:
> --
> 2.31.1
>
But the idea is reasonable, and I'm not sure if we're any closer to
John's idea of feature flags on union branches.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 4/5] qapi: Implement deprecated-input={reject,crash} for enum values
On Sat, Oct 09, 2021 at 02:09:43PM +0200, Markus Armbruster wrote:
> This copies the code implementing the policy from qapi/qmp-dispatch.c
> to qapi/qobject-input-visitor.c. Tolerable, but if we acquire more
> copes, we should look into factoring them out.
copies
>
> Signed-off-by: Markus Armbruster
> ---
> docs/devel/qapi-code-gen.rst | 6 --
> qapi/compat.json | 3 ++-
> include/qapi/util.h | 6 +-
> qapi/qapi-visit-core.c | 18 +++---
> scripts/qapi/types.py| 17 -
> 5 files changed, 42 insertions(+), 8 deletions(-)
>
> diff --git a/docs/devel/qapi-code-gen.rst b/docs/devel/qapi-code-gen.rst
> index 00334e9fb8..006a6f4a9a 100644
> --- a/docs/devel/qapi-code-gen.rst
> +++ b/docs/devel/qapi-code-gen.rst
> @@ -708,8 +708,10 @@ QEMU shows a certain behaviour.
> Special features
>
>
> -Feature "deprecated" marks a command, event, or struct member as
> -deprecated. It is not supported elsewhere so far.
> +Feature "deprecated" marks a command, event, struct or enum member as
Do we want the comma before the conjunction? (I've seen style guides
that recommend it, and style guides that discourage it; while I tend
to write by the former style, I usually don't care about the latter.
Rather, switching styles mid-patch caught my attention).
> +deprecated. It is not supported elsewhere so far. Interfaces so
> +marked may be withdrawn in future releases in accordance with QEMU's
> +deprecation policy.
>
>
> +++ b/qapi/qapi-visit-core.c
> @@ -393,7 +393,7 @@ static bool input_type_enum(Visitor *v, const char *name,
> int *obj,
> const QEnumLookup *lookup, Error **errp)
> {
> int64_t value;
> -char *enum_str;
> +g_autofree char *enum_str = NULL;
Nice change while touching the code. Is it worth mentioning in the
commit message?
>
> if (!visit_type_str(v, name, _str, errp)) {
> return false;
> @@ -402,11 +402,23 @@ static bool input_type_enum(Visitor *v, const char
> *name, int *obj,
> value = qapi_enum_parse(lookup, enum_str, -1, NULL);
> if (value < 0) {
> error_setg(errp, QERR_INVALID_PARAMETER, enum_str);
> -g_free(enum_str);
> return false;
> }
>
> -g_free(enum_str);
> +if (lookup->flags && (lookup->flags[value] & QAPI_ENUM_DEPRECATED)) {
> +switch (v->compat_policy.deprecated_input) {
> +case COMPAT_POLICY_INPUT_ACCEPT:
> +break;
> +case COMPAT_POLICY_INPUT_REJECT:
> +error_setg(errp, "Deprecated value '%s' disabled by policy",
> + enum_str);
> + return false;
> +case COMPAT_POLICY_INPUT_CRASH:
> +default:
> +abort();
> +}
> +}
> +
> *obj = value;
> return true;
> }
Grammar fixes are minor, so:
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 1/5] qapi: Enable enum member introspection to show more than name
On Sat, Oct 09, 2021 at 02:09:40PM +0200, Markus Armbruster wrote:
> The next commit will add feature flags to enum members. There's a
> problem, though: query-qmp-schema shows an enum type's members as an
> array of member names (SchemaInfoEnum member @values). If it showed
> an array of objects with a name member, we could simply add more
> members to these objects. Since it's just strings, we can't.
>
> I can see three ways to correct this design mistake:
>
> 1. Do it the way we should have done it, plus compatibility goo.
...
> 2. Like 1, but omit "boring" elements of @member, and empty @member.
> 3. Versioned query-qmp-schema.
> This commit implements 1. Libvirt developers prefer it.
>
> Signed-off-by: Markus Armbruster
> ---
> qapi/introspect.json | 21 +++--
> scripts/qapi/introspect.py | 18 ++
> 2 files changed, 33 insertions(+), 6 deletions(-)
Reviewed-by: Eric Blake
>
> diff --git a/qapi/introspect.json b/qapi/introspect.json
> index 39bd303778..f806bd7281 100644
> --- a/qapi/introspect.json
> +++ b/qapi/introspect.json
> @@ -142,14 +142,31 @@
> #
> # Additional SchemaInfo members for meta-type 'enum'.
> #
> -# @values: the enumeration type's values, in no particular order.
> +# @members: the enum type's members, in no particular order
> +# (since 6.2).
> +#
> +# @values: the enumeration type's member names, in no particular order.
> +# Redundant with @members. Just for backward compatibility.
> #
> # Values of this type are JSON string on the wire.
> #
> # Since: 2.5
> ##
> { 'struct': 'SchemaInfoEnum',
> - 'data': { 'values': ['str'] } }
> + 'data': { 'members': [ 'SchemaInfoEnumMember' ],
> +'values': ['str'] } }
Not deprecated at this time, I agree with your choice to make the
actual deprecation of 'values' to be in a later patch (if at all).
> +
> +##
> +# @SchemaInfoEnumMember:
> +#
> +# An object member.
> +#
> +# @name: the member's name, as defined in the QAPI schema.
> +#
> +# Since: 6.2
> +##
> +{ 'struct': 'SchemaInfoEnumMember',
> + 'data': { 'name': 'str' } }
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: [PATCH 11/11] Deprecate stable non-JSON -device and -object
On Fri, Sep 24, 2021 at 11:04:27AM +0200, Kevin Wolf wrote: > We want to switch both from QemuOpts to the keyval parser in the future, > which results in some incompatibilities, mainly around list handling. > Mark the non-JSON version of both as unstable syntax so that management > tools switch to JSON and we can later make the change without breaking > things. > > Signed-off-by: Kevin Wolf > --- > docs/about/deprecated.rst | 11 +++ > 1 file changed, 11 insertions(+) Reviewed-by: Eric Blake > > diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst > index 3c2be84d80..42f6a478fb 100644 > --- a/docs/about/deprecated.rst > +++ b/docs/about/deprecated.rst > @@ -160,6 +160,17 @@ Use ``-display sdl`` instead. > > Use ``-display curses`` instead. > > +Stable non-JSON ``-device`` and ``-object`` syntax (since 6.2) > +'' > + > +If you rely on a stable interface for ``-device`` and ``-object`` that > doesn't > +change incompatibly between QEMU versions (e.g. because you are using the > QEMU > +command line as a machine interface in scripts rather than interactively), > use > +JSON syntax for these options instead. > + > +There is no intention to remove support for non-JSON syntax entirely, but > +future versions may change the way to spell some options. > + > > Plugin argument passing through ``arg=`` (since 6.1) > > -- > 2.31.1 > -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [PATCH 10/11] vl: Enable JSON syntax for -device
On Fri, Sep 24, 2021 at 11:04:26AM +0200, Kevin Wolf wrote:
> Like we already do for -object, introduce support for JSON syntax in
> -device, which can be kept stable in the long term and guarantees that a
> single code path with identical behaviour is used for both QMP and the
> command line. Compared to the QemuOpts based code, the parser contains
> less surprises and has support for non-scalar options (lists and
> structs). Switching management tools to JSON means that we can more
> easily change the "human" CLI syntax from QemuOpts to the keyval parser
> later.
>
> In the QAPI schema, a feature flag is added to the device-add command to
> allow management tools to detect support for this.
>
> Signed-off-by: Kevin Wolf
> ---
> qapi/qdev.json | 15 +
> softmmu/vl.c | 58 --
> 2 files changed, 62 insertions(+), 11 deletions(-)
>
> diff --git a/qapi/qdev.json b/qapi/qdev.json
> index b83178220b..cdc8f911b5 100644
> --- a/qapi/qdev.json
> +++ b/qapi/qdev.json
> @@ -32,17 +32,23 @@
> ##
> # @device_add:
> #
> +# Add a device.
> +#
> # @driver: the name of the new device's driver
> #
> # @bus: the device's parent bus (device tree path)
> #
> # @id: the device's ID, must be unique
> #
> -# Additional arguments depend on the type.
> -#
> -# Add a device.
> +# Features:
> +# @json-cli: If present, the "-device" command line option supports JSON
> +#syntax with a structure identical to the arguments of this
> +#command.
> #
> # Notes:
> +#
> +# Additional arguments depend on the type.
> +#
> # 1. For detailed information about this command, please refer to the
> #'docs/qdev-device-use.txt' file.
> #
> @@ -67,7 +73,8 @@
> ##
> { 'command': 'device_add',
>'data': {'driver': 'str', '*bus': 'str', '*id': 'str'},
> - 'gen': false } # so we can get the additional arguments
> + 'gen': false, # so we can get the additional arguments
> + 'features': ['json-cli'] }
Eventually, we'll get rid of this 'gen':false, but this patch series
is already an improvement towards that goal.
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: [PATCH 09/11] qdev: Avoid QemuOpts in QMP device_add
On Fri, Sep 24, 2021 at 11:04:25AM +0200, Kevin Wolf wrote:
> Directly call qdev_device_add_from_qdict() for QMP device_add instead of
> first going through QemuOpts and converting back to QDict.
>
> Note that this changes the behaviour of device_add, though in ways that
> should be considered bug fixes:
>
> QemuOpts ignores differences between data types, so you could
> successfully pass a string "123" for an integer property, or a string
> "on" for a boolean property (and vice versa). After this change, the
> correct data type for the property must be used in the JSON input.
>
> qemu_opts_from_qdict() also silently ignores any options whose value is
> a QDict, QList or QNull.
>
> To illustrate, the following QMP command was accepted before and is now
> rejected for both reasons:
>
> { "execute": "device_add",
> "arguments": { "driver": "scsi-cd",
> "drive": { "completely": "invalid" },
> "physical_block_size": "4096" } }
>
> Signed-off-by: Kevin Wolf
> ---
> softmmu/qdev-monitor.c | 18 +++---
> 1 file changed, 11 insertions(+), 7 deletions(-)
>
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: [PATCH 08/11] qdev: Base object creation on QDict rather than QemuOpts
On Fri, Sep 24, 2021 at 11:04:24AM +0200, Kevin Wolf wrote: > QDicts are both what QMP natively uses and what the keyval parser > produces. Going through QemuOpts isn't useful for either one, so switch > the main device creation function to QDicts. By sharing more code with > the -object/object-add code path, we can even reduce the code size a > bit. > > This commit doesn't remove the detour through QemuOpts from any code > path yet, but it allows the following commits to do so. > > Signed-off-by: Kevin Wolf > --- > include/hw/qdev-core.h | 8 ++--- > hw/core/qdev.c | 5 ++-- > hw/net/virtio-net.c| 4 +-- > hw/vfio/pci.c | 4 +-- > softmmu/qdev-monitor.c | 67 +++--- > 5 files changed, 41 insertions(+), 47 deletions(-) > Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [PATCH 04/11] qdev: Avoid using string visitor for properties
On Fri, Sep 24, 2021 at 11:04:20AM +0200, Kevin Wolf wrote: > The only thing the string visitor adds compared to a keyval visitor is > list support. git grep for 'visit_start_list' and 'visit.*List' shows > that devices don't make use of this. > > In a world with a QAPIfied command line interface, the keyval visitor is > used to parse the command line. In order to make sure that no devices > start using this feature that would make backwards compatibility harder, > just switch away from object_property_parse(), which internally uses the > string visitor, to a keyval visitor and object_property_set(). > > Signed-off-by: Kevin Wolf > --- > softmmu/qdev-monitor.c | 20 +--- > 1 file changed, 17 insertions(+), 3 deletions(-) Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [PATCH 01/11] qom: Reduce use of error_propagate()
On Fri, Sep 24, 2021 at 11:04:17AM +0200, Kevin Wolf wrote: > ERRP_GUARD() makes debugging easier by making sure that _abort > still fails at the real origin of the error instead of > error_propagate(). > > Signed-off-by: Kevin Wolf > --- > qom/object.c| 7 +++ > qom/object_interfaces.c | 17 ++--- > 2 files changed, 9 insertions(+), 15 deletions(-) Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [PATCH RFC 3/5] qapi: Move compat policy from QObject to generic visitor
On Wed, Sep 15, 2021 at 09:24:23PM +0200, Markus Armbruster wrote: > The next commit needs to access compat policy from the generic visitor > core. Move it there from qobject input and output visitor. > > Signed-off-by: Markus Armbruster > --- Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [PATCH RFC 2/5] qapi: Add feature flags to enum members
On Wed, Sep 15, 2021 at 09:24:22PM +0200, Markus Armbruster wrote: > This is quite similar to commit 84ab008687 "qapi: Add feature flags to > struct members", only for enums instead of structs. > > Signed-off-by: Markus Armbruster > --- Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [PATCH RFC 1/5] qapi: Enable enum member introspection to show more than name
On Wed, Sep 15, 2021 at 09:24:21PM +0200, Markus Armbruster wrote:
> The next commit will add feature flags to enum members. There's a
> problem, though: query-qmp-schema shows an enum type's members as an
> array of member names (SchemaInfoEnum member @values). If it showed
> an array of objects with a name member, we could simply add more
> members to these objects. Since it's just strings, we can't.
>
> I can see three ways to correct this design mistake:
>
> 1. Do it the way we should have done it, plus compatibility goo.
>
>We want a ['SchemaInfoEnumMember'] member in SchemaInfoEnum. Since
>changing @values would be a compatibility break, add a new member
>@members instead.
>
>@values is now redundant. We should be able to get rid of it
>eventually.
>
>In my testing, output of qemu-system-x86_64's query-qmp-schema
>grows by 11% (18.5KiB).
This makes sense if we plan to deprecate @values - if so, that
deprecation would make sense as part of this series, although we may
drag our feet for how long before we actually remove it.
>
> 2. Like 1, but omit "boring" elements of @member, and empty @member.
>
>@values does not become redundant. Output of query-qmp-schema
>grows only as we make enum members non-boring.
Does not change whether libvirt would have to learn to query the new
members, but adds a mandatory fallback step for learning about boring
members (although the fallback step will have to be there anyway for
older qemu). Peter probably has a better idea of which version is
nicer.
>
> 3. Versioned query-qmp-schema.
>
>query-qmp-schema provides either @values or @members. The QMP
>client can select which version it wants.
Sounds more complicated to implement. I'm not opposed to it, but am
leaning towards 1 or 2 myself.
>
> This commit implements 1. simply because it's the solution I thought
> of first. I'm prepared to implement one of the others if we decide
> that's what we want.
>
> Signed-off-by: Markus Armbruster
> ---
> qapi/introspect.json | 20 ++--
> scripts/qapi/introspect.py | 18 ++
> 2 files changed, 32 insertions(+), 6 deletions(-)
>
> diff --git a/qapi/introspect.json b/qapi/introspect.json
> index 39bd303778..250748cd95 100644
> --- a/qapi/introspect.json
> +++ b/qapi/introspect.json
> @@ -142,14 +142,30 @@
> #
> # Additional SchemaInfo members for meta-type 'enum'.
> #
> -# @values: the enumeration type's values, in no particular order.
> +# @members: the enum type's members, in no particular order.
Missing a '(since 6.2)' tag.
> +#
> +# @values: the enumeration type's member names, in no particular order.
> +# Redundant with @members. Just for backward compatibility.
Worth marking as deprecated in this patch, or in a followup?
> #
> # Values of this type are JSON string on the wire.
> #
> # Since: 2.5
> ##
> { 'struct': 'SchemaInfoEnum',
> - 'data': { 'values': ['str'] } }
> + 'data': { 'members': [ 'SchemaInfoEnumMember' ],
> +'values': ['str'] } }
> +
> +##
> +# @SchemaInfoEnumMember:
> +#
> +# An object member.
> +#
> +# @name: the member's name, as defined in the QAPI schema.
> +#
> +# Since: 6.1
6.2
> +##
> +{ 'struct': 'SchemaInfoEnumMember',
> + 'data': { 'name': 'str' } }
>
Definitely more flexible.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: [PATCH 3/2] qemu-img: Improve error for rebase without backing format
On Thu, Jul 08, 2021 at 06:58:21PM +0200, Kevin Wolf wrote: > Am 08.07.2021 um 17:52 hat Eric Blake geschrieben: > > When removeing support for qemu-img being able to create backing removing (is it bad that I don't catch my own typos until seeing them through the mailing list?) > > chains without embedded backing formats, we caused a poor error > > message as caught by iotest 114. Improve the situation to inform the > > user what went wrong. > > > > Suggested-by: Kevin Wolf > > Signed-off-by: Eric Blake > > Thanks, applied to the block branch. And thanks for catching my obvious lack of filtering out local file names. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
[PATCH 3/2] qemu-img: Improve error for rebase without backing format
When removeing support for qemu-img being able to create backing
chains without embedded backing formats, we caused a poor error
message as caught by iotest 114. Improve the situation to inform the
user what went wrong.
Suggested-by: Kevin Wolf
Signed-off-by: Eric Blake
---
qemu-img.c | 3 +++
tests/qemu-iotests/114.out | 2 +-
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/qemu-img.c b/qemu-img.c
index 7c0e73882dd4..b017734c255a 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -3786,6 +3786,9 @@ static int img_rebase(int argc, char **argv)
if (ret == -ENOSPC) {
error_report("Could not change the backing file to '%s': No "
"space left in the file header", out_baseimg);
+} else if (ret == -EINVAL && out_baseimg && !out_basefmt) {
+error_report("Could not change the backing file to '%s': backing "
+ "format must be specified", out_baseimg);
} else if (ret < 0) {
error_report("Could not change the backing file to '%s': %s",
out_baseimg, strerror(-ret));
diff --git a/tests/qemu-iotests/114.out b/tests/qemu-iotests/114.out
index 172454401257..016e9ce3ecfb 100644
--- a/tests/qemu-iotests/114.out
+++ b/tests/qemu-iotests/114.out
@@ -14,7 +14,7 @@ qemu-io: can't open device TEST_DIR/t.qcow2: Could not open
backing file: Unknow
no file open, try 'help open'
read 4096/4096 bytes at offset 0
4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-qemu-img: Could not change the backing file to
'/home/eblake/qemu/build/tests/qemu-iotests/scratch/t.qcow2.base': Invalid
argument
+qemu-img: Could not change the backing file to
'/home/eblake/qemu/build/tests/qemu-iotests/scratch/t.qcow2.base': backing
format must be specified
read 4096/4096 bytes at offset 0
4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
*** done
--
2.31.1
Re: [PATCH 2/2] qemu-img: Require -F with -b backing image
On Thu, Jul 08, 2021 at 03:00:36PM +0200, Kevin Wolf wrote: > Am 03.05.2021 um 23:36 hat Eric Blake geschrieben: > > @@ -17,7 +14,7 @@ qemu-io: can't open device TEST_DIR/t.qcow2: Could not > > open backing file: Unknow > > no file open, try 'help open' > > read 4096/4096 bytes at offset 0 > > 4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) > > -qemu-img: warning: Deprecated use of backing file without explicit backing > > format, use of this image requires potentially unsafe format probing > > +qemu-img: Could not change the backing file to > > '/home/eblake/qemu/build/tests/qemu-iotests/scratch/t.qcow2.base': Invalid > > argument > > read 4096/4096 bytes at offset 0 > > 4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) > > This is not exactly an improvement for the error message. Maybe worth a > follow-up patch? Sure, will do. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [PATCH 0/2] Remove deprecated qemu-img backing file without format
On Mon, May 03, 2021 at 04:35:58PM -0500, Eric Blake wrote: > We've gone enough release cycles without noticeable pushback on our > intentions, so time to make it harder to create images that can form a > security hole due to a need for format probing rather than an explicit > format. > > Eric Blake (2): > qcow2: Prohibit backing file changes in 'qemu-img amend' > qemu-img: Require -F with -b backing image Ping. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [libvirt PATCH v2 03/10] util: generate a persistent system token
On 5/7/21 11:24 AM, Daniel P. Berrangé wrote: > When creating the system identity set the system token. The system > token is currently stored in a local path > >/var/run/libvirt/common/system.token > > Obviously with only traditional UNIX DAC in effect, this is largely > security through obscurity, if the client is running at the same > privilege level as the daemon. It does, however, reliably distinguish > an unprivilegd client from the system daemons. unprivileged > > With a MAC system like SELinux though, or possible use of containers, > access can be further restricted. > > A possible future improvement for Linux would be to populate the > kernel keyring with a secret for libvirt daemons to share. > > Reviewed-by: Michal Privoznik > Signed-off-by: Daniel P. Berrangé > --- > src/util/viridentity.c | 102 +++++ > 1 file changed, 102 insertions(+) > -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [libvirt PATCH v2 02/10] util: introduce concept of a system token into identities
On 5/7/21 11:24 AM, Daniel P. Berrangé wrote: > We want a way to distinguish between calls from a libvirt daemon, and a > regular client application when both are running as the same user > account. This is not possible with the current set of attributes > recorded against an identity, as there is nothing that is common to all > of the modular libvirt daemons, while distinct to all other processes. > > We thus introduce the idea of a system token, which is simply a random > hex string that is only known by the libvirt daemons, to be recored recorded > against the system identity. > > Reviewed-by: Michal Privoznik > Signed-off-by: Daniel P. Berrangé > --- -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH 2/2] qemu-img: Require -F with -b backing image
On 5/3/21 4:45 PM, Eric Blake wrote:
> On 5/3/21 4:36 PM, Eric Blake wrote:
>> Back in commit d9f059aa6c (qemu-img: Deprecate use of -b without -F),
>> we deprecated the ability to create a file with a backing image that
>> requires qemu to perform format probing. Qemu can still probe older
>> files for backwards compatibility, but it is time to finish off the
>> ability to create such images, due to the potential security risk they
>> present. Update a couple of iotests affected by the change.
>>
>> Signed-off-by: Eric Blake
>> ---
>> docs/system/deprecated.rst | 20 -
>> docs/system/removed-features.rst | 19
>> block.c | 37 ++--
>> qemu-img.c | 6 --
>> tests/qemu-iotests/114 | 18
>> tests/qemu-iotests/114.out | 11 --
>> tests/qemu-iotests/301 | 4 +---
>> tests/qemu-iotests/301.out | 16 ++
>> 8 files changed, 50 insertions(+), 81 deletions(-)
>
> I'll need a followup to fix iotest failures in 40 and 41 (apparently
> they weren't passing backing formats, but I did not catch them in my
> original cleanup of iotests back in commit b66ff2c298)
Squash in:
diff --git i/tests/qemu-iotests/040 w/tests/qemu-iotests/040
index ba7cb34ce8cf..f3677de9dfde 100755
--- i/tests/qemu-iotests/040
+++ w/tests/qemu-iotests/040
@@ -920,8 +920,8 @@ class
TestCommitWithOverriddenBacking(iotests.QMPTestCase):
def setUp(self):
qemu_img('create', '-f', iotests.imgfmt, self.img_base_a, '1M')
qemu_img('create', '-f', iotests.imgfmt, self.img_base_b, '1M')
-qemu_img('create', '-f', iotests.imgfmt, '-b', self.img_base_a, \
- self.img_top)
+qemu_img('create', '-f', iotests.imgfmt, '-b', self.img_base_a,
+ '-F', iotests.imgfmt, self.img_top)
self.vm = iotests.VM()
self.vm.launch()
diff --git i/tests/qemu-iotests/041 w/tests/qemu-iotests/041
index 5cc02b24fc7a..db9f5dc540e8 100755
--- i/tests/qemu-iotests/041
+++ w/tests/qemu-iotests/041
@@ -1295,8 +1295,10 @@ class TestReplaces(iotests.QMPTestCase):
class TestFilters(iotests.QMPTestCase):
def setUp(self):
qemu_img('create', '-f', iotests.imgfmt, backing_img, '1M')
-qemu_img('create', '-f', iotests.imgfmt, '-b', backing_img,
test_img)
-qemu_img('create', '-f', iotests.imgfmt, '-b', backing_img,
target_img)
+qemu_img('create', '-f', iotests.imgfmt, '-b', backing_img,
+ '-F', iotests.imgfmt, test_img)
+qemu_img('create', '-f', iotests.imgfmt, '-b', backing_img,
+ '-F', iotests.imgfmt, target_img)
qemu_io('-c', 'write -P 1 0 512k', backing_img)
qemu_io('-c', 'write -P 2 512k 512k', test_img)
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PATCH 2/2] qemu-img: Require -F with -b backing image
On 5/3/21 4:36 PM, Eric Blake wrote: > Back in commit d9f059aa6c (qemu-img: Deprecate use of -b without -F), > we deprecated the ability to create a file with a backing image that > requires qemu to perform format probing. Qemu can still probe older > files for backwards compatibility, but it is time to finish off the > ability to create such images, due to the potential security risk they > present. Update a couple of iotests affected by the change. > > Signed-off-by: Eric Blake > --- > docs/system/deprecated.rst | 20 - > docs/system/removed-features.rst | 19 > block.c | 37 ++-- > qemu-img.c | 6 -- > tests/qemu-iotests/114 | 18 > tests/qemu-iotests/114.out | 11 -- > tests/qemu-iotests/301 | 4 +--- > tests/qemu-iotests/301.out | 16 ++ > 8 files changed, 50 insertions(+), 81 deletions(-) I'll need a followup to fix iotest failures in 40 and 41 (apparently they weren't passing backing formats, but I did not catch them in my original cleanup of iotests back in commit b66ff2c298) -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
[PATCH 2/2] qemu-img: Require -F with -b backing image
Back in commit d9f059aa6c (qemu-img: Deprecate use of -b without -F),
we deprecated the ability to create a file with a backing image that
requires qemu to perform format probing. Qemu can still probe older
files for backwards compatibility, but it is time to finish off the
ability to create such images, due to the potential security risk they
present. Update a couple of iotests affected by the change.
Signed-off-by: Eric Blake
---
docs/system/deprecated.rst | 20 -
docs/system/removed-features.rst | 19
block.c | 37 ++--
qemu-img.c | 6 --
tests/qemu-iotests/114 | 18
tests/qemu-iotests/114.out | 11 --
tests/qemu-iotests/301 | 4 +---
tests/qemu-iotests/301.out | 16 ++
8 files changed, 50 insertions(+), 81 deletions(-)
diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
index 9ec1a9d0e03e..aa6f7d84e583 100644
--- a/docs/system/deprecated.rst
+++ b/docs/system/deprecated.rst
@@ -315,26 +315,6 @@ this CPU is also deprecated.
Related binaries
-qemu-img backing file without format (since 5.1)
-
-
-The use of ``qemu-img create``, ``qemu-img rebase``, or ``qemu-img
-convert`` to create or modify an image that depends on a backing file
-now recommends that an explicit backing format be provided. This is
-for safety: if QEMU probes a different format than what you thought,
-the data presented to the guest will be corrupt; similarly, presenting
-a raw image to a guest allows a potential security exploit if a future
-probe sees a non-raw image based on guest writes.
-
-To avoid the warning message, or even future refusal to create an
-unsafe image, you must pass ``-o backing_fmt=`` (or the shorthand
-``-F`` during create) to specify the intended backing format. You may
-use ``qemu-img rebase -u`` to retroactively add a backing format to an
-existing image. However, be aware that there are already potential
-security risks to blindly using ``qemu-img info`` to probe the format
-of an untrusted backing image, when deciding what format to add into
-an existing image.
-
Backwards compatibility
---
diff --git a/docs/system/removed-features.rst b/docs/system/removed-features.rst
index 28b5df757d35..1928d8a483c0 100644
--- a/docs/system/removed-features.rst
+++ b/docs/system/removed-features.rst
@@ -466,6 +466,25 @@ backing chain should be performed with ``qemu-img rebase
-u`` either
before or after the remaining changes being performed by amend, as
appropriate.
+qemu-img backing file without format (removed in 6.1)
+'
+
+The use of ``qemu-img create``, ``qemu-img rebase``, or ``qemu-img
+convert`` to create or modify an image that depends on a backing file
+now requires that an explicit backing format be provided. This is
+for safety: if QEMU probes a different format than what you thought,
+the data presented to the guest will be corrupt; similarly, presenting
+a raw image to a guest allows a potential security exploit if a future
+probe sees a non-raw image based on guest writes.
+
+To avoid creating unsafe backing chains, you must pass ``-o
+backing_fmt=`` (or the shorthand ``-F`` during create) to specify the
+intended backing format. You may use ``qemu-img rebase -u`` to
+retroactively add a backing format to an existing image. However, be
+aware that there are already potential security risks to blindly using
+``qemu-img info`` to probe the format of an untrusted backing image,
+when deciding what format to add into an existing image.
+
Block devices
-
diff --git a/block.c b/block.c
index 874c22c43e3d..931e37a8499b 100644
--- a/block.c
+++ b/block.c
@@ -5033,7 +5033,7 @@ int coroutine_fn bdrv_co_check(BlockDriverState *bs,
* -ENOTSUP - format driver doesn't support changing the backing file
*/
int bdrv_change_backing_file(BlockDriverState *bs, const char *backing_file,
- const char *backing_fmt, bool warn)
+ const char *backing_fmt, bool require)
{
BlockDriver *drv = bs->drv;
int ret;
@@ -5047,10 +5047,8 @@ int bdrv_change_backing_file(BlockDriverState *bs, const
char *backing_file,
return -EINVAL;
}
-if (warn && backing_file && !backing_fmt) {
-warn_report("Deprecated use of backing file without explicit "
-"backing format, use of this image requires "
-"potentially unsafe format probing");
+if (require && backing_file && !backing_fmt) {
+return -EINVAL;
}
if (drv->bdrv_change_backing_file != NULL) {
@@ -6556,24 +6554,11 @@ void bdrv_img_create(const char *filename, const char
*fmt,
[PATCH 1/2] qcow2: Prohibit backing file changes in 'qemu-img amend'
This was deprecated back in bc5ee6da7 (qcow2: Deprecate use of
qemu-img amend to change backing file), and no one in the meantime has
given any reasons why it should be supported. Time to make change
attempts a hard error (but for convenience, specifying the _same_
backing chain is not forbidden). Update a couple of iotests to match.
Signed-off-by: Eric Blake
---
docs/system/deprecated.rst | 12
docs/system/removed-features.rst | 12
block/qcow2.c| 13 -
tests/qemu-iotests/061 | 3 +++
tests/qemu-iotests/061.out | 3 ++-
tests/qemu-iotests/082.out | 6 --
6 files changed, 25 insertions(+), 24 deletions(-)
diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
index 80cae862528a..9ec1a9d0e03e 100644
--- a/docs/system/deprecated.rst
+++ b/docs/system/deprecated.rst
@@ -315,18 +315,6 @@ this CPU is also deprecated.
Related binaries
-qemu-img amend to adjust backing file (since 5.1)
-'
-
-The use of ``qemu-img amend`` to modify the name or format of a qcow2
-backing image is deprecated; this functionality was never fully
-documented or tested, and interferes with other amend operations that
-need access to the original backing image (such as deciding whether a
-v3 zero cluster may be left unallocated when converting to a v2
-image). Rather, any changes to the backing chain should be performed
-with ``qemu-img rebase -u`` either before or after the remaining
-changes being performed by amend, as appropriate.
-
qemu-img backing file without format (since 5.1)
diff --git a/docs/system/removed-features.rst b/docs/system/removed-features.rst
index 29e90601a51a..28b5df757d35 100644
--- a/docs/system/removed-features.rst
+++ b/docs/system/removed-features.rst
@@ -454,6 +454,18 @@ topologies described with -smp include all possible cpus,
i.e.
The ``enforce-config-section`` property was replaced by the
``-global migration.send-configuration={on|off}`` option.
+qemu-img amend to adjust backing file (removed in 6.1)
+''
+
+The use of ``qemu-img amend`` to modify the name or format of a qcow2
+backing image was never fully documented or tested, and interferes
+with other amend operations that need access to the original backing
+image (such as deciding whether a v3 zero cluster may be left
+unallocated when converting to a v2 image). Any changes to the
+backing chain should be performed with ``qemu-img rebase -u`` either
+before or after the remaining changes being performed by amend, as
+appropriate.
+
Block devices
-
diff --git a/block/qcow2.c b/block/qcow2.c
index 9727ae8fe341..898c4fc4464c 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -5620,15 +5620,10 @@ static int qcow2_amend_options(BlockDriverState *bs,
QemuOpts *opts,
if (backing_file || backing_format) {
if (g_strcmp0(backing_file, s->image_backing_file) ||
g_strcmp0(backing_format, s->image_backing_format)) {
-warn_report("Deprecated use of amend to alter the backing file; "
-"use qemu-img rebase instead");
-}
-ret = qcow2_change_backing_file(bs,
-backing_file ?: s->image_backing_file,
-backing_format ?: s->image_backing_format);
-if (ret < 0) {
-error_setg_errno(errp, -ret, "Failed to change the backing file");
-return ret;
+error_setg(errp, "Cannot amend the backing file");
+error_append_hint(errp,
+ "You can use 'qemu-img rebase' instead.\n");
+return -EINVAL;
}
}
diff --git a/tests/qemu-iotests/061 b/tests/qemu-iotests/061
index e26d94a0df31..9507c223bda4 100755
--- a/tests/qemu-iotests/061
+++ b/tests/qemu-iotests/061
@@ -167,6 +167,9 @@ _make_test_img -o "compat=1.1" 64M
TEST_IMG="$TEST_IMG.base" _make_test_img -o "compat=1.1" 64M
$QEMU_IO -c "write -P 0x2a 0 128k" "$TEST_IMG.base" | _filter_qemu_io
$QEMU_IO -c "read -P 0 0 128k" "$TEST_IMG" | _filter_qemu_io
+$QEMU_IMG amend -o "backing_file=$TEST_IMG.base,backing_fmt=qcow2" \
+ "$TEST_IMG" && echo "unexpected pass"
+$QEMU_IMG rebase -u -b "$TEST_IMG.base" -F qcow2 "$TEST_IMG"
$QEMU_IMG amend -o "backing_file=$TEST_IMG.base,backing_fmt=qcow2" "$TEST_IMG"
$QEMU_IO -c "read -P 0x2a 0 128k" "$TEST_IMG" | _filter_qemu_io
_check_test_img
diff --git a/tests/qemu-iotests/061.out b/tests/qemu-iotests/061.out
index ee30da266514..7ecbd4dea875 100644
--- a/tests/qemu-iotests/061.out
+++ b/tests/qemu-iotests/061.out
@@ -370,7 +370,8 @@ wr
Re: Let's remove some deprecated stuff
On 4/29/21 4:59 AM, Markus Armbruster wrote: > If you're cc'ed, you added a section to docs/system/deprecated.rst that > is old enough to permit removal. This is *not* a demand to remove, it's > a polite request to consider whether the time for removal has come. > Extra points for telling us in a reply. "We should remove, but I can't > do it myself right now" is a valid answer. Let's review the file: > [adjusting cc for this response] > > Eric Blake: > > qemu-img amend to adjust backing file (since 5.1) > ' > > The use of ``qemu-img amend`` to modify the name or format of a qcow2 > backing image is deprecated; this functionality was never fully > documented or tested, and interferes with other amend operations that > need access to the original backing image (such as deciding whether a > v3 zero cluster may be left unallocated when converting to a v2 > image). Rather, any changes to the backing chain should be performed > with ``qemu-img rebase -u`` either before or after the remaining > changes being performed by amend, as appropriate. > > qemu-img backing file without format (since 5.1) > > > The use of ``qemu-img create``, ``qemu-img rebase``, or ``qemu-img > convert`` to create or modify an image that depends on a backing file > now recommends that an explicit backing format be provided. This is > for safety: if QEMU probes a different format than what you thought, > the data presented to the guest will be corrupt; similarly, presenting > a raw image to a guest allows a potential security exploit if a future > probe sees a non-raw image based on guest writes. > > To avoid the warning message, or even future refusal to create an > unsafe image, you must pass ``-o backing_fmt=`` (or the shorthand > ``-F`` during create) to specify the intended backing format. You may > use ``qemu-img rebase -u`` to retroactively add a backing format to an > existing image. However, be aware that there are already potential > security risks to blindly using ``qemu-img info`` to probe the format > of an untrusted backing image, when deciding what format to add into > an existing image. I'm not sure how widely used these were, but I'm game for writing a patch to drop them. I'm fairly certain libvirt is not using them. > > Kevin Wolf: > > ``nbd-server-add`` and ``nbd-server-remove`` (since 5.2) > > > Use the more generic commands ``block-export-add`` and > ``block-export-del`` > instead. As part of this deprecation, where ``nbd-server-add`` used a > single ``bitmap``, the new ``block-export-add`` uses a list of > ``bitmaps``. Peter, is libvirt good for this one to go? -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH 3/3] NEWS: Mention support for full backups via virDomainBackupBegin
On 3/18/21 11:45 AM, Peter Krempa wrote: > Signed-off-by: Peter Krempa > --- > NEWS.rst | 9 + > 1 file changed, 9 insertions(+) > > diff --git a/NEWS.rst b/NEWS.rst > index 9d819a3cf2..c2013ecac9 100644 > --- a/NEWS.rst > +++ b/NEWS.rst > @@ -24,6 +24,15 @@ v7.2.0 (unreleased) > The memory dirty rate stats can be obtained through ``virsh domstats > --dirtyrate`` via the virConnectGetAllDomainStats API. > > + * qemu: Full disk backups via ``virDomainBackupBegin`` > + > +The qemu hypervisor dirver now allows taking full disk backups via the driver > +``virDomainBackupBegin`` API and the corresponding virsh wrapper. > + > +In future releases the feature will be extended to also support > incremental > +backups (where only the difference since the last backup is copied) when > +qemu adds the required functionality. > + > * **Improvements** > > * **Bug fixes** > -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 10/13] block: remove 'encryption_key_missing' flag from QAPI
On 3/15/21 12:45 PM, Daniel P. Berrangé wrote: > This has been hardcoded to "false" since 2.10.0, since secrets required > to unlock block devices are now always provided upfront instead of using up front > interactive prompts. > > Reviewed-by: Thomas Huth > Signed-off-by: Daniel P. Berrangé > --- > block/qapi.c | 1 - > docs/system/deprecated.rst | 10 --- > docs/system/removed-features.rst | 10 +++ > qapi/block-core.json | 8 -- > tests/qemu-iotests/184.out | 6 ++-- > tests/qemu-iotests/191.out | 48 +++- > tests/qemu-iotests/273.out | 15 -- > 7 files changed, 33 insertions(+), 65 deletions(-) Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 03/13] monitor: remove 'query-events' QMP command
On 3/15/21 12:45 PM, Daniel P. Berrangé wrote: > The code comment suggests removing QAPIEvent_(str|lookup) symbols too, > however, these are both auto-generated as standard for any enum in > QAPI. As such it they'll exist whether we use them or not. > > Reviewed-by: Thomas Huth > Signed-off-by: Daniel P. Berrangé > --- > docs/system/deprecated.rst | 6 - > docs/system/removed-features.rst | 6 + > monitor/qmp-cmds-control.c | 24 - > qapi/control.json| 45 > 4 files changed, 6 insertions(+), 75 deletions(-) Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH 14/14] block: remove support for using "file" driver with block/char devices
On 2/24/21 7:11 AM, Daniel P. Berrangé wrote:
> The 'host_device' and 'host_cdrom' drivers must be used instead.
>
> Signed-off-by: Daniel P. Berrangé
> ---
> block/file-posix.c | 17 ++---
> docs/system/deprecated.rst | 7 ---
> docs/system/removed-features.rst | 7 +++
> tests/qemu-iotests/226.out | 10 +-
> 4 files changed, 18 insertions(+), 23 deletions(-)
>
> if (!device) {
> -if (S_ISBLK(st.st_mode)) {
> -warn_report("Opening a block device as a file using the '%s' "
> -"driver is deprecated", bs->drv->format_name);
> -} else if (S_ISCHR(st.st_mode)) {
> -warn_report("Opening a character device as a file using the '%s'
> "
> -"driver is deprecated", bs->drv->format_name);
> -} else if (!S_ISREG(st.st_mode)) {
> -error_setg(errp, "A regular file was expected by the '%s'
> driver, "
> - "but something else was given", bs->drv->format_name);
> +if (!S_ISREG(st.st_mode)) {
We're testing with S_ISREG()...
> +++ b/docs/system/deprecated.rst
> @@ -21,13 +21,6 @@ deprecated.
> System emulator command line arguments
> --
>
> -``-drive file=json:{...{'driver':'file'}}`` (since 3.0)
> -'''
> -
> -The 'file' driver for drives is no longer appropriate for character or host
> -devices and will only accept regular files (S_IFREG). The correct driver
but documented with S_IFREG(). Thankfully, the two have semantically
equivalent purposes, so the difference doesn't invalidate the docs.
Reviewed-by: Eric Blake
but I wouldn't mind if at least one other block maintainer chimes in.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
[PULL 05/17] utils: Deprecate hex-with-suffix sizes
Supporting '0x20M' looks odd, particularly since we have a 'B' suffix
that is ambiguous for bytes, as well as a less-frequently-used 'E'
suffix for extremely large exibytes. In practice, people using hex
inputs are specifying values in bytes (and would have written
0x200, or possibly relied on default_suffix in the case of
qemu_strtosz_MiB), and the use of scaling suffixes makes the most
sense for inputs in decimal (where the user would write 32M). But
rather than outright dropping support for hex-with-suffix, let's
follow our deprecation policy. Sadly, since qemu_strtosz() does not
have an Err** parameter, and plumbing that in would be a much larger
task, we instead go with just directly emitting the deprecation
warning to stderr.
Signed-off-by: Eric Blake
Message-Id: <20210211204438.1184395-4-ebl...@redhat.com>
Reviewed-by: Daniel P. Berrangé
---
docs/system/deprecated.rst | 8
util/cutils.c | 10 +-
2 files changed, 17 insertions(+), 1 deletion(-)
diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
index cfabe6984677..ecff6bf8c633 100644
--- a/docs/system/deprecated.rst
+++ b/docs/system/deprecated.rst
@@ -166,6 +166,14 @@ Using ``-M kernel-irqchip=off`` with x86 machine types
that include a local
APIC is deprecated. The ``split`` setting is supported, as is using
``-M kernel-irqchip=off`` with the ISA PC machine type.
+hexadecimal sizes with scaling multipliers (since 6.0)
+''
+
+Input parameters that take a size value should only use a size suffix
+(such as 'k' or 'M') when the base is written in decimal, and not when
+the value is hexadecimal. That is, '0x20M' is deprecated, and should
+be written either as '32M' or as '0x200'.
+
QEMU Machine Protocol (QMP) commands
diff --git a/util/cutils.c b/util/cutils.c
index 189a184859c0..d89a40a8c325 100644
--- a/util/cutils.c
+++ b/util/cutils.c
@@ -250,6 +250,9 @@ static int64_t suffix_mul(char suffix, int64_t unit)
* fractional portion is truncated to byte
* - 0x7fEE - hexadecimal, unit determined by @default_suffix
*
+ * The following cause a deprecation warning, and may be removed in the future
+ * - 0xabc{kKmMgGtTpP} - hex with scaling suffix
+ *
* The following are intentionally not supported
* - octal, such as 08
* - fractional hex, such as 0x1.8
@@ -272,7 +275,7 @@ static int do_strtosz(const char *nptr, const char **end,
int retval;
const char *endptr, *f;
unsigned char c;
-bool mul_required = false;
+bool mul_required = false, hex = false;
uint64_t val;
int64_t mul;
double fraction = 0.0;
@@ -298,6 +301,7 @@ static int do_strtosz(const char *nptr, const char **end,
retval = -EINVAL;
goto out;
}
+hex = true;
} else if (*endptr == '.') {
/*
* Input looks like a fraction. Make sure even 1.k works
@@ -320,6 +324,10 @@ static int do_strtosz(const char *nptr, const char **end,
c = *endptr;
mul = suffix_mul(c, unit);
if (mul > 0) {
+if (hex) {
+warn_report("Using a multiplier suffix on hex numbers "
+"is deprecated: %s", nptr);
+}
endptr++;
} else {
mul = suffix_mul(default_suffix, unit);
--
2.30.1
Re: [PATCH v3 29/30] vl: QAPIfy -object
On 3/8/21 10:54 AM, Kevin Wolf wrote: > This switches the system emulator from a QemuOpts-based parser for > -object to user_creatable_parse_str() which uses a keyval parser and > enforces the QAPI schema. > > Apart from being a cleanup, this makes non-scalar properties accessible. > > This adopts a similar model as -blockdev uses: When parsing the option, > create the ObjectOptions and queue them. At the later point where we > used to create objects for the collected QemuOpts, the ObjectOptions > queue is processed instead. > > A complication compared to -blockdev is that object definitions are > supported in -readconfig and -writeconfig. > > After this patch, -readconfig still works, though it still goes through > the QemuOpts parser, which means that improvements like non-scalar > properties are still not available in config files. > > -writeconfig stops working for -object. Tough luck. It has never > supported all options (not even the common ones), so supporting one less > isn't the end of the world. As object definitions from -readconfig still > go through QemuOpts, they are still included in -writeconfig output, > which at least prevents destroying your existing configuration when you > just wanted to add another option. Maybe worth a tweak to this paragraph now that b979c931 has landed formally deprecating -writeconfig (all the more reason we don't care about it). > > Signed-off-by: Kevin Wolf > Acked-by: Peter Krempa > Reviewed-by: Eric Blake R-b stands either way. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH v3 26/30] qemu-img: Use user_creatable_process_cmdline() for --object
On 3/8/21 10:54 AM, Kevin Wolf wrote:
> This switches qemu-img from a QemuOpts-based parser for --object to
> user_creatable_process_cmdline() which uses a keyval parser and enforces
> the QAPI schema.
>
> Apart from being a cleanup, this makes non-scalar properties accessible.
>
> Signed-off-by: Kevin Wolf
> Acked-by: Peter Krempa
> ---
> qemu-img.c | 251 ++---
> 1 file changed, 45 insertions(+), 206 deletions(-)
>
> @@ -1353,7 +1303,7 @@ static int check_empty_sectors(BlockBackend *blk,
> int64_t offset,
> /*
> * Compares two images. Exit codes:
> *
> - * 0 - Images are identical
> + * 0 - Images are identical or the requested help was printed
Nice, but does the user-facing documentation need updating to match?
> * 1 - Images differ
> * >1 - Error occurred
> */
> @@ -1423,15 +1373,21 @@ static int img_compare(int argc, char **argv)
> case 'U':
> force_share = true;
> break;
> -case OPTION_OBJECT: {
> -QemuOpts *opts;
> -opts = qemu_opts_parse_noisily(_object_opts,
> - optarg, true);
> -if (!opts) {
> -ret = 2;
> -goto out4;
> +case OPTION_OBJECT:
> +{
> +Error *local_err = NULL;
> +
> +if (!user_creatable_add_from_str(optarg, _err)) {
> +if (local_err) {
> +error_report_err(local_err);
> +exit(2);
> +} else {
> +/* Help was printed */
> +exit(EXIT_SUCCESS);
> +}
The commit message needs to be updated to call out that this bug fix was
intentional, preferably mentioning the commit where we broke it
(334c43e2c3).
The code is fine, though, so with an improved commit message (and maybe
some matching doc tweaks),
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PATCH v3 06/30] qapi/qom: Add ObjectOptions for memory-backend-*
On 3/8/21 10:54 AM, Kevin Wolf wrote:
> This adds a QAPI schema for the properties of the memory-backend-*
> objects.
>
> HostMemPolicy has to be moved to an include file that can be used by the
> storage daemon, too, because ObjectOptions must be the same in all
> binaries if we don't want to compile the whole code multiple times.
>
> Signed-off-by: Kevin Wolf
> Acked-by: Peter Krempa
> ---
> qapi/common.json | 20
> qapi/machine.json | 22 +
> qapi/qom.json | 121 +-
> 3 files changed, 141 insertions(+), 22 deletions(-)
>
> @@ -287,7 +397,10 @@
> 'cryptodev-backend-builtin',
> 'cryptodev-vhost-user',
> 'dbus-vmstate',
> -'iothread'
> +'iothread',
> +'memory-backend-file',
> +'memory-backend-memfd',
> +'memory-backend-ram'
Another leaked enum value...
>] }
>
> ##
> @@ -315,7 +428,11 @@
>'cryptodev-vhost-user': { 'type': 'CryptodevVhostUserProperties',
>'if': 'defined(CONFIG_VIRTIO_CRYPTO)
> && defined(CONFIG_VHOST_CRYPTO)' },
>'dbus-vmstate': 'DBusVMStateProperties',
> - 'iothread': 'IothreadProperties'
> + 'iothread': 'IothreadProperties',
> + 'memory-backend-file':'MemoryBackendFileProperties',
> + 'memory-backend-memfd': { 'type': 'MemoryBackendMemfdProperties',
> + 'if': 'defined(CONFIG_LINUX)' },
> + 'memory-backend-ram': 'MemoryBackendProperties'
>} }
...when compared to the union branches.
Once fixed,
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PATCH v3 04/30] qapi/qom: Add ObjectOptions for cryptodev-*
On 3/8/21 10:54 AM, Kevin Wolf wrote:
> This adds a QAPI schema for the properties of the cryptodev-* objects.
>
> These interfaces have some questionable aspects (cryptodev-backend is
> really an abstract base class without function, and the queues option
> only makes sense for cryptodev-vhost-user), but as the goal is to
> represent the existing interface in QAPI, leave these things in place.
>
> Signed-off-by: Kevin Wolf
> Acked-by: Peter Krempa
> ---
> qapi/qom.json | 35 +++
> 1 file changed, 35 insertions(+)
>
> @@ -239,6 +267,9 @@
> 'authz-listfile',
> 'authz-pam',
> 'authz-simple',
> +'cryptodev-backend',
> +'cryptodev-backend-builtin',
> +'cryptodev-vhost-user',
Shouldn't the enum value be conditional...
> 'iothread'
>] }
>
> @@ -262,6 +293,10 @@
>'authz-listfile': 'AuthZListFileProperties',
>'authz-pam': 'AuthZPAMProperties',
>'authz-simple': 'AuthZSimpleProperties',
> + 'cryptodev-backend': 'CryptodevBackendProperties',
> + 'cryptodev-backend-builtin': 'CryptodevBackendProperties',
> + 'cryptodev-vhost-user': { 'type': 'CryptodevVhostUserProperties',
> + 'if': 'defined(CONFIG_VIRTIO_CRYPTO)
> && defined(CONFIG_VHOST_CRYPTO)' },
...if the union branch is likewise?
> 'iothread': 'IothreadProperties'
>} }
>
>
With that fixed,
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PATCH 2/5] virIndexToDiskName: Use g_string_prepend(_c) to improve readability
On 3/3/21 4:45 AM, Peter Krempa wrote: > Use a dynamic string helper so that we don't have to calculate the > string lenghts and then iterate from the rear. lengths > > Signed-off-by: Peter Krempa > --- > src/util/virutil.c | 21 ++--- > 1 file changed, 6 insertions(+), 15 deletions(-) > -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 12/31] qapi/qom: Add ObjectOptions for can-*
On 3/2/21 12:32 PM, Kevin Wolf wrote:
> Am 26.02.2021 um 20:42 hat Eric Blake geschrieben:
>> On 2/24/21 7:52 AM, Kevin Wolf wrote:
>>> This adds a QAPI schema for the properties of the can-* objects.
>>>
>>> can-bus doesn't have any properties, so it only needs to be added to the
>>> ObjectType enum without adding a new branch to ObjectOptions.
>>
>> I somewhat prefer
>>
>> 'can-bus': {},
>>
>> to make it explicit that we thought about it, but since we allow
>> defaulted union branches, your approach works too.
>
> The QAPI generator disagrees:
>
> ../qapi/qom.json: In union 'ObjectOptions':
> ../qapi/qom.json:492: 'data' member 'can-bus' misses key 'type'
>
> It seems we can't use inline definitions of struct types because we
> already use that for the extended description of branch types. And
> adding a whole named struct without content is probably a bit too much?
Oh, maybe I'm remembering an experiment I did with a patch to add that
once, but it never went anywhere, since in the meantime we added the
'any enum not listed is acceptable as adding no additional members'. So
my preference stems from (faulty?) memory on my part, and your patch is
fine as is.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PATCH 2/4] docs: Lift restriction on running API from the event loop thread
On 3/1/21 5:49 AM, Michal Privoznik wrote: > Since v6.2.0-rc1~238 (and friends) QMP processing was moved to a > per-domain thread. Therefore, it is now safe to call APIs from > the event loop thread (e.g. just like qemu shim is doing in > qemuShimEventLoop(). However, it is still important to let the > event loop run after each API call (obviously). > > Signed-off-by: Michal Privoznik > --- > docs/drvqemu.html.in | 6 -- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/docs/drvqemu.html.in b/docs/drvqemu.html.in > index 31d3fee213..3cdd04aa1e 100644 > --- a/docs/drvqemu.html.in > +++ b/docs/drvqemu.html.in > @@ -158,8 +158,10 @@ qemu+ssh://r...@example.com/system (remote access, SSH > tunnelled) >in mind, applications must NEVER invoke API >calls from the event loop thread itself, only other threads. >Not following this rule will lead to deadlocks in the API. > - This restriction is intended to be lifted in a future release > - of libvirt, once QMP processing moves to a dedicated thread. > + This restriction was lifted starting from 6.2.0 release, when > + QMP processing moved to a dedicated thread. However, it is > + important to let the event loop run after each API call, even > + the ones made from the vent loop thread itself. event loop > > > Driver security architecture > -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 31/31] qom: Drop QemuOpts based interfaces
On 2/24/21 7:52 AM, Kevin Wolf wrote: > user_creatable_add_opts() has only a single user left, which is a test > case. Rewrite the test to use user_creatable_add_type() instead (which > is the remaining function that doesn't require a QAPI schema) and drop > the QemuOpts related functions. > > Signed-off-by: Kevin Wolf > --- > include/qom/object_interfaces.h | 59 > qom/object_interfaces.c | 81 - > tests/check-qom-proplist.c | 42 - > 3 files changed, 20 insertions(+), 162 deletions(-) Yay! Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 30/31] vl: QAPIfy -object
On 2/24/21 7:52 AM, Kevin Wolf wrote: > This switches the system emulator from a QemuOpts-based parser for > -object to user_creatable_parse_str() which uses a keyval parser and > enforces the QAPI schema. > > Apart from being a cleanup, this makes non-scalar properties accessible. > > This adopts a similar model as -blockdev uses: When parsing the option, > create the ObjectOptions and queue them. At the later point where we > used to create objects for the collected QemuOpts, the ObjectOptions > queue is processed instead. > > A complication compared to -blockdev is that object definitions are > supported in -readconfig and -writeconfig. > > After this patch, -readconfig still works, though it still goes through > the QemuOpts parser, which means that improvements like non-scalar > properties are still not available in config files. > > -writeconfig stops working for -object. Tough luck. It has never > supported all options (not even the common ones), so supporting one less > isn't the end of the world. As object definitions from -readconfig still > go through QemuOpts, they are still included in -writeconfig output, > which at least prevents destroying your existing configuration when you > just wanted to add another option. And Paolo has submitted a patch deprecating it. > > Signed-off-by: Kevin Wolf > --- > softmmu/vl.c | 109 +++---- > 1 file changed, 84 insertions(+), 25 deletions(-) > Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 29/31] qom: Add user_creatable_parse_str()
On 2/24/21 7:52 AM, Kevin Wolf wrote: > The system emulator has a more complicated way of handling command line > options in that it reorders options before it processes them. This means > that parsing object options and creating the object happen at two > different points. Split the parsing part into a separate function that > can be reused by the system emulator command line. > > Signed-off-by: Kevin Wolf > --- > include/qom/object_interfaces.h | 15 +++ > qom/object_interfaces.c | 20 ++-- > 2 files changed, 29 insertions(+), 6 deletions(-) > Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 28/31] hmp: QAPIfy object_add
On 2/24/21 7:52 AM, Kevin Wolf wrote: > This switches the HMP command object_add from a QemuOpts-based parser to > user_creatable_add_from_str() which uses a keyval parser and enforces > the QAPI schema. > > Apart from being a cleanup, this makes non-scalar properties and help > accessible. In order for help to be printed to the monitor instead of > stdout, the printf() calls in the help functions are changed to > qemu_printf(). > > Signed-off-by: Kevin Wolf > --- > monitor/hmp-cmds.c | 17 ++--- > qom/object_interfaces.c | 11 ++- > hmp-commands.hx | 2 +- > 3 files changed, 9 insertions(+), 21 deletions(-) > Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 27/31] qom: Add user_creatable_add_from_str()
On 2/24/21 7:52 AM, Kevin Wolf wrote: > This is a version of user_creatable_process_cmdline() with an Error > parameter that never calls exit() and is therefore usable in HMP. > > Signed-off-by: Kevin Wolf > --- > include/qom/object_interfaces.h | 16 > qom/object_interfaces.c | 29 - > 2 files changed, 40 insertions(+), 5 deletions(-) > > +/** > + * user_creatable_add_from_str: > + * @optarg: the object definition string as passed on the command line > + * @errp: if an error occurs, a pointer to an area to store the error > + * > + * Create an instance of the user creatable object by parsing optarg > + * with a keyval parser and implicit key 'qom-type', converting the > + * result to ObjectOptions and calling into qmp_object_add(). > + * > + * If a help option is given, print help instead. > + * > + * Returns: true when an object was successfully created, false when an error > + * occurred (*errp is set then) or help was printed (*errp is not set). > + */ > +bool user_creatable_add_from_str(const char *optarg, Error **errp); This could be used to fix the exit status 2 issue in qemu-img convert, if you rearrange the series a bit. Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 26/31] qemu-nbd: Use user_creatable_process_cmdline() for --object
On 2/24/21 7:52 AM, Kevin Wolf wrote: > This switches qemu-nbd from a QemuOpts-based parser for --object to > user_creatable_process_cmdline() which uses a keyval parser and enforces > the QAPI schema. > > Apart from being a cleanup, this makes non-scalar properties accessible. > > Signed-off-by: Kevin Wolf > --- > qemu-nbd.c | 34 +++--- > 1 file changed, 3 insertions(+), 31 deletions(-) > Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 25/31] qemu-img: Use user_creatable_process_cmdline() for --object
On 2/26/21 3:56 PM, Eric Blake wrote:
> On 2/24/21 7:52 AM, Kevin Wolf wrote:
>> This switches qemu-img from a QemuOpts-based parser for --object to
>> user_creatable_process_cmdline() which uses a keyval parser and enforces
>> the QAPI schema.
>>
>> Apart from being a cleanup, this makes non-scalar properties accessible.
>>
>> Signed-off-by: Kevin Wolf
>> ---
>> qemu-img.c | 239 -
>> 1 file changed, 33 insertions(+), 206 deletions(-)
>>
>
>> @@ -1423,15 +1373,9 @@ static int img_compare(int argc, char **argv)
>> case 'U':
>> force_share = true;
>> break;
>> -case OPTION_OBJECT: {
>> -QemuOpts *opts;
>> -opts = qemu_opts_parse_noisily(_object_opts,
>> - optarg, true);
>> -if (!opts) {
>> -ret = 2;
>> -goto out4;
>
> Our exit status here of 2 on failure appears to be intentional (since we
> reserve 0 for identical, 1 for mismatch, >1 for error)...
>
>> -}
>> -} break;
>> +case OPTION_OBJECT:
>> +user_creatable_process_cmdline(optarg);
>> +break;
>
> ...but becomes 1 here. Does that matter?
>
> /me goes and tests...
>
> Ouch: with current qemu.git master and none of this series applied:
>
> $ ./qemu-img compare --object foo,id=x /dev/null /dev/null
> qemu-img: invalid object type: foo
> $ echo $?
> 1
Okay, that didn't do what I expected, but this does:
$ ./qemu-img compare --object foo,id=1 /dev/null /dev/null
qemu-img: Parameter 'id' expects an identifier
Identifiers consist of letters, digits, '-', '.', '_', starting with a
letter.
$ echo $?
2
> $ gdb --args ./qemu-img compare --object foo,id=x /dev/null /dev/null
> (gdb) b qemu_opts_pars
> (gdb) r
> (gdb) fin
> Run till exit from #0 qemu_opts_parse_noisily (
> list=0x5578f020 , params=0x7fffd8a8
> "foo,id=x",
> permit_abbrev=true) at ../util/qemu-option.c:948
> 0x555805f9 in img_compare (argc=5, argv=0x7fffd480)
> at ../qemu-img.c:1428
> 1428 opts = qemu_opts_parse_noisily(_object_opts,
> Value returned is $1 = (QemuOpts *) 0x5583b4b0
> (gdb) p *opts
> $3 = {id = 0x557a0d58 "`\264\203UUU", list = 0x51,
and this may be my confusion with gdb. Right after 'fin', *opts is not
the same as *$1 (apparently gdb has stopped at a point where the 'opts'
currently in scope is not the opts set by qemu_opts_parse_noisily, but
before the opts in scope has actually been assigned the returned value).
>
> That looks buggy. qemu_opts_parse_noisily() is NOT returning NULL, but
> rather a pointer to something garbage (that id pointing to a garbage
> string in the middle of qemu_trace_opts is fishy), and so we've been
> exiting with status 1 in spite of the code.
>
> Looks like we'll want a separate patch fixing that first.
So I was wrong on when qemu_opts_parse_noisily() returns NULL - it does
NOT reject unknown object names (that was the job of the
qemu_opts_foreach call later), but merely rejects bad/duplicate ids.
Thus this code was indeed giving an exit status of 2 when actually
triggered correctly,
>
>> case OPTION_IMAGE_OPTS:
>> image_opts = true;
>> break;
>> @@ -1450,13 +1394,6 @@ static int img_compare(int argc, char **argv)
>> filename1 = argv[optind++];
>> filename2 = argv[optind++];
>>
>> -if (qemu_opts_foreach(_object_opts,
>> - user_creatable_add_opts_foreach,
>> - qemu_img_object_print_help, _fatal)) {
>> -ret = 2;
>> -goto out4;
>
> Same deal with return value. Except here we used _fatal (which
> forces an exit status of 1 rather than returning), and so never even
> reach the ret=2 code. Looks like we broke that in commit 334c43e2c3,
> where we used to pass NULL instead of _fatal (although that commit
> was in turn fixing another problem).
...and THIS spot is why my original attempt to prove that your code was
causing a regression was seeing an exit status of 1, where I instead
ended up proving that we already regressed.
>
> The rest of this patch looks fine, although maybe
> user_creatable_process_cmdline() should be given an 'int status'
> parameter for specifying 1 vs. 2 (or any other non-zero value) if we
> intend to fix the status of qemu-img compare failures. (Thankfully,
> even though qemu-img check also has a variety of documented return
> values other than 1, at least it documented 1 as internal errors and was
> already using 1 for --object failures).
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 25/31] qemu-img: Use user_creatable_process_cmdline() for --object
On 2/24/21 7:52 AM, Kevin Wolf wrote:
> This switches qemu-img from a QemuOpts-based parser for --object to
> user_creatable_process_cmdline() which uses a keyval parser and enforces
> the QAPI schema.
>
> Apart from being a cleanup, this makes non-scalar properties accessible.
>
> Signed-off-by: Kevin Wolf
> ---
> qemu-img.c | 239 -
> 1 file changed, 33 insertions(+), 206 deletions(-)
>
> @@ -1423,15 +1373,9 @@ static int img_compare(int argc, char **argv)
> case 'U':
> force_share = true;
> break;
> -case OPTION_OBJECT: {
> -QemuOpts *opts;
> -opts = qemu_opts_parse_noisily(_object_opts,
> - optarg, true);
> -if (!opts) {
> -ret = 2;
> -goto out4;
Our exit status here of 2 on failure appears to be intentional (since we
reserve 0 for identical, 1 for mismatch, >1 for error)...
> -}
> -} break;
> +case OPTION_OBJECT:
> +user_creatable_process_cmdline(optarg);
> +break;
...but becomes 1 here. Does that matter?
/me goes and tests...
Ouch: with current qemu.git master and none of this series applied:
$ ./qemu-img compare --object foo,id=x /dev/null /dev/null
qemu-img: invalid object type: foo
$ echo $?
1
$ gdb --args ./qemu-img compare --object foo,id=x /dev/null /dev/null
(gdb) b qemu_opts_pars
(gdb) r
(gdb) fin
Run till exit from #0 qemu_opts_parse_noisily (
list=0x5578f020 , params=0x7fffd8a8
"foo,id=x",
permit_abbrev=true) at ../util/qemu-option.c:948
0x555805f9 in img_compare (argc=5, argv=0x7fffd480)
at ../qemu-img.c:1428
1428opts = qemu_opts_parse_noisily(_object_opts,
Value returned is $1 = (QemuOpts *) 0x5583b4b0
(gdb) p *opts
$3 = {id = 0x557a0d58 "`\264\203UUU", list = 0x51,
loc = {kind = (unknown: 0x557f08f0), num = 21845,
ptr = 0x5578f020 , prev = 0x0}, head = {
tqh_first = 0x0, tqh_circ = {tql_next = 0x0, tql_prev = 0x0}}, next = {
tqe_next = 0x5583b500, tqe_circ = {tql_next = 0x5583b500,
tql_prev = 0x5583b528}}}
(gdb)
That looks buggy. qemu_opts_parse_noisily() is NOT returning NULL, but
rather a pointer to something garbage (that id pointing to a garbage
string in the middle of qemu_trace_opts is fishy), and so we've been
exiting with status 1 in spite of the code.
Looks like we'll want a separate patch fixing that first.
> case OPTION_IMAGE_OPTS:
> image_opts = true;
> break;
> @@ -1450,13 +1394,6 @@ static int img_compare(int argc, char **argv)
> filename1 = argv[optind++];
> filename2 = argv[optind++];
>
> -if (qemu_opts_foreach(_object_opts,
> - user_creatable_add_opts_foreach,
> - qemu_img_object_print_help, _fatal)) {
> -ret = 2;
> -goto out4;
Same deal with return value. Except here we used _fatal (which
forces an exit status of 1 rather than returning), and so never even
reach the ret=2 code. Looks like we broke that in commit 334c43e2c3,
where we used to pass NULL instead of _fatal (although that commit
was in turn fixing another problem).
The rest of this patch looks fine, although maybe
user_creatable_process_cmdline() should be given an 'int status'
parameter for specifying 1 vs. 2 (or any other non-zero value) if we
intend to fix the status of qemu-img compare failures. (Thankfully,
even though qemu-img check also has a variety of documented return
values other than 1, at least it documented 1 as internal errors and was
already using 1 for --object failures).
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 24/31] qemu-io: Use user_creatable_process_cmdline() for --object
On 2/24/21 7:52 AM, Kevin Wolf wrote: > This switches qemu-io from a QemuOpts-based parser for --object to > user_creatable_process_cmdline() which uses a keyval parser and enforces > the QAPI schema. > > Apart from being a cleanup, this makes non-scalar properties accessible. > > Signed-off-by: Kevin Wolf > --- > qemu-io.c | 33 +++-- > 1 file changed, 3 insertions(+), 30 deletions(-) > Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 23/31] qom: Factor out user_creatable_process_cmdline()
On 2/24/21 7:52 AM, Kevin Wolf wrote: > The implementation for --object can be shared between > qemu-storage-daemon and other binaries, so move it into a function in > qom/object_interfaces.c that is accessible from everywhere. > > This also requires moving the implementation of qmp_object_add() into a > new user_creatable_add_qapi(), because qom/qom-qmp-cmds.c is not linked > for tools. > > user_creatable_print_help_from_qdict() can become static now. > > Signed-off-by: Kevin Wolf > --- Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 22/31] qom: Remove user_creatable_add_dict()
On 2/24/21 7:52 AM, Kevin Wolf wrote: > This function is now unused and can be removed. > > Signed-off-by: Kevin Wolf > --- > include/qom/object_interfaces.h | 18 -- > qom/object_interfaces.c | 32 > 2 files changed, 50 deletions(-) > Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 21/31] qemu-storage-daemon: Implement --object with qmp_object_add()
On 2/24/21 7:52 AM, Kevin Wolf wrote: > This QAPIfies --object and ensures that QMP and the command line option > behave the same. > > Signed-off-by: Kevin Wolf > --- > storage-daemon/qemu-storage-daemon.c | 21 ++--- > 1 file changed, 10 insertions(+), 11 deletions(-) > Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 20/31] qom: Make "object" QemuOptsList optional
On 2/24/21 7:52 AM, Kevin Wolf wrote: > This code is going away anyway, but for a few more commits, we'll be in > a state where some binaries still use QemuOpts and others don't. If the > "object" QemuOptsList doesn't even exist, we don't have to remove (or > fail to remove, and therefore abort) a user creatable object from it. > > Signed-off-by: Kevin Wolf > --- > qom/object_interfaces.c | 7 +-- > 1 file changed, 5 insertions(+), 2 deletions(-) > Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 19/31] qapi/qom: QAPIfy object-add
On 2/24/21 7:52 AM, Kevin Wolf wrote:
> This converts object-add from 'gen': false to the ObjectOptions QAPI
> type. As an immediate benefit, clients can now use QAPI schema
> introspection for user creatable QOM objects.
>
> It is also the first step towards making the QAPI schema the only
> external interface for the creation of user creatable objects. Once all
> other places (HMP and command lines of the system emulator and all
> tools) go through QAPI, too, some object implementations can be
> simplified because some checks (e.g. that mandatory options are set) are
> already performed by QAPI, and in another step, QOM boilerplate code
> could be generated from the schema.
>
> Signed-off-by: Kevin Wolf
> ---
> qapi/qom.json| 11 +--
> include/qom/object_interfaces.h | 7 ---
> hw/block/xen-block.c | 16
> monitor/misc.c | 2 --
> qom/qom-qmp-cmds.c | 25 +++--
> storage-daemon/qemu-storage-daemon.c | 2 --
> 6 files changed, 32 insertions(+), 31 deletions(-)
>
> +++ b/qapi/qom.json
> @@ -839,13 +839,6 @@
> #
> # Create a QOM object.
> #
> -# @qom-type: the class name for the object to be created
> -#
> -# @id: the name of the new object
> -#
> -# Additional arguments depend on qom-type and are passed to the backend
> -# unchanged.
> -#
> # Returns: Nothing on success
> # Error if @qom-type is not a valid class name
> #
> @@ -859,9 +852,7 @@
> # <- { "return": {} }
> #
> ##
> -{ 'command': 'object-add',
> - 'data': {'qom-type': 'str', 'id': 'str'},
> - 'gen': false } # so we can get the additional arguments
> +{ 'command': 'object-add', 'data': 'ObjectOptions', 'boxed': true }
So much more concise ;) A grep for TYPE_USER_CREATABLE doesn't seem to
turn up any *_class_init() functions that your earlier patches in the
series missed, so I think you captured an accurate 1:1 mapping. There
is include/chardev/char.h with the comment about "TODO: eventually use
TYPE_USER_CREATABLE" which may point to the next item to be added to
ObjectOptions, but that's not for this series.
> +++ b/qom/qom-qmp-cmds.c
>
> -void qmp_object_add(QDict *qdict, QObject **ret_data, Error **errp)
> +void qmp_object_add(ObjectOptions *options, Error **errp)
> {
> -user_creatable_add_dict(qdict, false, errp);
> +Visitor *v;
> +QObject *qobj;
> +QDict *props;
> +Object *obj;
> +
> +v = qobject_output_visitor_new();
> +visit_type_ObjectOptions(v, NULL, , _abort);
> +visit_complete(v, );
> +visit_free(v);
This part is nice...
> +
> +props = qobject_to(QDict, qobj);
> +qdict_del(props, "qom-type");
> +qdict_del(props, "id");
...while this part makes it seem like we still have more cleanup to come
later. But hey, progress!
> +
> +v = qobject_input_visitor_new(QOBJECT(props));
> +obj = user_creatable_add_type(ObjectType_str(options->qom_type),
> + options->id, props, v, errp);
> +object_unref(obj);
> +visit_free(v);
> }
>
Once you address Paolo's comment, you can also add
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 18/31] qapi/qom: Add ObjectOptions for x-remote-object
On 2/24/21 7:52 AM, Kevin Wolf wrote:
> This adds a QAPI schema for the properties of the x-remote-object
> object.
>
> Signed-off-by: Kevin Wolf
> ---
> qapi/qom.json | 20 ++--
> 1 file changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/qapi/qom.json b/qapi/qom.json
> index f8ff322df0..6793342e81 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -641,6 +641,20 @@
> { 'struct': 'PrManagerHelperProperties',
>'data': { 'path': 'str' } }
>
> +##
> +# @RemoteObjectProperties:
> +#
> +# Properties for x-remote-object objects.
> +#
> +# @fd: file descriptor name previously passed via 'getfd' command
> +#
> +# @devid: the id of the device to be associated with the file descriptor
> +#
> +# Since: 6.0
> +##
> +{ 'struct': 'RemoteObjectProperties',
> + 'data': { 'fd': 'str', 'devid': 'str' } }
> +
Matches hw/remote/remote-obj.c:remote_object_class_init().
> ##
> # @RngProperties:
> #
> @@ -762,7 +776,8 @@
> 'tls-creds-anon',
> 'tls-creds-psk',
> 'tls-creds-x509',
> -'tls-cipher-suites'
> +'tls-cipher-suites',
> +'x-remote-object'
>] }
>
> ##
> @@ -815,7 +830,8 @@
>'tls-creds-anon': 'TlsCredsAnonProperties',
>'tls-creds-psk': 'TlsCredsPskProperties',
>'tls-creds-x509': 'TlsCredsX509Properties',
> - 'tls-cipher-suites': 'TlsCredsProperties'
> + 'tls-cipher-suites': 'TlsCredsProperties',
> + 'x-remote-object':'RemoteObjectProperties'
>} }
>
> ##
>
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 17/31] qapi/qom: Add ObjectOptions for input-*
On 2/24/21 7:52 AM, Kevin Wolf wrote:
> This adds a QAPI schema for the properties of the input-* objects.
>
> ui.json cannot be included in qom.json because the storage daemon can't
> use it, so move GrabToggleKeys to common.json.
>
> Signed-off-by: Kevin Wolf
> ---
> qapi/common.json | 12 ++
> qapi/qom.json| 58
> qapi/ui.json | 13 +--
> 3 files changed, 71 insertions(+), 12 deletions(-)
>
> +##
> +# @InputBarrierProperties:
> +#
> +# Properties for input-barrier objects.
> +#
> +# @name: the screen name as declared in the screens section of barrier.conf
> +#
> +# @server: hostname of the Barrier server (default: "localhost")
> +#
> +# @port: TCP port of the Barrier server (default: "24800")
I can understand this being a string (if non-numeric, it can be treated
as a well-known service name instead), but...
> +#
> +# @x-origin: x coordinate of the leftmost pixel on the guest screen
> +#(default: "0")
...why are these other fields a string instead of an integer? But you
are just doing faithful translation of what we already have.
Bummer - our naming for this member implies that it is experimental,
which is a misnomer (it is quite stable, when viewed in tandem with
y-origin). Not your fault. Would 'origin-x' and 'origin-y' be any
better as new aliases in a followup patch?
> +#
> +# @y-origin: y coordinate of he topmost pixel on the guest screen (default:
> "0")
"the", long line
> +#
> +# @width: the width of secondary screen in pixels (default: "1920")
> +#
> +# @height: the height of secondary screen in pixels (default: "1080")
> +#
> +# Since: 4.2
> +##
> +{ 'struct': 'InputBarrierProperties',
> + 'data': { 'name': 'str',
> +'*server': 'str',
> +'*port': 'str',
> +'*x-origin': 'str',
> +'*y-origin': 'str',
> +'*width': 'str',
> +'*height': 'str' } }
Matches ui/input-barrier.c:input_barrier_class_init().
> +
> +##
> +# @InputLinuxProperties:
> +#
> +# Properties for input-linux objects.
> +#
> +# @evdev: the path of the host evdev device to use
> +#
> +# @grab_all: if true, grab is toggled for all devices (e.g. both keyboard and
> +#mouse) instead of just one device (default: false)
We have inconsistent naming within this object (see grab-toggle); a good
followup would be an alias for 'grab-all'.
> +#
> +# @repeat: enables auto-repeat events (default: false)
> +#
> +# @grab-toggle: the key or key combination that toggles device grab
> +# (default: ctrl-ctrl)
> +#
> +# Since: 2.6
> +##
> +{ 'struct': 'InputLinuxProperties',
> + 'data': { 'evdev': 'str',
> +'*grab_all': 'bool',
> +'*repeat': 'bool',
> +'*grab-toggle': 'GrabToggleKeys' } }
matches ui/input-linux.c.
> +
> ##
> # @IothreadProperties:
> #
> @@ -689,6 +743,8 @@
> 'filter-redirector',
> 'filter-replay',
> 'filter-rewriter',
> +'input-barrier',
> +'input-linux',
> 'iothread',
> 'memory-backend-file',
> 'memory-backend-memfd',
> @@ -741,6 +797,8 @@
>'filter-redirector': 'FilterRedirectorProperties',
>'filter-replay': 'NetfilterProperties',
>'filter-rewriter':'FilterRewriterProperties',
> + 'input-barrier': 'InputBarrierProperties',
> + 'input-linux':'InputLinuxProperties',
>'iothread': 'IothreadProperties',
>'memory-backend-file':'MemoryBackendFileProperties',
>'memory-backend-memfd': 'MemoryBackendMemfdProperties',
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 16/31] qapi/qom: Add ObjectOptions for confidential-guest-support
On 2/24/21 7:52 AM, Kevin Wolf wrote:
> This adds a QAPI schema for the properties of the objects implementing
> the confidential-guest-support interface.
>
> pef-guest and s390x-pv-guest don't have any properties, so they only
> need to be added to the ObjectType enum without adding a new branch to
> ObjectOptions.
>
> Signed-off-by: Kevin Wolf
> ---
> qapi/qom.json | 37 +
> 1 file changed, 37 insertions(+)
>
> diff --git a/qapi/qom.json b/qapi/qom.json
> index e7184122e9..d5f68b5c89 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -633,6 +633,38 @@
>'base': 'RngProperties',
>'data': { '*filename': 'str' } }
>
> +##
> +# @SevGuestProperties:
> +#
> +# Properties for sev-guest objects.
> +#
> +# @sev-device: SEV device to use (default: "/dev/sev")
> +#
> +# @dh-cert-file: guest owners DH certificate (encoded with base64)
> +#
> +# @session-file: guest owners session parameters (encoded with base64)
Matches target/i386/sev.c:sev_guest_class_init()...
> +#
> +# @policy: SEV policy value (default: 0x1)
> +#
> +# @handle: SEV firmware handle (default: 0)
> +#
> +# @cbitpos: C-bit location in page table entry (default: 0)
> +#
> +# @reduced-phys-bits: number of bits in physical addresses that become
> +# unavailable when SEV is enabled
...and sev_guest_instance_init().
> +#
> +# Since: 2.12
> +##
> +{ 'struct': 'SevGuestProperties',
> + 'data': { '*sev-device': 'str',
> +'*dh-cert-file': 'str',
> +'*session-file': 'str',
> +'*policy': 'uint32',
> +'*handle': 'uint32',
> +'*cbitpos': 'uint32',
> +'reduced-phys-bits': 'uint32' },
> + 'if': 'defined(CONFIG_SEV)' }
> +
> ##
> # @ObjectType:
> #
> @@ -661,12 +693,15 @@
> 'memory-backend-file',
> 'memory-backend-memfd',
> 'memory-backend-ram',
> +{'name': 'pef-guest', 'if': 'defined(CONFIG_PSERIES)' },
> 'pr-manager-helper',
> 'rng-builtin',
> 'rng-egd',
> 'rng-random',
> 'secret',
> 'secret_keyring',
> +{'name': 'sev-guest', 'if': 'defined(CONFIG_SEV)' },
> +'s390-pv-guest',
> 'throttle-group',
> 'tls-creds-anon',
> 'tls-creds-psk',
> @@ -716,6 +751,8 @@
>'rng-random': 'RngRandomProperties',
>'secret': 'SecretProperties',
>'secret_keyring': 'SecretKeyringProperties',
> + 'sev-guest': { 'type': 'SevGuestProperties',
> + 'if': 'defined(CONFIG_SEV)' },
>'throttle-group': 'ThrottleGroupProperties',
>'tls-creds-anon': 'TlsCredsAnonProperties',
>'tls-creds-psk': 'TlsCredsPskProperties',
>
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 15/31] qapi/qom: Add ObjectOptions for pr-manager-helper
On 2/24/21 7:52 AM, Kevin Wolf wrote:
> This adds a QAPI schema for the properties of the pr-manager-helper
> object.
>
> Signed-off-by: Kevin Wolf
> ---
> qapi/qom.json | 14 ++
> 1 file changed, 14 insertions(+)
>
> diff --git a/qapi/qom.json b/qapi/qom.json
> index e3357f5123..e7184122e9 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -575,6 +575,18 @@
> '*hugetlbsize': 'size',
> '*seal': 'bool' } }
>
> +##
> +# @PrManagerHelperProperties:
> +#
> +# Properties for pr-manager-helper objects.
> +#
> +# @path: the path to a Unix domain socket for connecting to the external
> helper
> +#
> +# Since: 2.11
> +##
> +{ 'struct': 'PrManagerHelperProperties',
> + 'data': { 'path': 'str' } }
> +
Matches scsi/pr-manager-helper.c:pr_manager_helper_class_init().
> ##
> # @RngProperties:
> #
> @@ -649,6 +661,7 @@
> 'memory-backend-file',
> 'memory-backend-memfd',
> 'memory-backend-ram',
> +'pr-manager-helper',
> 'rng-builtin',
> 'rng-egd',
> 'rng-random',
> @@ -697,6 +710,7 @@
>'memory-backend-file':'MemoryBackendFileProperties',
>'memory-backend-memfd': 'MemoryBackendMemfdProperties',
>'memory-backend-ram': 'MemoryBackendProperties',
> + 'pr-manager-helper': 'PrManagerHelperProperties',
>'rng-builtin':'RngProperties',
> 'rng-egd': 'RngEgdProperties',
>'rng-random': 'RngRandomProperties',
>
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 14/31] qapi/qom: Add ObjectOptions for filter-*
; + 'data': { 'outdev': 'str',
> +'*vnet_hdr_support': 'bool' } }
Matches filter-mirror.c:filter_mirror_class_init(). For the future, can
we rename to vnet-hdr-support?
> +
> +##
> +# @FilterRedirectorProperties:
> +#
> +# Properties for filter-redirector objects.
> +#
> +# At least one of @indev or @outdev must be present. If both are present,
> they
> +# must not refer to the same character device backend.
> +#
> +# @indev: the name of a character device backend from which packets are
> +# received and redirected to the filtered network device
> +#
> +# @outdev: the name of a character device backend to which all incoming
> packets
> +# are redirected
> +#
> +# @vnet_hdr_support: if true, vnet header support is enabled (default: false)
> +#
> +# Since: 2.6
> +##
> +{ 'struct': 'FilterRedirectorProperties',
> + 'base': 'NetfilterProperties',
> + 'data': { '*indev': 'str',
> +'*outdev': 'str',
> +'*vnet_hdr_support': 'bool' } }
Matches net/filter-mirror.c:filter_redirector_class_init().
> +
> +##
> +# @FilterRewriterProperties:
> +#
> +# Properties for filter-rewriter objects.
> +#
> +# @vnet_hdr_support: if true, vnet header support is enabled (default: false)
> +#
> +# Since: 2.8
> +##
> +{ 'struct': 'FilterRewriterProperties',
> + 'base': 'NetfilterProperties',
> + 'data': { '*vnet_hdr_support': 'bool' } }
> +
Matches net/filter-rewriter.c:filter_rewriter_class_init().
> ##
> # @IothreadProperties:
> #
> @@ -508,6 +639,12 @@
> 'cryptodev-backend-builtin',
> 'cryptodev-vhost-user',
> 'dbus-vmstate',
> +'filter-buffer',
> +'filter-dump',
> +'filter-mirror',
> +'filter-redirector',
> +'filter-replay',
> +'filter-rewriter',
> 'iothread',
> 'memory-backend-file',
> 'memory-backend-memfd',
> @@ -550,6 +687,12 @@
>'cryptodev-backend-builtin': 'CryptodevBackendProperties',
>'cryptodev-vhost-user': 'CryptodevVhostUserProperties',
>'dbus-vmstate': 'DBusVMStateProperties',
> + 'filter-buffer': 'FilterBufferProperties',
> + 'filter-dump':'FilterDumpProperties',
> + 'filter-mirror': 'FilterMirrorProperties',
> + 'filter-redirector': 'FilterRedirectorProperties',
> + 'filter-replay': 'NetfilterProperties',
> + 'filter-rewriter':'FilterRewriterProperties',
>'iothread': 'IothreadProperties',
>'memory-backend-file':'MemoryBackendFileProperties',
>'memory-backend-memfd': 'MemoryBackendMemfdProperties',
>
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 13/31] qapi/qom: Add ObjectOptions for colo-compare
On 2/24/21 7:52 AM, Kevin Wolf wrote:
> This adds a QAPI schema for the properties of the colo-compare object.
>
> Signed-off-by: Kevin Wolf
> ---
> qapi/qom.json | 49 +
> 1 file changed, 49 insertions(+)
>
> diff --git a/qapi/qom.json b/qapi/qom.json
> index 4b1cd4b8dc..8e4414f843 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -222,6 +222,53 @@
>'data': { 'if': 'str',
> 'canbus': 'str' } }
>
> +##
> +# @ColoCompareProperties:
> +#
> +# Properties for colo-compare objects.
> +#
> +# @primary_in: name of the character device backend to use for the primary
> +# input (incoming packets are redirected to @outdev)
> +#
> +# @secondary_in: name of the character device backend to use for secondary
> +#input (incoming packets are only compared to the input on
> +#@primary_in and then dropped)
> +#
Idea for future improvement: use aliases to shift over to 'primary-in',
'secondary-in', and so on as our preferred name. But not for this
patch, which is a mechanical conversion of what exists.
> +# @outdev: name of the character device backend to use for output
> +#
> +# @iothread: name of the iothread to run in
> +#
> +# @notify_dev: name of the character device backend to be used to communicate
> +# with the remote colo-frame (only for Xen COLO)
> +#
> +# @compare_timeout: the maximum time to hold a packet from @primary_in for
> +# comparison with an incoming packet on @secondary_in in
> +# milliseconds (default: 3000)
> +#
> +# @expired_scan_cycle: the interval at which colo-compare checks whether
> +# packets from @primary have timed out, in milliseconds
> +# (default: 3000)
> +#
> +# @max_queue_size: the maximum number of packets to keep in the queue for
> +# comparing with incoming packets from @secondary_in. If
> the
> +# queue is full and addtional packets are received, the
> +# addtional packets are dropped. (default: 1024)
> +#
> +# @vnet_hdr_support: if true, vnet header support is enabled (default: false)
> +#
> +# Since: 2.8
> +##
> +{ 'struct': 'ColoCompareProperties',
> + 'data': { 'primary_in': 'str',
> +'secondary_in': 'str',
> +'outdev': 'str',
> +'iothread': 'str',
> +'*notify_dev': 'str',
> +'*compare_timeout': 'uint64',
> +'*expired_scan_cycle': 'uint32',
> +'*max_queue_size': 'uint32',
> +'*vnet_hdr_support': 'bool' } }
Matches net/colo-compare.c:colo_compare_init().
> +
> ##
> # @CryptodevBackendProperties:
> #
> @@ -456,6 +503,7 @@
> 'authz-simple',
> 'can-bus',
> 'can-host-socketcan',
> +'colo-compare',
> 'cryptodev-backend',
> 'cryptodev-backend-builtin',
> 'cryptodev-vhost-user',
> @@ -497,6 +545,7 @@
>'authz-pam': 'AuthZPAMProperties',
>'authz-simple': 'AuthZSimpleProperties',
>'can-host-socketcan': 'CanHostSocketcanProperties',
> + 'colo-compare': 'ColoCompareProperties',
> 'cryptodev-backend': 'CryptodevBackendProperties',
>'cryptodev-backend-builtin': 'CryptodevBackendProperties',
>'cryptodev-vhost-user': 'CryptodevVhostUserProperties',
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 12/31] qapi/qom: Add ObjectOptions for can-*
On 2/24/21 7:52 AM, Kevin Wolf wrote:
> This adds a QAPI schema for the properties of the can-* objects.
>
> can-bus doesn't have any properties, so it only needs to be added to the
> ObjectType enum without adding a new branch to ObjectOptions.
I somewhat prefer
'can-bus': {},
to make it explicit that we thought about it, but since we allow
defaulted union branches, your approach works too.
>
> Signed-off-by: Kevin Wolf
> ---
> qapi/qom.json | 18 ++
> 1 file changed, 18 insertions(+)
>
> diff --git a/qapi/qom.json b/qapi/qom.json
> index f22b7aa99b..4b1cd4b8dc 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -207,6 +207,21 @@
>'returns': [ 'ObjectPropertyInfo' ],
>'allow-preconfig': true }
>
> +##
> +# @CanHostSocketcanProperties:
> +#
> +# Properties for can-host-socketcan objects.
> +#
> +# @if: interface name of the host system CAN bus to connect to
> +#
> +# @canbus: object ID of the can-bus object to connect to the host interface
> +#
> +# Since: 2.12
> +##
> +{ 'struct': 'CanHostSocketcanProperties',
> + 'data': { 'if': 'str',
> +'canbus': 'str' } }
> +
Okay, matches net/can/can_socketcan.c:can_host_socketcan_class_init()
(after chasing down the parent class in
net/can/can_host.c:can_host_class_init() to find "canbus").
> ##
> # @CryptodevBackendProperties:
> #
> @@ -439,6 +454,8 @@
> 'authz-listfile',
> 'authz-pam',
> 'authz-simple',
> +'can-bus',
> +'can-host-socketcan',
> 'cryptodev-backend',
> 'cryptodev-backend-builtin',
> 'cryptodev-vhost-user',
> @@ -479,6 +496,7 @@
>'authz-listfile': 'AuthZListFileProperties',
>'authz-pam': 'AuthZPAMProperties',
>'authz-simple': 'AuthZSimpleProperties',
> + 'can-host-socketcan': 'CanHostSocketcanProperties',
>'cryptodev-backend': 'CryptodevBackendProperties',
> 'cryptodev-backend-builtin': 'CryptodevBackendProperties',
>'cryptodev-vhost-user': 'CryptodevVhostUserProperties',
>
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 11/31] qapi/qom: Add ObjectOptions for tls-*, deprecate 'loaded'
e using the
> +#credentials (default: true)
> +#
> +# @passwordid: For the server-key.pem and client-key.pem files which contain
> +# sensitive private keys, it is possible to use an encrypted
> +# version by providing the @passwordid parameter. This provides
> +# the ID of a previously created secret object containing the
> +# password for decryption.
> +#
> +# Features:
> +# @deprecated: Member @loaded is deprecated. Setting true doesn't make
> sense,
> +# and false is already the default.
> +#
> +# Since: 2.5
> +##
> +{ 'struct': 'TlsCredsX509Properties',
> + 'base': 'TlsCredsProperties',
> + 'data': { '*loaded': { 'type': 'bool', 'features': ['deprecated'] },
> +'*sanity-check': 'bool',
> +'*passwordid': 'str' } }
This matches crypto/tlscredsx509.c:qcrypto_tls_creds_x509_class_init().
> diff --git a/qapi/qom.json b/qapi/qom.json
> index 2668ad8369..f22b7aa99b 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -452,7 +452,11 @@
> 'rng-random',
> 'secret',
> 'secret_keyring',
> -'throttle-group'
> +'throttle-group',
> +'tls-creds-anon',
> +'tls-creds-psk',
> +'tls-creds-x509',
> +'tls-cipher-suites'
Matches crypto/tls-cipher-suites.c:qcrypto_tls_cipher_suites_class_init().
>] }
>
> ##
> @@ -488,7 +492,11 @@
>'rng-random': 'RngRandomProperties',
>'secret': 'SecretProperties',
> 'secret_keyring': 'SecretKeyringProperties',
> - 'throttle-group': 'ThrottleGroupProperties'
> + 'throttle-group': 'ThrottleGroupProperties',
> + 'tls-creds-anon': 'TlsCredsAnonProperties',
> + 'tls-creds-psk': 'TlsCredsPskProperties',
> + 'tls-creds-x509': 'TlsCredsX509Properties',
> + 'tls-cipher-suites': 'TlsCredsProperties'
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 10/31] qapi/qom: Add ObjectOptions for secret*, deprecate 'loaded'
> > +``loaded`` property of ``secret`` and ``secret_keyring`` objects (since > 6.0.0) > +'' > + > +The only effect of specifying ``loaded=on`` in the command line or QMP > +``object-add`` is that the secret is loaded immediately, possibly before all > +other options have been processed. This will either have no effect (if > +``loaded`` was the last option) or cause options to be effectively ignored as > +if they were not given. The property is therefore useless and should not be > +specified. May be impacted if we rename to secret-keyring (in fact, if we rename, the new name wouldn't even need the deprecated field), but that may be trickier to coordinate. So with regards to just the mechanical conversion, Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 09/31] qapi/qom: Add ObjectOptions for throttle-group
On 2/24/21 7:52 AM, Kevin Wolf wrote:
> This adds a QAPI schema for the properties of the throttle-group object.
>
> The only purpose of the x-* properties is to make the nested options in
> 'limits' available for a command line parser that doesn't support
> structs. Any parser that will use the QAPI schema will supports structs,
> though, so they will not be needed in the schema in the future.
>
> To keep the conversion straightforward, add them to the schema anyway.
> We can then remove the options and adjust documentation, test cases etc.
> in a separate patch.
>
> Signed-off-by: Kevin Wolf
> ---
> qapi/block-core.json | 27 +++
> qapi/qom.json| 7 +--
> 2 files changed, 32 insertions(+), 2 deletions(-)
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 9f555d5c1d..a67fa0cc59 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -2504,6 +2504,33 @@
> '*bps-write-max' : 'int', '*bps-write-max-length' : 'int',
> '*iops-size' : 'int' } }
>
> +##
> +# @ThrottleGroupProperties:
> +#
> +# Properties for throttle-group objects.
Corresponds to block/throttle-groups.c:throttle_group_obj_class_init()
with its ThrottleParamInfo struct for the x- fields, and limits as-is.
> +#
> +# The options starting with x- are aliases for the same key without x- in
> +# the @limits object. As indicated by the x- prefix, this is not a stable
> +# interface and may be removed or changed incompatibly in the future. Use
> +# @limits for a supported stable interface.
> +#
> +# @limits: limits to apply for this throttle group
And I did check that qapi/block-core.json:ThrottleLimits has the same
fields as the ThrottleParamInfo x- fields. All this duplication! But
we're getting to a state where it will be easier to clean up the cruft.
> +#
> +# Since: 2.11
> +##
> +{ 'struct': 'ThrottleGroupProperties',
> + 'data': { '*limits': 'ThrottleLimits',
> +'*x-iops-total' : 'int', '*x-iops-total-max' : 'int',
> +'*x-iops-total-max-length' : 'int', '*x-iops-read' : 'int',
> +'*x-iops-read-max' : 'int', '*x-iops-read-max-length' : 'int',
> +'*x-iops-write' : 'int', '*x-iops-write-max' : 'int',
> +'*x-iops-write-max-length' : 'int', '*x-bps-total' : 'int',
> +'*x-bps-total-max' : 'int', '*x-bps-total-max-length' : 'int',
> +'*x-bps-read' : 'int', '*x-bps-read-max' : 'int',
> +'*x-bps-read-max-length' : 'int', '*x-bps-write' : 'int',
> +'*x-bps-write-max' : 'int', '*x-bps-write-max-length' : 'int',
> +'*x-iops-size' : 'int' } }
> +
> ##
> # @block-stream:
> #
> diff --git a/qapi/qom.json b/qapi/qom.json
> index 73f28f9608..449dca8ec5 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -5,6 +5,7 @@
> # See the COPYING file in the top-level directory.
>
> { 'include': 'authz.json' }
> +{ 'include': 'block-core.json' }
> { 'include': 'common.json' }
>
> ##
> @@ -447,7 +448,8 @@
> 'memory-backend-ram',
> 'rng-builtin',
> 'rng-egd',
> -'rng-random'
> +'rng-random',
> +'throttle-group'
>] }
>
> ##
> @@ -480,7 +482,8 @@
>'memory-backend-ram': 'MemoryBackendProperties',
>'rng-builtin':'RngProperties',
>'rng-egd':'RngEgdProperties',
> - 'rng-random': 'RngRandomProperties'
> + 'rng-random': 'RngRandomProperties',
> + 'throttle-group': 'ThrottleGroupProperties'
>} }
>
> ##
>
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 08/31] qapi/qom: Add ObjectOptions for rng-*, deprecate 'opened'
On 2/24/21 7:52 AM, Kevin Wolf wrote:
> This adds a QAPI schema for the properties of the rng-* objects.
>
> The 'opened' property doesn't seem to make sense as an external
> interface: It is automatically set to true in ucc->complete, and
> explicitly setting it to true earlier just means that trying to set
> additional options will result in an error. After the property has once
> been set to true (i.e. when the object construction has completed), it
> can never be reset to false. In other words, the 'opened' property is
> useless. Mark it as deprecated in the schema from the start.
>
> Signed-off-by: Kevin Wolf
> ---
> qapi/qom.json | 56 --
> docs/system/deprecated.rst | 9 ++
> 2 files changed, 63 insertions(+), 2 deletions(-)
>
> diff --git a/qapi/qom.json b/qapi/qom.json
> index 1a869006a1..73f28f9608 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -380,6 +380,52 @@
> '*hugetlbsize': 'size',
> '*seal': 'bool' } }
>
> +##
> +# @RngProperties:
> +#
> +# Properties for objects of classes derived from rng.
> +#
> +# @opened: if true, the device is opened immediately when applying this
> option
> +# and will probably fail when processing the next option. Don't use;
> +# only provided for compatibility. (default: false)
> +#
> +# Features:
> +# @deprecated: Member @opened is deprecated. Setting true doesn't make
> sense,
> +# and false is already the default.
> +#
> +# Since: 1.3
> +##
> +{ 'struct': 'RngProperties',
> + 'data': { '*opened': { 'type': 'bool', 'features': ['deprecated'] } } }
Matches backends/rng.c:rng_backend_class_init(), and I concur with the
deprecation.
> +
> +##
> +# @RngEgdProperties:
> +#
> +# Properties for rng-egd objects.
> +#
> +# @chardev: the name of a character device backend that provides the
> connection
> +# to the RNG daemon
> +#
> +# Since: 1.3
> +##
> +{ 'struct': 'RngEgdProperties',
> + 'base': 'RngProperties',
> + 'data': { 'chardev': 'str' } }
Matches backends/rng-egd.c:rng_egd_class_init().
> +
> +##
> +# @RngRandomProperties:
> +#
> +# Properties for rng-random objects.
> +#
> +# @filename: the filename of the device on the host to obtain entropy from
> +#(default: "/dev/urandom")
> +#
> +# Since: 1.3
> +##
> +{ 'struct': 'RngRandomProperties',
> + 'base': 'RngProperties',
> + 'data': { '*filename': 'str' } }
Matches backends/rng-random.c:rng_random_class_init().
> +
> ##
> # @ObjectType:
> #
> @@ -398,7 +444,10 @@
> 'iothread',
> 'memory-backend-file',
> 'memory-backend-memfd',
> -'memory-backend-ram'
> +'memory-backend-ram',
> +'rng-builtin',
> +'rng-egd',
> +'rng-random'
>] }
>
> ##
> @@ -428,7 +477,10 @@
>'iothread': 'IothreadProperties',
>'memory-backend-file':'MemoryBackendFileProperties',
>'memory-backend-memfd': 'MemoryBackendMemfdProperties',
> - 'memory-backend-ram': 'MemoryBackendProperties'
> + 'memory-backend-ram': 'MemoryBackendProperties',
> + 'rng-builtin':'RngProperties',
> + 'rng-egd':'RngEgdProperties',
> + 'rng-random': 'RngRandomProperties'
>} }
>
> ##
> diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
> index 00b694e053..79991c2893 100644
> --- a/docs/system/deprecated.rst
> +++ b/docs/system/deprecated.rst
> @@ -146,6 +146,15 @@ library enabled as a cryptography provider.
> Neither the ``nettle`` library, or the built-in cryptography provider are
> supported on FIPS enabled hosts.
>
> +``opened`` property of ``rng-*`` objects (since 6.0.0)
> +''
> +
> +The only effect of specifying ``opened=on`` in the command line or QMP
> +``object-add`` is that the device is opened immediately, possibly before all
> +other options have been processed. This will either have no effect (if
> +``opened`` was the last option) or cause errors. The property is therefore
> +useless and should not be specified.
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 07/31] qapi/qom: Add ObjectOptions for memory-backend-*
ata': { '*align': 'size',
> +'*discard-data': 'bool',
> +'mem-path': 'str',
> +'*pmem': 'bool',
To match the C code, this should be
'*pmem': { 'type':'bool', 'if':'defined(CONFIG_LIBPMEM)' },
> +'*readonly': 'bool' } }
> +
> +##
> +# @MemoryBackendMemfdProperties:
> +#
> +# Properties for memory-backend-memfd objects.
> +#
> +# The @share boolean option is true by default with memfd.
> +#
> +# @hugetlb: if true, the file to be created resides in the hugetlbfs
> filesystem
> +# (default: false)
> +#
> +# @hugetlbsize: the hugetlb page size on systems that support multiple
> hugetlb
> +# page sizes (it must be a power of 2 value supported by the
> +# system). 0 selects a default page size. This option is
> ignored
> +# if @hugetlb is false. (default: 0)
> +#
> +# @seal: if true, create a sealed-file, which will block further resizing of
> +#the memory (default: true)
> +#
> +# Since: 2.12
> +##
> +{ 'struct': 'MemoryBackendMemfdProperties',
> + 'base': 'MemoryBackendProperties',
> + 'data': { '*hugetlb': 'bool',
> +'*hugetlbsize': 'size',
> +'*seal': 'bool' } }
backends/hostmem-memfd.c makes 'hugetlb' and 'hugetlbsize' conditional
on qemu_memfd_check(MFD_HUGETLB), and only registers the overal type
based on qemu_memfd_check(MFD_ALLOW_SEALING). In turn, qemu_memfd_check
returns false except for CONFIG_LINUX,...
> +
> ##
> # @ObjectType:
> #
> @@ -287,7 +395,10 @@
> 'cryptodev-backend-builtin',
> 'cryptodev-vhost-user',
> 'dbus-vmstate',
> -'iothread'
> +'iothread',
> +'memory-backend-file',
> +'memory-backend-memfd',
> +'memory-backend-ram'
>] }
>
> ##
> @@ -314,7 +425,10 @@
>'cryptodev-backend-builtin': 'CryptodevBackendProperties',
>'cryptodev-vhost-user': 'CryptodevVhostUserProperties',
>'dbus-vmstate': 'DBusVMStateProperties',
> - 'iothread': 'IothreadProperties'
> + 'iothread': 'IothreadProperties',
> + 'memory-backend-file':'MemoryBackendFileProperties',
> + 'memory-backend-memfd': 'MemoryBackendMemfdProperties',
...so I'm wondering if this branch should be:
'memory-backend-memfd', { 'type':'MemoryBackendMemfdProperties',
'if': 'defined(CONFIG_LINUX)' },
and whether we are risking problems by always having the 'hugetlb*'
fields even when the runtime does not register them.
> + 'memory-backend-ram': 'MemoryBackendProperties'
>} }
>
> ##
>
Because of my questions on conditional compilation, I'm not comfortable
with R-b yet.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 06/31] qapi/qom: Add ObjectOptions for dbus-vmstate
On 2/24/21 7:52 AM, Kevin Wolf wrote:
> This adds a QAPI schema for the properties of the dbus-vmstate object.
>
> A list represented as a comma separated string is clearly not very
> QAPI-like, but for now just describe the existing interface.
Does your alias proposal give us a path forward for improving that down
the road? Or maybe it's not an alias we need, but a new field with
better QAPI-like semantics, deprecate the old one, and wait out the 2
release cycles?
>
> Signed-off-by: Kevin Wolf
> ---
> qapi/qom.json | 18 ++
> 1 file changed, 18 insertions(+)
>
> diff --git a/qapi/qom.json b/qapi/qom.json
> index 1dbc95fb53..a6a5049707 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -232,6 +232,22 @@
>'base': 'CryptodevBackendProperties',
>'data': { 'chardev': 'str' } }
>
> +##
> +# @DBusVMStateProperties:
> +#
> +# Properties for dbus-vmstate objects.
> +#
> +# @addr: the name of the DBus bus to connect to
> +#
> +# @id-list: a comma separated list of DBus IDs of helpers whose data should
> be
> +# included in the VM state on migration
> +#
> +# Since: 5.0
> +##
> +{ 'struct': 'DBusVMStateProperties',
> + 'data': { 'addr': 'str' ,
> +'*id-list': 'str' } }
Matches backends/dbus-vmstate.c:dbus_vmstate_class_init(), including
splitting id-list into a GHashTable with get_id_list_set().
Since there is benefit to documenting/converting our existing API in
this series without dragging it out by also trying to fix the warts,
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 05/31] qapi/qom: Add ObjectOptions for cryptodev-*
On 2/24/21 7:52 AM, Kevin Wolf wrote:
> This adds a QAPI schema for the properties of the cryptodev-* objects.
>
> These interfaces have some questionable aspects (cryptodev-backend is
> really an abstract base class without function, and the queues option
> only makes sense for cryptodev-vhost-user), but as the goal is to
> represent the existing interface in QAPI, leave these things in place.
>
> Signed-off-by: Kevin Wolf
> ---
> qapi/qom.json | 34 ++
> 1 file changed, 34 insertions(+)
>
> diff --git a/qapi/qom.json b/qapi/qom.json
> index 30ed179bc1..1dbc95fb53 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -204,6 +204,34 @@
>'returns': [ 'ObjectPropertyInfo' ],
>'allow-preconfig': true }
>
> +##
> +# @CryptodevBackendProperties:
> +#
> +# Properties for cryptodev-backend and cryptodev-backend-builtin objects.
> +#
> +# @queues: the number of queues for the cryptodev backend. Ignored for
> +# cryptodev-backend and must be 1 for cryptodev-backend-builtin.
> +# (default: 1)
> +#
> +# Since: 2.8
> +##
> +{ 'struct': 'CryptodevBackendProperties',
> + 'data': { '*queues': 'uint32' } }
Matches backend/cryptodev.c:cryptodev_backend_class_init() and
backend/cryptodev-builtin.c:cryptodev_builtin_class_init().
> +
> +##
> +# @CryptodevVhostUserProperties:
> +#
> +# Properties for cryptodev-vhost-user objects.
> +#
> +# @chardev: the name of a unix domain socket character device that connects
> to
Should that b s/unix/Unix/ ?
> +# the vhost-user server
> +#
> +# Since: 2.12
> +##
> +{ 'struct': 'CryptodevVhostUserProperties',
> + 'base': 'CryptodevBackendProperties',
> + 'data': { 'chardev': 'str' } }
Matches backend/cryptodev-vhost-user.c:cryptodev_vhost_user_init_class().
> +
> ##
> # @IothreadProperties:
> #
> @@ -239,6 +267,9 @@
> 'authz-listfile',
> 'authz-pam',
> 'authz-simple',
> +'cryptodev-backend',
> +'cryptodev-backend-builtin',
> +'cryptodev-vhost-user',
> 'iothread'
>] }
>
> @@ -262,6 +293,9 @@
>'authz-listfile': 'AuthZListFileProperties',
>'authz-pam': 'AuthZPAMProperties',
>'authz-simple': 'AuthZSimpleProperties',
> + 'cryptodev-backend': 'CryptodevBackendProperties',
> + 'cryptodev-backend-builtin': 'CryptodevBackendProperties',
> + 'cryptodev-vhost-user': 'CryptodevVhostUserProperties',
>'iothread': 'IothreadProperties'
>} }
>
>
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 04/31] qapi/qom: Add ObjectOptions for authz-*
On 2/24/21 7:52 AM, Kevin Wolf wrote:
> This adds a QAPI schema for the properties of the authz-* objects.
>
> Signed-off-by: Kevin Wolf
> ---
> qapi/authz.json | 62
> qapi/qom.json| 10 +
> storage-daemon/qapi/qapi-schema.json | 1 +
> 3 files changed, 73 insertions(+)
>
> diff --git a/qapi/authz.json b/qapi/authz.json
> index 42afe752d1..99d49aa563 100644
> --- a/qapi/authz.json
> +++ b/qapi/authz.json
> @@ -59,3 +59,65 @@
> ##
> { 'struct': 'QAuthZListRuleListHack',
>'data': { 'unused': ['QAuthZListRule'] } }
This hack is no longer necessary...
> +
> +##
> +# @AuthZListProperties:
> +#
> +# Properties for authz-list objects.
> +#
> +# @policy: Default policy to apply when no rule matches (default: deny)
> +#
> +# @rules: Authorization rules based on matching user
> +#
> +# Since: 4.0
> +##
> +{ 'struct': 'AuthZListProperties',
> + 'data': { '*policy': 'QAuthZListPolicy',
> +'*rules': ['QAuthZListRule'] } }
...now that we have a real type using the same array and forcing the
QAPI generator to instantiate it.
Matches authz/list.c:qauthz_list_class_init().
> +
> +##
> +# @AuthZListFileProperties:
> +#
> +# Properties for authz-listfile objects.
> +#
> +# @filename: File name to load the configuration from. The file must
> +#contain valid JSON for AuthZListProperties.
> +#
> +# @refresh: If true, inotify is used to monitor the file, automatically
> +# reloading changes. If an error occurs during reloading, all
> +# authorizations will fail until the file is next successfully
> +# loaded. (default: true if the binary was built with
> +# CONFIG_INOTIFY1, false otherwise)
> +#
> +# Since: 4.0
> +##
> +{ 'struct': 'AuthZListFileProperties',
> + 'data': { 'filename': 'str',
> +'*refresh': 'bool' } }
Matches authz/listfile.c:qauthz_list_file_class_init().
> +
> +##
> +# @AuthZPAMProperties:
> +#
> +# Properties for authz-pam objects.
> +#
> +# @service: PAM service name to use for authorization
> +#
> +# Since: 4.0
> +##
> +{ 'struct': 'AuthZPAMProperties',
> + 'data': { 'service': 'str' } }
Matches authz/pamacct.c:qauthz_pam_class_init().
> +
> +##
> +# @AuthZSimpleProperties:
> +#
> +# Properties for authz-simple objects.
> +#
> +# @identity: Identifies the allowed user. Its format depends on the network
> +#service that authorization object is associated with. For
> +#authorizing based on TLS x509 certificates, the identity must be
> +#the x509 distinguished name.
> +#
> +# Since: 4.0
> +##
> +{ 'struct': 'AuthZSimpleProperties',
> + 'data': { 'identity': 'str' } }
Matches authz/simple.c:qauthz_simple_class_init().
> diff --git a/qapi/qom.json b/qapi/qom.json
> index bf2ecb34be..30ed179bc1 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -4,6 +4,8 @@
> # This work is licensed under the terms of the GNU GPL, version 2 or later.
> # See the COPYING file in the top-level directory.
>
> +{ 'include': 'authz.json' }
> +
> ##
> # = QEMU Object Model (QOM)
> ##
> @@ -233,6 +235,10 @@
> ##
> { 'enum': 'ObjectType',
>'data': [
> +'authz-list',
> +'authz-listfile',
> +'authz-pam',
> +'authz-simple',
> 'iothread'
>] }
>
> @@ -252,6 +258,10 @@
> 'id': 'str' },
>'discriminator': 'qom-type',
>'data': {
> + 'authz-list': 'AuthZListProperties',
> + 'authz-listfile': 'AuthZListFileProperties',
> + 'authz-pam': 'AuthZPAMProperties',
> + 'authz-simple': 'AuthZSimpleProperties',
>'iothread': 'IothreadProperties'
>} }
>
> diff --git a/storage-daemon/qapi/qapi-schema.json
> b/storage-daemon/qapi/qapi-schema.json
> index 28117c3aac..67749d1101 100644
> --- a/storage-daemon/qapi/qapi-schema.json
> +++ b/storage-daemon/qapi/qapi-schema.json
> @@ -26,6 +26,7 @@
> { 'include': '../../qapi/crypto.json' }
> { 'include': '../../qapi/introspect.json' }
> { 'include': '../../qapi/job.json' }
> +{ 'include': '../../qapi/authz.json' }
> { 'include': '../../qapi/qom.json' }
> { 'include': '../../qapi/sockets.json' }
> { 'include': '../../qapi/transaction.json' }
>
Once you delete the dead QAPI hack,
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 03/31] qapi/qom: Add ObjectOptions for iothread
On 2/24/21 7:52 AM, Kevin Wolf wrote:
> Add an ObjectOptions union that will eventually describe the options of
> all user creatable object types. As unions can't exist without any
> branches, also add the first object type.
>
> This adds a QAPI schema for the properties of the iothread object.
>
> Signed-off-by: Kevin Wolf
> ---
> qapi/qom.json | 53 +++
> 1 file changed, 53 insertions(+)
>
> diff --git a/qapi/qom.json b/qapi/qom.json
> index 96c91c1faf..bf2ecb34be 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -202,6 +202,59 @@
>'returns': [ 'ObjectPropertyInfo' ],
>'allow-preconfig': true }
>
> +##
> +# @IothreadProperties:
> +#
> +# Properties for iothread objects.
> +#
> +# @poll-max-ns: the maximum number of nanoseconds to busy wait for events.
> +# 0 means polling is disabled (default: 32768 on POSIX hosts,
> +# 0 otherwise)
> +#
> +# @poll-grow: the multiplier used to increase the polling time when the
> +# algorithm detects it is missing events due to not polling long
> +# enough. 0 selects a default behaviour (default: 0)
> +#
> +# @poll-shrink: the divisor used to decrease the polling time when the
> +# algorithm detects it is spending too long polling without
> +# encountering events. 0 selects a default behaviour (default:
> 0)
Matches PollParamInfo declarations in iothread.c.
> +#
> +# Since: 2.0
How did you determine this value? (I'm not questioning it being
something other than 6.0, because we have indeed supported QMP
configuration of these values via the untyped magic previously present
in add-object).
> +##
> +{ 'struct': 'IothreadProperties',
> + 'data': { '*poll-max-ns': 'int',
> +'*poll-grow': 'int',
> +'*poll-shrink': 'int' } }
These are correctly typed per the code in iothread.c, but it does raise
the question of whether a signed 64-bit value is the best choice, or if
we might later want to revisit things to pick more constrained types. I
don't think such an audit should hold up this series, though.
> +
> +##
> +# @ObjectType:
> +#
> +# Since: 6.0
> +##
> +{ 'enum': 'ObjectType',
> + 'data': [
> +'iothread'
> + ] }
Will be fun to watch this grow over the series.
> +
> +##
> +# @ObjectOptions:
> +#
> +# Describes the options of a user creatable QOM object.
> +#
> +# @qom-type: the class name for the object to be created
> +#
> +# @id: the name of the new object
> +#
> +# Since: 6.0
> +##
> +{ 'union': 'ObjectOptions',
> + 'base': { 'qom-type': 'ObjectType',
> + 'id': 'str' },
> + 'discriminator': 'qom-type',
> + 'data': {
> + 'iothread': 'IothreadProperties'
> + } }
> +
> ##
> # @object-add:
> #
>
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 02/31] qapi/qom: Drop deprecated 'props' from object-add
On 2/24/21 7:52 AM, Kevin Wolf wrote: > The option has been deprecated in QEMU 5.0, remove it. > > Signed-off-by: Kevin Wolf > --- > qapi/qom.json| 6 +- > docs/system/deprecated.rst | 5 - > docs/system/removed-features.rst | 5 + > qom/qom-qmp-cmds.c | 21 - > 4 files changed, 6 insertions(+), 31 deletions(-) > Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 01/31] tests: Drop 'props' from object-add calls
On 2/24/21 7:52 AM, Kevin Wolf wrote: > The 'props' option has been deprecated in 5.0 in favour of a flattened > object-add command. Time to change our test cases to drop the deprecated > option. > > Signed-off-by: Kevin Wolf > --- Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 4/4] utils: Deprecate inexact fractional suffix sizes
On 2/23/21 11:20 AM, Daniel P. Berrangé wrote: > On Thu, Feb 11, 2021 at 02:44:38PM -0600, Eric Blake wrote: >> The value '1.1k' is inexact; 1126.4 bytes is not possible, so we >> happen to truncate it to 1126. Our use of fractional sizes is >> intended for convenience, but when a user specifies a fraction that is >> not a clean translation to binary, truncating/rounding behind their >> backs can cause confusion. Better is to deprecate inexact values, >> which still leaves '1.5k' as valid, but alerts the user to spell out >> their values as a precise byte number in cases where they are >> currently being rounded. >> >> Note that values like '0.1G' in the testsuite need adjustment as a >> result. >> >> Since qemu_strtosz() does not have an Err** parameter, and plumbing >> that in would be a much larger task, we instead go with just directly >> emitting the deprecation warning to stderr. >> >> Signed-off-by: Eric Blake >> >> --- >> >> I'm not a fan of this patch, but am proposing it for discussion purposes. > > Likewise. I'm *not* in favour of this patch. Glad we're in agreement. Consider this one dropped, and I will queue 1-3 through my NBD tree. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
[PATCH v2 4/4] utils: Deprecate inexact fractional suffix sizes
The value '1.1k' is inexact; 1126.4 bytes is not possible, so we
happen to truncate it to 1126. Our use of fractional sizes is
intended for convenience, but when a user specifies a fraction that is
not a clean translation to binary, truncating/rounding behind their
backs can cause confusion. Better is to deprecate inexact values,
which still leaves '1.5k' as valid, but alerts the user to spell out
their values as a precise byte number in cases where they are
currently being rounded.
Note that values like '0.1G' in the testsuite need adjustment as a
result.
Since qemu_strtosz() does not have an Err** parameter, and plumbing
that in would be a much larger task, we instead go with just directly
emitting the deprecation warning to stderr.
Signed-off-by: Eric Blake
---
I'm not a fan of this patch, but am proposing it for discussion purposes.
---
docs/system/deprecated.rst | 9 +
tests/test-cutils.c| 6 +++---
tests/test-keyval.c| 4 ++--
tests/test-qemu-opts.c | 4 ++--
util/cutils.c | 9 +++--
5 files changed, 23 insertions(+), 9 deletions(-)
diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
index 113c2e933f1b..2c9cb849eec5 100644
--- a/docs/system/deprecated.rst
+++ b/docs/system/deprecated.rst
@@ -154,6 +154,15 @@ Input parameters that take a size value should only use a
size suffix
the value is hexadecimal. That is, '0x20M' is deprecated, and should
be written either as '32M' or as '0x200'.
+inexact sizes via scaled fractions (since 6.0)
+''
+
+Input parameters that take a size value should only use a fractional
+size (such as '1.5M') that will result in an exact byte value. The
+use of inexact values (such as '1.1M') that require truncation or
+rounding is deprecated, and you should instead consider writing your
+unusual size in bytes (here, '1153433' or '1153434' as desired).
+
QEMU Machine Protocol (QMP) commands
diff --git a/tests/test-cutils.c b/tests/test-cutils.c
index bad3a6099389..c6c33866277b 100644
--- a/tests/test-cutils.c
+++ b/tests/test-cutils.c
@@ -2124,11 +2124,11 @@ static void test_qemu_strtosz_float(void)
g_assert_cmpint(res, ==, 1024);
g_assert(endptr == str + 3);
-/* For convenience, we permit values that are not byte-exact */
-str = "12.345M";
+/* Fractional values should still be byte-exact */
+str = "12.125M";
err = qemu_strtosz(str, , );
g_assert_cmpint(err, ==, 0);
-g_assert_cmpint(res, ==, (uint64_t) (12.345 * MiB));
+g_assert_cmpint(res, ==, (uint64_t) (12.125 * MiB));
g_assert(endptr == str + 7);
}
diff --git a/tests/test-keyval.c b/tests/test-keyval.c
index e20c07cf3ea5..7a45c22942e6 100644
--- a/tests/test-keyval.c
+++ b/tests/test-keyval.c
@@ -525,7 +525,7 @@ static void test_keyval_visit_size(void)
visit_free(v);
/* Suffixes */
-qdict = keyval_parse("sz1=8b,sz2=1.5k,sz3=2M,sz4=0.1G,sz5=16777215T",
+qdict = keyval_parse("sz1=8b,sz2=1.5k,sz3=2M,sz4=0.125G,sz5=16777215T",
NULL, NULL, _abort);
v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
qobject_unref(qdict);
@@ -537,7 +537,7 @@ static void test_keyval_visit_size(void)
visit_type_size(v, "sz3", , _abort);
g_assert_cmphex(sz, ==, 2 * MiB);
visit_type_size(v, "sz4", , _abort);
-g_assert_cmphex(sz, ==, GiB / 10);
+g_assert_cmphex(sz, ==, GiB / 8);
visit_type_size(v, "sz5", , _abort);
g_assert_cmphex(sz, ==, 16777215ULL * TiB);
visit_check_struct(v, _abort);
diff --git a/tests/test-qemu-opts.c b/tests/test-qemu-opts.c
index 6568e31a7229..549e994938fe 100644
--- a/tests/test-qemu-opts.c
+++ b/tests/test-qemu-opts.c
@@ -720,10 +720,10 @@ static void test_opts_parse_size(void)
g_assert_cmphex(qemu_opt_get_size(opts, "size1", 0), ==, 8);
g_assert_cmphex(qemu_opt_get_size(opts, "size2", 0), ==, 1536);
g_assert_cmphex(qemu_opt_get_size(opts, "size3", 0), ==, 2 * MiB);
-opts = qemu_opts_parse(_list_02, "size1=0.1G,size2=16777215T",
+opts = qemu_opts_parse(_list_02, "size1=0.125G,size2=16777215T",
false, _abort);
g_assert_cmpuint(opts_count(opts), ==, 2);
-g_assert_cmphex(qemu_opt_get_size(opts, "size1", 0), ==, GiB / 10);
+g_assert_cmphex(qemu_opt_get_size(opts, "size1", 0), ==, GiB / 8);
g_assert_cmphex(qemu_opt_get_size(opts, "size2", 0), ==, 16777215ULL *
TiB);
/* Beyond limit with suffix */
diff --git a/util/cutils.c b/util/cutils.c
index 6a8a175e0d71..1154b9de131a 100644
--- a/util/cutils.c
+++ b/util/cutils.c
@@ -246,12 +246,13 @@ static int64_t suffix_mul(char suffix, int64_t unit)
* The size parsing supports the following syntaxes
* - 12345 - decimal, scale determined by @default_
[PATCH v2 3/4] utils: Deprecate hex-with-suffix sizes
Supporting '0x20M' looks odd, particularly since we have a 'B' suffix
that is ambiguous for bytes, as well as a less-frequently-used 'E'
suffix for extremely large exibytes. In practice, people using hex
inputs are specifying values in bytes (and would have written
0x200, or possibly relied on default_suffix in the case of
qemu_strtosz_MiB), and the use of scaling suffixes makes the most
sense for inputs in decimal (where the user would write 32M). But
rather than outright dropping support for hex-with-suffix, let's
follow our deprecation policy. Sadly, since qemu_strtosz() does not
have an Err** parameter, and plumbing that in would be a much larger
task, we instead go with just directly emitting the deprecation
warning to stderr.
Signed-off-by: Eric Blake
---
docs/system/deprecated.rst | 8
util/cutils.c | 10 +-
2 files changed, 17 insertions(+), 1 deletion(-)
diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
index 2fcac7861e07..113c2e933f1b 100644
--- a/docs/system/deprecated.rst
+++ b/docs/system/deprecated.rst
@@ -146,6 +146,14 @@ library enabled as a cryptography provider.
Neither the ``nettle`` library, or the built-in cryptography provider are
supported on FIPS enabled hosts.
+hexadecimal sizes with scaling multipliers (since 6.0)
+''
+
+Input parameters that take a size value should only use a size suffix
+(such as 'k' or 'M') when the base is written in decimal, and not when
+the value is hexadecimal. That is, '0x20M' is deprecated, and should
+be written either as '32M' or as '0x200'.
+
QEMU Machine Protocol (QMP) commands
diff --git a/util/cutils.c b/util/cutils.c
index 22d27b0fd21b..6a8a175e0d71 100644
--- a/util/cutils.c
+++ b/util/cutils.c
@@ -250,6 +250,9 @@ static int64_t suffix_mul(char suffix, int64_t unit)
* fractional portion is truncated to byte
* - 0x7fEE - hexadecimal, unit determined by @default_suffix
*
+ * The following cause a deprecation warning, and may be removed in the future
+ * - 0xabc{kKmMgGtTpP} - hex with scaling suffix
+ *
* The following are intentionally not supported
* - octal, such as 08
* - fractional hex, such as 0x1.8
@@ -272,7 +275,7 @@ static int do_strtosz(const char *nptr, const char **end,
int retval;
const char *endptr, *f;
unsigned char c;
-bool mul_required = false;
+bool mul_required = false, hex = false;
uint64_t val;
int64_t mul;
double fraction = 0.0;
@@ -298,6 +301,7 @@ static int do_strtosz(const char *nptr, const char **end,
retval = -EINVAL;
goto out;
}
+hex = true;
} else if (*endptr == '.') {
/*
* Input looks like a fraction. Make sure even 1.k works
@@ -320,6 +324,10 @@ static int do_strtosz(const char *nptr, const char **end,
c = *endptr;
mul = suffix_mul(c, unit);
if (mul > 0) {
+if (hex) {
+warn_report("Using a multiplier suffix on hex numbers "
+"is deprecated: %s", nptr);
+}
endptr++;
} else {
mul = suffix_mul(default_suffix, unit);
--
2.30.1
Re: [PATCH 2/3] utils: Deprecate hex-with-suffix sizes
On 2/5/21 5:13 AM, Daniel P. Berrangé wrote:
> On Thu, Feb 04, 2021 at 01:07:07PM -0600, Eric Blake wrote:
>> Supporting '0x20M' looks odd, particularly since we have an 'E' suffix
>> that is ambiguous between a hex digit and the extremely large exibyte
>> suffix, as well as a 'B' suffix for bytes. In practice, people using
>> hex inputs are specifying values in bytes (and would have written
>> 0x200, or possibly relied on default_suffix in the case of
>> qemu_strtosz_MiB), and the use of scaling suffixes makes the most
>> sense for inputs in decimal (where the user would write 32M). But
>> rather than outright dropping support for hex-with-suffix, let's
>> follow our deprecation policy. Sadly, since qemu_strtosz() does not
>> have an Err** parameter, we pollute to stderr.
>
> Err** is only appropriate for errors, not warnings, so this statement
> can be removed.
The point of the comment was that we have no other mechanism for
reporting the deprecation other than polluting to stderr. And if we
ever remove the support, we'll either have to silently reject input that
we used to accept, or plumb through Err** handling to allow better
reporting of WHY we are rejecting input. But I didn't feel like adding
Err** support now.
>> @@ -309,6 +309,10 @@ static int do_strtosz(const char *nptr, const char
>> **end,
>> c = *endptr;
>> mul = suffix_mul(c, unit);
>> if (mul > 0) {
>> +if (hex) {
>> +fprintf(stderr, "Using a multiplier suffix on hex numbers "
>> +"is deprecated: %s\n", nptr);
>
> warn_report(), since IIUC, that'll get into HMP response correctly.
Yes, that sounds better. I'll use that in v2, as well as tweaking the
commit message.
>
>> +}
>> endptr++;
>> } else {
>> mul = suffix_mul(default_suffix, unit);
>
> Regards,
> Daniel
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PATCH 2/3] utils: Deprecate hex-with-suffix sizes
On 2/5/21 4:25 AM, Vladimir Sementsov-Ogievskiy wrote:
> 04.02.2021 22:07, Eric Blake wrote:
>> Supporting '0x20M' looks odd, particularly since we have an 'E' suffix
>
> What about also deprecating 'E' suffix? (just my problem of reviewing
> previous patch)
No, we want to keep '1E' as a valid way to spell 1 exabyte. That has
uses in the wild (admittedly corner-case, as that is a LOT of storage).
It is only '0x1E' where the use of 'E' as a hex digit has priority over
'E' as a suffix meaning exabyte.
>
>> that is ambiguous between a hex digit and the extremely large exibyte
>> suffix, as well as a 'B' suffix for bytes. In practice, people using
>> hex inputs are specifying values in bytes (and would have written
>> 0x200, or possibly relied on default_suffix in the case of
>> qemu_strtosz_MiB), and the use of scaling suffixes makes the most
>> sense for inputs in decimal (where the user would write 32M). But
>> rather than outright dropping support for hex-with-suffix, let's
>> follow our deprecation policy. Sadly, since qemu_strtosz() does not
>> have an Err** parameter, we pollute to stderr.
>>
>> Signed-off-by: Eric Blake
>> ---
>> +++ b/util/cutils.c
>> @@ -264,7 +264,7 @@ static int do_strtosz(const char *nptr, const char
>> **end,
>> int retval;
>> const char *endptr;
>> unsigned char c;
>> - bool mul_required = false;
>> + bool mul_required = false, hex = false;
>> uint64_t val;
>> int64_t mul;
>> double fraction = 0.0;
>> @@ -309,6 +309,10 @@ static int do_strtosz(const char *nptr, const
>> char **end,
>
> you forget to set hex to true in corresponding if(){...}
>
>> c = *endptr;
>> mul = suffix_mul(c, unit);
>> if (mul > 0) {
>> + if (hex) {
>> + fprintf(stderr, "Using a multiplier suffix on hex numbers "
>> + "is deprecated: %s\n", nptr);
>> + }
D'oh. Now I get to rerun my tests to see when the warning triggers.
>> endptr++;
>> } else {
>> mul = suffix_mul(default_suffix, unit);
>
> should we also deprecate hex where default_suffix is not 'B' ?
That's exactly what this patch is (supposed to be) doing. If we parsed
a hex number, and there was an explicit suffix at all (which is
necessarily neither 'B' nor 'E', since those were already consumed while
parsing the hex number), issue a warning.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PATCH 3/3] utils: Deprecate inexact fractional suffix sizes
On 2/4/21 1:07 PM, Eric Blake wrote: > The value '1.1k' is inexact; 1126.4 bytes is not possible, so we > happen to truncate it to 1126. Our use of fractional sizes is > intended for convenience, but when a user specifies a fraction that is > not a clean translation to binary, truncating/rounding behind their > backs can cause confusion. Better is to deprecate inexact values, > which still leaves '1.5k' as valid, but alerts the user to spell out > their values as a precise byte number in cases where they are > currently being rounded. And I promptly forgot to save my changes in my editor. Consider this squashed in: diff --git i/docs/system/deprecated.rst w/docs/system/deprecated.rst index c404c3926e6f..8e5e05a10642 100644 --- i/docs/system/deprecated.rst +++ w/docs/system/deprecated.rst @@ -154,6 +154,15 @@ Input parameters that take a size value should only use a size suffix the value is hexadecimal. That is, '0x20M' is deprecated, and should be written either as '32M' or as '0x200'. +inexact sizes via scaled fractions (since 6.0) +'' + +Input parameters that take a size value should only use a fractional +size (such as '1.5M') that will result in an exact byte value. The +use of inexact values (such as '1.1M') that require truncation or +rounding is deprecated, and you should instead consider writing your +unusual size in bytes (here, '1153433' or '1153434' as desired). + QEMU Machine Protocol (QMP) commands ---- -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
[PATCH 2/3] utils: Deprecate hex-with-suffix sizes
Supporting '0x20M' looks odd, particularly since we have an 'E' suffix
that is ambiguous between a hex digit and the extremely large exibyte
suffix, as well as a 'B' suffix for bytes. In practice, people using
hex inputs are specifying values in bytes (and would have written
0x200, or possibly relied on default_suffix in the case of
qemu_strtosz_MiB), and the use of scaling suffixes makes the most
sense for inputs in decimal (where the user would write 32M). But
rather than outright dropping support for hex-with-suffix, let's
follow our deprecation policy. Sadly, since qemu_strtosz() does not
have an Err** parameter, we pollute to stderr.
Signed-off-by: Eric Blake
---
docs/system/deprecated.rst | 8
util/cutils.c | 6 +-
2 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
index 6ac757ed9fa7..c404c3926e6f 100644
--- a/docs/system/deprecated.rst
+++ b/docs/system/deprecated.rst
@@ -146,6 +146,14 @@ library enabled as a cryptography provider.
Neither the ``nettle`` library, or the built-in cryptography provider are
supported on FIPS enabled hosts.
+hexadecimal sizes with scaling multipliers (since 6.0)
+''
+
+Input parameters that take a size value should only use a size suffix
+(such as 'k' or 'M') when the base is written in decimal, and not when
+the value is hexadecimal. That is, '0x20M' is deprecated, and should
+be written either as '32M' or as '0x200'.
+
QEMU Machine Protocol (QMP) commands
diff --git a/util/cutils.c b/util/cutils.c
index 0234763bd70b..75190565cbb5 100644
--- a/util/cutils.c
+++ b/util/cutils.c
@@ -264,7 +264,7 @@ static int do_strtosz(const char *nptr, const char **end,
int retval;
const char *endptr;
unsigned char c;
-bool mul_required = false;
+bool mul_required = false, hex = false;
uint64_t val;
int64_t mul;
double fraction = 0.0;
@@ -309,6 +309,10 @@ static int do_strtosz(const char *nptr, const char **end,
c = *endptr;
mul = suffix_mul(c, unit);
if (mul > 0) {
+if (hex) {
+fprintf(stderr, "Using a multiplier suffix on hex numbers "
+"is deprecated: %s\n", nptr);
+}
endptr++;
} else {
mul = suffix_mul(default_suffix, unit);
--
2.30.0
Re: [PATCH for-6.0 6/6] qapi: Deprecate 'query-kvm'
On 11/27/20 10:30 AM, Peter Krempa wrote: > On Fri, Nov 27, 2020 at 16:44:05 +0100, Markus Armbruster wrote: >> Peter Krempa writes: >> >>> On Fri, Nov 27, 2020 at 14:45:12 +0300, Roman Bolshakov wrote: >>>> On Fri, Nov 27, 2020 at 12:21:54PM +0100, Peter Krempa wrote: > > [...] > >>> As you can see this has an issue when we have to add support for a >>> unreleased interface, which may change during the dev cycle or plainly >>> forget that something got deprecated as we've already added an override. >>> >>> This mainly comes from libvirt trying to keep on top of the changes so >>> we refresh the QMP schema during qemu's dev cycle multiple times. >>> >>> Obviously the argument that we try to depend on unreleased functionality >>> can be used, but that would be to detrement of progress IMO. >> >> I understand your concerns. >> >> We have a somewhat similar problem in QEMU: there's nothing to remind us >> later on that the old interface still needs to be deprecated. > > Oh, yes. That's basically the same thing. > > The thing is that changes to the new interface are very rare, but very > real. Since I don't really want to increase the burden for any normal > scenario. Case in point: our last-minute changes to block-export-add in qemu commit cbad81ce. The original deprecation of nbd-server-add occurred much earlier in the 5.2 devel cycle, in qemu 443127e8, and also forgot to tell libvirt (https://www.redhat.com/archives/libvir-list/2020-October/msg00855.html); then in my efforts to improve qemu-nbd, I made more changes to block-export-add, but didn't cc libvirt on v1. We eventually got everything coordinated with libvirt in cc, but it did lead to some last minute churn in libvirt to avoid a parity mismatch between versions (https://www.redhat.com/archives/libvir-list/2020-October/msg01369.html). > > I'd also very much like to keep libvirt pulling in snapshots of qemu's > capabilities/qapi schema etc throughout the development cycle. It allows > us to adapt faster and develop features simultaneously. > > This unfortunately undermines my own arguments partially as libvirt > regularly develops features on top of unreleased qemu features so we are > regularly risking very similar circumstances. The small but important > difference though is, that releasing a broken feature is not as bad as > breaking an existing feature. > > As a conclusion of the above I'd basically prefer a sort of gentleman's > agreement, that new APIs become 'somewhat' stable at the moment the > commit deprecating the old interface hits upstream. Yes, moving towards this goal makes sense. And because we've called attention to the fact, I'll try harder to remember in my qapi reviews any time where a new interface exists _because_ it has replaced an interface we already marked as deprecated. > > The 'somewhat'-stable API would mean that any changes to the new API > should be consulted with libvirt so that we can either give a go-ahead > that we didn't adapt yet, disable the adaptation until the changes can > be done, or another compromise depending on what's the state. > > I know it's hard to enforce, but probably the cheapest in terms of > drawbacks any other solution would be. > > I'll probably keep notifying patchsets which implement and deprecate old > api at the same time to keep in mind that we need to be kept in touch, > but I really don't want to impose any kind of extra process to > development on either side. > -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH 5/6] qemu: conf: Enable 'backup_tls_x509_verify' by default
On 11/13/20 9:01 AM, Peter Krempa wrote: > The NBD server used to export pull-mode backups doesn't have any other > form of client authentication on top of the TLS transport, so the only > way to authenticate clients is to verify their certificate. > > Enable this option by defauilt when both 'backup_tls_x509_verify' and > 'default_tls_x509_verify' were not configured. > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1879477 > Signed-off-by: Peter Krempa > --- > src/qemu/qemu.conf | 3 ++- > src/qemu/qemu_conf.c | 2 +- > 2 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf > index d621dad53b..cc46a34ae2 100644 > --- a/src/qemu/qemu.conf > +++ b/src/qemu/qemu.conf > @@ -422,7 +422,8 @@ > # CA in the backup_tls_x509_cert_dir (or default_tls_x509_cert_dir). > # > # If this option is not supplied, it will be set to the value of > -# "default_tls_x509_verify". > +# "default_tls_x509_verify". If "default_tls_x509_verify" is not supplied > either > +# the default is "1". s/either/either,/ Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH 4/6] qemu: conf: Enable 'migrate_tls_x509_verify' by default
On 11/13/20 9:01 AM, Peter Krempa wrote: > The migration stream connection and also the NBD server for non-shared > storage migration don't have any other form of client authentication on > top of the TLS transport, so the only way to authenticate clients is to > verify their certificate. > > Enable this option by defauilt when both 'migrate_tls_x509_verify' and > 'default_tls_x509_verify' were not configured. > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1879477 > Signed-off-by: Peter Krempa > --- > src/qemu/qemu.conf | 3 ++- > src/qemu/qemu_conf.c | 2 +- > 2 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf > index 8a1a50d664..d621dad53b 100644 > --- a/src/qemu/qemu.conf > +++ b/src/qemu/qemu.conf > @@ -385,7 +385,8 @@ > # CA in the migrate_tls_x509_cert_dir (or default_tls_x509_cert_dir). > # > # If this option is not supplied, it will be set to the value of > -# "default_tls_x509_verify". > +# "default_tls_x509_verify". If "default_tls_x509_verify" is not supplied > +# either the default is "1". s/either/either,/ Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH] qemu: backup: Install bitmap for incremental backup to appropriate node only
On 11/13/20 8:55 AM, Peter Krempa wrote:
> Libvirt's backup code has two modes:
>
> 1) push - where qemu actively writes the difference since the checkpoint
> into the output file
>
> 2) pull - where we instruct qemu to expose a frozen disk state along
> with a bitmap of blocks which changed since the checkpoint
>
> For push mode qemu needs the temporary bitmap we use where we calculate
> the actual changes to be present on the block node backing the disk.
>
> For pull mode where we expose the bitmap via NBD qemu actually wants the
> bitmap to be present for the exported block node which is the scratch
> file.
>
> Until now we've calculated the bitmap twice and installed it both to the
> scratch file and to the disk node, but we don't need to since we know
> when it's needed.
>
> Pass in the 'pull' flag and decide where to install the bitmap according
> to it and also when to register the bitmap name with the blockjob.
>
> Signed-off-by: Peter Krempa
> ---
> src/qemu/qemu_backup.c | 42 ++----
> 1 file changed, 26 insertions(+), 16 deletions(-)
Reviewed-by: Eric Blake
> +/* For pull-mode backup, we need the bitmap to be present in the scratch
> + * file as that will be exported. For push-mode backup the bitmap is
> + * actually required on top of the image backing the disk */
>
> -if (qemuBackupDiskPrepareOneBitmapsChain(dd->domdisk->src,
> - dd->store,
> - dd->incrementalBitmap,
> - dd->backupdisk->incremental,
> - actions,
> - blockNamedNodeData) < 0)
> -return -1;
> +if (pull) {
> +if (qemuBackupDiskPrepareOneBitmapsChain(dd->domdisk->src,
> + dd->store,
> + dd->incrementalBitmap,
> + dd->backupdisk->incremental,
> + actions,
> + blockNamedNodeData) < 0)
Technically, with both qemu 5.0 and qemu 5.2, the bitmap can live in the
backing store for BOTH modes, (in 5.2 and beyond, because qemu will
chase through the backing chain to find the named bitmap if it is not
present in the temporary store; in 5.0 because the temporary store is
not a filter node). But since qemu 5.1 used a filter node for the
temporary store but was unable to chase through filter nodes, always
placing the bitmap in the temporary node for pull mode is fine.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PATCH 1/3] qemu: capabilities: Disable detection of QEMU_CAPS_BLOCK_EXPORT_ADD
On 10/29/20 12:10 PM, Peter Krempa wrote: > On Mon, Oct 26, 2020 at 08:36:50 -0500, Eric Blake wrote: >> On 10/26/20 8:19 AM, Peter Krempa wrote: >>> We use the capability to switch to using 'block-export-add' in the >>> upcoming qemu release instead of the at the same time deprecated >>> 'nbd-server-add'. >>> >>> Unfortunately qemu wants to change the interface of 'block-export-add' >>> before the release. Since we've tried to stay up to date and added the >>> code before it was written in stone, we need to disable the use of the >>> new interface for the upcoming libvirt release so that we don't have a >>> version of libvirt which would not work with the upcoming qemu version. >>> >>> Remove the detection of 'block-export-add' until we are more sure how >>> the qemu interface will look. >>> >>> This patch partially reverts commit adb9f7123adb94645 >>> >>> Signed-off-by: Peter Krempa >>> --- >>> src/qemu/qemu_capabilities.c | 1 - >>> tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml | 1 - >>> 2 files changed, 2 deletions(-) >> >> Reviewed-by: Eric Blake >> >> Safe for libvirt now no matter what happens in qemu's soft freeze this week. > > I've pushed this patch, since Jirka is about to cut 'rc2' of libvirt and > the qemu changes didn't hit upstream yet. I see libvirt 6.9 is out now, so we can now proceed with the rest of the series. The qemu patches are now officially upstream (the whole series is in, but the qapi changes are in this one): commit cbad81cef8cc7b220f04600997ea29d7302bae00 Author: Eric Blake Date: Tue Oct 27 00:05:49 2020 -0500 nbd: Update qapi to support exporting multiple bitmaps Since 'block-export-add' is new to 5.2, we can still tweak the interface; there, allowing 'bitmaps':['str'] is nicer than 'bitmap':'str'. This wires up the qapi and qemu-nbd changes to permit passing multiple bitmaps as distinct metadata contexts that the NBD client may request, but the actual support for more than one will require a further patch to the server. Note that there are no changes made to the existing deprecated 'nbd-server-add' command; this required splitting the QAPI type BlockExportOptionsNbd, which fortunately does not affect QMP introspection. Signed-off-by: Eric Blake Message-Id: <20201027050556.269064-5-ebl...@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Peter Krempa -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
[PULL 06/12] nbd: Update qapi to support exporting multiple bitmaps
Since 'block-export-add' is new to 5.2, we can still tweak the
interface; there, allowing 'bitmaps':['str'] is nicer than
'bitmap':'str'. This wires up the qapi and qemu-nbd changes to permit
passing multiple bitmaps as distinct metadata contexts that the NBD
client may request, but the actual support for more than one will
require a further patch to the server.
Note that there are no changes made to the existing deprecated
'nbd-server-add' command; this required splitting the QAPI type
BlockExportOptionsNbd, which fortunately does not affect QMP
introspection.
Signed-off-by: Eric Blake
Message-Id: <20201027050556.269064-5-ebl...@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy
Reviewed-by: Peter Krempa
---
docs/system/deprecated.rst | 3 ++-
qapi/block-export.json | 41 +++---
blockdev-nbd.c | 6 +-
nbd/server.c | 19 --
qemu-nbd.c | 18 -
5 files changed, 58 insertions(+), 29 deletions(-)
diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
index 0ebce37a1919..32a0e620dbb9 100644
--- a/docs/system/deprecated.rst
+++ b/docs/system/deprecated.rst
@@ -257,7 +257,8 @@ the 'wait' field, which is only applicable to sockets in
server mode
Use the more generic commands ``block-export-add`` and ``block-export-del``
-instead.
+instead. As part of this deprecation, where ``nbd-server-add`` used a
+single ``bitmap``, the new ``block-export-add`` uses a list of ``bitmaps``.
Human Monitor Protocol (HMP) commands
-
diff --git a/qapi/block-export.json b/qapi/block-export.json
index 480c497690b0..c4125f4d2104 100644
--- a/qapi/block-export.json
+++ b/qapi/block-export.json
@@ -63,10 +63,10 @@
'*max-connections': 'uint32' } }
##
-# @BlockExportOptionsNbd:
+# @BlockExportOptionsNbdBase:
#
-# An NBD block export (options shared between nbd-server-add and the NBD branch
-# of block-export-add).
+# An NBD block export (common options shared between nbd-server-add and
+# the NBD branch of block-export-add).
#
# @name: Export name. If unspecified, the @device parameter is used as the
#export name. (Since 2.12)
@@ -74,15 +74,27 @@
# @description: Free-form description of the export, up to 4096 bytes.
# (Since 5.0)
#
-# @bitmap: Also export the dirty bitmap reachable from @device, so the
-# NBD client can use NBD_OPT_SET_META_CONTEXT with
-# "qemu:dirty-bitmap:NAME" to inspect the bitmap. (since 4.0)
-#
# Since: 5.0
##
+{ 'struct': 'BlockExportOptionsNbdBase',
+ 'data': { '*name': 'str', '*description': 'str' } }
+
+##
+# @BlockExportOptionsNbd:
+#
+# An NBD block export (distinct options used in the NBD branch of
+# block-export-add).
+#
+# @bitmaps: Also export each of the named dirty bitmaps reachable from
+# @device, so the NBD client can use NBD_OPT_SET_META_CONTEXT with
+# the metadata context name "qemu:dirty-bitmap:BITMAP" to inspect
+# each bitmap.
+#
+# Since: 5.2
+##
{ 'struct': 'BlockExportOptionsNbd',
- 'data': { '*name': 'str', '*description': 'str',
-'*bitmap': 'str' } }
+ 'base': 'BlockExportOptionsNbdBase',
+ 'data': { '*bitmaps': ['str'] } }
##
# @BlockExportOptionsVhostUserBlk:
@@ -106,19 +118,24 @@
##
# @NbdServerAddOptions:
#
-# An NBD block export.
+# An NBD block export, per legacy nbd-server-add command.
#
# @device: The device name or node name of the node to be exported
#
# @writable: Whether clients should be able to write to the device via the
#NBD connection (default false).
#
+# @bitmap: Also export a single dirty bitmap reachable from @device, so the
+# NBD client can use NBD_OPT_SET_META_CONTEXT with the metadata
+# context name "qemu:dirty-bitmap:BITMAP" to inspect the bitmap
+# (since 4.0).
+#
# Since: 5.0
##
{ 'struct': 'NbdServerAddOptions',
- 'base': 'BlockExportOptionsNbd',
+ 'base': 'BlockExportOptionsNbdBase',
'data': { 'device': 'str',
-'*writable': 'bool' } }
+'*writable': 'bool', '*bitmap': 'str' } }
##
# @nbd-server-add:
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index cee9134b12eb..d8443d235b73 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -209,8 +209,12 @@ void qmp_nbd_server_add(NbdServerAddOptions *arg, Error
**errp)
.has_writable = arg->has_writable,
.writable = arg->writable,
};
-QAPI_CLONE_MEMBERS(BlockExportOptionsNbd, _opts->u.nbd,
+QAPI_CLONE_MEMBERS(BlockExportOptionsNbdBase, _opts->u.nbd,
qapi_NbdServerAddOptions_base(arg));
+if (arg->has_bitmap) {
+export_opts->u.nbd.has_bitmaps = true;
+QAPI_LIST_PREPEND(export_opts->u.nbd.bitmaps, g_strdup(arg->bitmap));
+}
[PATCH v6 04/11] nbd: Update qapi to support exporting multiple bitmaps
Since 'block-export-add' is new to 5.2, we can still tweak the
interface; there, allowing 'bitmaps':['str'] is nicer than
'bitmap':'str'. This wires up the qapi and qemu-nbd changes to permit
passing multiple bitmaps as distinct metadata contexts that the NBD
client may request, but the actual support for more than one will
require a further patch to the server.
Note that there are no changes made to the existing deprecated
'nbd-server-add' command; this required splitting the QAPI type
BlockExportOptionsNbd, which fortunately does not affect QMP
introspection.
Signed-off-by: Eric Blake
---
docs/system/deprecated.rst | 3 ++-
qapi/block-export.json | 41 +++---
blockdev-nbd.c | 6 +-
nbd/server.c | 19 --
qemu-nbd.c | 18 -
5 files changed, 58 insertions(+), 29 deletions(-)
diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
index 0ebce37a1919..32a0e620dbb9 100644
--- a/docs/system/deprecated.rst
+++ b/docs/system/deprecated.rst
@@ -257,7 +257,8 @@ the 'wait' field, which is only applicable to sockets in
server mode
Use the more generic commands ``block-export-add`` and ``block-export-del``
-instead.
+instead. As part of this deprecation, where ``nbd-server-add`` used a
+single ``bitmap``, the new ``block-export-add`` uses a list of ``bitmaps``.
Human Monitor Protocol (HMP) commands
-
diff --git a/qapi/block-export.json b/qapi/block-export.json
index 480c497690b0..c4125f4d2104 100644
--- a/qapi/block-export.json
+++ b/qapi/block-export.json
@@ -63,10 +63,10 @@
'*max-connections': 'uint32' } }
##
-# @BlockExportOptionsNbd:
+# @BlockExportOptionsNbdBase:
#
-# An NBD block export (options shared between nbd-server-add and the NBD branch
-# of block-export-add).
+# An NBD block export (common options shared between nbd-server-add and
+# the NBD branch of block-export-add).
#
# @name: Export name. If unspecified, the @device parameter is used as the
#export name. (Since 2.12)
@@ -74,15 +74,27 @@
# @description: Free-form description of the export, up to 4096 bytes.
# (Since 5.0)
#
-# @bitmap: Also export the dirty bitmap reachable from @device, so the
-# NBD client can use NBD_OPT_SET_META_CONTEXT with
-# "qemu:dirty-bitmap:NAME" to inspect the bitmap. (since 4.0)
-#
# Since: 5.0
##
+{ 'struct': 'BlockExportOptionsNbdBase',
+ 'data': { '*name': 'str', '*description': 'str' } }
+
+##
+# @BlockExportOptionsNbd:
+#
+# An NBD block export (distinct options used in the NBD branch of
+# block-export-add).
+#
+# @bitmaps: Also export each of the named dirty bitmaps reachable from
+# @device, so the NBD client can use NBD_OPT_SET_META_CONTEXT with
+# the metadata context name "qemu:dirty-bitmap:BITMAP" to inspect
+# each bitmap.
+#
+# Since: 5.2
+##
{ 'struct': 'BlockExportOptionsNbd',
- 'data': { '*name': 'str', '*description': 'str',
-'*bitmap': 'str' } }
+ 'base': 'BlockExportOptionsNbdBase',
+ 'data': { '*bitmaps': ['str'] } }
##
# @BlockExportOptionsVhostUserBlk:
@@ -106,19 +118,24 @@
##
# @NbdServerAddOptions:
#
-# An NBD block export.
+# An NBD block export, per legacy nbd-server-add command.
#
# @device: The device name or node name of the node to be exported
#
# @writable: Whether clients should be able to write to the device via the
#NBD connection (default false).
#
+# @bitmap: Also export a single dirty bitmap reachable from @device, so the
+# NBD client can use NBD_OPT_SET_META_CONTEXT with the metadata
+# context name "qemu:dirty-bitmap:BITMAP" to inspect the bitmap
+# (since 4.0).
+#
# Since: 5.0
##
{ 'struct': 'NbdServerAddOptions',
- 'base': 'BlockExportOptionsNbd',
+ 'base': 'BlockExportOptionsNbdBase',
'data': { 'device': 'str',
-'*writable': 'bool' } }
+'*writable': 'bool', '*bitmap': 'str' } }
##
# @nbd-server-add:
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index cee9134b12eb..d1d41f635564 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -209,8 +209,12 @@ void qmp_nbd_server_add(NbdServerAddOptions *arg, Error
**errp)
.has_writable = arg->has_writable,
.writable = arg->writable,
};
-QAPI_CLONE_MEMBERS(BlockExportOptionsNbd, _opts->u.nbd,
+QAPI_CLONE_MEMBERS(BlockExportOptionsNbdBase, _opts->u.nbd,
qapi_NbdServerAddOptions_base(arg));
+if (arg->has_bitmap) {
+export_opts->u.nbd.has_bitmaps = true;
+QAPI_LIST_ADD(export_opts->u.nbd.bitmaps, g_strdup(arg->bitmap));
+}
/*
* nbd-server-add doesn't complain when a read-only device should be
diff --git a/nbd/server.c b/nbd/server.c
index 08b621f7
Re: [PATCH 09/13] util: hash: Don't use 'const' with virHashTablePtr
On 10/26/20 10:45 AM, Peter Krempa wrote: > We didn't use it rigorously and some helpers even dasted it away. Remove s/dasted/cast/ (both the typo, and the odd English word whose present and past tense forms are identical) > const from all hash utility functions. > > Signed-off-by: Peter Krempa > --- -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
