Re: [libvirt PATCH 0/9] qemu: report guest disks informations
Hi On Mon, Nov 30, 2020 at 11:16 PM Michal Privoznik wrote: > > On 11/20/20 7:09 PM, marcandre.lur...@redhat.com wrote: > > From: Marc-André Lureau > > > > Hi, > > > > The following series extends virDomainGetGuestInfo to report disk > > informations > > provided by the new QGA "guest-get-disks" command in QEMU 5.2. > > > > Please review, > > > > Fixes: > > https://bugzilla.redhat.com/show_bug.cgi?id=1899527 > > > > Marc-André Lureau (9): > >qemu_agent: rename qemuAgentDiskInfo->qemuAgentDiskAddress > >qemu_agent: export qemuAgentDiskAddressFree & add g_auto > >qemu_agent: factor out qemuAgentGetDiskAddress > >util: json: add virJSONValueObjectGetStringArray convenience > >qemu: use virJSONValueObjectGetStringArray > >qemu_agent: add qemuAgentGetDisks > >domain: add disk informations to virDomainGetGuestInfo > >qemu_driver: report guest disk informations > >virsh: add --disk informations to guestinfo command > > > > include/libvirt/libvirt-domain.h | 1 + > > src/libvirt-domain.c | 17 +++ > > src/libvirt_private.syms | 1 + > > src/qemu/qemu_agent.c| 182 +++ > > src/qemu/qemu_agent.h| 25 - > > src/qemu/qemu_driver.c | 103 - > > src/qemu/qemu_monitor_json.c | 34 +- > > src/util/virjson.c | 30 + > > src/util/virjson.h | 1 + > > tests/qemuagenttest.c| 111 +++ > > tools/virsh-domain.c | 6 + > > 11 files changed, 430 insertions(+), 81 deletions(-) > > > > Patches look good. A huge sorry for letting these slip the release. To > make it up to you, I'm proposing couple of adjustments and if you agree > I will squash in proposed diffs before pushing - so that you don't have > to send v2. > Fine to me, > Tentative: > Reviewed-by: Michal Privoznik > thanks!
Re: [libvirt PATCH 9/9] virsh: add --disk informations to guestinfo command
Hi On Mon, Nov 30, 2020 at 11:16 PM Michal Privoznik wrote: > > On 11/20/20 7:09 PM, marcandre.lur...@redhat.com wrote: > > From: Marc-André Lureau > > > > Signed-off-by: Marc-André Lureau > > --- > > tools/virsh-domain.c | 6 ++ > > 1 file changed, 6 insertions(+) > > Missing manpage addition. How about: > > diff --git i/docs/manpages/virsh.rst w/docs/manpages/virsh.rst > index 86deaa486d..9ef6b68422 100644 > --- i/docs/manpages/virsh.rst > +++ w/docs/manpages/virsh.rst > @@ -2679,6 +2679,7 @@ guestinfo > :: > > guestinfo domain [--user] [--os] [--timezone] [--hostname] > [--filesystem] > + [--disks] > > Print information about the guest from the point of view of the guest > agent. > Note that this command requires a guest agent to be configured and > running in > @@ -2689,7 +2690,7 @@ are supported by the guest agent. You can limit > the types of information that > are returned by specifying one or more flags. If a requested information > type is not supported, the processes will provide an exit code of 1. > Available information types flags are *--user*, *--os*, > -*--timezone*, *--hostname*, and *--filesystem*. > +*--timezone*, *--hostname*, *--filesystem* and *--disks*. > > Note that depending on the hypervisor type and the version of the > guest agent > running within the domain, not all of the following information may be > @@ -2746,6 +2747,16 @@ returned: > * ``fs..disk..serial`` - the serial number of disk > * ``fs..disk..device`` - the device node of disk > > +*--disks* returns: > + > +* ``disks.count`` - the number of disks defined on this domain > +* ``disks..name`` - device node (Linux) or device UNC (Windows) > +* ``disks..partition`` - whether this is a partition or disk > +* ``disks..dependencies.count`` - the number of device dependencies > +* ``disks..dependencies..name`` - a dependency name > +* ``disks..alias`` - the device alias of the disk (e.g. sda) > +* ``disks..guest_alias`` - optional alias assigned to the disk > + > > guestvcpus > -- Looks good, thanks
Re: [libvirt PATCH 5/9] qemu: use virJSONValueObjectGetStringArray
On Mon, Nov 30, 2020 at 11:16 PM Michal Privoznik wrote:
>
> On 11/20/20 7:09 PM, marcandre.lur...@redhat.com wrote:
> > From: Marc-André Lureau
> >
> > There might be more potential users around, I haven't looked thoroughly.
>
>
> Quick git grep showed two more: qemuAgentSSHGetAuthorizedKeys() and
> qemuMonitorJSONGetStringListProperty(). But that can be done in a
> separate patch.
>
> >
> > Signed-off-by: Marc-André Lureau
> > ---
> > src/qemu/qemu_monitor_json.c | 34 ++
> > 1 file changed, 6 insertions(+), 28 deletions(-)
> >
> > diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> > index 723bdb4426..3588a0c426 100644
> > --- a/src/qemu/qemu_monitor_json.c
> > +++ b/src/qemu/qemu_monitor_json.c
> > @@ -7533,10 +7533,7 @@ qemuMonitorJSONGetStringArray(qemuMonitorPtr mon,
> > const char *qmpCmd,
> > int ret = -1;
> > virJSONValuePtr cmd;
> > virJSONValuePtr reply = NULL;
> > -virJSONValuePtr data;
> > -char **list = NULL;
> > -size_t n = 0;
> > -size_t i;
> > +g_auto(GStrv) list = NULL;
> >
> > *array = NULL;
> >
> > @@ -7554,32 +7551,13 @@ qemuMonitorJSONGetStringArray(qemuMonitorPtr mon,
> > const char *qmpCmd,
> > if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_ARRAY) < 0)
> > goto cleanup;
> >
> > -data = virJSONValueObjectGetArray(reply, "return");
> > -n = virJSONValueArraySize(data);
> > -
> > -/* null-terminated list */
> > -list = g_new0(char *, n + 1);
> > -
> > -for (i = 0; i < n; i++) {
> > -virJSONValuePtr child = virJSONValueArrayGet(data, i);
> > -const char *tmp;
> > -
> > -if (!(tmp = virJSONValueGetString(child))) {
> > -virReportError(VIR_ERR_INTERNAL_ERROR,
> > - _("%s array element does not contain data"),
> > - qmpCmd);
> > -goto cleanup;
> > -}
> > -
> > -list[i] = g_strdup(tmp);
> > -}
> > -
> > -ret = n;
> > -*array = list;
> > -list = NULL;
> > +list = virJSONValueObjectGetStringArray(reply, "return");
>
> We can ditch the @list variable and assign to *array directly.
>
> > +if (!list)
> > +goto cleanup;
> > +ret = g_strv_length(list);
> > +*array = g_steal_pointer(&list);
> >
> >cleanup:
> > -g_strfreev(list);
> > virJSONValueFree(cmd);
> > virJSONValueFree(reply);
> > return ret;
> >
>
>
> How about this squashed in:
>
>
> diff --git c/src/qemu/qemu_monitor_json.c i/src/qemu/qemu_monitor_json.c
> index 3588a0c426..e7aa6b6ffd 100644
> --- c/src/qemu/qemu_monitor_json.c
> +++ i/src/qemu/qemu_monitor_json.c
> @@ -7533,7 +7533,6 @@ qemuMonitorJSONGetStringArray(qemuMonitorPtr mon,
> const char *qmpCmd,
> int ret = -1;
> virJSONValuePtr cmd;
> virJSONValuePtr reply = NULL;
> -g_auto(GStrv) list = NULL;
>
> *array = NULL;
>
> @@ -7551,11 +7550,10 @@ qemuMonitorJSONGetStringArray(qemuMonitorPtr
> mon, const char *qmpCmd,
> if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_ARRAY) < 0)
> goto cleanup;
>
> -list = virJSONValueObjectGetStringArray(reply, "return");
> -if (!list)
> +if (!(*array = virJSONValueObjectGetStringArray(reply, "return")))
> goto cleanup;
> -ret = g_strv_length(list);
> -*array = g_steal_pointer(&list);
> +
> +ret = g_strv_length(*array);
>
>cleanup:
> virJSONValueFree(cmd);
>
Looks good to me, thanks
[PATCH v2 0/2] gitlab: Add issue templates
Gitlab allows projects to define templates for filing issues [1]. Libvirt didn't have them yet. We can use them to encourage users to file better bug reports and feature requests. Differences to v1: - dropped bug-qemu.md template - vastly shortened comment blocks - all comments are single line only to prevent users adding info inside of them - more fill-in fields instead of comment guidance You can test-drive the proposed templates in my test repo: https://gitlab.com/pipo.sk/test/-/issues/new Peter Krempa (2): gitlab: Add issue template for reporting a generic bug gitlab: Add issue template for a feature request .gitlab/issue_templates/bug.md | 26 ++ .gitlab/issue_templates/feature.md | 14 ++ 2 files changed, 40 insertions(+) create mode 100644 .gitlab/issue_templates/bug.md create mode 100644 .gitlab/issue_templates/feature.md -- 2.28.0
[PATCH v2 1/2] gitlab: Add issue template for reporting a generic bug
When reporting an issue in gitlab, the project can define a template for various scenarios which are meant to guide the users to add the relevant information the project needs to the reported issue. Add a template for a generic bug report against libvirt. The template adds sections which motivate users to add version information and also link to documentation about fetching logs and such. Another section then motivates the users to describe the steps taken and also the expected outcome if it's not obvious. Finally a template also automatically applies one or more labels, so new issues are already partially pre-triaged. Note that markdown seems to be the only supported format for now. Signed-off-by: Peter Krempa --- .gitlab/issue_templates/bug.md | 26 ++ 1 file changed, 26 insertions(+) create mode 100644 .gitlab/issue_templates/bug.md diff --git a/.gitlab/issue_templates/bug.md b/.gitlab/issue_templates/bug.md new file mode 100644 index 00..f3a9f3e722 --- /dev/null +++ b/.gitlab/issue_templates/bug.md @@ -0,0 +1,26 @@ + + +## Software environment + - Operating system: + - Architecture: + - kernel version: + - libvirt version: + - Hypervisor and version: + +## Description of problem + +## Steps to reproduce +1. +2. +3. + +## Expected behavior + +## Additional information + + + + + + +/label ~bug -- 2.28.0
[PATCH v2 2/2] gitlab: Add issue template for a feature request
Try to motivate the users to describe what they want to achieve before diving down into technical specifics. Signed-off-by: Peter Krempa --- .gitlab/issue_templates/feature.md | 14 ++ 1 file changed, 14 insertions(+) create mode 100644 .gitlab/issue_templates/feature.md diff --git a/.gitlab/issue_templates/feature.md b/.gitlab/issue_templates/feature.md new file mode 100644 index 00..f32faae82b --- /dev/null +++ b/.gitlab/issue_templates/feature.md @@ -0,0 +1,14 @@ +## Goal + + + +## Technical details + + + +## Additional information + + + + +/label ~enhancement -- 2.28.0
[PATCH] polkit: Allow libvirt group access to libvirtd ro socket
As a normal user, 'virsh connect qemu:///system' and
'virsh connect --readonly qemu:///system' will prompt for root password.
If the user is added to the libvirt group, only
'virsh connect --readonly qemu:///system' will prompt for root password.
The libvirt polkit rules already allow libvirt group members access to
the rw socket. Add a rule allowing to access the ro socket.
Signed-off-by: Jim Fehlig
---
src/remote/libvirtd.rules | 11 +--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/src/remote/libvirtd.rules b/src/remote/libvirtd.rules
index 01a15fac2e..d9be94fcc4 100644
--- a/src/remote/libvirtd.rules
+++ b/src/remote/libvirtd.rules
@@ -1,5 +1,12 @@
-// Allow any user in the 'libvirt' group to connect to system libvirtd
-// without entering a password.
+// Allow any user in the 'libvirt' group to connect to the system libvirtd
+// ro and rw sockets without entering a password.
+
+polkit.addRule(function(action, subject) {
+if (action.id == "org.libvirt.unix.monitor" &&
+subject.isInGroup("libvirt")) {
+return polkit.Result.YES;
+}
+});
polkit.addRule(function(action, subject) {
if (action.id == "org.libvirt.unix.manage" &&
--
2.29.2
Re: [PATCH] polkit: Allow libvirt group access to libvirtd ro socket
On Mon, Nov 30, 2020 at 7:29 PM Jim Fehlig wrote:
>
> As a normal user, 'virsh connect qemu:///system' and
> 'virsh connect --readonly qemu:///system' will prompt for root password.
> If the user is added to the libvirt group, only
> 'virsh connect --readonly qemu:///system' will prompt for root password.
>
> The libvirt polkit rules already allow libvirt group members access to
> the rw socket. Add a rule allowing to access the ro socket.
>
> Signed-off-by: Jim Fehlig
> ---
> src/remote/libvirtd.rules | 11 +--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/src/remote/libvirtd.rules b/src/remote/libvirtd.rules
> index 01a15fac2e..d9be94fcc4 100644
> --- a/src/remote/libvirtd.rules
> +++ b/src/remote/libvirtd.rules
> @@ -1,5 +1,12 @@
> -// Allow any user in the 'libvirt' group to connect to system libvirtd
> -// without entering a password.
> +// Allow any user in the 'libvirt' group to connect to the system libvirtd
> +// ro and rw sockets without entering a password.
> +
> +polkit.addRule(function(action, subject) {
> +if (action.id == "org.libvirt.unix.monitor" &&
> +subject.isInGroup("libvirt")) {
> +return polkit.Result.YES;
> +}
> +});
>
> polkit.addRule(function(action, subject) {
> if (action.id == "org.libvirt.unix.manage" &&
> --
> 2.29.2
>
>
LGTM.
Reviewed-by: Neal Gompa
--
真実はいつも一つ！/ Always, there's only one truth!
RE: [PATCH] qemu_conf: Fix double free problem for cfg->firmwares
> -Original Message- > From: Ján Tomko [mailto:jto...@redhat.com] > Sent: Tuesday, November 24, 2020 6:57 PM > To: tuguoyi (Cloud) > Cc: libvir-list@redhat.com > Subject: Re: [PATCH] qemu_conf: Fix double free problem for cfg->firmwares > > On a Tuesday in 2020, Tuguoyi wrote: > >cfg->firmwares still points to the original memory address after being > >freed by virFirmwareFreeList(). As cfg get freed, it will be freed again > >even if cfg->nfirmwares=0 which eventually lead to crash. > > > >The patch fix it by setting cfg->firmwares to NULL explicitly after > >virFirmwareFreeList() returns > > > >Signed-off-by: Tuguoyi > > Should there be a space separating your name(s)? > > >--- > > src/qemu/qemu_conf.c | 1 + > > 1 file changed, 1 insertion(+) > > > > Reviewed-by: Ján Tomko > > Jano Hi there, It's my first time to submit patch to libvirt, so I'm wondering will this patch be applied to the upstream?
Re: [PATCH for 7.0.0 v1 00/26] Introduce virtio memory support
On 11/27/20 12:02 PM, Michal Privoznik wrote:
Available also here:
https://gitlab.com/MichalPrivoznik/libvirt/-/commits/virtio_mem/
There are new virtio variants of pc-dimm and nvdimm devices. This is the
first attempt to impalement support for them in libvirt.
Thanks to David Hildenbrand for his valuable input!
Series LGTM:
Reviewed-by: Daniel Henrique Barboza
I did a field test in my x86_64 (since pSeries does not support neither
virtio-mem nor virtio-pmem), played around with 'virsh setmem --virtio'
and, as a non-x86 expert, looks good to me as well:
Tested-by: Daniel Henrique Barboza
Michal Prívozník (26):
viruuid: Rework virUUIDIsValid()
internal.h: Introduce VIR_IS_POW2()
docs: Fix nvdimm example wrt to
domain_conf: Check NVDIMM UUID in ABI stability
qemu_domain_address: Reformat qemuDomainAssignS390Addresses()
conf: Require nvdimm path in validate step
domain_conf: Fix virDomainMemoryModel type
virDomainMemorySourceDefFormat: Utilize virXMLFormatElement()
virDomainMemoryTargetDefFormat: Utilize virXMLFormatElement()
qemu: Move mem validation into post parse validator
conf: Move some of virDomainMemoryDef members into a union
conf: Introduce virtio-pmem model
qemu_capabilities: Introduce QEMU_CAPS_DEVICE_VIRTIO_{P}MEM_PCI
qemu_validate: Require virtio-mem device for mem model virtio
security: Relabel virtio mem
qemu: Allow virtio-pmem in CGroups
qemu: Create virtio-pmem in domain namespace
qemu_command: Move dimm into qemuBuildDeviceAddressStr()
qemu: Build command line for virtio-pmem
conf: Introduce virtio-mem model
qemu: Build command line for virtio-mem
qemu: Wire up live update
qemu: Wire up MEMORY_DEVICE_SIZE_CHANGE event
qemu: Refresh the actual size of virtio-mem on monitor reconnect
virsh: Simplify @flags handing in cmdSetmem() and cmdSetmaxmem()
virsh: Introduce --virtio to setmem
docs/formatdomain.rst | 70 +++-
docs/schemas/domaincommon.rng | 16 +
src/conf/domain_conf.c| 372 ++
src/conf/domain_conf.h| 38 +-
src/internal.h| 10 +
src/libvirt_private.syms | 2 +
src/qemu/qemu_alias.c | 59 ++-
src/qemu/qemu_capabilities.c | 4 +
src/qemu/qemu_capabilities.h | 2 +
src/qemu/qemu_cgroup.c| 43 +-
src/qemu/qemu_command.c | 172 +---
src/qemu/qemu_command.h | 5 +-
src/qemu/qemu_domain.c| 99 +++--
src/qemu/qemu_domain.h| 1 +
src/qemu/qemu_domain_address.c| 98 +++--
src/qemu/qemu_domain_address.h| 3 +-
src/qemu/qemu_driver.c| 198 +-
src/qemu/qemu_hotplug.c | 22 +-
src/qemu/qemu_hotplug.h | 5 +
src/qemu/qemu_monitor.c | 37 ++
src/qemu/qemu_monitor.h | 27 ++
src/qemu/qemu_monitor_json.c | 94 +++--
src/qemu/qemu_monitor_json.h | 5 +
src/qemu/qemu_namespace.c | 19 +-
src/qemu/qemu_process.c | 52 ++-
src/qemu/qemu_validate.c | 78 ++--
src/security/security_apparmor.c | 35 +-
src/security/security_dac.c | 48 ++-
src/security/security_selinux.c | 48 ++-
src/security/virt-aa-helper.c | 22 +-
src/util/virrandom.c | 2 +-
src/util/viruuid.c| 17 +-
src/util/viruuid.h| 2 +-
.../caps_4.1.0.x86_64.xml | 1 +
.../caps_4.2.0.x86_64.xml | 1 +
.../caps_5.0.0.x86_64.xml | 1 +
.../caps_5.1.0.x86_64.xml | 2 +
.../caps_5.2.0.x86_64.xml | 2 +
...mory-hotplug-virtio-mem.x86_64-latest.args | 53 +++
.../memory-hotplug-virtio-mem.xml | 78
...ory-hotplug-virtio-pmem.x86_64-latest.args | 45 +++
.../memory-hotplug-virtio-pmem.xml| 54 +++
tests/qemuxml2argvtest.c | 2 +
...emory-hotplug-virtio-mem.x86_64-latest.xml | 1 +
...mory-hotplug-virtio-pmem.x86_64-latest.xml | 1 +
tests/qemuxml2xmltest.c | 3 +
tools/virsh-domain.c | 138 ++-
47 files changed, 1703 insertions(+), 384 deletions(-)
create mode 100644
tests/qemuxml2argvdata/memory-hotplug-virtio-mem.x86_64-latest.args
create mode 100644 tests/qemuxml2argvdata/memory-hotplug-virtio-mem.xml
create mode 100644
tests/qemuxml2argvdata/memory-hotplug-virtio-pmem.x86_64-latest.args
create mode 100644 tests/qemuxml2ar
Re: [PATCH v1 23/26] qemu: Wire up MEMORY_DEVICE_SIZE_CHANGE event
On 11/27/20 12:03 PM, Michal Privoznik wrote:
As advertised in previous commit, this event is delivered to us
when virtio-mem module changes the allocation inside the guest.
It comes with one attribute - size - which holds the new size of
the virtio-mem (well, allocated size), in bytes.
Mind you, this is not necessarily the same number as 'requested
size'. It almost certainly will be when sizing the memory up, but
it might not be when sizing the memory down - the guest kernel
might be unable to free some blocks.
This actual size is reported in the domain XML as an output
element only.
TODO: Fix up total domain memory calculation.
I don't mind the 'TODO' here, but it would be good to clarify what
we can/can't expect while this isn't looked at.
E.g. I took the series for a spin in my x86 dev box (apparently pSeries
does no support virtio-pmem and virtio-mem, so here I am doing x86
work hehe) and I saw that 'virsh setmem --virtio' does not update the
'maxMemory' of the live XML. Is that the intended effect of this pending
'TODO' the commit is referring to?
Code LGTM.
Reviewed-by: Daniel Henrique Barboza
Signed-off-by: Michal Privoznik
---
docs/formatdomain.rst | 7 ++
docs/schemas/domaincommon.rng | 5 +
src/conf/domain_conf.c| 24 ++--
src/conf/domain_conf.h| 7 ++
src/libvirt_private.syms | 1 +
src/qemu/qemu_domain.c| 3 +++
src/qemu/qemu_domain.h| 1 +
src/qemu/qemu_driver.c| 33
src/qemu/qemu_monitor.c | 24
src/qemu/qemu_monitor.h | 20 +
src/qemu/qemu_monitor_json.c | 24
src/qemu/qemu_process.c | 41 +++
12 files changed, 188 insertions(+), 2 deletions(-)
diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
index 3990728939..ac87d03b33 100644
--- a/docs/formatdomain.rst
+++ b/docs/formatdomain.rst
@@ -7196,6 +7196,7 @@ Example: usage of the memory devices
0
2048
524288
+ 524288
@@ -7322,6 +7323,12 @@ Example: usage of the memory devices
granularity. This is valid for ``virtio`` model only and mutually
exclusive with ``pmem``.
+ ``actual``
+ The active XML for a ``virtio`` model may contain ``actual`` element that
+ reflects the actual size of the corresponding virtio memory device. The
+ element is formatted into live XML and never parsed, i.e. it is
+ output-only element.
+
:anchor:``
IOMMU devices
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index d478b639fa..3b12902e04 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -6063,6 +6063,11 @@
+
+
+
+
+
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 5db1fee16b..05f5d70cee 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -18804,6 +18804,21 @@ virDomainMemoryFindByDeviceInfo(virDomainDefPtr def,
}
+ssize_t
+virDomainMemoryFindByDeviceAlias(virDomainDefPtr def,
+ const char *alias)
+{
+size_t i;
+
+for (i = 0; i < def->nmems; i++) {
+if (STREQ_NULLABLE(def->mems[i]->info.alias, alias))
+return i;
+}
+
+return -1;
+}
+
+
/**
* virDomainMemoryInsert:
*
@@ -28124,7 +28139,8 @@ virDomainMemorySourceDefFormat(virBufferPtr buf,
static void
virDomainMemoryTargetDefFormat(virBufferPtr buf,
- virDomainMemoryDefPtr def)
+ virDomainMemoryDefPtr def,
+ unsigned int flags)
{
g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(buf);
@@ -28146,6 +28162,10 @@ virDomainMemoryTargetDefFormat(virBufferPtr buf,
virBufferAsprintf(&childBuf, "%llu\n",
def->requestedsize);
+if (!(flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE)) {
+virBufferAsprintf(&childBuf, "%llu\n",
+ def->actualsize);
+}
}
virXMLFormatElement(buf, "target", NULL, &childBuf);
@@ -28180,7 +28200,7 @@ virDomainMemoryDefFormat(virBufferPtr buf,
if (virDomainMemorySourceDefFormat(buf, def) < 0)
return -1;
-virDomainMemoryTargetDefFormat(buf, def);
+virDomainMemoryTargetDefFormat(buf, def, flags);
virDomainDeviceInfoFormat(buf, &def->info, flags);
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 31892c4941..633c07b59c 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2342,6 +2342,9 @@ struct _virDomainMemoryDef {
unsigned long long labelsize; /* kibibytes; valid only for NVDIMM */
unsigned long long blocksize; /* kibibytes, valid for virti
Re: [PATCH v1 22/26] qemu: Wire up live update
On 11/27/20 12:03 PM, Michal Privoznik wrote:
As advertised in the previous commit, we want' to be able to
Extra ' after "want".
Reviewed-by: Daniel Henrique Barboza
change 'requested-size' attribute of virtio-mem on the fly. This
commit does exactly that. Changing anything else is checked for
and forbidden.
Signed-off-by: Michal Privoznik
---
src/conf/domain_conf.c | 23 +
src/conf/domain_conf.h | 3 +
src/libvirt_private.syms | 1 +
src/qemu/qemu_driver.c | 165 ++-
src/qemu/qemu_hotplug.c | 18
src/qemu/qemu_hotplug.h | 5 ++
src/qemu/qemu_monitor.c | 13 +++
src/qemu/qemu_monitor.h | 4 +
src/qemu/qemu_monitor_json.c | 15
src/qemu/qemu_monitor_json.h | 5 ++
10 files changed, 251 insertions(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 0a402f1a51..5db1fee16b 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -18781,6 +18781,29 @@ virDomainMemoryFindInactiveByDef(virDomainDefPtr def,
}
+ssize_t
+virDomainMemoryFindByDeviceInfo(virDomainDefPtr def,
+virDomainDeviceInfoPtr info)
+{
+size_t i;
+
+for (i = 0; i < def->nmems; i++) {
+virDomainMemoryDefPtr tmp = def->mems[i];
+
+if (!virDomainDeviceInfoAddressIsEqual(&tmp->info, info))
+continue;
+
+/* alias, if present */
+if (STRNEQ_NULLABLE(tmp->info.alias, info->alias))
+continue;
+
+return i;
+}
+
+return -1;
+}
+
+
/**
* virDomainMemoryInsert:
*
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 3f111e994b..31892c4941 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -3580,6 +3580,9 @@ int virDomainMemoryFindByDef(virDomainDefPtr def,
virDomainMemoryDefPtr mem)
int virDomainMemoryFindInactiveByDef(virDomainDefPtr def,
virDomainMemoryDefPtr mem)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) G_GNUC_WARN_UNUSED_RESULT;
+ssize_t virDomainMemoryFindByDeviceInfo(virDomainDefPtr dev,
+virDomainDeviceInfoPtr info)
+ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) G_GNUC_WARN_UNUSED_RESULT;
int virDomainShmemDefInsert(virDomainDefPtr def, virDomainShmemDefPtr shmem)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) G_GNUC_WARN_UNUSED_RESULT;
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index f581676227..1bed019aac 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -497,6 +497,7 @@ virDomainMemballoonModelTypeFromString;
virDomainMemballoonModelTypeToString;
virDomainMemoryDefFree;
virDomainMemoryFindByDef;
+virDomainMemoryFindByDeviceInfo;
virDomainMemoryFindInactiveByDef;
virDomainMemoryInsert;
virDomainMemoryModelTypeToString;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 391596ba11..677f921920 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -7109,6 +7109,158 @@ qemuDomainChangeDiskLive(virDomainObjPtr vm,
return 0;
}
+
+static bool
+qemuDomainChangeMemoryLiveValidateChange(const virDomainMemoryDef *oldDef,
+ const virDomainMemoryDef *newDef)
+{
+/* The only thing that is allowed to change is 'requestedsize' for virtio
+ * model. */
+if (oldDef->model != newDef->model) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("cannot modify memory model from '%s' to '%s'"),
+ virDomainMemoryModelTypeToString(oldDef->model),
+ virDomainMemoryModelTypeToString(newDef->model));
+return false;
+}
+
+if (oldDef->access != newDef->access) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("cannot modify memory access from '%s' to '%s'"),
+ virDomainMemoryAccessTypeToString(oldDef->access),
+ virDomainMemoryAccessTypeToString(newDef->access));
+return false;
+}
+
+if (oldDef->discard != newDef->discard) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("cannot modify memory discard from '%s' to '%s'"),
+ virTristateBoolTypeToString(oldDef->discard),
+ virTristateBoolTypeToString(newDef->discard));
+return false;
+}
+
+if (oldDef->targetNode != newDef->targetNode) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("cannot modify memory targetNode from '%d' to '%d'"),
+ oldDef->targetNode, newDef->targetNode);
+return false;
+}
+
+if (oldDef->size != newDef->size) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("cannot modify memory size from '%llu' to '%llu'"),
+ oldDef->size, newDef->size);
+return false
Re: [PATCH 00/18] qapi/qom: QAPIfy object-add
On 30/11/20 19:10, Kevin Wolf wrote: Am 30.11.2020 um 17:57 hat Paolo Bonzini geschrieben: The main problem is that it wouldn't extend well, if at all, to machines and devices. So those would still not be integrated into the QAPI schema. What do you think is the biggest difference there? Don't devices work the same as user creatable objects in that you first set a bunch of properties (which would now be done through QAPI instead) and then call the completion/realize method that actually makes use of them? For devices it's just the practical issue that there are too many to have something like this series. For machine types the main issue is that the version-specific machine types would have to be defined in one more place. The single struct doesn't bother me _too much_ actually. What bothers me is that it won't be a single source of all QOM objects, only those that happen to be created by object-add. But isn't it only natural that a list of these objects will exist in some way, implicitly or explicitly? object-add must somehow decide which object types it allows the user to create. There's already one, it's objects that implement user creatable. I don't think having it as a QAPI enum (or discriminator) is worthwhile, and it's one more thing that can go out of sync between the QAPI schema and the C code. I'm also pretty sure that QOM as it exists now is not the right solution for any of them because it has some major shortcomings. It's too easy to get things wrong (like the writable properties after creation), its introspection is rather weak and separated from the QAPI introspection, it doesn't encourage documentation, and it involves quite a bit of boilerplate and duplicated code between class implementations. A modified QOM might be the right solution, though. I would like to bring QAPI and QOM together because most of these weaknesses are strengths of QAPI. I agree wholeheartedly. But your series goes to the opposite excess. Eduardo is doing work in QOM to mitigate the issues you point out, and you have to meet in the middle with him. Starting with the user-visible parts focuses on the irrelevant parts. Mostly because it's more or less the same issue that you have with BlockdevOptions, with the extra complication that this series only deals with the easy one of the four above categories. I'm not sure which exact problem with BlockdevOptions you mean. The reason why the block layer doesn't use BlockdevOptions everywhere is -drive support. You don't have a single BlockdevOptions* field in the structs of block/. My interpretation of this is that BlockdevOptions* is a good schema for configuring things but not for run-time state. Maybe if we don't want to commit to keeping the ObjectOptions schema, the part that should wait is object-add and I should do the command line options first? Then the schema remains an implementation detail for now that is invisible in introspection. I don't see much benefit in converting _any_ of the three actually. The only good reason I see for QAPIfying this is the documentation, and the promise of deduplicating QOM boilerplate. The latter is only a promise, but documentation alone is a damn good reason and it's enough to get this work into a mergeable shape as soon as possible! I think having some input validation done by QAPI instead of in each QOM object individually is nice, too. You get it after CLI, QMP and HMP all go through QAPI. But the right way to do that is to use Eduardo's field properties: they make QOM do the right thing, adding another layer of parsing is just adding more complexity. Are there any validation bugs that you're fixing? Is that something that cannot be fixed elsewhere, or are you papering over bad QOM coding? (Again, I'm not debating that QOM properties are hard to write correctly). But maybe, we could start in the opposite direction: start with the use QAPI to eliminate QOM boilerplate. Basing your work on Eduardo's field properties series, you could add a new 'object' "kind" to QAPI that would create an array of field properties (e.g. a macro expanding to a compound literal?) There is a very simple reason why I don't want to start with the QAPI generator: John has multiple series pending that touch basically every part of the QAPI generator. This means not only that I need to be careful about merge conflict (or base my work on top of five other series, which feels adventurous), but also that I would be competing with John for Markus' reviewer capacity, further slowing things down. That's something that you have to discuss with John and Markus. It may be that John has to shelve part of his work or rebase on top of yours instead. By adding an extra parsing layer you're adding complexity that may not be needed in the end, and we're very bad of removing it later. So I'm worried that this series will become technical debt in just a few months. Well, two reasons: Also bec
Re: [PATCH v1 04/26] domain_conf: Check NVDIMM UUID in ABI stability
On 11/27/20 12:02 PM, Michal Privoznik wrote:
The UUID is guest visible and thus shouldn't change if we want to
not break guest ABI.
Fixes: 08ed673901bb5b4f419b37bcce9b11d31ce370e6
Thanks for fixing this up.
Reviewed-by: Daniel Henrique Barboza
Signed-off-by: Michal Privoznik
---
src/conf/domain_conf.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index b1534dcc1e..631165c3fa 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -24238,6 +24238,12 @@
virDomainMemoryDefCheckABIStability(virDomainMemoryDefPtr src,
"source NVDIMM readonly flag"));
return false;
}
+
+if (memcmp(src->uuid, dst->uuid, VIR_UUID_BUFLEN) != 0) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Target NVDIMM UUID doesn't match source
NVDIMM"));
+return false;
+}
}
return virDomainDeviceInfoCheckABIStability(&src->info, &dst->info);
Re: [libvirt PATCH 0/9] qemu: report guest disks informations
On 11/20/20 7:09 PM, marcandre.lur...@redhat.com wrote: From: Marc-André Lureau Hi, The following series extends virDomainGetGuestInfo to report disk informations provided by the new QGA "guest-get-disks" command in QEMU 5.2. Please review, Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1899527 Marc-André Lureau (9): qemu_agent: rename qemuAgentDiskInfo->qemuAgentDiskAddress qemu_agent: export qemuAgentDiskAddressFree & add g_auto qemu_agent: factor out qemuAgentGetDiskAddress util: json: add virJSONValueObjectGetStringArray convenience qemu: use virJSONValueObjectGetStringArray qemu_agent: add qemuAgentGetDisks domain: add disk informations to virDomainGetGuestInfo qemu_driver: report guest disk informations virsh: add --disk informations to guestinfo command include/libvirt/libvirt-domain.h | 1 + src/libvirt-domain.c | 17 +++ src/libvirt_private.syms | 1 + src/qemu/qemu_agent.c| 182 +++ src/qemu/qemu_agent.h| 25 - src/qemu/qemu_driver.c | 103 - src/qemu/qemu_monitor_json.c | 34 +- src/util/virjson.c | 30 + src/util/virjson.h | 1 + tests/qemuagenttest.c| 111 +++ tools/virsh-domain.c | 6 + 11 files changed, 430 insertions(+), 81 deletions(-) Patches look good. A huge sorry for letting these slip the release. To make it up to you, I'm proposing couple of adjustments and if you agree I will squash in proposed diffs before pushing - so that you don't have to send v2. Tentative: Reviewed-by: Michal Privoznik Michal
Re: [libvirt PATCH 5/9] qemu: use virJSONValueObjectGetStringArray
On 11/20/20 7:09 PM, marcandre.lur...@redhat.com wrote:
From: Marc-André Lureau
There might be more potential users around, I haven't looked thoroughly.
Quick git grep showed two more: qemuAgentSSHGetAuthorizedKeys() and
qemuMonitorJSONGetStringListProperty(). But that can be done in a
separate patch.
Signed-off-by: Marc-André Lureau
---
src/qemu/qemu_monitor_json.c | 34 ++
1 file changed, 6 insertions(+), 28 deletions(-)
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 723bdb4426..3588a0c426 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -7533,10 +7533,7 @@ qemuMonitorJSONGetStringArray(qemuMonitorPtr mon, const
char *qmpCmd,
int ret = -1;
virJSONValuePtr cmd;
virJSONValuePtr reply = NULL;
-virJSONValuePtr data;
-char **list = NULL;
-size_t n = 0;
-size_t i;
+g_auto(GStrv) list = NULL;
*array = NULL;
@@ -7554,32 +7551,13 @@ qemuMonitorJSONGetStringArray(qemuMonitorPtr mon, const char *qmpCmd,
if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_ARRAY) < 0)
goto cleanup;
-data = virJSONValueObjectGetArray(reply, "return");
-n = virJSONValueArraySize(data);
-
-/* null-terminated list */
-list = g_new0(char *, n + 1);
-
-for (i = 0; i < n; i++) {
-virJSONValuePtr child = virJSONValueArrayGet(data, i);
-const char *tmp;
-
-if (!(tmp = virJSONValueGetString(child))) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
- _("%s array element does not contain data"),
- qmpCmd);
-goto cleanup;
-}
-
-list[i] = g_strdup(tmp);
-}
-
-ret = n;
-*array = list;
-list = NULL;
+list = virJSONValueObjectGetStringArray(reply, "return");
We can ditch the @list variable and assign to *array directly.
+if (!list)
+goto cleanup;
+ret = g_strv_length(list);
+*array = g_steal_pointer(&list);
cleanup:
-g_strfreev(list);
virJSONValueFree(cmd);
virJSONValueFree(reply);
return ret;
How about this squashed in:
diff --git c/src/qemu/qemu_monitor_json.c i/src/qemu/qemu_monitor_json.c
index 3588a0c426..e7aa6b6ffd 100644
--- c/src/qemu/qemu_monitor_json.c
+++ i/src/qemu/qemu_monitor_json.c
@@ -7533,7 +7533,6 @@ qemuMonitorJSONGetStringArray(qemuMonitorPtr mon,
const char *qmpCmd,
int ret = -1;
virJSONValuePtr cmd;
virJSONValuePtr reply = NULL;
-g_auto(GStrv) list = NULL;
*array = NULL;
@@ -7551,11 +7550,10 @@ qemuMonitorJSONGetStringArray(qemuMonitorPtr
mon, const char *qmpCmd,
if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_ARRAY) < 0)
goto cleanup;
-list = virJSONValueObjectGetStringArray(reply, "return");
-if (!list)
+if (!(*array = virJSONValueObjectGetStringArray(reply, "return")))
goto cleanup;
-ret = g_strv_length(list);
-*array = g_steal_pointer(&list);
+
+ret = g_strv_length(*array);
cleanup:
virJSONValueFree(cmd);
Michal
Re: [libvirt PATCH 9/9] virsh: add --disk informations to guestinfo command
On 11/20/20 7:09 PM, marcandre.lur...@redhat.com wrote: From: Marc-André Lureau Signed-off-by: Marc-André Lureau --- tools/virsh-domain.c | 6 ++ 1 file changed, 6 insertions(+) Missing manpage addition. How about: diff --git i/docs/manpages/virsh.rst w/docs/manpages/virsh.rst index 86deaa486d..9ef6b68422 100644 --- i/docs/manpages/virsh.rst +++ w/docs/manpages/virsh.rst @@ -2679,6 +2679,7 @@ guestinfo :: guestinfo domain [--user] [--os] [--timezone] [--hostname] [--filesystem] + [--disks] Print information about the guest from the point of view of the guest agent. Note that this command requires a guest agent to be configured and running in @@ -2689,7 +2690,7 @@ are supported by the guest agent. You can limit the types of information that are returned by specifying one or more flags. If a requested information type is not supported, the processes will provide an exit code of 1. Available information types flags are *--user*, *--os*, -*--timezone*, *--hostname*, and *--filesystem*. +*--timezone*, *--hostname*, *--filesystem* and *--disks*. Note that depending on the hypervisor type and the version of the guest agent running within the domain, not all of the following information may be @@ -2746,6 +2747,16 @@ returned: * ``fs..disk..serial`` - the serial number of disk * ``fs..disk..device`` - the device node of disk +*--disks* returns: + +* ``disks.count`` - the number of disks defined on this domain +* ``disks..name`` - device node (Linux) or device UNC (Windows) +* ``disks..partition`` - whether this is a partition or disk +* ``disks..dependencies.count`` - the number of device dependencies +* ``disks..dependencies..name`` - a dependency name +* ``disks..alias`` - the device alias of the disk (e.g. sda) +* ``disks..guest_alias`` - optional alias assigned to the disk + guestvcpus -- Michal
Re: [PATCH 00/18] qapi/qom: QAPIfy object-add
On Mon, Nov 30, 2020 at 13:25:20 +0100, Kevin Wolf wrote: > This series adds a QAPI type for the properties of all user creatable > QOM types and finally makes QMP object-add use the new ObjectOptions > union so that QAPI introspection can be used for user creatable objects. FYI, here's a libvirt patchset which (apart from refactors) adds compatibility with the deprecated/removed 'props' sub-object and also adds testing to see whether all objects used by libvirt conform to the new schema: https://www.redhat.com/archives/libvir-list/2020-November/msg01625.html Spoiler alert: surprisingly they do match so no updates are necessary!
[PATCH 17/24] tests: qemuxml2argv: Validate generation of JSON props for object-add
Similarly to the validation for blockdev-add and netdev_add, use the
qemuxml2argv test repository to drive validation of props for
object-add.
Signed-off-by: Peter Krempa
---
tests/qemuxml2argvtest.c | 20
1 file changed, 20 insertions(+)
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 225cb70edf..fafc214e57 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -526,6 +526,7 @@ testCompareXMLToArgvValidateSchema(virQEMUDriverPtr drv,
g_autoptr(virCommand) cmd = NULL;
unsigned int parseFlags = info->parseFlags;
bool netdevQAPIfied = false;
+bool objectQAPIfied = false;
if (info->schemafile)
schema = testQEMUSchemaLoad(info->schemafile);
@@ -560,6 +561,7 @@ testCompareXMLToArgvValidateSchema(virQEMUDriverPtr drv,
return -1;
netdevQAPIfied =
!virQEMUQAPISchemaPathExists("netdev_add/arg-type/type/!string", schema);
+objectQAPIfied =
virQEMUQAPISchemaPathExists("object-add/arg-type/qom-type/^secret", schema);
for (i = 0; i < nargs; i++) {
g_auto(virBuffer) debug = VIR_BUFFER_INITIALIZER;
@@ -593,6 +595,24 @@ testCompareXMLToArgvValidateSchema(virQEMUDriverPtr drv,
return -1;
}
+i++;
+} else if (STREQ(args[i], "-object")) {
+
+if (!objectQAPIfied) {
+i++;
+continue;
+}
+
+if (!(jsonargs = virJSONValueFromString(args[i + 1])))
+return -1;
+
+if (testQEMUSchemaValidateCommand("object-add", jsonargs,
+ schema, false, false, &debug) <
0) {
+VIR_TEST_VERBOSE("failed to validate -object '%s' against QAPI
schema: %s",
+ args[i + 1], virBufferCurrentContent(&debug));
+return -1;
+}
+
i++;
}
}
--
2.28.0
[PATCH 13/24] qemu: command: Generate commandline of 'masterKey0' secret via JSON
While the 'masterKey0' secret object will never be hotplugged we want to generate it through JSON so that we'll be able to validate all parameters of '-object' against the QAPI schema once 'object-add' is qapified in qemu. Signed-off-by: Peter Krempa --- src/qemu/qemu_command.c | 12 ++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index f0798b5595..86b3c3d642 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -194,6 +194,7 @@ qemuBuildMasterKeyCommandLine(virCommandPtr cmd, g_autofree char *alias = NULL; g_autofree char *path = NULL; g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; +g_autoptr(virJSONValue) props = NULL; /* If the -object secret does not exist, then just return. This just * means the domain won't be able to use a secret master key and is @@ -215,9 +216,16 @@ qemuBuildMasterKeyCommandLine(virCommandPtr cmd, if (!(path = qemuDomainGetMasterKeyFilePath(priv->libDir))) return -1; +if (qemuMonitorCreateObjectProps(&props, "secret", alias, + "s:format", "raw", + "s:file", path, + NULL) < 0) +return -1; + +if (virQEMUBuildObjectCommandlineFromJSON(&buf, props) < 0) +return -1; + virCommandAddArg(cmd, "-object"); -virBufferAsprintf(&buf, "secret,id=%s,format=raw,file=", alias); -virQEMUBuildBufferEscapeComma(&buf, path); virCommandAddArgBuffer(cmd, &buf); return 0; -- 2.28.0
[PATCH 20/24] qemuMonitorCreateObjectPropsWrap: Open-code in qemuBuildMemoryBackendProps
There's just one caller left. Since qemuBuildMemoryBackendProps is too
complex to be modified for now, just move the adding of 'id' and 'qom'
type directly into the function.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_command.c | 6 --
src/qemu/qemu_monitor.c | 15 ---
src/qemu/qemu_monitor.h | 4
3 files changed, 4 insertions(+), 21 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 4050e1e7af..e21cbb451e 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3206,10 +3206,12 @@ qemuBuildMemoryBackendProps(virJSONValuePtr
*backendProps,
rc = 0;
}
-if (!(*backendProps = qemuMonitorCreateObjectPropsWrap(backendType, alias,
- &props)))
+if (virJSONValueObjectPrependString(props, "id", alias) < 0 ||
+virJSONValueObjectPrependString(props, "qom-type", backendType) < 0)
return -1;
+*backendProps = g_steal_pointer(&props);
+
return rc;
}
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 5da3213976..e0eee3ce9e 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -3008,21 +3008,6 @@ qemuMonitorAddDeviceArgs(qemuMonitorPtr mon,
}
-virJSONValuePtr
-qemuMonitorCreateObjectPropsWrap(const char *type,
- const char *alias,
- virJSONValuePtr *props)
-{
-
-if (virJSONValueObjectPrependString(*props, "id", alias) < 0 ||
-virJSONValueObjectPrependString(*props, "qom-type", type))
-return NULL;
-
-return g_steal_pointer(props);
-}
-
-
-
/**
* qemuMonitorCreateObjectProps:
* @propsret: returns full object properties
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index a7fba393bf..e02e131e3b 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -1009,10 +1009,6 @@ int qemuMonitorAddDeviceWithFd(qemuMonitorPtr mon,
int qemuMonitorDelDevice(qemuMonitorPtr mon,
const char *devalias);
-virJSONValuePtr qemuMonitorCreateObjectPropsWrap(const char *type,
- const char *alias,
- virJSONValuePtr *props);
-
int qemuMonitorCreateObjectProps(virJSONValuePtr *propsret,
const char *type,
const char *alias,
--
2.28.0
[PATCH 24/24] [DONTMERGE] qemuxml2argvtest: Force QMP validation with latest caps
Note that it's expected that the 'vxhs' and 'sev-guest' test cases fail
as this overrides just the QMP schema but not the qemu capabilities
which result in enabling the features that are missing.
---
tests/qemuxml2argvtest.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index fafc214e57..a73408c029 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -531,9 +531,9 @@ testCompareXMLToArgvValidateSchema(virQEMUDriverPtr drv,
if (info->schemafile)
schema = testQEMUSchemaLoad(info->schemafile);
-/* comment out with line comment to enable schema checking for non _CAPS
tests
-if (!schema)
-schema = testQEMUSchemaLoadLatest(virArchToString(info->arch));
+///* comment out with line comment to enable schema checking for non _CAPS
tests
+//if (!schema)
+schema = testQEMUSchemaLoadLatest("x86_64");
// */
if (!schema)
--
2.28.0
[PATCH 22/24] qemumonitorjsontest: Remove bomb guarding object-add
Libvirt is now prepared for QAPIfied object-add.
Signed-off-by: Peter Krempa
---
tests/qemumonitorjsontest.c | 14 --
1 file changed, 14 deletions(-)
diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c
index 5a3926bb11..47c909fd7a 100644
--- a/tests/qemumonitorjsontest.c
+++ b/tests/qemumonitorjsontest.c
@@ -2906,20 +2906,6 @@ testQAPISchemaObjectDeviceAdd(const void *opaque)
return -1;
}
-if (virQEMUQAPISchemaPathGet("object-add/arg-type", schema, &entry) < 0) {
-fprintf(stderr, "schema for 'objectadd' not found\n");
-return -1;
-}
-
-if (testQEMUSchemaEntryMatchTemplate(entry,
- "str:qom-type",
- "str:id",
- "any:props",
- NULL) < 0) {
-VIR_TEST_VERBOSE("object-add has unexpected members in schema");
-return -1;
-}
-
return 0;
}
--
2.28.0
[PATCH 21/24] qemu: monitor: Don't add 'props' wrapper if qemu has QEMU_CAPS_OBJECT_QAPIFIED
Set 'objectAddNoWrap' when the capability is present.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_monitor.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index e0eee3ce9e..145c03413f 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -32,6 +32,7 @@
#include "qemu_monitor_json.h"
#include "qemu_domain.h"
#include "qemu_process.h"
+#include "qemu_capabilities.h"
#include "virerror.h"
#include "viralloc.h"
#include "virlog.h"
@@ -676,6 +677,7 @@ qemuMonitorOpenInternal(virDomainObjPtr vm,
qemuMonitorCallbacksPtr cb,
void *opaque)
{
+qemuDomainObjPrivatePtr priv = vm->privateData;
qemuMonitorPtr mon;
g_autoptr(GError) gerr = NULL;
@@ -708,6 +710,9 @@ qemuMonitorOpenInternal(virDomainObjPtr vm,
mon->cb = cb;
mon->callbackOpaque = opaque;
+if (priv)
+mon->objectAddNoWrap = virQEMUCapsGet(priv->qemuCaps,
QEMU_CAPS_OBJECT_QAPIFIED);
+
if (virSetCloseExec(mon->fd) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
"%s", _("Unable to set monitor close-on-exec flag"));
--
2.28.0
[PATCH 19/24] qemu: monitor: Make wrapping of 'props' of 'object-add' optional
Construct the JSON object which is used for object-add without the
'props' wrapper and add the wrapper only in the monitor code.
This simplifies the JSON->commandline generator in the first place and
also prepares for upcoming qemu where 'props' will be removed.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_monitor.c | 68 +
src/util/virqemu.c | 42 +
2 files changed, 62 insertions(+), 48 deletions(-)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index b26fba303d..5da3213976 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -112,6 +112,9 @@ struct _qemuMonitor {
qemuMonitorReportDomainLogError logFunc;
void *logOpaque;
virFreeCallback logDestroy;
+
+/* true if qemu no longer wants 'props' sub-object of object-add */
+bool objectAddNoWrap;
};
/**
@@ -3010,14 +3013,12 @@ qemuMonitorCreateObjectPropsWrap(const char *type,
const char *alias,
virJSONValuePtr *props)
{
-virJSONValuePtr ret;
-ignore_value(virJSONValueObjectCreate(&ret,
- "s:qom-type", type,
- "s:id", alias,
- "A:props", props,
- NULL));
-return ret;
+if (virJSONValueObjectPrependString(*props, "id", alias) < 0 ||
+virJSONValueObjectPrependString(*props, "qom-type", type))
+return NULL;
+
+return g_steal_pointer(props);
}
@@ -3037,26 +3038,28 @@ qemuMonitorCreateObjectProps(virJSONValuePtr *propsret,
const char *alias,
...)
{
-virJSONValuePtr props = NULL;
-int ret = -1;
+g_autoptr(virJSONValue) props = NULL;
+int rc;
va_list args;
-*propsret = NULL;
+if (virJSONValueObjectCreate(&props,
+ "s:qom-type", type,
+ "s:id", alias,
+ NULL) < 0)
+return -1;
+
va_start(args, alias);
-if (virJSONValueObjectCreateVArgs(&props, args) < 0)
-goto cleanup;
+rc = virJSONValueObjectAddVArgs(props, args);
-if (!(*propsret = qemuMonitorCreateObjectPropsWrap(type, alias, &props)))
-goto cleanup;
+va_end(args);
-ret = 0;
+if (rc < 0)
+return -1;
- cleanup:
-virJSONValueFree(props);
-va_end(args);
-return ret;
+*propsret = g_steal_pointer(&props);
+return 0;
}
@@ -3076,6 +3079,7 @@ qemuMonitorAddObject(qemuMonitorPtr mon,
virJSONValuePtr *props,
char **alias)
{
+g_autoptr(virJSONValue) pr = NULL;
const char *type = NULL;
const char *id = NULL;
g_autofree char *aliasCopy = NULL;
@@ -3103,7 +3107,31 @@ qemuMonitorAddObject(qemuMonitorPtr mon,
if (alias)
aliasCopy = g_strdup(id);
-if (qemuMonitorJSONAddObject(mon, props) < 0)
+if (mon->objectAddNoWrap) {
+pr = g_steal_pointer(props);
+} else {
+/* we need to create a wrapper which has the 'qom-type' and 'id' and
+ * store everything else under a 'props' sub-object */
+g_autoptr(virJSONValue) typeobj = NULL;
+g_autoptr(virJSONValue) idobj = NULL;
+
+ignore_value(virJSONValueObjectRemoveKey(*props, "qom-type",
&typeobj));
+ignore_value(virJSONValueObjectRemoveKey(*props, "id", &idobj));
+
+if (!virJSONValueObjectGetKey(*props, 0)) {
+virJSONValueFree(*props);
+*props = NULL;
+}
+
+if (virJSONValueObjectCreate(&pr,
+ "s:qom-type", type,
+ "s:id", id,
+ "A:props", props,
+ NULL) < 0)
+return -1;
+}
+
+if (qemuMonitorJSONAddObject(mon, &pr) < 0)
return -1;
if (alias)
diff --git a/src/util/virqemu.c b/src/util/virqemu.c
index fd8661951a..8a705b79a0 100644
--- a/src/util/virqemu.c
+++ b/src/util/virqemu.c
@@ -320,32 +320,6 @@ virQEMUBuildNetdevCommandlineFromJSON(virJSONValuePtr
props,
}
-static int
-virQEMUBuildObjectCommandlineFromJSONInternal(virBufferPtr buf,
- const char *type,
- const char *alias,
- virJSONValuePtr props)
-{
-if (!type || !alias) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
- _("missing 'type'(%s) or 'alias'(%s) field of QOM
'object'"),
- NULLSTR(type), NULLSTR(alias));
-return -1;
-}
-
-virBufferAsprintf(buf, "%s,id=%s", type, alias);
-
-if (props) {
-virBufferAddLit(buf, ",");
-if (virQEMUBuildCommandLineJSON(props, buf, NULL, false,
-
[PATCH 16/24] qemu: capabilities: Introduce QEMU_CAPS_OBJECT_QAPIFIED
Starting from qemu-6.0 the parameters of -object/object-add are formally
described by the QAPI schema. Additionally this changes the nesting of
the properties as the 'props' nested object will be flattened to the
parent.
We'll need to detect whether qemu switched to this new approach to
generate the objects with proper nesting and also allow testing.
The capability is based on the presence of the 'secret' object in the
'qom-type' enum.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_capabilities.c | 2 ++
src/qemu/qemu_capabilities.h | 1 +
2 files changed, 3 insertions(+)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 538551e772..e2ee418f39 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -609,6 +609,7 @@ VIR_ENUM_IMPL(virQEMUCaps,
"ncr53c90",
"dc390",
"am53c974",
+ "object.qapified"
);
@@ -1533,6 +1534,7 @@ static struct virQEMUCapsStringFlags
virQEMUCapsQMPSchemaQueries[] = {
{ "migrate-set-parameters/arg-type/xbzrle-cache-size",
QEMU_CAPS_MIGRATION_PARAM_XBZRLE_CACHE_SIZE },
{ "set-numa-node/arg-type/+hmat-lb", QEMU_CAPS_NUMA_HMAT },
{ "netdev_add/arg-type/+vhost-vdpa", QEMU_CAPS_NETDEV_VHOST_VDPA },
+{ "object-add/arg-type/qom-type/^secret", QEMU_CAPS_OBJECT_QAPIFIED },
};
typedef struct _virQEMUCapsObjectTypeProps virQEMUCapsObjectTypeProps;
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index 0f90efa459..d942c5536d 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -589,6 +589,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for
syntax-check */
QEMU_CAPS_SCSI_NCR53C90, /* built-in SCSI */
QEMU_CAPS_SCSI_DC390, /* -device dc-390 */
QEMU_CAPS_SCSI_AM53C974, /* -device am53c974 */
+QEMU_CAPS_OBJECT_QAPIFIED, /* parameters for object-add are formally
described */
QEMU_CAPS_LAST /* this must always be the last item */
} virQEMUCapsFlags;
--
2.28.0
[PATCH 09/24] util: json: Replace virJSONValueObjectSteal by virJSONValueObjectRemoveKey
virJSONValueObjectRemoveKey can be used as direct replacement. Fix the
one caller and remove the duplicate function.
Signed-off-by: Peter Krempa
---
src/util/virjson.c | 29 -
1 file changed, 4 insertions(+), 25 deletions(-)
diff --git a/src/util/virjson.c b/src/util/virjson.c
index 3fa9a9ca24..9b01e28eb5 100644
--- a/src/util/virjson.c
+++ b/src/util/virjson.c
@@ -883,30 +883,6 @@ virJSONValueObjectGet(virJSONValuePtr object,
}
-static virJSONValuePtr
-virJSONValueObjectSteal(virJSONValuePtr object,
-const char *key)
-{
-size_t i;
-virJSONValuePtr obj = NULL;
-
-if (object->type != VIR_JSON_TYPE_OBJECT)
-return NULL;
-
-for (i = 0; i < object->data.object.npairs; i++) {
-if (STREQ(object->data.object.pairs[i].key, key)) {
-obj = g_steal_pointer(&object->data.object.pairs[i].value);
-VIR_FREE(object->data.object.pairs[i].key);
-VIR_DELETE_ELEMENT(object->data.object.pairs, i,
- object->data.object.npairs);
-break;
-}
-}
-
-return obj;
-}
-
-
/* Return the value associated with KEY within OBJECT, but return NULL
* if the key is missing or if value is not the correct TYPE. */
virJSONValuePtr
@@ -929,7 +905,10 @@ virJSONValueObjectStealByType(virJSONValuePtr object,
const char *key,
virJSONType type)
{
-virJSONValuePtr value = virJSONValueObjectSteal(object, key);
+virJSONValuePtr value;
+
+if (virJSONValueObjectRemoveKey(object, key, &value) <= 0)
+return NULL;
if (value && value->type == type)
return value;
--
2.28.0
[PATCH 10/24] tests: qemuxml2argv: Don't check whether -netdev was QAPIfied repeatedly
Check once before looping through the args.
Signed-off-by: Peter Krempa
---
tests/qemuxml2argvtest.c | 12
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 42d147243e..ae8eaa7768 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -524,6 +524,7 @@ testCompareXMLToArgvValidateSchema(virQEMUDriverPtr drv,
g_autoptr(GHashTable) schema = NULL;
g_autoptr(virCommand) cmd = NULL;
unsigned int parseFlags = info->parseFlags;
+bool netdevQAPIfied = false;
if (info->schemafile)
schema = testQEMUSchemaLoad(info->schemafile);
@@ -552,6 +553,8 @@ testCompareXMLToArgvValidateSchema(virQEMUDriverPtr drv,
if (virCommandGetArgList(cmd, &args, &nargs) < 0)
return -1;
+netdevQAPIfied =
!virQEMUQAPISchemaPathExists("netdev_add/arg-type/type/!string", schema);
+
for (i = 0; i < nargs; i++) {
g_auto(virBuffer) debug = VIR_BUFFER_INITIALIZER;
g_autoptr(virJSONValue) jsonargs = NULL;
@@ -569,13 +572,14 @@ testCompareXMLToArgvValidateSchema(virQEMUDriverPtr drv,
i++;
} else if (STREQ(args[i], "-netdev")) {
+if (!netdevQAPIfied) {
+i++;
+continue;
+}
+
if (!(jsonargs = virJSONValueFromString(args[i + 1])))
return -1;
-/* skip the validation for pre-QAPIfication cases */
-if
(virQEMUQAPISchemaPathExists("netdev_add/arg-type/type/!string", schema))
-continue;
-
if (testQEMUSchemaValidateCommand("netdev_add", jsonargs,
schema, false, false, &debug) <
0) {
VIR_TEST_VERBOSE("failed to validate -netdev '%s' against QAPI
schema: %s",
--
2.28.0
[PATCH 18/24] qemu: command: Introduce raw JSON passthrough for '-object' for testing
The qemu commandline builder's QEMU_BUILD_COMMANDLINE_VALIDATE_KEEP_JSON
flag disables JSON->commandline conversion so that our qemuxml2argvtest
can use the commandline test repostitory for validating our JSON props
generators which are in many cases used on the montitor where we need to
conform to the schema.
Wire this up for the -object/object-add pair by adding a flag to
virQEMUBuildObjectCommandlineFromJSON to pass through JSON and pipe
through the 'flags' variable where necessary.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_command.c | 264
src/util/virqemu.c | 16 ++-
src/util/virqemu.h | 3 +-
3 files changed, 176 insertions(+), 107 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 4dde946e82..4050e1e7af 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -189,7 +189,8 @@ VIR_ENUM_IMPL(qemuNumaPolicy,
*/
static int
qemuBuildMasterKeyCommandLine(virCommandPtr cmd,
- qemuDomainObjPrivatePtr priv)
+ qemuDomainObjPrivatePtr priv,
+ unsigned int flags)
{
g_autofree char *alias = NULL;
g_autofree char *path = NULL;
@@ -222,7 +223,8 @@ qemuBuildMasterKeyCommandLine(virCommandPtr cmd,
NULL) < 0)
return -1;
-if (virQEMUBuildObjectCommandlineFromJSON(&buf, props) < 0)
+if (virQEMUBuildObjectCommandlineFromJSON(&buf, props,
+ (flags &
QEMU_BUILD_COMMANDLINE_VALIDATE_KEEP_JSON)) < 0)
return -1;
virCommandAddArg(cmd, "-object");
@@ -677,6 +679,7 @@ qemuBuildSecretInfoProps(qemuDomainSecretInfoPtr secinfo,
* qemuBuildObjectSecretCommandLine:
* @cmd: the command to modify
* @secinfo: pointer to the secret info object
+ * @flags: commandline builder flags
*
* If the secinfo is available and associated with an AES secret,
* then format the command line for the secret object. This object
@@ -687,7 +690,8 @@ qemuBuildSecretInfoProps(qemuDomainSecretInfoPtr secinfo,
*/
static int
qemuBuildObjectSecretCommandLine(virCommandPtr cmd,
- qemuDomainSecretInfoPtr secinfo)
+ qemuDomainSecretInfoPtr secinfo,
+ unsigned int flags)
{
g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
g_autoptr(virJSONValue) props = NULL;
@@ -695,7 +699,8 @@ qemuBuildObjectSecretCommandLine(virCommandPtr cmd,
if (qemuBuildSecretInfoProps(secinfo, &props) < 0)
return -1;
-if (virQEMUBuildObjectCommandlineFromJSON(&buf, props) < 0)
+if (virQEMUBuildObjectCommandlineFromJSON(&buf, props,
+ (flags &
QEMU_BUILD_COMMANDLINE_VALIDATE_KEEP_JSON)) < 0)
return -1;
virCommandAddArg(cmd, "-object");
@@ -842,6 +847,7 @@ qemuBuildTLSx509BackendProps(const char *tlspath,
* (optional)
* @alias: TLS object alias
* @qemuCaps: capabilities
+ * @flags: commandline builder flags
*
* Create the command line for a TLS object
*
@@ -854,7 +860,8 @@ qemuBuildTLSx509CommandLine(virCommandPtr cmd,
bool verifypeer,
const char *certEncSecretAlias,
const char *alias,
-virQEMUCapsPtr qemuCaps)
+virQEMUCapsPtr qemuCaps,
+unsigned int flags)
{
g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
g_autoptr(virJSONValue) props = NULL;
@@ -863,7 +870,8 @@ qemuBuildTLSx509CommandLine(virCommandPtr cmd,
certEncSecretAlias, qemuCaps, &props) < 0)
return -1;
-if (virQEMUBuildObjectCommandlineFromJSON(&buf, props) < 0)
+if (virQEMUBuildObjectCommandlineFromJSON(&buf, props,
+ (flags &
QEMU_BUILD_COMMANDLINE_VALIDATE_KEEP_JSON)) < 0)
return -1;
virCommandAddArg(cmd, "-object");
@@ -1938,14 +1946,16 @@
qemuBuildFloppyCommandLineControllerOptions(virCommandPtr cmd,
static int
qemuBuildObjectCommandline(virCommandPtr cmd,
- virJSONValuePtr objProps)
+ virJSONValuePtr objProps,
+ unsigned int flags)
{
g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
if (!objProps)
return 0;
-if (virQEMUBuildObjectCommandlineFromJSON(&buf, objProps) < 0)
+if (virQEMUBuildObjectCommandlineFromJSON(&buf, objProps,
+ (flags &
QEMU_BUILD_COMMANDLINE_VALIDATE_KEEP_JSON)) < 0)
return -1;
virCommandAddArg(cmd, "-object");
@@ -1957,16 +1967,17 @@ qemuBuildObjectCommandline(virCommandPtr cmd,
static int
qemuBuildBlockStorageSourceAttachDataCommandline(virCommandPtr cmd,
-
[PATCH 15/24] qemu: command: Generate commandline of iothread objects JSON
The commandline generator for 'iothread' objects has a private
implementation of the properties. Convert it to JSON so that it can be
later validated.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_command.c | 18 +++---
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index feec884e85..4dde946e82 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7137,15 +7137,19 @@ qemuBuildIOThreadCommandLine(virCommandPtr cmd,
if (def->niothreadids == 0)
return 0;
-/* Create iothread objects using the defined iothreadids list
- * and the defined id and name from the list. These may be used
- * by a disk definition which will associate to an iothread by
- * supplying a value of an id from the list
- */
for (i = 0; i < def->niothreadids; i++) {
+g_autoptr(virJSONValue) props = NULL;
+g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
+g_autofree char *alias = g_strdup_printf("iothread%u",
def->iothreadids[i]->iothread_id);
+
+if (qemuMonitorCreateObjectProps(&props, "iothread", alias, NULL) < 0)
+return -1;
+
+if (virQEMUBuildObjectCommandlineFromJSON(&buf, props) < 0)
+return -1;
+
virCommandAddArg(cmd, "-object");
-virCommandAddArgFormat(cmd, "iothread,id=iothread%u",
- def->iothreadids[i]->iothread_id);
+virCommandAddArgBuffer(cmd, &buf);
}
return 0;
--
2.28.0
[PATCH 08/24] qemuMonitorAddObject: Refactor cleanup
Remove freeing/clearing of @props as the function doesn't guarantee that
it happens on success, rename the variable hodling copy of the alias and
use g_autofree to automatically free it and remove the cleanup label as
well as 'ret' variable.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_monitor.c | 23 ---
1 file changed, 8 insertions(+), 15 deletions(-)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index c4fa111836..b26fba303d 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -3078,13 +3078,12 @@ qemuMonitorAddObject(qemuMonitorPtr mon,
{
const char *type = NULL;
const char *id = NULL;
-char *tmp = NULL;
-int ret = -1;
+g_autofree char *aliasCopy = NULL;
if (!*props) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("object props can't be NULL"));
-goto cleanup;
+return -1;
}
type = virJSONValueObjectGetString(*props, "qom-type");
@@ -3092,31 +3091,25 @@ qemuMonitorAddObject(qemuMonitorPtr mon,
VIR_DEBUG("type=%s id=%s", NULLSTR(type), NULLSTR(id));
-QEMU_CHECK_MONITOR_GOTO(mon, cleanup);
+QEMU_CHECK_MONITOR(mon);
if (!id || !type) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("missing alias or qom-type for qemu object '%s'"),
NULLSTR(type));
-goto cleanup;
+return -1;
}
if (alias)
-tmp = g_strdup(id);
+aliasCopy = g_strdup(id);
if (qemuMonitorJSONAddObject(mon, props) < 0)
-goto cleanup;
+return -1;
if (alias)
-*alias = g_steal_pointer(&tmp);
-
-ret = 0;
+*alias = g_steal_pointer(&aliasCopy);
- cleanup:
-VIR_FREE(tmp);
-virJSONValueFree(*props);
-*props = NULL;
-return ret;
+return 0;
}
--
2.28.0
[PATCH 07/24] qemuMonitorAddObject: Fix semantics of @alias
The callers of qemuMonitorAddObject rely on the fact that @alias is filled only when the object is added successfully. This is documented but the code didn't behave like that. Signed-off-by: Peter Krempa --- src/qemu/qemu_monitor.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 7f58c16987..c4fa111836 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -3104,11 +3104,14 @@ qemuMonitorAddObject(qemuMonitorPtr mon, if (alias) tmp = g_strdup(id); -ret = qemuMonitorJSONAddObject(mon, props); +if (qemuMonitorJSONAddObject(mon, props) < 0) +goto cleanup; if (alias) *alias = g_steal_pointer(&tmp); +ret = 0; + cleanup: VIR_FREE(tmp); virJSONValueFree(*props); -- 2.28.0
[PATCH 11/24] qemuBuildChrChardevStr: Rename 'flags' to 'cdevflags'
The monitor code uses 'flags' for the flags of the monitor builder,
while in this function it's a different set of flags. All callers pass a
variable named 'cdevflags', so rename the argument to suit.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_command.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 479bcc0b0c..f0798b5595 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -4738,7 +4738,7 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager,
const virDomainChrSourceDef *dev,
const char *alias,
virQEMUCapsPtr qemuCaps,
- unsigned int flags)
+ unsigned int cdevflags)
{
qemuDomainChrSourcePrivatePtr chrSourcePriv =
QEMU_DOMAIN_CHR_SOURCE_PRIVATE(dev);
g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
@@ -4771,7 +4771,7 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager,
case VIR_DOMAIN_CHR_TYPE_FILE:
virBufferAsprintf(&buf, "file,id=%s", charAlias);
-if (qemuBuildChrChardevFileStr(flags & QEMU_BUILD_CHARDEV_FILE_LOGD ?
+if (qemuBuildChrChardevFileStr(cdevflags &
QEMU_BUILD_CHARDEV_FILE_LOGD ?
logManager : NULL,
cmd, def, &buf,
"path", dev->data.file.path,
@@ -4820,7 +4820,7 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager,
if (dev->data.tcp.listen) {
virBufferAddLit(&buf, ",server");
-if (flags & QEMU_BUILD_CHARDEV_TCP_NOWAIT)
+if (cdevflags & QEMU_BUILD_CHARDEV_TCP_NOWAIT)
virBufferAddLit(&buf, ",nowait");
}
@@ -4860,7 +4860,7 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager,
case VIR_DOMAIN_CHR_TYPE_UNIX:
virBufferAsprintf(&buf, "socket,id=%s", charAlias);
if (dev->data.nix.listen &&
-(flags & QEMU_BUILD_CHARDEV_UNIX_FD_PASS) &&
+(cdevflags & QEMU_BUILD_CHARDEV_UNIX_FD_PASS) &&
virQEMUCapsGet(qemuCaps, QEMU_CAPS_CHARDEV_FD_PASS)) {
int fd;
@@ -4883,7 +4883,7 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager,
}
if (dev->data.nix.listen) {
virBufferAddLit(&buf, ",server");
-if (flags & QEMU_BUILD_CHARDEV_TCP_NOWAIT)
+if (cdevflags & QEMU_BUILD_CHARDEV_TCP_NOWAIT)
virBufferAddLit(&buf, ",nowait");
}
--
2.28.0
[PATCH 14/24] qemu: command: Generate commandline of 'sev0' sev-guest object via JSON
While the 'sev0' sev-guest object will never be hotplugged, but we want
to generate it through JSON so that we'll be able to validate all
parameters of '-object' against the QAPI schema once 'object-add' is
qapified in qemu.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_command.c | 32 +++
...v-missing-platform-info.x86_64-2.12.0.args | 2 +-
.../launch-security-sev.x86_64-2.12.0.args| 2 +-
3 files changed, 20 insertions(+), 16 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 86b3c3d642..feec884e85 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -9341,9 +9341,11 @@ static int
qemuBuildSEVCommandLine(virDomainObjPtr vm, virCommandPtr cmd,
virDomainSEVDefPtr sev)
{
+g_autoptr(virJSONValue) props = NULL;
g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
qemuDomainObjPrivatePtr priv = vm->privateData;
-char *path = NULL;
+g_autofree char *dhpath = NULL;
+g_autofree char *sessionpath = NULL;
if (!sev)
return 0;
@@ -9351,21 +9353,23 @@ qemuBuildSEVCommandLine(virDomainObjPtr vm,
virCommandPtr cmd,
VIR_DEBUG("policy=0x%x cbitpos=%d reduced_phys_bits=%d",
sev->policy, sev->cbitpos, sev->reduced_phys_bits);
-virBufferAsprintf(&buf, "sev-guest,id=sev0,cbitpos=%d", sev->cbitpos);
-virBufferAsprintf(&buf, ",reduced-phys-bits=%d", sev->reduced_phys_bits);
-virBufferAsprintf(&buf, ",policy=0x%x", sev->policy);
+if (sev->dh_cert)
+dhpath = g_strdup_printf("%s/dh_cert.base64", priv->libDir);
-if (sev->dh_cert) {
-path = g_strdup_printf("%s/dh_cert.base64", priv->libDir);
-virBufferAsprintf(&buf, ",dh-cert-file=%s", path);
-VIR_FREE(path);
-}
+if (sev->session)
+sessionpath = g_strdup_printf("%s/session.base64", priv->libDir);
-if (sev->session) {
-path = g_strdup_printf("%s/session.base64", priv->libDir);
-virBufferAsprintf(&buf, ",session-file=%s", path);
-VIR_FREE(path);
-}
+if (qemuMonitorCreateObjectProps(&props, "sev-guest", "sev0",
+ "u:cbitpos", sev->cbitpos,
+ "u:reduced-phys-bits",
sev->reduced_phys_bits,
+ "u:policy", sev->policy,
+ "S:dh-cert-file", dhpath,
+ "S:session-file", sessionpath,
+ NULL) < 0)
+return -1;
+
+if (virQEMUBuildObjectCommandlineFromJSON(&buf, props) < 0)
+return -1;
virCommandAddArg(cmd, "-object");
virCommandAddArgBuffer(cmd, &buf);
diff --git
a/tests/qemuxml2argvdata/launch-security-sev-missing-platform-info.x86_64-2.12.0.args
b/tests/qemuxml2argvdata/launch-security-sev-missing-platform-info.x86_64-2.12.0.args
index 378c3b681c..9fad85737a 100644
---
a/tests/qemuxml2argvdata/launch-security-sev-missing-platform-info.x86_64-2.12.0.args
+++
b/tests/qemuxml2argvdata/launch-security-sev-missing-platform-info.x86_64-2.12.0.args
@@ -29,7 +29,7 @@ file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes \
-device piix3-usb-uhci,id=usb,bus=pci.0,addr=0x1.0x2 \
-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \
-device ide-hd,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,bootindex=1 \
--object sev-guest,id=sev0,cbitpos=47,reduced-phys-bits=1,policy=0x1,\
+-object sev-guest,id=sev0,cbitpos=47,reduced-phys-bits=1,policy=1,\
dh-cert-file=/tmp/lib/domain--1-QEMUGuest1/dh_cert.base64,\
session-file=/tmp/lib/domain--1-QEMUGuest1/session.base64 \
-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\
diff --git a/tests/qemuxml2argvdata/launch-security-sev.x86_64-2.12.0.args
b/tests/qemuxml2argvdata/launch-security-sev.x86_64-2.12.0.args
index 378c3b681c..9fad85737a 100644
--- a/tests/qemuxml2argvdata/launch-security-sev.x86_64-2.12.0.args
+++ b/tests/qemuxml2argvdata/launch-security-sev.x86_64-2.12.0.args
@@ -29,7 +29,7 @@ file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes \
-device piix3-usb-uhci,id=usb,bus=pci.0,addr=0x1.0x2 \
-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \
-device ide-hd,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,bootindex=1 \
--object sev-guest,id=sev0,cbitpos=47,reduced-phys-bits=1,policy=0x1,\
+-object sev-guest,id=sev0,cbitpos=47,reduced-phys-bits=1,policy=1,\
dh-cert-file=/tmp/lib/domain--1-QEMUGuest1/dh_cert.base64,\
session-file=/tmp/lib/domain--1-QEMUGuest1/session.base64 \
-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\
--
2.28.0
[PATCH 04/24] testQemuMonitorJSONqemuMonitorJSONGetMigrationCapabilities: refactor cleanup
Use automatic memory freeing to remove the 'cleanup:' label and 'ret'
variable.
Signed-off-by: Peter Krempa
---
tests/qemumonitorjsontest.c | 25 +
1 file changed, 9 insertions(+), 16 deletions(-)
diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c
index 954a074f15..5a3926bb11 100644
--- a/tests/qemumonitorjsontest.c
+++ b/tests/qemumonitorjsontest.c
@@ -2217,11 +2217,10 @@
testQemuMonitorJSONqemuMonitorJSONGetMigrationCapabilities(const void *opaque)
{
const testGenericData *data = opaque;
virDomainXMLOptionPtr xmlopt = data->xmlopt;
-int ret = -1;
const char *cap;
-char **caps = NULL;
-virBitmapPtr bitmap = NULL;
-virJSONValuePtr json = NULL;
+g_auto(GStrv) caps = NULL;
+g_autoptr(virBitmap) bitmap = NULL;
+g_autoptr(virJSONValue) json = NULL;
const char *reply =
"{"
"\"return\": ["
@@ -2240,32 +2239,26 @@
testQemuMonitorJSONqemuMonitorJSONGetMigrationCapabilities(const void *opaque)
if (qemuMonitorTestAddItem(test, "query-migrate-capabilities", reply) < 0
||
qemuMonitorTestAddItem(test, "migrate-set-capabilities",
"{\"return\":{}}") < 0)
-goto cleanup;
+return -1;
if (qemuMonitorGetMigrationCapabilities(qemuMonitorTestGetMonitor(test),
&caps) < 0)
-goto cleanup;
+return -1;
cap = qemuMigrationCapabilityTypeToString(QEMU_MIGRATION_CAP_XBZRLE);
if (!virStringListHasString((const char **) caps, cap)) {
virReportError(VIR_ERR_INTERNAL_ERROR,
"Expected capability %s is missing", cap);
-goto cleanup;
+return -1;
}
bitmap = virBitmapNew(QEMU_MIGRATION_CAP_LAST);
ignore_value(virBitmapSetBit(bitmap, QEMU_MIGRATION_CAP_XBZRLE));
if (!(json = qemuMigrationCapsToJSON(bitmap, bitmap)))
-goto cleanup;
-
-ret =
qemuMonitorJSONSetMigrationCapabilities(qemuMonitorTestGetMonitor(test),
- &json);
+return -1;
- cleanup:
-virJSONValueFree(json);
-g_strfreev(caps);
-virBitmapFree(bitmap);
-return ret;
+return
qemuMonitorJSONSetMigrationCapabilities(qemuMonitorTestGetMonitor(test),
+ &json);
}
static int
--
2.28.0
[PATCH 06/24] qemuMonitorJSONMakeCommandInternal: Clear @arguments when stolen
All callers of qemuMonitorJSONMakeCommandInternal will benefit from
making @arguments a double pointer and passing it to
virJSONValueObjectCreate directly which will clear it if it steals the
value.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_monitor_json.c | 32
1 file changed, 12 insertions(+), 20 deletions(-)
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index fbcd1d8e08..272f7a9433 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -546,20 +546,18 @@ qemuMonitorJSONTransactionAdd(virJSONValuePtr actions,
*
* Create a JSON object used on the QMP monitor to call a command.
*
- * Note that @arguments is always consumed and should not be referenced after
- * the call to this function.
+ * Note that @arguments is consumed and cleared.
*/
static virJSONValuePtr
qemuMonitorJSONMakeCommandInternal(const char *cmdname,
- virJSONValuePtr arguments)
+ virJSONValuePtr *arguments)
{
virJSONValuePtr ret = NULL;
ignore_value(virJSONValueObjectCreate(&ret,
"s:execute", cmdname,
- "A:arguments", &arguments, NULL));
+ "A:arguments", arguments, NULL));
-virJSONValueFree(arguments);
return ret;
}
@@ -569,7 +567,7 @@ qemuMonitorJSONMakeCommand(const char *cmdname,
...)
{
virJSONValuePtr obj = NULL;
-virJSONValuePtr jargs = NULL;
+g_autoptr(virJSONValue) jargs = NULL;
va_list args;
va_start(args, cmdname);
@@ -577,7 +575,7 @@ qemuMonitorJSONMakeCommand(const char *cmdname,
if (virJSONValueObjectCreateVArgs(&jargs, args) < 0)
goto cleanup;
-obj = qemuMonitorJSONMakeCommandInternal(cmdname, jargs);
+obj = qemuMonitorJSONMakeCommandInternal(cmdname, &jargs);
cleanup:
va_end(args);
@@ -3505,9 +3503,8 @@ qemuMonitorJSONSetMigrationParams(qemuMonitorPtr mon,
{
g_autoptr(virJSONValue) cmd = NULL;
g_autoptr(virJSONValue) reply = NULL;
-virJSONValuePtr par = g_steal_pointer(params);
-if (!(cmd = qemuMonitorJSONMakeCommandInternal("migrate-set-parameters",
par)))
+if (!(cmd = qemuMonitorJSONMakeCommandInternal("migrate-set-parameters",
params)))
return -1;
if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0)
@@ -4024,7 +4021,7 @@ int qemuMonitorJSONAddFileHandleToSet(qemuMonitorPtr mon,
return -1;
}
-if (!(cmd = qemuMonitorJSONMakeCommandInternal("add-fd",
g_steal_pointer(&args
+if (!(cmd = qemuMonitorJSONMakeCommandInternal("add-fd", &args)))
return -1;
if (qemuMonitorJSONCommandWithFd(mon, cmd, fd, &reply) < 0)
@@ -4208,9 +4205,8 @@ qemuMonitorJSONAddNetdev(qemuMonitorPtr mon,
{
g_autoptr(virJSONValue) cmd = NULL;
g_autoptr(virJSONValue) reply = NULL;
-virJSONValuePtr pr = g_steal_pointer(props);
-if (!(cmd = qemuMonitorJSONMakeCommandInternal("netdev_add", pr)))
+if (!(cmd = qemuMonitorJSONMakeCommandInternal("netdev_add", props)))
return -1;
if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0)
@@ -4637,9 +4633,8 @@ qemuMonitorJSONAddObject(qemuMonitorPtr mon,
{
g_autoptr(virJSONValue) cmd = NULL;
g_autoptr(virJSONValue) reply = NULL;
-virJSONValuePtr pr = g_steal_pointer(props);
-if (!(cmd = qemuMonitorJSONMakeCommandInternal("object-add", pr)))
+if (!(cmd = qemuMonitorJSONMakeCommandInternal("object-add", props)))
return -1;
if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0)
@@ -7506,9 +7501,8 @@ qemuMonitorJSONBlockExportAdd(qemuMonitorPtr mon,
{
g_autoptr(virJSONValue) cmd = NULL;
g_autoptr(virJSONValue) reply = NULL;
-virJSONValuePtr pr = g_steal_pointer(props);
-if (!(cmd = qemuMonitorJSONMakeCommandInternal("block-export-add", pr)))
+if (!(cmd = qemuMonitorJSONMakeCommandInternal("block-export-add", props)))
return -1;
if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0)
@@ -9036,9 +9030,8 @@ qemuMonitorJSONBlockdevAdd(qemuMonitorPtr mon,
{
g_autoptr(virJSONValue) cmd = NULL;
g_autoptr(virJSONValue) reply = NULL;
-virJSONValuePtr pr = g_steal_pointer(props);
-if (!(cmd = qemuMonitorJSONMakeCommandInternal("blockdev-add", pr)))
+if (!(cmd = qemuMonitorJSONMakeCommandInternal("blockdev-add", props)))
return -1;
if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0)
@@ -9057,9 +9050,8 @@ qemuMonitorJSONBlockdevReopen(qemuMonitorPtr mon,
{
g_autoptr(virJSONValue) cmd = NULL;
g_autoptr(virJSONValue) reply = NULL;
-virJSONValuePtr pr = g_steal_pointer(props);
-if (!(cmd = qemuMonitorJSONMakeCommandInternal("blockdev-reopen", pr)))
+if (!(cmd = qemuMonitorJSONMakeCommandInternal("blockdev-reopen", props)))
return -1;
if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0)
--
2.2
[PATCH 05/24] qemuMonitorJSONAddObject: Take double pointer for @props
Prepare for a refactor of qemuMonitorJSONMakeCommandInternal.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_monitor.c | 3 +--
src/qemu/qemu_monitor_json.c | 5 +++--
src/qemu/qemu_monitor_json.h | 2 +-
3 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index b29779f8e4..7f58c16987 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -3104,8 +3104,7 @@ qemuMonitorAddObject(qemuMonitorPtr mon,
if (alias)
tmp = g_strdup(id);
-ret = qemuMonitorJSONAddObject(mon, *props);
-*props = NULL;
+ret = qemuMonitorJSONAddObject(mon, props);
if (alias)
*alias = g_steal_pointer(&tmp);
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 8fd53a565d..fbcd1d8e08 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -4633,12 +4633,13 @@ qemuMonitorJSONAddDevice(qemuMonitorPtr mon,
int
qemuMonitorJSONAddObject(qemuMonitorPtr mon,
- virJSONValuePtr props)
+ virJSONValuePtr *props)
{
g_autoptr(virJSONValue) cmd = NULL;
g_autoptr(virJSONValue) reply = NULL;
+virJSONValuePtr pr = g_steal_pointer(props);
-if (!(cmd = qemuMonitorJSONMakeCommandInternal("object-add", props)))
+if (!(cmd = qemuMonitorJSONMakeCommandInternal("object-add", pr)))
return -1;
if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0)
diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
index 9cb4ec264b..d123c38812 100644
--- a/src/qemu/qemu_monitor_json.h
+++ b/src/qemu/qemu_monitor_json.h
@@ -246,7 +246,7 @@ int qemuMonitorJSONDelDevice(qemuMonitorPtr mon,
const char *devalias);
int qemuMonitorJSONAddObject(qemuMonitorPtr mon,
- virJSONValuePtr props);
+ virJSONValuePtr *props);
int qemuMonitorJSONDelObject(qemuMonitorPtr mon,
const char *objalias,
--
2.28.0
[PATCH 12/24] testCompareXMLToArgvValidateSchema: Populate autoNodeset
We create a new 'vm' so we must also fake the nodeset.
Signed-off-by: Peter Krempa
---
tests/qemuxml2argvtest.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index ae8eaa7768..225cb70edf 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -519,6 +519,7 @@ testCompareXMLToArgvValidateSchema(virQEMUDriverPtr drv,
{
VIR_AUTOSTRINGLIST args = NULL;
g_autoptr(virDomainObj) vm = NULL;
+qemuDomainObjPrivatePtr priv = NULL;
size_t nargs = 0;
size_t i;
g_autoptr(GHashTable) schema = NULL;
@@ -546,6 +547,11 @@ testCompareXMLToArgvValidateSchema(virQEMUDriverPtr drv,
NULL, parseFlags)))
return -1;
+priv = vm->privateData;
+
+if (virBitmapParse("0-3", &priv->autoNodeset, 4) < 0)
+return -1;
+
if (!(cmd = testCompareXMLToArgvCreateArgs(drv, vm, migrateURI, info,
flags,
true)))
return -1;
--
2.28.0
[PATCH 01/24] qemuMonitorJSONSetMigrationParams: Take double pointer for @params
This allows simplification of the caller as well as will enable a later
refactor of qemuMonitorJSONMakeCommandInternal.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_migration_params.c | 9 +++--
src/qemu/qemu_monitor.c | 11 +++
src/qemu/qemu_monitor.h | 2 +-
src/qemu/qemu_monitor_json.c | 5 +++--
src/qemu/qemu_monitor_json.h | 2 +-
5 files changed, 11 insertions(+), 18 deletions(-)
diff --git a/src/qemu/qemu_migration_params.c b/src/qemu/qemu_migration_params.c
index df5560d39f..d1d59aeb01 100644
--- a/src/qemu/qemu_migration_params.c
+++ b/src/qemu/qemu_migration_params.c
@@ -843,12 +843,9 @@ qemuMigrationParamsApply(virQEMUDriverPtr driver,
if (!(params = qemuMigrationParamsToJSON(migParams)))
goto cleanup;
-if (virJSONValueObjectKeysNumber(params) > 0) {
-rc = qemuMonitorSetMigrationParams(priv->mon, params);
-params = NULL;
-if (rc < 0)
-goto cleanup;
-}
+if (virJSONValueObjectKeysNumber(params) > 0 &&
+qemuMonitorSetMigrationParams(priv->mon, ¶ms) < 0)
+goto cleanup;
ret = 0;
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index f2ed165b22..f8287d375f 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -2492,22 +2492,17 @@ qemuMonitorGetMigrationParams(qemuMonitorPtr mon,
* @mon: Pointer to the monitor object.
* @params: Migration parameters.
*
- * The @params object is consumed and should not be referenced by the caller
- * after this function returns.
+ * The @params object is consumed and cleared.
*
* Returns 0 on success, -1 on error.
*/
int
qemuMonitorSetMigrationParams(qemuMonitorPtr mon,
- virJSONValuePtr params)
+ virJSONValuePtr *params)
{
-QEMU_CHECK_MONITOR_GOTO(mon, error);
+QEMU_CHECK_MONITOR(mon);
return qemuMonitorJSONSetMigrationParams(mon, params);
-
- error:
-virJSONValueFree(params);
-return -1;
}
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index d301568e40..c8b9111633 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -799,7 +799,7 @@ int qemuMonitorSetMigrationCacheSize(qemuMonitorPtr mon,
int qemuMonitorGetMigrationParams(qemuMonitorPtr mon,
virJSONValuePtr *params);
int qemuMonitorSetMigrationParams(qemuMonitorPtr mon,
- virJSONValuePtr params);
+ virJSONValuePtr *params);
typedef enum {
QEMU_MONITOR_MIGRATION_STATUS_INACTIVE,
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 723bdb4426..bb454bf49d 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -3501,12 +3501,13 @@ qemuMonitorJSONGetMigrationParams(qemuMonitorPtr mon,
int
qemuMonitorJSONSetMigrationParams(qemuMonitorPtr mon,
- virJSONValuePtr params)
+ virJSONValuePtr *params)
{
g_autoptr(virJSONValue) cmd = NULL;
g_autoptr(virJSONValue) reply = NULL;
+virJSONValuePtr par = g_steal_pointer(params);
-if (!(cmd = qemuMonitorJSONMakeCommandInternal("migrate-set-parameters",
params)))
+if (!(cmd = qemuMonitorJSONMakeCommandInternal("migrate-set-parameters",
par)))
return -1;
if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0)
diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
index b588722d90..b3f3e79041 100644
--- a/src/qemu/qemu_monitor_json.h
+++ b/src/qemu/qemu_monitor_json.h
@@ -142,7 +142,7 @@ int qemuMonitorJSONSetMigrationCacheSize(qemuMonitorPtr mon,
int qemuMonitorJSONGetMigrationParams(qemuMonitorPtr mon,
virJSONValuePtr *params);
int qemuMonitorJSONSetMigrationParams(qemuMonitorPtr mon,
- virJSONValuePtr params);
+ virJSONValuePtr *params);
int qemuMonitorJSONGetMigrationStats(qemuMonitorPtr mon,
qemuMonitorMigrationStatsPtr stats,
--
2.28.0
[PATCH 03/24] qemuMonitorJSONSetMigrationCapabilities: Refactor cleanup
Use automatic memory freeing and remove the 'cleanup' label and 'ret'
variable.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_monitor_json.c | 24 +---
1 file changed, 9 insertions(+), 15 deletions(-)
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index fe5c26f37f..8fd53a565d 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -7133,27 +7133,21 @@ int
qemuMonitorJSONSetMigrationCapabilities(qemuMonitorPtr mon,
virJSONValuePtr *caps)
{
-int ret = -1;
-virJSONValuePtr cmd = NULL;
-virJSONValuePtr reply = NULL;
+g_autoptr(virJSONValue) cmd = NULL;
+g_autoptr(virJSONValue) reply = NULL;
-cmd = qemuMonitorJSONMakeCommand("migrate-set-capabilities",
- "a:capabilities", caps,
- NULL);
-if (!cmd)
-goto cleanup;
+if (!(cmd = qemuMonitorJSONMakeCommand("migrate-set-capabilities",
+ "a:capabilities", caps,
+ NULL)))
+return -1;
if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0)
-goto cleanup;
+return -1;
if (qemuMonitorJSONCheckError(cmd, reply) < 0)
-goto cleanup;
+return -1;
-ret = 0;
- cleanup:
-virJSONValueFree(cmd);
-virJSONValueFree(reply);
-return ret;
+return 0;
}
--
2.28.0
[PATCH 00/24] qemu: Prepare for QAPIfied 'object-add'
Prepare libvirt for the changes proposed for qemu: https://www.redhat.com/archives/libvir-list/2020-November/msg01580.html Patches 1-12/24 are pure refactors and can be merged right away. Patches 13-15/24 can be merged, but the justification will be weak without the rest. Patches 16-22/24 are RFC until the qemu bits are merged. Patch 23/24 adds capabilities and needs to be udpated later Patch 24/24 is not to be merged! Note that patch 23/24 is intentionally missing from the mailing list posting as it's just a dump of QEMU capabilities from newest qemu. No need to litter the list with it. Also note that after the last patch tests will fail. See that patch for the reason. Fetch the full series at: git fetch https://gitlab.com/pipo.sk/libvirt.git object-add-qapi-3 Peter Krempa (24): qemuMonitorJSONSetMigrationParams: Take double pointer for @params qemuMonitorSetMigrationCapabilities: Take double pointer for @caps qemuMonitorJSONSetMigrationCapabilities: Refactor cleanup testQemuMonitorJSONqemuMonitorJSONGetMigrationCapabilities: refactor cleanup qemuMonitorJSONAddObject: Take double pointer for @props qemuMonitorJSONMakeCommandInternal: Clear @arguments when stolen qemuMonitorAddObject: Fix semantics of @alias qemuMonitorAddObject: Refactor cleanup util: json: Replace virJSONValueObjectSteal by virJSONValueObjectRemoveKey tests: qemuxml2argv: Don't check whether -netdev was QAPIfied repeatedly qemuBuildChrChardevStr: Rename 'flags' to 'cdevflags' testCompareXMLToArgvValidateSchema: Populate autoNodeset qemu: command: Generate commandline of 'masterKey0' secret via JSON qemu: command: Generate commandline of 'sev0' sev-guest object via JSON qemu: command: Generate commandline of iothread objects JSON qemu: capabilities: Introduce QEMU_CAPS_OBJECT_QAPIFIED tests: qemuxml2argv: Validate generation of JSON props for object-add qemu: command: Introduce raw JSON passthrough for '-object' for testing qemu: monitor: Make wrapping of 'props' of 'object-add' optional qemuMonitorCreateObjectPropsWrap: Open-code in qemuBuildMemoryBackendProps qemu: monitor: Don't add 'props' wrapper if qemu has QEMU_CAPS_OBJECT_QAPIFIED qemumonitorjsontest: Remove bomb guarding object-add qemu: capabilities: Add 6.0 capabilities with qapified object-add [DONTMERGE] qemuxml2argvtest: Force QMP validation with latest caps src/qemu/qemu_capabilities.c | 2 + src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 332 +- src/qemu/qemu_migration_params.c |22 +- src/qemu/qemu_monitor.c | 121 +- src/qemu/qemu_monitor.h | 8 +- src/qemu/qemu_monitor_json.c |57 +- src/qemu/qemu_monitor_json.h | 6 +- src/util/virjson.c|29 +- src/util/virqemu.c|48 +- src/util/virqemu.h| 3 +- .../domaincapsdata/qemu_6.0.0-q35.x86_64.xml | 197 + .../domaincapsdata/qemu_6.0.0-tcg.x86_64.xml | 203 + tests/domaincapsdata/qemu_6.0.0.x86_64.xml| 197 + .../caps_6.0.0.x86_64.replies | 32021 .../caps_6.0.0.x86_64.xml | 3205 ++ tests/qemumonitorjsontest.c |40 +- ...v-missing-platform-info.x86_64-2.12.0.args | 2 +- .../launch-security-sev.x86_64-2.12.0.args| 2 +- tests/qemuxml2argvtest.c |44 +- 20 files changed, 36202 insertions(+), 338 deletions(-) create mode 100644 tests/domaincapsdata/qemu_6.0.0-q35.x86_64.xml create mode 100644 tests/domaincapsdata/qemu_6.0.0-tcg.x86_64.xml create mode 100644 tests/domaincapsdata/qemu_6.0.0.x86_64.xml create mode 100644 tests/qemucapabilitiesdata/caps_6.0.0.x86_64.replies create mode 100644 tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml -- 2.28.0
[PATCH 02/24] qemuMonitorSetMigrationCapabilities: Take double pointer for @caps
This allows simplification of the callers.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_migration_params.c | 13 -
src/qemu/qemu_monitor.c | 11 +++
src/qemu/qemu_monitor.h | 2 +-
src/qemu/qemu_monitor_json.c | 5 ++---
src/qemu/qemu_monitor_json.h | 2 +-
tests/qemumonitorjsontest.c | 3 +--
6 files changed, 12 insertions(+), 24 deletions(-)
diff --git a/src/qemu/qemu_migration_params.c b/src/qemu/qemu_migration_params.c
index d1d59aeb01..8c019bf2ce 100644
--- a/src/qemu/qemu_migration_params.c
+++ b/src/qemu/qemu_migration_params.c
@@ -803,7 +803,6 @@ qemuMigrationParamsApply(virQEMUDriverPtr driver,
g_autoptr(virJSONValue) caps = NULL;
qemuMigrationParam xbzrle = QEMU_MIGRATION_PARAM_XBZRLE_CACHE_SIZE;
int ret = -1;
-int rc;
if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
return -1;
@@ -819,12 +818,9 @@ qemuMigrationParamsApply(virQEMUDriverPtr driver,
if (!(caps = qemuMigrationCapsToJSON(priv->migrationCaps,
migParams->caps)))
goto cleanup;
-if (virJSONValueArraySize(caps) > 0) {
-rc = qemuMonitorSetMigrationCapabilities(priv->mon, caps);
-caps = NULL;
-if (rc < 0)
-goto cleanup;
-}
+if (virJSONValueArraySize(caps) > 0 &&
+qemuMonitorSetMigrationCapabilities(priv->mon, &caps) < 0)
+goto cleanup;
}
/* If QEMU is too old to support xbzrle-cache-size migration parameter,
@@ -1389,8 +1385,7 @@ qemuMigrationCapsCheck(virQEMUDriverPtr driver,
if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
return -1;
-rc = qemuMonitorSetMigrationCapabilities(priv->mon, json);
-json = NULL;
+rc = qemuMonitorSetMigrationCapabilities(priv->mon, &json);
if (qemuDomainObjExitMonitor(driver, vm) < 0)
return -1;
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index f8287d375f..b29779f8e4 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -3942,22 +3942,17 @@ qemuMonitorGetMigrationCapabilities(qemuMonitorPtr mon,
* @mon: Pointer to the monitor object.
* @caps: Migration capabilities.
*
- * The @caps object is consumed and should not be referenced by the caller
- * after this function returns.
+ * The @caps object is consumed cleared.
*
* Returns 0 on success, -1 on error.
*/
int
qemuMonitorSetMigrationCapabilities(qemuMonitorPtr mon,
-virJSONValuePtr caps)
+virJSONValuePtr *caps)
{
-QEMU_CHECK_MONITOR_GOTO(mon, error);
+QEMU_CHECK_MONITOR(mon);
return qemuMonitorJSONSetMigrationCapabilities(mon, caps);
-
- error:
-virJSONValueFree(caps);
-return -1;
}
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index c8b9111633..a7fba393bf 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -870,7 +870,7 @@ int qemuMonitorGetMigrationStats(qemuMonitorPtr mon,
int qemuMonitorGetMigrationCapabilities(qemuMonitorPtr mon,
char ***capabilities);
int qemuMonitorSetMigrationCapabilities(qemuMonitorPtr mon,
-virJSONValuePtr caps);
+virJSONValuePtr *caps);
int qemuMonitorGetGICCapabilities(qemuMonitorPtr mon,
virGICCapability **capabilities);
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index bb454bf49d..fe5c26f37f 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -7131,14 +7131,14 @@ qemuMonitorJSONGetMigrationCapabilities(qemuMonitorPtr
mon,
int
qemuMonitorJSONSetMigrationCapabilities(qemuMonitorPtr mon,
-virJSONValuePtr caps)
+virJSONValuePtr *caps)
{
int ret = -1;
virJSONValuePtr cmd = NULL;
virJSONValuePtr reply = NULL;
cmd = qemuMonitorJSONMakeCommand("migrate-set-capabilities",
- "a:capabilities", &caps,
+ "a:capabilities", caps,
NULL);
if (!cmd)
goto cleanup;
@@ -7151,7 +7151,6 @@ qemuMonitorJSONSetMigrationCapabilities(qemuMonitorPtr
mon,
ret = 0;
cleanup:
-virJSONValueFree(caps);
virJSONValueFree(cmd);
virJSONValueFree(reply);
return ret;
diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
index b3f3e79041..9cb4ec264b 100644
--- a/src/qemu/qemu_monitor_json.h
+++ b/src/qemu/qemu_monitor_json.h
@@ -151,7 +151,7 @@ int qemuMonitorJSONGetMigrationStats(qemuMonitorPtr mon,
int qemuMonitorJSONGetMigrationCapabilities(qemuMonitorPtr mon,
char ***capabilities);
int qemuMonitorJSONSetMigrationCapabilities(qemuMonitorPtr
Re: [PATCH 00/18] qapi/qom: QAPIfy object-add
Am 30.11.2020 um 17:57 hat Paolo Bonzini geschrieben: > On 30/11/20 16:46, Kevin Wolf wrote: > > Am 30.11.2020 um 15:58 hat Paolo Bonzini geschrieben: > > > With this series it's basically pointless to have QOM properties at > > > all. > > > > Not entirely, because there are still some writable properties that can > > be changed later on. > > Are there really any (that are not bugs like opened/loaded)? That's also why > Eduardo and I discussed a class-wide allow_set function for his field > properties series. Yes. I don't really know most objects apart from what I had to learn for this series, but I know at least two that actually allow this intentionally: iothread and throttle-group. > > So with this in mind, I think I'm in favour of completely leaving the > > initialisation of properties on object creation to QAPI, and only > > providing individual setters if we actually intend to allow property > > changes after creation. > > The main problem is that it wouldn't extend well, if at all, to > machines and devices. So those would still not be integrated into the > QAPI schema. What do you think is the biggest difference there? Don't devices work the same as user creatable objects in that you first set a bunch of properties (which would now be done through QAPI instead) and then call the completion/realize method that actually makes use of them? I must admit that I don't know how machine types work. > > > So the question is, are we okay with shoveling half of QEMU's backend data > > > model into a single struct? If so, there are important consequences. > > > > Yeah, the single struct bothers me a bit, both in the QAPI schema and in > > the C source. > > The single struct doesn't bother me _too much_ actually. What bothers me is > that it won't be a single source of all QOM objects, only those that happen > to be created by object-add. But isn't it only natural that a list of these objects will exist in some way, implicitly or explicitly? object-add must somehow decide which object types it allows the user to create. Once we describe the object types in the schema (rather than only their properties), which is required if we want to generate the QOM boilerplate, this can possibly become implicit because then QAPI can know which objects implement USER_CREATABLE. > So I start to wonder if QOM as it exists now is the right solution for > all kind of objects: > > - backends and other object-add types (not per-target, relatively few > classes and even fewer hierarchies) > > - machine (per-target, many classes, no hierarchy) > > - device (can be treated as not per-target, many many classes, very few > hierarchies) > > - accelerator (per-target, few classes, no hierarchy) > > - chardev (ok those are the same as the first category) > > If QOM is the right solution, this patch goes in the wrong direction. > > If QOM is the wrong solution, this patch is okay but then we have another > problem to solve. :) I think the requirements for all of them are probably similar enough that they can potentially be covered by a single thing. I'm also pretty sure that QOM as it exists now is not the right solution for any of them because it has some major shortcomings. It's too easy to get things wrong (like the writable properties after creation), its introspection is rather weak and separated from the QAPI introspection, it doesn't encourage documentation, and it involves quite a bit of boilerplate and duplicated code between class implementations. A modified QOM might be the right solution, though. I would like to bring QAPI and QOM together because most of these weaknesses are strengths of QAPI. I guess the real question is what aspects of QOM need to be changed to make it the right solution. > > > The problem with this series is that you are fine with deduplicating > > > things > > > as a third step, but you cannot be sure that such deduplication is > > > possible > > > at all. So while I don't have any problems in principle with the > > > ObjectOptions concept, I don't think it should be committed without a > > > clear > > > idea of how to do the third step. > > > > Do you have any specific concerns why the deduplication might not > > possible, or just because it's uncharted territory? > > Mostly because it's more or less the same issue that you have with > BlockdevOptions, with the extra complication that this series only deals > with the easy one of the four above categories. I'm not sure which exact problem with BlockdevOptions you mean. The reason why the block layer doesn't use BlockdevOptions everywhere is -drive support. > > Maybe if we don't want to commit to keeping the ObjectOptions schema, > > the part that should wait is object-add and I should do the command line > > options first? Then the schema remains an implementation detail for now > > that is invisible in introspection. > > I don't see much benefit in converting _any_ of the three actually. The > only good reason I see f
Re: [PATCH 0/3] qemu: Remove caching in handler for query-command-line-options
On 11/30/20 6:25 PM, Peter Krempa wrote: Shuffle around where we extract the data, so that we don't have to call the monitor multiple times which necessitates caching. Peter Krempa (3): qemu: monitor: Implement new handlers for 'query-command-line-options' virQEMUCapsProbeQMPCommandLine: Rewrite using qemuMonitorGetCommandLineOptions qemuMonitorGetCommandLineOptionParameters: remove the unused function and helpers src/qemu/qemu_capabilities.c | 34 + src/qemu/qemu_monitor.c | 34 + src/qemu/qemu_monitor.h | 9 +-- src/qemu/qemu_monitor_json.c | 138 +-- src/qemu/qemu_monitor_json.h | 6 +- tests/qemumonitorjsontest.c | 117 - 6 files changed, 58 insertions(+), 280 deletions(-) Reviewed-by: Michal Privoznik Michal
[PATCH 2/3] virQEMUCapsProbeQMPCommandLine: Rewrite using qemuMonitorGetCommandLineOptions
Use the new handler to fetch the required data and do the extraction
locally without conversion to string list.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_capabilities.c | 34 +++---
1 file changed, 19 insertions(+), 15 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 538551e772..584bd21be3 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -3226,28 +3226,32 @@ static int
virQEMUCapsProbeQMPCommandLine(virQEMUCapsPtr qemuCaps,
qemuMonitorPtr mon)
{
-bool found = false;
-int nvalues;
-char **values;
-size_t i, j;
+g_autoptr(GHashTable) options = NULL;
+size_t i;
+
+if (!(options = qemuMonitorGetCommandLineOptions(mon)))
+return -1;
for (i = 0; i < G_N_ELEMENTS(virQEMUCapsCommandLine); i++) {
-if ((nvalues = qemuMonitorGetCommandLineOptionParameters(mon,
-
virQEMUCapsCommandLine[i].option,
- &values,
- &found)) < 0)
-return -1;
+virJSONValuePtr option = g_hash_table_lookup(options,
virQEMUCapsCommandLine[i].option);
+size_t j;
-if (found && !virQEMUCapsCommandLine[i].param)
+if (!option)
+continue;
+
+/* not looking for a specific argument */
+if (!virQEMUCapsCommandLine[i].param) {
virQEMUCapsSet(qemuCaps, virQEMUCapsCommandLine[i].flag);
+continue;
+}
-for (j = 0; j < nvalues; j++) {
-if (STREQ_NULLABLE(virQEMUCapsCommandLine[i].param, values[j])) {
+for (j = 0; j < virJSONValueArraySize(option); j++) {
+virJSONValuePtr param = virJSONValueArrayGet(option, j);
+const char *paramname = virJSONValueObjectGetString(param, "name");
+
+if (STREQ_NULLABLE(virQEMUCapsCommandLine[i].param, paramname))
virQEMUCapsSet(qemuCaps, virQEMUCapsCommandLine[i].flag);
-break;
-}
}
-g_strfreev(values);
}
return 0;
--
2.28.0
[PATCH 0/3] qemu: Remove caching in handler for query-command-line-options
Shuffle around where we extract the data, so that we don't have to call the monitor multiple times which necessitates caching. Peter Krempa (3): qemu: monitor: Implement new handlers for 'query-command-line-options' virQEMUCapsProbeQMPCommandLine: Rewrite using qemuMonitorGetCommandLineOptions qemuMonitorGetCommandLineOptionParameters: remove the unused function and helpers src/qemu/qemu_capabilities.c | 34 + src/qemu/qemu_monitor.c | 34 + src/qemu/qemu_monitor.h | 9 +-- src/qemu/qemu_monitor_json.c | 138 +-- src/qemu/qemu_monitor_json.h | 6 +- tests/qemumonitorjsontest.c | 117 - 6 files changed, 58 insertions(+), 280 deletions(-) -- 2.28.0
[PATCH 1/3] qemu: monitor: Implement new handlers for 'query-command-line-options'
Add a new set hander for getting the data for
'query-command-line-options' which returns everything at once and lets
the caller extract the data. This way we don't need to cache the output
of the monitor command for repeated calls.
Note that we will have enough testing of this code path via
qemucapabilitiestest.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_monitor.c | 9 +++
src/qemu/qemu_monitor.h | 1 +
src/qemu/qemu_monitor_json.c | 48
src/qemu/qemu_monitor_json.h | 1 +
4 files changed, 59 insertions(+)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index f2ed165b22..a60d04f78a 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -3873,6 +3873,15 @@ qemuMonitorGetCommandLineOptionParameters(qemuMonitorPtr
mon,
}
+GHashTable *
+qemuMonitorGetCommandLineOptions(qemuMonitorPtr mon)
+{
+QEMU_CHECK_MONITOR_NULL(mon);
+
+return qemuMonitorJSONGetCommandLineOptions(mon);
+}
+
+
int
qemuMonitorGetKVMState(qemuMonitorPtr mon,
bool *enabled,
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index d301568e40..6b2e435f70 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -1288,6 +1288,7 @@ int
qemuMonitorGetCommandLineOptionParameters(qemuMonitorPtr mon,
const char *option,
char ***params,
bool *found);
+GHashTable *qemuMonitorGetCommandLineOptions(qemuMonitorPtr mon);
int qemuMonitorGetKVMState(qemuMonitorPtr mon,
bool *enabled,
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 723bdb4426..44d0152812 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -6402,6 +6402,54 @@ int qemuMonitorJSONGetEvents(qemuMonitorPtr mon,
}
+static int
+qemuMonitorJSONGetCommandLineOptionsWorker(size_t pos G_GNUC_UNUSED,
+ virJSONValuePtr item,
+ void *opaque)
+{
+const char *name = virJSONValueObjectGetString(item, "option");
+g_autoptr(virJSONValue) parameters = NULL;
+GHashTable *options = opaque;
+
+if (!name ||
+virJSONValueObjectRemoveKey(item, "parameters", ¶meters) <= 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("reply data was missing 'option' name or
parameters"));
+return -1;
+}
+
+g_hash_table_insert(options, g_strdup(name), parameters);
+parameters = NULL;
+
+return 0;
+}
+
+
+GHashTable *
+qemuMonitorJSONGetCommandLineOptions(qemuMonitorPtr mon)
+{
+g_autoptr(GHashTable) ret = virHashNew(virJSONValueHashFree);
+g_autoptr(virJSONValue) cmd = NULL;
+g_autoptr(virJSONValue) reply = NULL;
+
+if (!(cmd = qemuMonitorJSONMakeCommand("query-command-line-options",
NULL)))
+return NULL;
+
+if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0)
+return NULL;
+
+if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_ARRAY) < 0)
+return NULL;
+
+if (virJSONValueArrayForeachSteal(virJSONValueObjectGetArray(reply,
"return"),
+
qemuMonitorJSONGetCommandLineOptionsWorker,
+ ret) < 0)
+return NULL;
+
+return g_steal_pointer(&ret);
+}
+
+
int
qemuMonitorJSONGetCommandLineOptionParameters(qemuMonitorPtr mon,
const char *option,
diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
index b588722d90..53445b97bb 100644
--- a/src/qemu/qemu_monitor_json.h
+++ b/src/qemu/qemu_monitor_json.h
@@ -430,6 +430,7 @@ int
qemuMonitorJSONGetCommandLineOptionParameters(qemuMonitorPtr mon,
char ***params,
bool *found)
ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
+GHashTable *qemuMonitorJSONGetCommandLineOptions(qemuMonitorPtr mon);
int qemuMonitorJSONGetKVMState(qemuMonitorPtr mon,
bool *enabled,
--
2.28.0
[PATCH 3/3] qemuMonitorGetCommandLineOptionParameters: remove the unused function and helpers
Remove the function along with helpers for caching the reply and tests.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_monitor.c | 35 --
src/qemu/qemu_monitor.h | 8 ---
src/qemu/qemu_monitor_json.c | 120 ---
src/qemu/qemu_monitor_json.h | 5 --
tests/qemumonitorjsontest.c | 117 --
5 files changed, 285 deletions(-)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index a60d04f78a..c333fc1364 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -101,9 +101,6 @@ struct _qemuMonitor {
bool waitGreeting;
-/* cache of query-command-line-options results */
-virJSONValuePtr options;
-
/* If found, path to the virtio memballoon driver */
char *balloonpath;
bool ballooninit;
@@ -240,7 +237,6 @@ qemuMonitorDispose(void *obj)
virResetError(&mon->lastError);
virCondDestroy(&mon->notify);
VIR_FREE(mon->buffer);
-virJSONValueFree(mon->options);
VIR_FREE(mon->balloonpath);
}
@@ -992,20 +988,6 @@ qemuMonitorLastError(qemuMonitorPtr mon)
}
-virJSONValuePtr
-qemuMonitorGetOptions(qemuMonitorPtr mon)
-{
-return mon->options;
-}
-
-
-void
-qemuMonitorSetOptions(qemuMonitorPtr mon, virJSONValuePtr options)
-{
-mon->options = options;
-}
-
-
/**
* Search the qom objects for the balloon driver object by its known names
* of "virtio-balloon-pci" or "virtio-balloon-ccw". The entry for the driver
@@ -3856,23 +3838,6 @@ qemuMonitorGetEvents(qemuMonitorPtr mon,
}
-/* Collect the parameters associated with a given command line option.
- * Return count of known parameters or -1 on error. */
-int
-qemuMonitorGetCommandLineOptionParameters(qemuMonitorPtr mon,
- const char *option,
- char ***params,
- bool *found)
-{
-VIR_DEBUG("option=%s params=%p", option, params);
-
-QEMU_CHECK_MONITOR(mon);
-
-return qemuMonitorJSONGetCommandLineOptionParameters(mon, option,
- params, found);
-}
-
-
GHashTable *
qemuMonitorGetCommandLineOptions(qemuMonitorPtr mon)
{
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index 6b2e435f70..3dcceffef8 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -444,10 +444,6 @@ int qemuMonitorSetLink(qemuMonitorPtr mon,
char *qemuMonitorNextCommandID(qemuMonitorPtr mon);
int qemuMonitorSend(qemuMonitorPtr mon,
qemuMonitorMessagePtr msg) G_GNUC_NO_INLINE;
-virJSONValuePtr qemuMonitorGetOptions(qemuMonitorPtr mon)
-ATTRIBUTE_NONNULL(1);
-void qemuMonitorSetOptions(qemuMonitorPtr mon, virJSONValuePtr options)
-ATTRIBUTE_NONNULL(1);
int qemuMonitorUpdateVideoMemorySize(qemuMonitorPtr mon,
virDomainVideoDefPtr video,
const char *videoName)
@@ -1284,10 +1280,6 @@ int qemuMonitorGetCommands(qemuMonitorPtr mon,
char ***commands);
int qemuMonitorGetEvents(qemuMonitorPtr mon,
char ***events);
-int qemuMonitorGetCommandLineOptionParameters(qemuMonitorPtr mon,
- const char *option,
- char ***params,
- bool *found);
GHashTable *qemuMonitorGetCommandLineOptions(qemuMonitorPtr mon);
int qemuMonitorGetKVMState(qemuMonitorPtr mon,
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 44d0152812..5990163519 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -6450,126 +6450,6 @@ qemuMonitorJSONGetCommandLineOptions(qemuMonitorPtr mon)
}
-int
-qemuMonitorJSONGetCommandLineOptionParameters(qemuMonitorPtr mon,
- const char *option,
- char ***params,
- bool *found)
-{
-int ret = -1;
-virJSONValuePtr cmd = NULL;
-virJSONValuePtr reply = NULL;
-virJSONValuePtr data = NULL;
-virJSONValuePtr array = NULL;
-char **paramlist = NULL;
-size_t n = 0;
-size_t i;
-
-*params = NULL;
-if (found)
-*found = false;
-
-/* query-command-line-options has fixed output for a given qemu
- * binary; but since callers want to query parameters for one
- * option at a time, we cache the option list from qemu. */
-if (!(array = qemuMonitorGetOptions(mon))) {
-if (!(cmd = qemuMonitorJSONMakeCommand("query-command-line-options",
- NULL)))
-return -1;
-
-if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0)
-goto cleanup;
-
-if (qemuMonitorJSONHasError(reply, "CommandNotFound")) {
-ret = 0;
-
Re: [PATCH 00/18] qapi/qom: QAPIfy object-add
On 30/11/20 16:46, Kevin Wolf wrote:
Am 30.11.2020 um 15:58 hat Paolo Bonzini geschrieben:
With this series it's basically pointless to have QOM properties at
all.
Not entirely, because there are still some writable properties that can
be changed later on.
Are there really any (that are not bugs like opened/loaded)? That's also
why Eduardo and I discussed a class-wide allow_set function for his
field properties series.
So with this in mind, I think I'm in favour of completely leaving the
initialisation of properties on object creation to QAPI, and only
providing individual setters if we actually intend to allow property
changes after creation.
The main problem is that it wouldn't extend well, if at all, to machines
and devices. So those would still not be integrated into the QAPI schema.
So the question is, are we okay with shoveling half of QEMU's backend data
model into a single struct? If so, there are important consequences.
Yeah, the single struct bothers me a bit, both in the QAPI schema and in
the C source.
The single struct doesn't bother me _too much_ actually. What bothers
me is that it won't be a single source of all QOM objects, only those
that happen to be created by object-add. So I start to wonder if QOM as
it exists now is the right solution for all kind of objects:
- backends and other object-add types (not per-target, relatively few
classes and even fewer hierarchies)
- machine (per-target, many classes, no hierarchy)
- device (can be treated as not per-target, many many classes, very few
hierarchies)
- accelerator (per-target, few classes, no hierarchy)
- chardev (ok those are the same as the first category)
If QOM is the right solution, this patch goes in the wrong direction.
If QOM is the wrong solution, this patch is okay but then we have
another problem to solve. :)
The problem with this series is that you are fine with deduplicating things
as a third step, but you cannot be sure that such deduplication is possible
at all. So while I don't have any problems in principle with the
ObjectOptions concept, I don't think it should be committed without a clear
idea of how to do the third step.
Do you have any specific concerns why the deduplication might not
possible, or just because it's uncharted territory?
Mostly because it's more or less the same issue that you have with
BlockdevOptions, with the extra complication that this series only deals
with the easy one of the four above categories.
Maybe if we don't want to commit to keeping the ObjectOptions schema,
the part that should wait is object-add and I should do the command line
options first? Then the schema remains an implementation detail for now
that is invisible in introspection.
I don't see much benefit in converting _any_ of the three actually. The
only good reason I see for QAPIfying this is the documentation, and the
promise of deduplicating QOM boilerplate. The latter is only a promise,
but documentation alone is a damn good reason and it's enough to get
this work into a mergeable shape as soon as possible!
But maybe, we could start in the opposite direction: start with the use
QAPI to eliminate QOM boilerplate. Basing your work on Eduardo's field
properties series, you could add a new 'object' "kind" to QAPI that
would create an array of field properties (e.g. a macro expanding to a
compound literal?)
. Something like
+{ 'object': 'InputBarrier',
+ 'data': { 'name': 'str',
+'x-origin': 'int16',
+'y-origin': 'int16',
+'width': 'int16',
+'height': 'int16' },
+ 'properties': { 'server': 'str',
+ 'port': 'str' } }
would create a macro QOM_InputBarrier_FIELDS defining properties for the
following fields of the InputBarrier struct:
gchar *name;
int16_t x_origin, y_origin;
int16_t width, height;
while server and port would only appear in the documentation (or
alternatively you could leave them out completely, as you wish).
The advantages would be noticeable:
1) the information would be moved in the QAPI schema JSON from the
beginning, decoupling the conflict-heavy part from the complex question
of how to expose the QOM schema in the introspection data
2) there would not be any more duplication than before (there would be
duplication between structs and QAPI schema, but not between structs and
C code that defines properties).
3) it would be opt-in, so it doesn't put on you the burden of keeping
the series in sync with new objects that are added (I have one for the
qtest server for example). At the same time it would be quite appealing
for owners of QOM code to convert their objects to field properties and
get documentation for free.
4) we could special-case 'object' definitions and generate them in the
system manual as well, since they are also useful to document -object.
Yes it's a huge change but you have the boring part already done. What
do you think?
Re: [PATCH 00/18] qapi/qom: QAPIfy object-add
On Mon, Nov 30, 2020 at 05:13:57PM +0100, Kevin Wolf wrote:
> Am 30.11.2020 um 16:30 hat Daniel P. Berrangé geschrieben:
> > On Mon, Nov 30, 2020 at 03:58:23PM +0100, Paolo Bonzini wrote:
> > > On 30/11/20 13:25, Kevin Wolf wrote:
> > > > This series adds a QAPI type for the properties of all user creatable
> > > > QOM types and finally makes QMP object-add use the new ObjectOptions
> > > > union so that QAPI introspection can be used for user creatable objects.
> > > >
> > > > After this series, there is least one obvious next step that needs to be
> > > > done: Change HMP and all of the command line parser to use
> > > > ObjectOptions, too, so that the QAPI schema is consistently enforced in
> > > > all external interfaces. I am planning to send another series to address
> > > > this.
> > > >
> > > > In a third step, we can try to start deduplicating and integrating
> > > > things
> > > > better between QAPI and the QOM implementation, e.g. by generating parts
> > > > of the QOM boilerplate from the QAPI schema.
> > >
> > > With this series it's basically pointless to have QOM properties at all.
> > > Instead, you are basically having half of QEMU's backend data model into a
> > > single struct.
> > >
> > > So the question is, are we okay with shoveling half of QEMU's backend data
> > > model into a single struct? If so, there are important consequences.
> >
> > In theory they should have the same set of options, but nothing in
> > this series will enforce that. So we're introducing the danger that
> > QMP object-add will miss some property, and thus be less functional
> > than the CLI -object. If we convert CLI -object to use the QAPI
> > parser too, we eliminate that danger, but we still have the struct
> > duplication.
>
> I think converting the CLI is doable in the short term. I already have
> the patch for qemu-storage-daemon, but decided to keep it for a separate
> series.
>
> The most difficult part is probably -readconfig, but with Paolo's RFC
> patches to move it away from QemuOpts, even that shouldn't be very hard.
>
> > > 1) QOM basically does not need properties anymore except for devices and
> > > machines (accelerators could be converted to QAPI as well). All
> > > user-creatable objects can be changed to something like chardev's "get a
> > > struct and use it fill in the fields", and only leave properties to
> > > devices
> > > and machines.
> > >
> > > 2) User-creatable objects can have a much more flexible schema. This
> > > means
> > > there's no reason to have block device creation as its own command and
> > > struct for example.
> > >
> > > The problem with this series is that you are fine with deduplicating
> > > things
> > > as a third step, but you cannot be sure that such deduplication is
> > > possible
> > > at all. So while I don't have any problems in principle with the
> > > ObjectOptions concept, I don't think it should be committed without a
> > > clear
> > > idea of how to do the third step.
> >
> > I feel like we should at least aim to kill the struct duplication, even if
> > we ignore the bigger QOM stuff like setters/getters/constructors/etc. The
> > generated structs are not far off being usable.
> >
> > eg for the secret object we have the QAPI schema
> >
> > { 'struct': 'SecretCommonProperties',
> > 'data': { '*loaded': { 'type': 'bool', 'features': ['deprecated'] },
> > '*format': 'QCryptoSecretFormat',
> > '*keyid': 'str',
> > '*iv': 'str' } }
> >
> > { 'struct': 'SecretProperties',
> > 'base': 'SecretCommonProperties',
> > 'data': { '*data': 'str',
> > '*file': 'str' } }
> >
> > IIUC this will resulting in a QAPI generated flattened struct:
> >
> > struct SecretProperties {
> > bool loaded;
> > QCryptoSecretFormat format;
> > char *keyid;
> > char *iv;
> > char *data;
> > char *file;
> > };
> >
> > vs the QOM manually written structs
> >
> > struct QCryptoSecretCommon {
> > Object parent_obj;
> > uint8_t *rawdata;
> > size_t rawlen;
> > QCryptoSecretFormat format;
> > char *keyid;
> > char *iv;
> > };
> >
> > struct QCryptoSecret {
> > QCryptoSecretCommon parent_obj;
> > char *data;
> > char *file;
> > };
> >
> > The key differences
> >
> > - The parent struct is embedded, rather than flattened
> > - The "loaded" property doesn't need to exist
> > - Some extra fields are live state (rawdata, rawlen)
> >
> > Lets pretend we just kill "loaded" entirely, so ignore that.
> >
> > We could simply make QOM "Object" a well known built-in type, so
> > we can reference it as a "parent". Then any struct with "Object"
> > as a parent could use struct embedding rather flattening and thus
> > just work.
> >
> > Can we invent a "state" field for fields that are internal
> > only, separate from the public "data" fields.
> >
> > eg the secret QAPI def would only need a couple of changes:
> >
> > { 'struct': 'QCrypto
Re: [PATCH 00/18] qapi/qom: QAPIfy object-add
On 30/11/20 16:30, Daniel P. Berrangé wrote:
{ 'struct': 'QCryptoSecretCommon',
'base': 'Object',
'state': { 'rawdata': '*uint8_t',
'rawlen': 'size_t' },
'data': { '*format': 'QCryptoSecretFormat',
'*keyid': 'str',
'*iv': 'str' } }
{ 'struct': 'QCryptoSecret',
'base': 'QCryptoSecretCommon',
'data': { '*data': 'str',
'*file': 'str' } }
No, please don't do this. I want to keep writing C code, not a weird
JSON syntax for C.
I'd much rather have a FooOptions field in my struct so that I can just
do visit_FooOptions in the UserCreatable.complete() method, that's feasible.
Paolo
Re: [PATCH 00/18] qapi/qom: QAPIfy object-add
Am 30.11.2020 um 16:30 hat Daniel P. Berrangé geschrieben:
> On Mon, Nov 30, 2020 at 03:58:23PM +0100, Paolo Bonzini wrote:
> > On 30/11/20 13:25, Kevin Wolf wrote:
> > > This series adds a QAPI type for the properties of all user creatable
> > > QOM types and finally makes QMP object-add use the new ObjectOptions
> > > union so that QAPI introspection can be used for user creatable objects.
> > >
> > > After this series, there is least one obvious next step that needs to be
> > > done: Change HMP and all of the command line parser to use
> > > ObjectOptions, too, so that the QAPI schema is consistently enforced in
> > > all external interfaces. I am planning to send another series to address
> > > this.
> > >
> > > In a third step, we can try to start deduplicating and integrating things
> > > better between QAPI and the QOM implementation, e.g. by generating parts
> > > of the QOM boilerplate from the QAPI schema.
> >
> > With this series it's basically pointless to have QOM properties at all.
> > Instead, you are basically having half of QEMU's backend data model into a
> > single struct.
> >
> > So the question is, are we okay with shoveling half of QEMU's backend data
> > model into a single struct? If so, there are important consequences.
>
> In theory they should have the same set of options, but nothing in
> this series will enforce that. So we're introducing the danger that
> QMP object-add will miss some property, and thus be less functional
> than the CLI -object. If we convert CLI -object to use the QAPI
> parser too, we eliminate that danger, but we still have the struct
> duplication.
I think converting the CLI is doable in the short term. I already have
the patch for qemu-storage-daemon, but decided to keep it for a separate
series.
The most difficult part is probably -readconfig, but with Paolo's RFC
patches to move it away from QemuOpts, even that shouldn't be very hard.
> > 1) QOM basically does not need properties anymore except for devices and
> > machines (accelerators could be converted to QAPI as well). All
> > user-creatable objects can be changed to something like chardev's "get a
> > struct and use it fill in the fields", and only leave properties to devices
> > and machines.
> >
> > 2) User-creatable objects can have a much more flexible schema. This means
> > there's no reason to have block device creation as its own command and
> > struct for example.
> >
> > The problem with this series is that you are fine with deduplicating things
> > as a third step, but you cannot be sure that such deduplication is possible
> > at all. So while I don't have any problems in principle with the
> > ObjectOptions concept, I don't think it should be committed without a clear
> > idea of how to do the third step.
>
> I feel like we should at least aim to kill the struct duplication, even if
> we ignore the bigger QOM stuff like setters/getters/constructors/etc. The
> generated structs are not far off being usable.
>
> eg for the secret object we have the QAPI schema
>
> { 'struct': 'SecretCommonProperties',
> 'data': { '*loaded': { 'type': 'bool', 'features': ['deprecated'] },
> '*format': 'QCryptoSecretFormat',
> '*keyid': 'str',
> '*iv': 'str' } }
>
> { 'struct': 'SecretProperties',
> 'base': 'SecretCommonProperties',
> 'data': { '*data': 'str',
> '*file': 'str' } }
>
> IIUC this will resulting in a QAPI generated flattened struct:
>
> struct SecretProperties {
> bool loaded;
> QCryptoSecretFormat format;
> char *keyid;
> char *iv;
> char *data;
> char *file;
> };
>
> vs the QOM manually written structs
>
> struct QCryptoSecretCommon {
> Object parent_obj;
> uint8_t *rawdata;
> size_t rawlen;
> QCryptoSecretFormat format;
> char *keyid;
> char *iv;
> };
>
> struct QCryptoSecret {
> QCryptoSecretCommon parent_obj;
> char *data;
> char *file;
> };
>
> The key differences
>
> - The parent struct is embedded, rather than flattened
> - The "loaded" property doesn't need to exist
> - Some extra fields are live state (rawdata, rawlen)
>
> Lets pretend we just kill "loaded" entirely, so ignore that.
>
> We could simply make QOM "Object" a well known built-in type, so
> we can reference it as a "parent". Then any struct with "Object"
> as a parent could use struct embedding rather flattening and thus
> just work.
>
> Can we invent a "state" field for fields that are internal
> only, separate from the public "data" fields.
>
> eg the secret QAPI def would only need a couple of changes:
>
> { 'struct': 'QCryptoSecretCommon',
> 'base': 'Object',
> 'state': { 'rawdata': '*uint8_t',
> 'rawlen': 'size_t' },
> 'data': { '*format': 'QCryptoSecretFormat',
> '*keyid': 'str',
> '*iv': 'str' } }
>
> { 'struct': 'QCryptoSecret',
> 'base': 'QCryptoSecretCommon',
> 'data': { '*data': 'str',
> '*file'
Re: [PATCH for-6.0 6/6] qapi: Deprecate 'query-kvm'
Peter Krempa writes: > On Mon, Nov 30, 2020 at 10:21:08 +0100, Markus Armbruster wrote: >> Peter Krempa writes: >> >> > On Fri, Nov 27, 2020 at 16:44:05 +0100, Markus Armbruster wrote: >> >> Peter Krempa writes: > > [...] > >> > I know it's hard to enforce, but probably the cheapest in terms of >> > drawbacks any other solution would be. >> >> We can and should try. >> >> Can we flag problematic interface changes automatically? Semantic >> changes, no. What about changes visible in query-qmp-schema? > > I don't know the internals of qemu good enough, but from the perspective > of an user of 'query-qmp-schema' it might be possible but not without > additional tooling. > > The output of query-qmp-schema is basically unreviewable when we are > updating it. A small change in the schema may trigger a re-numbering of > the internal type names so the result is a giant messy piece of JSON > where it's impossible to differentiate changes from churn. A structural comparison could fare better. qapi-gen option -u might help: -u, --unmask-non-abi-names expose non-ABI names in introspection > I've played with generating/expanding all the possibilites and thus > stripping the internal numbering for a tool which would simplify writing > the query strings (a libvirtism for querying whether the QMP schema has > something [1]). This tool could be used in this case to generate a fully > expanded and sorted list which should in most cases be append only when > new stuff is added. This could be then used to see whether something > changed when we'd store the output and compare it against the new one. Such an expansion is one way to compare structurally. It reports differences for "command C, argument A.B...". Mapping to the QAPI schema is straightforward. > Unfortunately that would just make query-qmp-schema dumps more > reviewable in libvirt though. A change in an interface would be noticed > only after it hits qemu upstream. Yes, implementing the comparison in the QEMU repository would be more useful. > [1] > https://gitlab.com/libvirt/libvirt/-/blob/08ae9e5f40f8bae0c3cf48f84181884ddd310fa0/src/qemu/qemu_qapi.c#L392 > > https://gitlab.com/libvirt/libvirt/-/blob/08ae9e5f40f8bae0c3cf48f84181884ddd310fa0/src/qemu/qemu_capabilities.c#L1512
Re: [PATCH 01/18] qapi/qom: Add ObjectOptions for iothread
Am 30.11.2020 um 16:00 hat Paolo Bonzini geschrieben:
> On 30/11/20 13:25, Kevin Wolf wrote:
> > +##
> > +# @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)
> > +#
> > +# Since: 6.0
> > +##
> > +{ 'struct': 'IothreadProperties',
> > + 'data': { '*poll-max-ns': 'int',
> > +'*poll-grow': 'int',
> > +'*poll-shrink': 'int' } }
> > +
>
> Documentation is the main advantage of the ObjectOptions concept. However,
> please use the version where each object and property was introduced for the
> "since" value. Otherwise the documentation will appear to show that none of
> these objects was available before 6.0.
>
> Yes, there is no documentation at all right now for QOM objects. However,
> wrong documentation sometimes is worse than non-existing documentation.
I think we generally use the version when the schema was introduced (so
blockdev-add has 2.9 for most things even if they existed before in
-drive and drive_add), but I agree that your suggestion is more useful.
And object-add isn't a new command, we're just giving it a schema now.
So let's first discuss the general direction, but if we agree on that,
using the version numbers where objects and properties were first
introduced makes sense.
Maybe if maintainers can include the right version numbers in the review
of the patch for "their" object, that would help me updating the
patches.
Kevin
Re: [PATCH 00/18] qapi/qom: QAPIfy object-add
Am 30.11.2020 um 15:58 hat Paolo Bonzini geschrieben: > On 30/11/20 13:25, Kevin Wolf wrote: > > This series adds a QAPI type for the properties of all user creatable > > QOM types and finally makes QMP object-add use the new ObjectOptions > > union so that QAPI introspection can be used for user creatable objects. > > > > After this series, there is least one obvious next step that needs to be > > done: Change HMP and all of the command line parser to use > > ObjectOptions, too, so that the QAPI schema is consistently enforced in > > all external interfaces. I am planning to send another series to address > > this. > > > > In a third step, we can try to start deduplicating and integrating things > > better between QAPI and the QOM implementation, e.g. by generating parts > > of the QOM boilerplate from the QAPI schema. > > With this series it's basically pointless to have QOM properties at > all. Not entirely, because there are still some writable properties that can be changed later on. After working through all the user creatable objects, I would say that separating these from the creation-time options is actually a good thing because there are basically two types of property setters in the existing implementations: 1. It starts with something like 'if (completed)' and takes two different paths, so they are already separated. Often one path is just returning an error, but sometimes we actually make an effort to update the internal state according to the new value. 2. No distinction is made. Usually the result is inconsistent state because the property values themselves are updated, but they have been interpreted once in ucc->complete and are ignored afterwards. Or maybe even worse, they are still used, but no care is taken that they are consistent with the rest of the internal state. Unfortunately my impression is that this is the more common type. So with this in mind, I think I'm in favour of completely leaving the initialisation of properties on object creation to QAPI, and only providing individual setters if we actually intend to allow property changes after creation. > Instead, you are basically having half of QEMU's backend data model > into a single struct. > > So the question is, are we okay with shoveling half of QEMU's backend data > model into a single struct? If so, there are important consequences. Yeah, the single struct bothers me a bit, both in the QAPI schema and in the C source. We probably need to have it present in the schema in some way so we can actually check input against the schema. Maybe we can have it automatically compiled by the QAPI generator so that we don't need to manually update the enum and the union each time. In the C source, I guess the other option would be to have pointers rather than directly embedding all struct types. In the long run this might make more sense. As long as it's only user-creatable objects, it's no worse than BlockdevOptions. > 1) QOM basically does not need properties anymore except for devices and > machines (accelerators could be converted to QAPI as well). All > user-creatable objects can be changed to something like chardev's "get a > struct and use it fill in the fields", and only leave properties to devices > and machines. True for those properties that don't support updates after object creation. For these, leaving the work to QAPI simplifies things a lot. > 2) User-creatable objects can have a much more flexible schema. This means > there's no reason to have block device creation as its own command and > struct for example. In theory yes. The block layer isn't really QAPIfied, though, it just has a QAPI wrapper (similar to how this series doesn't QAPIfy QOM, but justs wraps it). But for the long term vision, I think it's a reasonable goal to have block nodes represented as QOM-with-QAPI objects. > The problem with this series is that you are fine with deduplicating things > as a third step, but you cannot be sure that such deduplication is possible > at all. So while I don't have any problems in principle with the > ObjectOptions concept, I don't think it should be committed without a clear > idea of how to do the third step. Do you have any specific concerns why the deduplication might not possible, or just because it's uncharted territory? The only reason why I wouldn't like to wait too long with merging this is because of merge conflicts (the list of properties or their details might change and this could go unnoticed). Maybe if we don't want to commit to keeping the ObjectOptions schema, the part that should wait is object-add and I should do the command line options first? Then the schema remains an implementation detail for now that is invisible in introspection. > In the meanwhile, of course I have no problem with deprecating the opened > and loaded properties. If we decide that we don't want to have the schema at all (which I hope we won't decide), I can split the deprecation in
Re: [PATCH 00/18] qapi/qom: QAPIfy object-add
On Mon, Nov 30, 2020 at 03:58:23PM +0100, Paolo Bonzini wrote:
> On 30/11/20 13:25, Kevin Wolf wrote:
> > This series adds a QAPI type for the properties of all user creatable
> > QOM types and finally makes QMP object-add use the new ObjectOptions
> > union so that QAPI introspection can be used for user creatable objects.
> >
> > After this series, there is least one obvious next step that needs to be
> > done: Change HMP and all of the command line parser to use
> > ObjectOptions, too, so that the QAPI schema is consistently enforced in
> > all external interfaces. I am planning to send another series to address
> > this.
> >
> > In a third step, we can try to start deduplicating and integrating things
> > better between QAPI and the QOM implementation, e.g. by generating parts
> > of the QOM boilerplate from the QAPI schema.
>
> With this series it's basically pointless to have QOM properties at all.
> Instead, you are basically having half of QEMU's backend data model into a
> single struct.
>
> So the question is, are we okay with shoveling half of QEMU's backend data
> model into a single struct? If so, there are important consequences.
In theory they should have the same set of options, but nothing in
this series will enforce that. So we're introducing the danger that
QMP object-add will miss some property, and thus be less functional
than the CLI -object. If we convert CLI -object to use the QAPI
parser too, we eliminate that danger, but we still have the struct
duplication.
> 1) QOM basically does not need properties anymore except for devices and
> machines (accelerators could be converted to QAPI as well). All
> user-creatable objects can be changed to something like chardev's "get a
> struct and use it fill in the fields", and only leave properties to devices
> and machines.
>
> 2) User-creatable objects can have a much more flexible schema. This means
> there's no reason to have block device creation as its own command and
> struct for example.
>
> The problem with this series is that you are fine with deduplicating things
> as a third step, but you cannot be sure that such deduplication is possible
> at all. So while I don't have any problems in principle with the
> ObjectOptions concept, I don't think it should be committed without a clear
> idea of how to do the third step.
I feel like we should at least aim to kill the struct duplication, even if
we ignore the bigger QOM stuff like setters/getters/constructors/etc. The
generated structs are not far off being usable.
eg for the secret object we have the QAPI schema
{ 'struct': 'SecretCommonProperties',
'data': { '*loaded': { 'type': 'bool', 'features': ['deprecated'] },
'*format': 'QCryptoSecretFormat',
'*keyid': 'str',
'*iv': 'str' } }
{ 'struct': 'SecretProperties',
'base': 'SecretCommonProperties',
'data': { '*data': 'str',
'*file': 'str' } }
IIUC this will resulting in a QAPI generated flattened struct:
struct SecretProperties {
bool loaded;
QCryptoSecretFormat format;
char *keyid;
char *iv;
char *data;
char *file;
};
vs the QOM manually written structs
struct QCryptoSecretCommon {
Object parent_obj;
uint8_t *rawdata;
size_t rawlen;
QCryptoSecretFormat format;
char *keyid;
char *iv;
};
struct QCryptoSecret {
QCryptoSecretCommon parent_obj;
char *data;
char *file;
};
The key differences
- The parent struct is embedded, rather than flattened
- The "loaded" property doesn't need to exist
- Some extra fields are live state (rawdata, rawlen)
Lets pretend we just kill "loaded" entirely, so ignore that.
We could simply make QOM "Object" a well known built-in type, so
we can reference it as a "parent". Then any struct with "Object"
as a parent could use struct embedding rather flattening and thus
just work.
Can we invent a "state" field for fields that are internal
only, separate from the public "data" fields.
eg the secret QAPI def would only need a couple of changes:
{ 'struct': 'QCryptoSecretCommon',
'base': 'Object',
'state': { 'rawdata': '*uint8_t',
'rawlen': 'size_t' },
'data': { '*format': 'QCryptoSecretFormat',
'*keyid': 'str',
'*iv': 'str' } }
{ 'struct': 'QCryptoSecret',
'base': 'QCryptoSecretCommon',
'data': { '*data': 'str',
'*file': 'str' } }
There would need to be a
void QCryptoSecretCommonFreeState(QCryptoSecretCommon *obj)
method defined manually by the programmer to take care of free'ing any
pointers in the "state".
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH 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 01/18] qapi/qom: Add ObjectOptions for iothread
On 30/11/20 13:25, Kevin Wolf wrote:
+##
+# @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)
+#
+# Since: 6.0
+##
+{ 'struct': 'IothreadProperties',
+ 'data': { '*poll-max-ns': 'int',
+'*poll-grow': 'int',
+'*poll-shrink': 'int' } }
+
Documentation is the main advantage of the ObjectOptions concept.
However, please use the version where each object and property was
introduced for the "since" value. Otherwise the documentation will
appear to show that none of these objects was available before 6.0.
Yes, there is no documentation at all right now for QOM objects.
However, wrong documentation sometimes is worse than non-existing
documentation.
Paolo
Re: [PATCH 00/18] qapi/qom: QAPIfy object-add
On 30/11/20 13:25, Kevin Wolf wrote: This series adds a QAPI type for the properties of all user creatable QOM types and finally makes QMP object-add use the new ObjectOptions union so that QAPI introspection can be used for user creatable objects. After this series, there is least one obvious next step that needs to be done: Change HMP and all of the command line parser to use ObjectOptions, too, so that the QAPI schema is consistently enforced in all external interfaces. I am planning to send another series to address this. In a third step, we can try to start deduplicating and integrating things better between QAPI and the QOM implementation, e.g. by generating parts of the QOM boilerplate from the QAPI schema. With this series it's basically pointless to have QOM properties at all. Instead, you are basically having half of QEMU's backend data model into a single struct. So the question is, are we okay with shoveling half of QEMU's backend data model into a single struct? If so, there are important consequences. 1) QOM basically does not need properties anymore except for devices and machines (accelerators could be converted to QAPI as well). All user-creatable objects can be changed to something like chardev's "get a struct and use it fill in the fields", and only leave properties to devices and machines. 2) User-creatable objects can have a much more flexible schema. This means there's no reason to have block device creation as its own command and struct for example. The problem with this series is that you are fine with deduplicating things as a third step, but you cannot be sure that such deduplication is possible at all. So while I don't have any problems in principle with the ObjectOptions concept, I don't think it should be committed without a clear idea of how to do the third step. In the meanwhile, of course I have no problem with deprecating the opened and loaded properties. Paolo
Re: [PATCH libvirt v3 03/11] nodedev: detect AP queues
On Tue, Nov 17, 2020 at 01:10:57PM +0100, Shalini Chellathurai Saroja wrote:
> Each AP card device can support upto 256 AP queues. AP queues are
> also detected by udev, so add support for libvirt nodedev driver.
>
> Signed-off-by: Farhan Ali
> Signed-off-by: Shalini Chellathurai Saroja
> Reviewed-by: Bjoern Walk
> Reviewed-by: Boris Fiuczynski
> ---
> docs/formatnode.html.in| 15 +++
> docs/schemas/nodedev.rng | 25 +++
> src/conf/node_device_conf.c| 66 ++
> src/conf/node_device_conf.h| 9
> src/conf/virnodedeviceobj.c| 1 +
> src/node_device/node_device_udev.c | 30 ++
> tools/virsh-nodedev.c | 1 +
> 7 files changed, 147 insertions(+)
>
...
> +static int
> +virNodeDevCapAPQueueParseXML(xmlXPathContextPtr ctxt,
> +virNodeDeviceDefPtr def,
> +xmlNodePtr node,
> +virNodeDevCapAPQueuePtr ap_queue)
> +{
> +xmlNodePtr orig;
We could benefit from VIR_XPATH_NODE_AUTORESTORE(ctxt) here as well.
> +int ret = -1;
> +g_autofree char *adapter = NULL, *dom = NULL;
^One definition per line please.
> +
> +orig = ctxt->node;
> +ctxt->node = node;
> +
> + if (!(adapter = virXPathString("string(./ap-adapter[1])", ctxt))) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("missing ap-adapter value for '%s'"), def->name);
> +return -1;
> +}
> +
> +if (virStrToLong_uip(adapter, NULL, 0, &ap_queue->ap_adapter) < 0) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("invalid ap-adapter value '%s' for '%s'"),
> + adapter, def->name);
> +goto out;
> +}
Like I mentioned in patch 1, we could extract ^this into a tiny static helper.
Regards,
Erik
Re: [PATCH libvirt v3 01/11] nodedev: detect AP card device
On Tue, Nov 17, 2020 at 01:10:55PM +0100, Shalini Chellathurai Saroja wrote:
> Introduce support for the Adjunct Processor (AP) crypto card device.
> Udev already detects the device, so add support for libvirt nodedev
> driver.
>
> Signed-off-by: Farhan Ali
> Signed-off-by: Shalini Chellathurai Saroja
> Reviewed-by: Bjoern Walk
> Reviewed-by: Boris Fiuczynski
> ---
> docs/formatnode.html.in| 8 ++
> docs/schemas/nodedev.rng | 10
> src/conf/node_device_conf.c| 41 ++
> src/conf/node_device_conf.h| 8 ++
> src/conf/virnodedeviceobj.c| 1 +
> src/node_device/node_device_udev.c | 29 +
> tools/virsh-nodedev.c | 1 +
> 7 files changed, 98 insertions(+)
>
...
> +static int
> +virNodeDevCapAPCardParseXML(xmlXPathContextPtr ctxt,
> +virNodeDeviceDefPtr def,
> +xmlNodePtr node,
> +virNodeDevCapAPCardPtr ap_card)
> +{
> +xmlNodePtr orig;
> +g_autofree char *adapter = NULL;
> +
> +orig = ctxt->node;
> +ctxt->node = node;
> +
> +if (!(adapter = virXPathString("string(./ap-adapter[1])", ctxt))) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("missing ap-adapter value for '%s'"), def->name);
> +return -1;
> +}
> +
> +if (virStrToLong_uip(adapter, NULL, 0, &ap_card->ap_adapter) < 0) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("invalid ap-adapter value '%s' for '%s'"),
> + adapter, def->name);
> +return -1;
> +}
> +
> +ctxt->node = orig;
^this restore will not happen if any of the above fails. I'd suggest using the
VIR_XPATH_NODE_AUTORESTORE(ctxt) macro which already adopts our latest g_auto*
standard.
In context of patch 3, I'd also suggest to extract the adapter parsing logic
into another static helper function e.g. virNodeDevAPAdapterParseXML which
could be reused later from virNodeDevCapAPQueueParseXML to avoid duplication.
Erik
Re: [PATCH libvirt v2 06/11] nodedev: detect AP matrix device
On Fri, Nov 13, 2020 at 03:45:33PM +0100, Shalini Chellathurai Saroja wrote:
>
> On 11/12/20 9:29 PM, Jonathon Jongsma wrote:
> > > diff --git a/src/node_device/node_device_udev.c
> > > b/src/node_device/node_device_udev.c index 6bbff571..5f57000e 100644
> > > --- a/src/node_device/node_device_udev.c
> > > +++ b/src/node_device/node_device_udev.c
> > > @@ -1241,6 +1241,25 @@ udevProcessAPQueue(struct udev_device *device,
> > > }
> > > +static int
> > > +udevProcessAPMatrix(struct udev_device *device,
> > > +virNodeDeviceDefPtr def)
> > > +{
> > > +size_t i;
> > > +virNodeDevCapDataPtr data = &def->caps->data;
> > > +
> > > +data->ap_matrix.addr =
> > > g_strdup(udev_device_get_sysname(device));
> > > +def->name = g_strdup_printf("ap_%s", data->ap_matrix.addr);
> > So you're setting this 'addr' field, but it is not used anywhere else in
> > this patch. Perhaps you'll use it in upcoming patches. But it is not
> > formatted into the node device xml or anything like that. Is that
> > intentional?
> >
> Yes, it is not formatted into the node device xml.
>
> I will include this change in the patch 10.
>
> >
> > > +
> > > +for (i = 0; i < strlen(def->name); i++) {
> > > +if (!(g_ascii_isalnum(*(def->name + i
> > > +*(def->name + i) = '_';
> > > +}
> > > +
> > > +return 0;
> > > +}
> > Out of curiosity, what's the reason that you're
> > hard-coding an "ap_" prefix to the nodedev name rather than just using
> > udevGenerateDeviceName() like all of the other device types?
> The name generated with udevGenerateDeviceName() is matrix_matrix (as both
> udev_device_get_subsystem and udev_device_get_sysname returns matrix), which
> is not very helpful, so we decided to go with ap_matrix instead.
But if that is the case, I'm wondering why are you trying to generate it
dynamically in the first place, why not hardcoding it to "ap_matrix"? I must be
missing something here.
Regards,
Erik
Re: [libvirt PATCH] vircgroupv2: fix virCgroupV2DenyDevice
On 11/30/20 8:22 AM, Pavel Hrdina wrote: The original logic is incorrect. We would delete the device entry from eBPF map only if the newval would be same as current val in the map. In case that the device was allowed only as read-only but later we remove all permissions for that device it would remain in the table with empty values. The old code would still deny the device but it's not working as intended. Instead we will update the value in advance. If the updated value is 0 it means that we are removing all permissions so it should be removed from the map, otherwise we will update the value in map. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1810356 Signed-off-by: Pavel Hrdina --- src/util/vircgroupv2.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) Reviewed-by: Michal Privoznik Michal
Re: [PATCH] qemu: Relax memory pre-allocation rules
On 11/30/20 12:14 PM, Daniel P. Berrangé wrote: On Mon, Nov 30, 2020 at 11:48:28AM +0100, Michal Privoznik wrote: On 11/30/20 11:16 AM, Daniel P. Berrangé wrote: On Mon, Nov 30, 2020 at 11:06:14AM +0100, Michal Privoznik wrote: Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1894053 Signed-off-by: Michal Privoznik The difference is that an real NVDIMM is inherantly preallocated. QEMU would not be ignoring the prealloc=yes arg - its implementation would merely be a no-op. Okay, let me switch the bug back onto qemu. Michal
Re: Adding an nftables backend in addition to iptables?
On Sat, Nov 28, 2020 at 04:39:26PM +0100, Aljoscha Lautenbach wrote: > Hi! > > First of all, thanks for your work on libvirt, it is highly appreciated! > > When I wanted to create a new VM using virt-manager on my Debian > Testing machine yesterday, I ran into the following problem: > > ~# virsh net-list --all > Name State Autostart Persistent > -- > default inactive yes yes > > ~# virsh net-start default > error: Failed to start network default > error: internal error: Failed to apply firewall rules > /usr/sbin/iptables --table filter --list-rules: iptables v1.8.6 > (nf_tables): table `filter' is incompatible, use 'nft' tool. > > It turns out the Debian package for iptables includes two versions of > iptables: iptables-nft and iptables-legacy. It looks like iptables-nft > has been the default in Debian for a while, which led to the error > above. This doesn't make much sense. The whole point of iptables-nft is that apps can continue using the (fake) iptables userspace tools and they magically turn into NFT rules at the kernel level. IOW, libvirt should "just work" with both iptables-legacy and iptables-nft - that's certainly the case on Fedora/RHEL, so I wonder what's broken on Debian to cause this error message. > After setting iptables-legacy to be the default and restarting the > libvirtd service, everything worked as expected. > > But it did make me wonder, are there any plans to add a backend for nftables? Regardless of whether iptables-nft works or not, at some point it would be nice to directly use the "nft" tool for creating rules. We don't have anyone with active plans to work on this, so there's no ETA though. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Adding an nftables backend in addition to iptables?
Hi! First of all, thanks for your work on libvirt, it is highly appreciated! When I wanted to create a new VM using virt-manager on my Debian Testing machine yesterday, I ran into the following problem: ~# virsh net-list --all Name State Autostart Persistent -- default inactive yes yes ~# virsh net-start default error: Failed to start network default error: internal error: Failed to apply firewall rules /usr/sbin/iptables --table filter --list-rules: iptables v1.8.6 (nf_tables): table `filter' is incompatible, use 'nft' tool. It turns out the Debian package for iptables includes two versions of iptables: iptables-nft and iptables-legacy. It looks like iptables-nft has been the default in Debian for a while, which led to the error above. After setting iptables-legacy to be the default and restarting the libvirtd service, everything worked as expected. But it did make me wonder, are there any plans to add a backend for nftables? Thanks, Aljoscha
[PATCH] coding-style: Document 80 chars limit for line length
The idea is to have it like a soft limit: if possible then break
lines, if not then have a long line instead of some creative
approach.
Signed-off-by: Michal Privoznik
---
docs/coding-style.rst | 14 +-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/docs/coding-style.rst b/docs/coding-style.rst
index cfd7b16638..813128bfb6 100644
--- a/docs/coding-style.rst
+++ b/docs/coding-style.rst
@@ -131,7 +131,7 @@ around operators and keywords:
indent-libvirt()
{
-indent -bad -bap -bbb -bli4 -br -ce -brs -cs -i4 -l75 -lc75 \
+indent -bad -bap -bbb -bli4 -br -ce -brs -cs -i4 -l80 -lc80 \
-sbi4 -psl -saf -sai -saw -sbi4 -ss -sc -cdw -cli4 -npcs -nbc \
--no-tabs "$@"
}
@@ -141,6 +141,18 @@ further, by piping it through ``expand -i``, since some
leading
TABs can get through. Usually they're in macro definitions or
strings, and should be converted anyhow.
+The recommended length for lines is 80 characters, but common sense
+should prevail. It may get tricky around some names (because of how
+Libvirt constructs names for functions/enums/etc.)
+
+::
+
+ foo(
+ arg1, arg2
+ );// Bad
+ foo(arg1,
+ arg2);// Good
+
Libvirt requires a C99 compiler for various reasons. However, most
of the code base prefers to stick to C89 syntax unless there is a
compelling reason otherwise. For example, it is preferable to use
--
2.26.2
Re: [PATCH v1 05/26] qemu_domain_address: Reformat qemuDomainAssignS390Addresses()
On 11/30/20 12:48 PM, Thomas Huth wrote:
On 30/11/2020 11.18, Michal Privoznik wrote:
On 11/30/20 10:38 AM, Thomas Huth wrote:
On 27/11/2020 16.02, Michal Privoznik wrote:
Signed-off-by: Michal Privoznik
---
src/qemu/qemu_domain_address.c | 10 --
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index 2788dc7fb3..d872f75b38 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -408,18 +408,16 @@ qemuDomainAssignS390Addresses(virDomainDefPtr def,
if (qemuDomainIsS390CCW(def) &&
virQEMUCapsGet(qemuCaps, QEMU_CAPS_CCW)) {
if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VFIO_CCW))
- qemuDomainPrimeVfioDeviceAddresses(
- def, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW);
- qemuDomainPrimeVirtioDeviceAddresses(
- def, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW);
+ qemuDomainPrimeVfioDeviceAddresses(def,
VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW);
Looks fine to me, but docs/coding-style.rst still suggest to format code
with "indent -l75", so is this really the right thing to do here?
It's true that we have 80 characters limit, but that is more of a soft limit
and common sense should be used. Personally, I find
func(
arg1, arg2
);
worse than exceeding that 80 char rule. My common sense tells me that the
rule tries to avoid the following pattern (among others):
func(arg1, arg2, , very_long_list_of_arguments, which, could, easily,
go_on_multiple_lines, but, didnt);
Fair point, but then this should IMHO be reflected in the coding-style doc
first. Otherwise the next contributor to this file might simply undo your
change to fit everything again into the 80 (or even 75) columns limit...?
I can try. But these coding style rules are always hard to get right.
There is always some counter example. Well, let me try.
Michal
[PATCH 18/18] qapi/qom: QAPIfy object-add
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 +++--
5 files changed, 32 insertions(+), 29 deletions(-)
diff --git a/qapi/qom.json b/qapi/qom.json
index 7e0d26a728..f6c6ba6748 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -809,13 +809,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
#
@@ -829,9 +822,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 }
##
# @object-del:
diff --git a/include/qom/object_interfaces.h b/include/qom/object_interfaces.h
index 07d5cc8832..9b9938b8c0 100644
--- a/include/qom/object_interfaces.h
+++ b/include/qom/object_interfaces.h
@@ -196,11 +196,4 @@ bool user_creatable_del(const char *id, Error **errp);
*/
void user_creatable_cleanup(void);
-/**
- * qmp_object_add:
- *
- * QMP command handler for object-add. See the QAPI schema for documentation.
- */
-void qmp_object_add(QDict *qdict, QObject **ret_data, Error **errp);
-
#endif
diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
index 8a7a3f5452..d8960ff55b 100644
--- a/hw/block/xen-block.c
+++ b/hw/block/xen-block.c
@@ -842,17 +842,17 @@ static XenBlockIOThread *xen_block_iothread_create(const
char *id,
{
ERRP_GUARD();
XenBlockIOThread *iothread = g_new(XenBlockIOThread, 1);
-QDict *opts;
-QObject *ret_data = NULL;
+ObjectOptions *opts;
iothread->id = g_strdup(id);
-opts = qdict_new();
-qdict_put_str(opts, "qom-type", TYPE_IOTHREAD);
-qdict_put_str(opts, "id", id);
-qmp_object_add(opts, &ret_data, errp);
-qobject_unref(opts);
-qobject_unref(ret_data);
+opts = g_new(ObjectOptions, 1);
+*opts = (ObjectOptions) {
+.qom_type = OBJECT_TYPE_IOTHREAD,
+.id = g_strdup(id),
+};
+qmp_object_add(opts, errp);
+qapi_free_ObjectOptions(opts);
if (*errp) {
g_free(iothread->id);
diff --git a/monitor/misc.c b/monitor/misc.c
index 398211a034..0586ee1f4f 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -240,8 +240,6 @@ static void monitor_init_qmp_commands(void)
qmp_query_qmp_schema, QCO_ALLOW_PRECONFIG);
qmp_register_command(&qmp_commands, "device_add", qmp_device_add,
QCO_NO_OPTIONS);
-qmp_register_command(&qmp_commands, "object-add", qmp_object_add,
- QCO_NO_OPTIONS);
QTAILQ_INIT(&qmp_cap_negotiation_commands);
qmp_register_command(&qmp_cap_negotiation_commands, "qmp_capabilities",
diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c
index 0e7d7247fc..5c73f04913 100644
--- a/qom/qom-qmp-cmds.c
+++ b/qom/qom-qmp-cmds.c
@@ -19,8 +19,11 @@
#include "qapi/error.h"
#include "qapi/qapi-commands-qdev.h"
#include "qapi/qapi-commands-qom.h"
+#include "qapi/qapi-visit-qom.h"
#include "qapi/qmp/qdict.h"
#include "qapi/qmp/qerror.h"
+#include "qapi/qobject-input-visitor.h"
+#include "qapi/qobject-output-visitor.h"
#include "qemu/cutils.h"
#include "qom/object_interfaces.h"
#include "qom/qom-qobject.h"
@@ -241,9 +244,27 @@ ObjectPropertyInfoList *qmp_qom_list_properties(const char
*typename,
return prop_list;
}
-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(&qobj);
+visit_type_ObjectOptions(v, NULL, &options, &error_abort);
+visit_complete(v, &qobj);
+visit_free(v);
+
+props = qobject_to(QDict, qobj);
+qdict_del(props, "qom-type");
+qdict_del(props, "id");
+
+v = qobject_input_visitor_new(QOBJECT(props));
+obj = user_creatable_add_type(ObjectType_str(options
[PATCH 17/18] qapi/qom: Drop deprecated 'props' from object-add
The option has been deprecated in QEMU 5.0, remove it.
Signed-off-by: Kevin Wolf
---
qapi/qom.json | 6 +-
docs/system/deprecated.rst | 10 +-
qom/qom-qmp-cmds.c | 21 -
3 files changed, 6 insertions(+), 31 deletions(-)
diff --git a/qapi/qom.json b/qapi/qom.json
index 29b229394e..7e0d26a728 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -813,10 +813,6 @@
#
# @id: the name of the new object
#
-# @props: a dictionary of properties to be passed to the backend. Deprecated
-# since 5.0, specify the properties on the top level instead. It is an
-# error to specify the same option both on the top level and in @props.
-#
# Additional arguments depend on qom-type and are passed to the backend
# unchanged.
#
@@ -834,7 +830,7 @@
#
##
{ 'command': 'object-add',
- 'data': {'qom-type': 'str', 'id': 'str', '*props': 'any'},
+ 'data': {'qom-type': 'str', 'id': 'str'},
'gen': false } # so we can get the additional arguments
##
diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
index 04e41254f9..e6043799a4 100644
--- a/docs/system/deprecated.rst
+++ b/docs/system/deprecated.rst
@@ -224,11 +224,6 @@ Use ``migrate-set-parameters`` and
``query-migrate-parameters`` instead.
Use arguments ``base-node`` and ``top-node`` instead.
-``object-add`` option ``props`` (since 5.0)
-'''
-
-Specify the properties for the object as top-level arguments instead.
-
``query-named-block-nodes`` and ``query-block`` result dirty-bitmaps[i].status
(since 4.0)
''
@@ -558,6 +553,11 @@ are automatically loaded from qcow2 images.
Use ``device_add`` for hotplugging vCPUs instead of ``cpu-add``. See
documentation of ``query-hotpluggable-cpus`` for additional details.
+``object-add`` option ``props`` (removed in 6.0)
+
+
+Specify the properties for the object as top-level arguments instead.
+
Human Monitor Protocol (HMP) commands
-
diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c
index 310ab2d048..0e7d7247fc 100644
--- a/qom/qom-qmp-cmds.c
+++ b/qom/qom-qmp-cmds.c
@@ -243,27 +243,6 @@ ObjectPropertyInfoList *qmp_qom_list_properties(const char
*typename,
void qmp_object_add(QDict *qdict, QObject **ret_data, Error **errp)
{
-QObject *props;
-QDict *pdict;
-
-props = qdict_get(qdict, "props");
-if (props) {
-pdict = qobject_to(QDict, props);
-if (!pdict) {
-error_setg(errp, QERR_INVALID_PARAMETER_TYPE, "props", "dict");
-return;
-}
-qobject_ref(pdict);
-qdict_del(qdict, "props");
-qdict_join(qdict, pdict, false);
-if (qdict_size(pdict) != 0) {
-error_setg(errp, "Option in 'props' conflicts with top level");
-qobject_unref(pdict);
-return;
-}
-qobject_unref(pdict);
-}
-
user_creatable_add_dict(qdict, false, errp);
}
--
2.28.0
[PATCH 16/18] tests: Drop 'props' from object-add calls
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
---
tests/qtest/qmp-cmd-test.c | 16 +--
tests/qtest/test-netfilter.c | 54
tests/qemu-iotests/087 | 8 ++
tests/qemu-iotests/184 | 18
tests/qemu-iotests/218 | 2 +-
tests/qemu-iotests/235 | 2 +-
tests/qemu-iotests/245 | 4 +--
tests/qemu-iotests/258 | 6 ++--
tests/qemu-iotests/258.out | 4 +--
tests/qemu-iotests/295 | 2 +-
tests/qemu-iotests/296 | 2 +-
11 files changed, 51 insertions(+), 67 deletions(-)
diff --git a/tests/qtest/qmp-cmd-test.c b/tests/qtest/qmp-cmd-test.c
index 1c7186e53c..c98b78d033 100644
--- a/tests/qtest/qmp-cmd-test.c
+++ b/tests/qtest/qmp-cmd-test.c
@@ -230,14 +230,14 @@ static void test_object_add_failure_modes(void)
/* attempt to create 2 objects with duplicate id */
resp = qtest_qmp(qts, "{'execute': 'object-add', 'arguments':"
" {'qom-type': 'memory-backend-ram', 'id': 'ram1',"
- " 'props': {'size': 1048576 } } }");
+ " 'size': 1048576 } }");
g_assert_nonnull(resp);
g_assert(qdict_haskey(resp, "return"));
qobject_unref(resp);
resp = qtest_qmp(qts, "{'execute': 'object-add', 'arguments':"
" {'qom-type': 'memory-backend-ram', 'id': 'ram1',"
- " 'props': {'size': 1048576 } } }");
+ " 'size': 1048576 } }");
g_assert_nonnull(resp);
qmp_expect_error_and_unref(resp, "GenericError");
@@ -251,14 +251,14 @@ static void test_object_add_failure_modes(void)
/* attempt to create an object with a property of a wrong type */
resp = qtest_qmp(qts, "{'execute': 'object-add', 'arguments':"
" {'qom-type': 'memory-backend-ram', 'id': 'ram1',"
- " 'props': {'size': '1048576' } } }");
+ " 'size': '1048576' } }");
g_assert_nonnull(resp);
/* now do it right */
qmp_expect_error_and_unref(resp, "GenericError");
resp = qtest_qmp(qts, "{'execute': 'object-add', 'arguments':"
" {'qom-type': 'memory-backend-ram', 'id': 'ram1',"
- " 'props': {'size': 1048576 } } }");
+ " 'size': 1048576 } }");
g_assert_nonnull(resp);
g_assert(qdict_haskey(resp, "return"));
qobject_unref(resp);
@@ -273,14 +273,14 @@ static void test_object_add_failure_modes(void)
/* attempt to create an object without the id */
resp = qtest_qmp(qts, "{'execute': 'object-add', 'arguments':"
" {'qom-type': 'memory-backend-ram',"
- " 'props': {'size': 1048576 } } }");
+ " 'size': 1048576 } }");
g_assert_nonnull(resp);
qmp_expect_error_and_unref(resp, "GenericError");
/* now do it right */
resp = qtest_qmp(qts, "{'execute': 'object-add', 'arguments':"
" {'qom-type': 'memory-backend-ram', 'id': 'ram1',"
- " 'props': {'size': 1048576 } } }");
+ " 'size': 1048576 } }");
g_assert_nonnull(resp);
g_assert(qdict_haskey(resp, "return"));
qobject_unref(resp);
@@ -295,14 +295,14 @@ static void test_object_add_failure_modes(void)
/* attempt to set a non existing property */
resp = qtest_qmp(qts, "{'execute': 'object-add', 'arguments':"
" {'qom-type': 'memory-backend-ram', 'id': 'ram1',"
- " 'props': {'sized': 1048576 } } }");
+ " 'sized': 1048576 } }");
g_assert_nonnull(resp);
qmp_expect_error_and_unref(resp, "GenericError");
/* now do it right */
resp = qtest_qmp(qts, "{'execute': 'object-add', 'arguments':"
" {'qom-type': 'memory-backend-ram', 'id': 'ram1',"
- " 'props': {'size': 1048576 } } }");
+ " 'size': 1048576 } }");
g_assert_nonnull(resp);
g_assert(qdict_haskey(resp, "return"));
qobject_unref(resp);
diff --git a/tests/qtest/test-netfilter.c b/tests/qtest/test-netfilter.c
index 22927ee6ab..785b6f3226 100644
--- a/tests/qtest/test-netfilter.c
+++ b/tests/qtest/test-netfilter.c
@@ -21,11 +21,10 @@ static void add_one_netfilter(void)
" 'arguments': {"
" 'qom-type': 'filter-buffer',"
" 'id': 'qtest-f0',"
- " 'props': {"
- " 'netdev': 'qtest-bn0',"
- " 'queue': 'rx',"
- " 'interval': 1000"
- "}}}");
+ " 'netdev': 'qtest-bn0',"
+ " 'queue': 'rx',"
+ " 'interval': 1000"
+ "}}");
g_assert(response);
g_assert(!qdict_haskey(response, "err
[PATCH 15/18] qapi/qom: Add ObjectOptions for input-*
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(-)
diff --git a/qapi/common.json b/qapi/common.json
index b87e7f9039..7c976296f0 100644
--- a/qapi/common.json
+++ b/qapi/common.json
@@ -185,3 +185,15 @@
##
{ 'enum': 'NetFilterDirection',
'data': [ 'all', 'rx', 'tx' ] }
+
+##
+# @GrabToggleKeys:
+#
+# Keys to toggle input-linux between host and guest.
+#
+# Since: 4.0
+#
+##
+{ 'enum': 'GrabToggleKeys',
+ 'data': [ 'ctrl-ctrl', 'alt-alt', 'shift-shift','meta-meta', 'scrolllock',
+'ctrl-scrolllock' ] }
diff --git a/qapi/qom.json b/qapi/qom.json
index 43d081cb42..29b229394e 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -444,6 +444,60 @@
'base': 'NetfilterProperties',
'data': { '*vnet_hdr_support': 'bool' } }
+##
+# @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")
+#
+# @x-origin: x coordinate of the leftmost pixel on the guest screen
+#(default: "0")
+#
+# @y-origin: y coordinate of he topmost pixel on the guest screen (default:
"0")
+#
+# @width: the width of secondary screen in pixels (default: "1920")
+#
+# @height: the height of secondary screen in pixels (default: "1080")
+#
+# Since: 6.0
+##
+{ 'struct': 'InputBarrierProperties',
+ 'data': { 'name': 'str',
+'*server': 'str',
+'*port': 'str',
+'*x-origin': 'str',
+'*y-origin': 'str',
+'*width': 'str',
+'*height': 'str' } }
+
+##
+# @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)
+#
+# @repeat: enables auto-repeat events (default: false)
+#
+# @grab-toggle: the key or key combination that toggles device grab
+# (default: ctrl-ctrl)
+#
+# Since: 6.0
+##
+{ 'struct': 'InputLinuxProperties',
+ 'data': { 'evdev': 'str',
+'*grab_all': 'bool',
+'*repeat': 'bool',
+'*grab-toggle': 'GrabToggleKeys' } }
+
##
# @IothreadProperties:
#
@@ -677,6 +731,8 @@
'filter-redirector',
'filter-replay',
'filter-rewriter',
+'input-barrier',
+'input-linux',
'iothread',
'memory-backend-file',
'memory-backend-memfd',
@@ -727,6 +783,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',
diff --git a/qapi/ui.json b/qapi/ui.json
index 6c7b33cb72..bdbf6d076d 100644
--- a/qapi/ui.json
+++ b/qapi/ui.json
@@ -6,6 +6,7 @@
# = Remote desktop
##
+{ 'include': 'common.json' }
{ 'include': 'sockets.json' }
##
@@ -1021,18 +1022,6 @@
'*head' : 'int',
'events' : [ 'InputEvent' ] } }
-##
-# @GrabToggleKeys:
-#
-# Keys to toggle input-linux between host and guest.
-#
-# Since: 4.0
-#
-##
-{ 'enum': 'GrabToggleKeys',
- 'data': [ 'ctrl-ctrl', 'alt-alt', 'shift-shift','meta-meta', 'scrolllock',
-'ctrl-scrolllock' ] }
-
##
# @DisplayGTK:
#
--
2.28.0
[PATCH 14/18] qapi/qom: Add ObjectOptions for sev-guest
This adds a QAPI schema for the properties of the sev-guest object.
Signed-off-by: Kevin Wolf
---
qapi/qom.json | 35 +++
1 file changed, 35 insertions(+)
diff --git a/qapi/qom.json b/qapi/qom.json
index 24bfa83af5..43d081cb42 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -621,6 +621,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)
+#
+# @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
+#
+# Since: 6.0
+##
+{ '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:
#
@@ -655,6 +687,7 @@
'rng-random',
'secret',
'secret_keyring',
+{'name': 'sev-guest', 'if': 'defined(CONFIG_SEV)' },
'throttle-group',
'tls-creds-anon',
'tls-creds-psk',
@@ -704,6 +737,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',
--
2.28.0
[PATCH 13/18] qapi/qom: Add ObjectOptions for pr-manager-helper
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 c8ee02081c..24bfa83af5 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -563,6 +563,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: 6.0
+##
+{ 'struct': 'PrManagerHelperProperties',
+ 'data': { 'path': 'str' } }
+
##
# @RngProperties:
#
@@ -637,6 +649,7 @@
'memory-backend-file',
'memory-backend-memfd',
'memory-backend-ram',
+'pr-manager-helper',
'rng-builtin',
'rng-egd',
'rng-random',
@@ -685,6 +698,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',
--
2.28.0
[PATCH 12/18] qapi/qom: Add ObjectOptions for filter-*
This adds a QAPI schema for the properties of the filter-* objects.
Some parts of the interface (in particular NetfilterProperties.position)
are very unusual for QAPI, but for now just describe the existing
interface.
net.json can't be included in qom.json because the storage daemon
doesn't have it. NetFilterDirection is still required in the new object
property definitions in qom.json, so move this enum to common.json.
Signed-off-by: Kevin Wolf
---
qapi/common.json | 20 +++
qapi/net.json| 20 ---
qapi/qom.json| 143 +++
3 files changed, 163 insertions(+), 20 deletions(-)
diff --git a/qapi/common.json b/qapi/common.json
index 2dad4fadc3..b87e7f9039 100644
--- a/qapi/common.json
+++ b/qapi/common.json
@@ -165,3 +165,23 @@
##
{ 'enum': 'HostMemPolicy',
'data': [ 'default', 'preferred', 'bind', 'interleave' ] }
+
+##
+# @NetFilterDirection:
+#
+# Indicates whether a netfilter is attached to a netdev's transmit queue or
+# receive queue or both.
+#
+# @all: the filter is attached both to the receive and the transmit
+# queue of the netdev (default).
+#
+# @rx: the filter is attached to the receive queue of the netdev,
+# where it will receive packets sent to the netdev.
+#
+# @tx: the filter is attached to the transmit queue of the netdev,
+# where it will receive packets sent by the netdev.
+#
+# Since: 2.5
+##
+{ 'enum': 'NetFilterDirection',
+ 'data': [ 'all', 'rx', 'tx' ] }
diff --git a/qapi/net.json b/qapi/net.json
index a3a1336001..e19f6df468 100644
--- a/qapi/net.json
+++ b/qapi/net.json
@@ -492,26 +492,6 @@
'vhost-user': 'NetdevVhostUserOptions',
'vhost-vdpa': 'NetdevVhostVDPAOptions' } }
-##
-# @NetFilterDirection:
-#
-# Indicates whether a netfilter is attached to a netdev's transmit queue or
-# receive queue or both.
-#
-# @all: the filter is attached both to the receive and the transmit
-# queue of the netdev (default).
-#
-# @rx: the filter is attached to the receive queue of the netdev,
-# where it will receive packets sent to the netdev.
-#
-# @tx: the filter is attached to the transmit queue of the netdev,
-# where it will receive packets sent by the netdev.
-#
-# Since: 2.5
-##
-{ 'enum': 'NetFilterDirection',
- 'data': [ 'all', 'rx', 'tx' ] }
-
##
# @RxState:
#
diff --git a/qapi/qom.json b/qapi/qom.json
index 4c4f2841c3..c8ee02081c 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -313,6 +313,137 @@
'data': { 'addr': 'str' ,
'*id-list': 'str' } }
+##
+# @NetfilterInsert:
+#
+# Indicates where to insert a netfilter relative to a given other filter.
+#
+# @before: insert before the specified filter
+#
+# @behind: insert behind the specified filter
+#
+# Since: 6.0
+##
+{ 'enum': 'NetfilterInsert',
+ 'data': [ 'before', 'behind' ] }
+
+##
+# @NetfilterProperties:
+#
+# Properties for objects of classes derived from netfilter.
+#
+# @netdev: id of the network device backend to filter
+#
+# @queue: indicates which queue(s) to filter (default: all)
+#
+# @status: indicates whether the filter is enabled ("on") or disabled ("off")
+# (default: "on")
+#
+# @position: specifies where the filter should be inserted in the filter list.
+#"head" means the filter is inserted at the head of the filter
list,
+#before any existing filters.
+#"tail" means the filter is inserted at the tail of the filter
list,
+#behind any existing filters (default).
+#"id=" means the filter is inserted before or behind the filter
+#specified by , depending on the @insert property.
+#(default: "tail")
+#
+# @insert: where to insert the filter relative to the filter given in
@position.
+# Ignored if @position is "head" or "tail". (default: behind)
+#
+# Since: 6.0
+##
+{ 'struct': 'NetfilterProperties',
+ 'data': { 'netdev': 'str',
+'*queue': 'NetFilterDirection',
+'*status': 'str',
+'*position': 'str',
+'*insert': 'NetfilterInsert' } }
+
+##
+# @FilterBufferProperties:
+#
+# Properties for filter-buffer objects.
+#
+# @interval: a non-zero interval in microseconds. All packets arriving in the
+#given interval are delayed until the end of the interval.
+#
+# Since: 6.0
+##
+{ 'struct': 'FilterBufferProperties',
+ 'base': 'NetfilterProperties',
+ 'data': { 'interval': 'uint32' } }
+
+##
+# @FilterDumpProperties:
+#
+# Properties for filter-dump objects.
+#
+# @file: the filename where the dumped packets should be stored
+#
+# @maxlen: maximum number of bytes in a packet that are stored (default: 65536)
+#
+# Since: 6.0
+##
+{ 'struct': 'FilterDumpProperties',
+ 'base': 'NetfilterProperties',
+ 'data': { 'file': 'str',
+'*maxlen': 'uint32' } }
+
+##
+# @FilterMirrorProperties:
+#
+# Properties for filter-mirror objects.
+#
+# @outdev: the name of a character device backend to which all incoming packets
+#
[PATCH 11/18] qapi/qom: Add ObjectOptions for colo-compare
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 ff9e51ee19..4c4f2841c3 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)
+#
+# @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: 6.0
+##
+{ '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' } }
+
##
# @CryptodevBackendProperties:
#
@@ -444,6 +491,7 @@
'authz-simple',
'can-bus',
'can-host-socketcan',
+'colo-compare',
'cryptodev-backend',
'cryptodev-backend-builtin',
'cryptodev-vhost-user',
@@ -485,6 +533,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',
--
2.28.0
[PATCH 10/18] qapi/qom: Add ObjectOptions for can-*
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.
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 31e381b3bb..ff9e51ee19 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: 6.0
+##
+{ 'struct': 'CanHostSocketcanProperties',
+ 'data': { 'if': 'str',
+'canbus': 'str' } }
+
##
# @CryptodevBackendProperties:
#
@@ -427,6 +442,8 @@
'authz-listfile',
'authz-pam',
'authz-simple',
+'can-bus',
+'can-host-socketcan',
'cryptodev-backend',
'cryptodev-backend-builtin',
'cryptodev-vhost-user',
@@ -467,6 +484,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',
--
2.28.0
[PATCH 09/18] qapi/qom: Add ObjectOptions for tls-*, deprecate 'loaded'
This adds a QAPI schema for the properties of the tls-* objects.
The 'loaded' 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 additional options
will be silently ignored.
In other words, the 'loaded' property is useless. Mark it as deprecated
in the schema from the start.
Signed-off-by: Kevin Wolf
---
qapi/crypto.json | 98
qapi/qom.json| 12 +-
2 files changed, 108 insertions(+), 2 deletions(-)
diff --git a/qapi/crypto.json b/qapi/crypto.json
index 4cb6bb00ed..a32dfa320a 100644
--- a/qapi/crypto.json
+++ b/qapi/crypto.json
@@ -442,3 +442,101 @@
{ 'struct': 'SecretKeyringProperties',
'base': 'SecretCommonProperties',
'data': { 'serial': 'int32' } }
+
+##
+# @TlsCredsProperties:
+#
+# Properties for objects of classes derived from tls-creds.
+#
+# @verify-peer: if true the peer credentials will be verified once the
+# handshake is completed. This is a no-op for anonymous
+# credentials. (default: true)
+#
+# @dir: the path of the directory that contains the credential files
+#
+# @endpoint: whether the QEMU network backend that uses the credentials will be
+#acting as a client or as a server (default: client)
+#
+# @priority: a gnutls priority string as described at
+#https://gnutls.org/manual/html_node/Priority-Strings.html
+#
+# Since: 6.0
+##
+{ 'struct': 'TlsCredsProperties',
+ 'data': { '*verify-peer': 'bool',
+'*dir': 'str',
+'*endpoint': 'QCryptoTLSCredsEndpoint',
+'*priority': 'str' } }
+
+##
+# @TlsCredsAnonProperties:
+#
+# Properties for tls-creds-anon objects.
+#
+# @loaded: if true, the credentials are loaded immediately when applying this
+# option and will ignore options that are processed later. Don't use;
+# only provided for compatibility. (default: false)
+#
+# Features:
+# @deprecated: Member @loaded is deprecated. Setting true doesn't make sense,
+# and false is already the default.
+#
+# Since: 6.0
+##
+{ 'struct': 'TlsCredsAnonProperties',
+ 'base': 'TlsCredsProperties',
+ 'data': { '*loaded': { 'type': 'bool', 'features': ['deprecated'] } } }
+
+##
+# @TlsCredsPskProperties:
+#
+# Properties for tls-creds-psk objects.
+#
+# @loaded: if true, the credentials are loaded immediately when applying this
+# option and will ignore options that are processed later. Don't use;
+# only provided for compatibility. (default: false)
+#
+# @username: the username which will be sent to the server. For clients only.
+#If absent, "qemu" is sent and the property will read back as an
+#empty string.
+#
+# Features:
+# @deprecated: Member @loaded is deprecated. Setting true doesn't make sense,
+# and false is already the default.
+#
+# Since: 6.0
+##
+{ 'struct': 'TlsCredsPskProperties',
+ 'base': 'TlsCredsProperties',
+ 'data': { '*loaded': { 'type': 'bool', 'features': ['deprecated'] },
+'*username': 'str' } }
+
+##
+# @TlsCredsX509Properties:
+#
+# Properties for tls-creds-x509 objects.
+#
+# @loaded: if true, the credentials are loaded immediately when applying this
+# option and will ignore options that are processed later. Don't use;
+# only provided for compatibility. (default: false)
+#
+# @sanity-check: if true, perform some sanity checks before 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: 6.0
+##
+{ 'struct': 'TlsCredsX509Properties',
+ 'base': 'TlsCredsProperties',
+ 'data': { '*loaded': { 'type': 'bool', 'features': ['deprecated'] },
+'*sanity-check': 'bool',
+'*passwordid': 'str' } }
diff --git a/qapi/qom.json b/qapi/qom.json
index 05ec67b0d1..31e381b3bb 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -440,7 +440,11 @@
'rng-random',
'secret',
'secret_keyring',
-'throttle-group'
+'throttle-group',
+'tls-creds-anon',
+'tls-creds-psk',
+'tls-creds-x509',
+'tls-cipher-suites'
] }
##
@@ -476,7 +480,11 @@
'rng-random': 'RngRandomProperties',
'secret': 'SecretProperties',
'secret_keyring': 'SecretKeyringProperties',
- 'throttle-group': 'ThrottleGroupProperties'
+ 'throttle-group': 'ThrottleGroupProperties',
+ '
[PATCH 08/18] qapi/qom: Add ObjectOptions for secret*, deprecate 'loaded'
This adds a QAPI schema for the properties of the secret* objects.
The 'loaded' 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 additional options
will be silently ignored.
In other words, the 'loaded' property is useless. Mark it as deprecated
in the schema from the start.
Signed-off-by: Kevin Wolf
---
qapi/crypto.json | 61 ++
qapi/qom.json | 5
docs/system/deprecated.rst | 10 +++
3 files changed, 76 insertions(+)
diff --git a/qapi/crypto.json b/qapi/crypto.json
index 2aebe6fa20..4cb6bb00ed 100644
--- a/qapi/crypto.json
+++ b/qapi/crypto.json
@@ -381,3 +381,64 @@
'discriminator': 'format',
'data': {
'luks': 'QCryptoBlockAmendOptionsLUKS' } }
+
+##
+# @SecretCommonProperties:
+#
+# Properties for objects of classes derived from secret-common.
+#
+# @loaded: if true, the secret is loaded immediately when applying this option
+# and will probably fail when processing the next option. Don't use;
+# only provided for compatibility. (default: false)
+#
+# @format: the data format that the secret is provided in (default: raw)
+#
+# @keyid: the name of another secret that should be used to decrypt the
+# provided data. If not present, the data is assumed to be unencrypted.
+#
+# @iv: the random initialization vector used for encryption of this particular
+# secret. Should be a base64 encrypted string of the 16-byte IV. Mandatory
+# if @keyid is given. Ignored if @keyid is absent.
+#
+# Features:
+# @deprecated: Member @loaded is deprecated. Setting true doesn't make sense,
+# and false is already the default.
+#
+# Since: 6.0
+##
+{ 'struct': 'SecretCommonProperties',
+ 'data': { '*loaded': { 'type': 'bool', 'features': ['deprecated'] },
+'*format': 'QCryptoSecretFormat',
+'*keyid': 'str',
+'*iv': 'str' } }
+
+##
+# @SecretProperties:
+#
+# Properties for secret objects.
+#
+# Either @data or @file must be provided, but not both.
+#
+# @data: the associated with the secret from
+#
+# @file: the filename to load the data associated with the secret from
+#
+# Since: 6.0
+##
+{ 'struct': 'SecretProperties',
+ 'base': 'SecretCommonProperties',
+ 'data': { '*data': 'str',
+'*file': 'str' } }
+
+##
+# @SecretKeyringProperties:
+#
+# Properties for secret_keyring objects.
+#
+# @serial: serial number that identifies a key to get from the kernel
+#
+# Since: 6.0
+##
+{ 'struct': 'SecretKeyringProperties',
+ 'base': 'SecretCommonProperties',
+ 'data': { 'serial': 'int32' } }
diff --git a/qapi/qom.json b/qapi/qom.json
index 8c9785d2dd..05ec67b0d1 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -7,6 +7,7 @@
{ 'include': 'authz.json' }
{ 'include': 'block-core.json' }
{ 'include': 'common.json' }
+{ 'include': 'crypto.json' }
##
# = QEMU Object Model (QOM)
@@ -437,6 +438,8 @@
'rng-builtin',
'rng-egd',
'rng-random',
+'secret',
+'secret_keyring',
'throttle-group'
] }
@@ -471,6 +474,8 @@
'rng-builtin':'RngProperties',
'rng-egd':'RngEgdProperties',
'rng-random': 'RngRandomProperties',
+ 'secret': 'SecretProperties',
+ 'secret_keyring': 'SecretKeyringProperties',
'throttle-group': 'ThrottleGroupProperties'
} }
diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
index 30a694a39a..04e41254f9 100644
--- a/docs/system/deprecated.rst
+++ b/docs/system/deprecated.rst
@@ -155,6 +155,16 @@ 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.
+``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.
+
QEMU Machine Protocol (QMP) commands
--
2.28.0
[PATCH 07/18] qapi/qom: Add ObjectOptions for throttle-group
This adds a QAPI schema for the properties of the throttle-group object.
The x-* properties are not represented in the schema. Their only purpose
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.
Signed-off-by: Kevin Wolf
---
qapi/block-core.json | 12
qapi/qom.json| 7 +--
2 files changed, 17 insertions(+), 2 deletions(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 04ad80bc1e..d7a4cdc11c 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2478,6 +2478,18 @@
'*bps-write-max' : 'int', '*bps-write-max-length' : 'int',
'*iops-size' : 'int' } }
+##
+# @ThrottleGroupProperties:
+#
+# Properties for throttle-group objects.
+#
+# @limits: limits to apply for this throttle group
+#
+# Since: 6.0
+##
+{ 'struct': 'ThrottleGroupProperties',
+ 'data': { '*limits': 'ThrottleLimits' } }
+
##
# @block-stream:
#
diff --git a/qapi/qom.json b/qapi/qom.json
index f3d1a55cb8..8c9785d2dd 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' }
##
@@ -435,7 +436,8 @@
'memory-backend-ram',
'rng-builtin',
'rng-egd',
-'rng-random'
+'rng-random',
+'throttle-group'
] }
##
@@ -468,7 +470,8 @@
'memory-backend-ram': 'MemoryBackendProperties',
'rng-builtin':'RngProperties',
'rng-egd':'RngEgdProperties',
- 'rng-random': 'RngRandomProperties'
+ 'rng-random': 'RngRandomProperties',
+ 'throttle-group': 'ThrottleGroupProperties'
} }
##
--
2.28.0
[PATCH 06/18] qapi/qom: Add ObjectOptions for rng-*, deprecate 'opened'
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 f8a1da43ad..f3d1a55cb8 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -368,6 +368,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: 6.0
+##
+{ 'struct': 'RngProperties',
+ 'data': { '*opened': { 'type': 'bool', 'features': ['deprecated'] } } }
+
+##
+# @RngEgdProperties:
+#
+# Properties for rng-egd objects.
+#
+# @chardev: the name of a character device backend that provides the connection
+# to the RNG daemon
+#
+# Since: 6.0
+##
+{ 'struct': 'RngEgdProperties',
+ 'base': 'RngProperties',
+ 'data': { 'chardev': 'str' } }
+
+##
+# @RngRandomProperties:
+#
+# Properties for rng-random objects.
+#
+# @filename: the filename of the device on the host to obtain entropy from
+#(default: "/dev/urandom")
+#
+# Since: 6.0
+##
+{ 'struct': 'RngRandomProperties',
+ 'base': 'RngProperties',
+ 'data': { '*filename': 'str' } }
+
##
# @ObjectType:
#
@@ -386,7 +432,10 @@
'iothread',
'memory-backend-file',
'memory-backend-memfd',
-'memory-backend-ram'
+'memory-backend-ram',
+'rng-builtin',
+'rng-egd',
+'rng-random'
] }
##
@@ -416,7 +465,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 565389697e..30a694a39a 100644
--- a/docs/system/deprecated.rst
+++ b/docs/system/deprecated.rst
@@ -146,6 +146,15 @@ Drives with interface types other than ``if=none`` are for
onboard
devices. It is possible to use drives the board doesn't pick up with
-device. This usage is now deprecated. Use ``if=none`` instead.
+``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.
+
QEMU Machine Protocol (QMP) commands
--
2.28.0
[PATCH 05/18] qapi/qom: Add ObjectOptions for memory-backend-*
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
---
qapi/common.json | 20 +
qapi/machine.json | 22 +-
qapi/qom.json | 106 +-
3 files changed, 126 insertions(+), 22 deletions(-)
diff --git a/qapi/common.json b/qapi/common.json
index 716712d4b3..2dad4fadc3 100644
--- a/qapi/common.json
+++ b/qapi/common.json
@@ -145,3 +145,23 @@
##
{ 'enum': 'PCIELinkWidth',
'data': [ '1', '2', '4', '8', '12', '16', '32' ] }
+
+##
+# @HostMemPolicy:
+#
+# Host memory policy types
+#
+# @default: restore default policy, remove any nondefault policy
+#
+# @preferred: set the preferred host nodes for allocation
+#
+# @bind: a strict policy that restricts memory allocation to the
+#host nodes specified
+#
+# @interleave: memory allocations are interleaved across the set
+# of host nodes specified
+#
+# Since: 2.1
+##
+{ 'enum': 'HostMemPolicy',
+ 'data': [ 'default', 'preferred', 'bind', 'interleave' ] }
diff --git a/qapi/machine.json b/qapi/machine.json
index 7c9a263778..75b9737213 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -8,6 +8,8 @@
# = Machines
##
+{ 'include': 'common.json' }
+
##
# @SysEmuTarget:
#
@@ -897,26 +899,6 @@
'policy': 'HmatCacheWritePolicy',
'line': 'uint16' }}
-##
-# @HostMemPolicy:
-#
-# Host memory policy types
-#
-# @default: restore default policy, remove any nondefault policy
-#
-# @preferred: set the preferred host nodes for allocation
-#
-# @bind: a strict policy that restricts memory allocation to the
-#host nodes specified
-#
-# @interleave: memory allocations are interleaved across the set
-# of host nodes specified
-#
-# Since: 2.1
-##
-{ 'enum': 'HostMemPolicy',
- 'data': [ 'default', 'preferred', 'bind', 'interleave' ] }
-
##
# @memsave:
#
diff --git a/qapi/qom.json b/qapi/qom.json
index 2319c9bad6..f8a1da43ad 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': 'common.json' }
##
# = QEMU Object Model (QOM)
@@ -272,6 +273,101 @@
'*poll-grow': 'int',
'*poll-shrink': 'int' } }
+##
+# @MemoryBackendProperties:
+#
+# Properties for objects of classes derived from memory-backend.
+#
+# @merge: if true, mark the memory as mergeable (default depends on the machine
+# type)
+#
+# @dump: if true, include the memory in core dumps (default depends on the
+#machine type)
+#
+# @host-nodes: the list of NUMA host nodes to bind the memory to
+#
+# @policy: the NUMA policy (default: 'default')
+#
+# @prealloc: if true, preallocate memory (default: false)
+#
+# @prealloc-threads: number of CPU threads to use for prealloc (default: 1)
+#
+# @share: if false, the memory is private to QEMU; if true, it is shared
+# (default: false)
+#
+# @size: size of the memory region in bytes
+#
+# Since: 6.0
+##
+{ 'struct': 'MemoryBackendProperties',
+ 'data': { '*dump': 'bool',
+'*host-nodes': ['uint16'],
+'*merge': 'bool',
+'*policy': 'HostMemPolicy',
+'*prealloc': 'bool',
+'*prealloc-threads': 'uint32',
+'*share': 'bool',
+'size': 'size' } }
+
+##
+# @MemoryBackendFileProperties:
+#
+# Properties for memory-backend-file objects.
+#
+# @align: the base address alignment when QEMU mmap(2) @mem-path. Some
+# backend store specified by @mem-path requires an alignment different
+# than the default one used by QEMU, e.g. the device DAX /dev/dax0.0
+# requires 2M alignment rather than 4K. In such cases, users can
+# specify the required alignment via this option.
+# 0 selects a default alignment (currently the page size). (default: 0)
+#
+# @discard-data: if true, the file contents can be destroyed when QEMU exits,
+#to avoid unnecessarily flushing data to the backing file. Note
+#that ``discard-data`` is only an optimization, and QEMU might
+#not discard file contents if it aborts unexpectedly or is
+#terminated using SIGKILL. (default: false)
+#
+# @mem-path: the path to either a shared memory or huge page filesystem mount
+#
+# @pmem: specifies whether the backing file specified by @mem-path is in
+#host persistent memory that can be accessed using the SNIA NVM
+#programming model (e.g. Intel NVDIMM).
+#
+# Since: 6.0
+##
+{ 'struct': 'MemoryBackendFileProperties',
+ 'base': 'MemoryBackendProperties',
+ 'data': { '*align': 'size',
+'*discard-data': 'bool',
+'mem-path': 'str',
+'*pmem': 'bool' } }
+
+##
+# @MemoryBack
[PATCH 04/18] qapi/qom: Add ObjectOptions for dbus-vmstate
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.
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 7cbc0a3c54..2319c9bad6 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: 6.0
+##
+{ 'struct': 'DBusVMStateProperties',
+ 'data': { 'addr': 'str' ,
+'*id-list': 'str' } }
+
##
# @IothreadProperties:
#
@@ -270,6 +286,7 @@
'cryptodev-backend',
'cryptodev-backend-builtin',
'cryptodev-vhost-user',
+'dbus-vmstate',
'iothread'
] }
@@ -296,6 +313,7 @@
'cryptodev-backend': 'CryptodevBackendProperties',
'cryptodev-backend-builtin': 'CryptodevBackendProperties',
'cryptodev-vhost-user': 'CryptodevVhostUserProperties',
+ 'dbus-vmstate': 'DBusVMStateProperties',
'iothread': 'IothreadProperties'
} }
--
2.28.0
[PATCH 03/18] qapi/qom: Add ObjectOptions for cryptodev-*
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 0ac4b1c9fb..7cbc0a3c54 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: 6.0
+##
+{ 'struct': 'CryptodevBackendProperties',
+ 'data': { '*queues': 'uint32' } }
+
+##
+# @CryptodevVhostUserProperties:
+#
+# Properties for cryptodev-vhost-user objects.
+#
+# @chardev: the name of a unix domain socket character device that connects to
+# the vhost-user server
+#
+# Since: 6.0
+##
+{ 'struct': 'CryptodevVhostUserProperties',
+ 'base': 'CryptodevBackendProperties',
+ 'data': { 'chardev': 'str' } }
+
##
# @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'
} }
--
2.28.0
[PATCH 02/18] qapi/qom: Add ObjectOptions for authz-*
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..ac72ad5c9e 100644
--- a/qapi/authz.json
+++ b/qapi/authz.json
@@ -59,3 +59,65 @@
##
{ 'struct': 'QAuthZListRuleListHack',
'data': { 'unused': ['QAuthZListRule'] } }
+
+##
+# @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: 6.0
+##
+{ 'struct': 'AuthZListProperties',
+ 'data': { '*policy': 'QAuthZListPolicy',
+'*rules': ['QAuthZListRule'] } }
+
+##
+# @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: 6.0
+##
+{ 'struct': 'AuthZListFileProperties',
+ 'data': { 'filename': 'str',
+'*refresh': 'bool' } }
+
+##
+# @AuthZPAMProperties:
+#
+# Properties for authz-pam objects.
+#
+# @service: PAM service name to use for authorization
+#
+# Since: 6.0
+##
+{ 'struct': 'AuthZPAMProperties',
+ 'data': { 'service': 'str' } }
+
+##
+# @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: 6.0
+##
+{ 'struct': 'AuthZSimpleProperties',
+ 'data': { 'identity': 'str' } }
diff --git a/qapi/qom.json b/qapi/qom.json
index 57d1386758..0ac4b1c9fb 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 c6ad5ae1e3..39982d8cac 100644
--- a/storage-daemon/qapi/qapi-schema.json
+++ b/storage-daemon/qapi/qapi-schema.json
@@ -23,6 +23,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' }
--
2.28.0
[PATCH 01/18] qapi/qom: Add ObjectOptions for iothread
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 0b0b92944b..57d1386758 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)
+#
+# Since: 6.0
+##
+{ 'struct': 'IothreadProperties',
+ 'data': { '*poll-max-ns': 'int',
+'*poll-grow': 'int',
+'*poll-shrink': 'int' } }
+
+##
+# @ObjectType:
+#
+# Since: 6.0
+##
+{ 'enum': 'ObjectType',
+ 'data': [
+'iothread'
+ ] }
+
+##
+# @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:
#
--
2.28.0
[PATCH 00/18] qapi/qom: QAPIfy object-add
This series adds a QAPI type for the properties of all user creatable QOM types and finally makes QMP object-add use the new ObjectOptions union so that QAPI introspection can be used for user creatable objects. If you are in the CC list and didn't expect this series, it's probably because you're the maintainer of one of the objects for which I'm adding a QAPI schema description. Please just have a look at the specific patch for your object and check whether the schema and its documentation make sense to you. You can ignore all other patches. After this series, there is least one obvious next step that needs to be done: Change HMP and all of the command line parser to use ObjectOptions, too, so that the QAPI schema is consistently enforced in all external interfaces. I am planning to send another series to address this. In a third step, we can try to start deduplicating and integrating things better between QAPI and the QOM implementation, e.g. by generating parts of the QOM boilerplate from the QAPI schema. Kevin Wolf (18): qapi/qom: Add ObjectOptions for iothread qapi/qom: Add ObjectOptions for authz-* qapi/qom: Add ObjectOptions for cryptodev-* qapi/qom: Add ObjectOptions for dbus-vmstate qapi/qom: Add ObjectOptions for memory-backend-* qapi/qom: Add ObjectOptions for rng-*, deprecate 'opened' qapi/qom: Add ObjectOptions for throttle-group qapi/qom: Add ObjectOptions for secret*, deprecate 'loaded' qapi/qom: Add ObjectOptions for tls-*, deprecate 'loaded' qapi/qom: Add ObjectOptions for can-* qapi/qom: Add ObjectOptions for colo-compare qapi/qom: Add ObjectOptions for filter-* qapi/qom: Add ObjectOptions for pr-manager-helper qapi/qom: Add ObjectOptions for sev-guest qapi/qom: Add ObjectOptions for input-* tests: Drop 'props' from object-add calls qapi/qom: Drop deprecated 'props' from object-add qapi/qom: QAPIfy object-add qapi/authz.json | 62 +++ qapi/block-core.json | 12 + qapi/common.json | 52 +++ qapi/crypto.json | 159 +++ qapi/machine.json| 22 +- qapi/net.json| 20 - qapi/qom.json| 609 ++- qapi/ui.json | 13 +- docs/system/deprecated.rst | 29 +- include/qom/object_interfaces.h | 7 - hw/block/xen-block.c | 16 +- monitor/misc.c | 2 - qom/qom-qmp-cmds.c | 44 +- tests/qtest/qmp-cmd-test.c | 16 +- tests/qtest/test-netfilter.c | 54 ++- storage-daemon/qapi/qapi-schema.json | 1 + tests/qemu-iotests/087 | 8 +- tests/qemu-iotests/184 | 18 +- tests/qemu-iotests/218 | 2 +- tests/qemu-iotests/235 | 2 +- tests/qemu-iotests/245 | 4 +- tests/qemu-iotests/258 | 6 +- tests/qemu-iotests/258.out | 4 +- tests/qemu-iotests/295 | 2 +- tests/qemu-iotests/296 | 2 +- 25 files changed, 993 insertions(+), 173 deletions(-) -- 2.28.0
Re: [PATCH v1 05/26] qemu_domain_address: Reformat qemuDomainAssignS390Addresses()
On 30/11/2020 11.18, Michal Privoznik wrote:
> On 11/30/20 10:38 AM, Thomas Huth wrote:
>> On 27/11/2020 16.02, Michal Privoznik wrote:
>>> Signed-off-by: Michal Privoznik
>>> ---
>>> src/qemu/qemu_domain_address.c | 10 --
>>> 1 file changed, 4 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
>>> index 2788dc7fb3..d872f75b38 100644
>>> --- a/src/qemu/qemu_domain_address.c
>>> +++ b/src/qemu/qemu_domain_address.c
>>> @@ -408,18 +408,16 @@ qemuDomainAssignS390Addresses(virDomainDefPtr def,
>>> if (qemuDomainIsS390CCW(def) &&
>>> virQEMUCapsGet(qemuCaps, QEMU_CAPS_CCW)) {
>>> if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VFIO_CCW))
>>> - qemuDomainPrimeVfioDeviceAddresses(
>>> - def, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW);
>>> - qemuDomainPrimeVirtioDeviceAddresses(
>>> - def, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW);
>>> + qemuDomainPrimeVfioDeviceAddresses(def,
>>> VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW);
>>
>> Looks fine to me, but docs/coding-style.rst still suggest to format code
>> with "indent -l75", so is this really the right thing to do here?
>
> It's true that we have 80 characters limit, but that is more of a soft limit
> and common sense should be used. Personally, I find
>
> func(
> arg1, arg2
> );
>
> worse than exceeding that 80 char rule. My common sense tells me that the
> rule tries to avoid the following pattern (among others):
>
> func(arg1, arg2, , very_long_list_of_arguments, which, could, easily,
> go_on_multiple_lines, but, didnt);
Fair point, but then this should IMHO be reflected in the coding-style doc
first. Otherwise the next contributor to this file might simply undo your
change to fit everything again into the 80 (or even 75) columns limit...?
Thomas
Re: [libvirt PATCH 0/9] qemu: report guest disks informations
On Fri, Nov 20, 2020 at 10:10 PM wrote: > From: Marc-André Lureau > > Hi, > > The following series extends virDomainGetGuestInfo to report disk > informations > provided by the new QGA "guest-get-disks" command in QEMU 5.2. > > Please review, > ping > Fixes: > https://bugzilla.redhat.com/show_bug.cgi?id=1899527 > > Marc-André Lureau (9): > qemu_agent: rename qemuAgentDiskInfo->qemuAgentDiskAddress > qemu_agent: export qemuAgentDiskAddressFree & add g_auto > qemu_agent: factor out qemuAgentGetDiskAddress > util: json: add virJSONValueObjectGetStringArray convenience > qemu: use virJSONValueObjectGetStringArray > qemu_agent: add qemuAgentGetDisks > domain: add disk informations to virDomainGetGuestInfo > qemu_driver: report guest disk informations > virsh: add --disk informations to guestinfo command > > include/libvirt/libvirt-domain.h | 1 + > src/libvirt-domain.c | 17 +++ > src/libvirt_private.syms | 1 + > src/qemu/qemu_agent.c| 182 +++ > src/qemu/qemu_agent.h| 25 - > src/qemu/qemu_driver.c | 103 - > src/qemu/qemu_monitor_json.c | 34 +- > src/util/virjson.c | 30 + > src/util/virjson.h | 1 + > tests/qemuagenttest.c| 111 +++ > tools/virsh-domain.c | 6 + > 11 files changed, 430 insertions(+), 81 deletions(-) > > -- > 2.29.0 > > > -- Marc-André Lureau
Re: [PATCH] qemu: Relax memory pre-allocation rules
On Mon, Nov 30, 2020 at 11:48:28AM +0100, Michal Privoznik wrote:
> On 11/30/20 11:16 AM, Daniel P. Berrangé wrote:
> > On Mon, Nov 30, 2020 at 11:06:14AM +0100, Michal Privoznik wrote:
> > > Currently, we configure QEMU to prealloc memory almost by
> > > default. Well, by default for NVDIMMs, hugepages and if user
> > > asked us to (via memoryBacking ).
> > >
> > > However, there are two cases where this approach is not the best:
> > >
> > > 1) in case when guest's NVDIMM is backed by real life NVDIMM. In
> > > this case users should put into the device
> > > , like this:
> > >
> > >
> > >
> > > /dev/pmem0
> > >
> > >
> > >
> > >
> > > Instructing QEMU to do prealloc in this case means that each
> > > page of the NVDIMM is "touched" (the first byte is read and
> > > written back - see QEMU commit v2.9.0-rc1~26^2) which cripples
> > > device wear.
> > >
> > > 2) if free-page-reporting is turned on. While the
> > > free-page-reporting feature might not have a catchy or obvious
> > > name, when enabled it instructs KVM and subsequently QEMU to
> > > free pages no longer used by guest resulting in smaller memory
> > > footprint. And preallocating whole memory goes against this.
> > >
> > > The BZ comment 11 mentions another, third case 'virtio-mem' but
> > > that is not implemented in libvirt, yet.
> > >
> > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1894053
> > > Signed-off-by: Michal Privoznik
> > > ---
> > > src/qemu/qemu_command.c | 11 +--
> > > .../memory-hotplug-nvdimm-pmem.x86_64-latest.args | 2 +-
> > > 2 files changed, 10 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > > index 479bcc0b0c..3df8b5ac76 100644
> > > --- a/src/qemu/qemu_command.c
> > > +++ b/src/qemu/qemu_command.c
> > > @@ -2977,7 +2977,11 @@ qemuBuildMemoryBackendProps(virJSONValuePtr
> > > *backendProps,
> > > if (discard == VIR_TRISTATE_BOOL_ABSENT)
> > > discard = def->mem.discard;
> > > -if (def->mem.allocation == VIR_DOMAIN_MEMORY_ALLOCATION_IMMEDIATE)
> > > +/* The whole point of free_page_reporting is that as soon as guest
> > > frees
> > > + * any memory it is freed in the host too. Prealloc doesn't make
> > > much sense
> > > + * then. */
> > > +if (def->mem.allocation == VIR_DOMAIN_MEMORY_ALLOCATION_IMMEDIATE &&
> > > +def->memballoon->free_page_reporting != VIR_TRISTATE_SWITCH_ON)
> > > prealloc = true;
> >
> > If the user asked for allocation == immediate, we should not be
> > silently ignoring that request. Isn't the scenario described simply
> > a wierd user configuration scenario and if they don't want that, then
> > then they can set instead.
>
> Okay.
>
> >
> > > if (virDomainNumatuneGetMode(def->numa, mem->targetNode, &mode) < 0
> > > &&
> > > @@ -3064,7 +3068,10 @@ qemuBuildMemoryBackendProps(virJSONValuePtr
> > > *backendProps,
> > > if (mem->nvdimmPath) {
> > > memPath = g_strdup(mem->nvdimmPath);
> > > -prealloc = true;
> >
> >
> >
> > > +/* If the NVDIMM is a real device then there's nothing to
> > > prealloc.
> > > + * If anyhing, we would be only wearing off the device. */
> > > +if (!mem->nvdimmPmem)
> > > +prealloc = true;
> >
> > I wonder if QEMU itself should take this optimization to skip its
> > allocation logic ?
>
> Also would make sense. This is that kind of bug which lies in between
> libvirt and qemu. Although, since we are worried in silently ignoring user
> requests, then wouldn't this be exactly what QEMU would be doing? I mean, if
> an user/libvirt put both .prealloc=yes and .pmem=yes onto cmd line then
> these would cancel out, wouldn't they?
The difference is that an real NVDIMM is inherantly preallocated. QEMU
would not be ignoring the prealloc=yes arg - its implementation would
merely be a no-op.
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH] qemu: Relax memory pre-allocation rules
On 11/30/20 11:21 AM, Peter Krempa wrote:
On Mon, Nov 30, 2020 at 11:06:14 +0100, Michal Privoznik wrote:
Currently, we configure QEMU to prealloc memory almost by
default. Well, by default for NVDIMMs, hugepages and if user
asked us to (via memoryBacking ).
However, there are two cases where this approach is not the best:
1) in case when guest's NVDIMM is backed by real life NVDIMM. In
this case users should put into the device
, like this:
/dev/pmem0
Instructing QEMU to do prealloc in this case means that each
page of the NVDIMM is "touched" (the first byte is read and
written back - see QEMU commit v2.9.0-rc1~26^2) which cripples
device wear.
2) if free-page-reporting is turned on. While the
free-page-reporting feature might not have a catchy or obvious
name, when enabled it instructs KVM and subsequently QEMU to
free pages no longer used by guest resulting in smaller memory
footprint. And preallocating whole memory goes against this.
The BZ comment 11 mentions another, third case 'virtio-mem' but
that is not implemented in libvirt, yet.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1894053
Signed-off-by: Michal Privoznik
---
src/qemu/qemu_command.c | 11 +--
.../memory-hotplug-nvdimm-pmem.x86_64-latest.args | 2 +-
2 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 479bcc0b0c..3df8b5ac76 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -2977,7 +2977,11 @@ qemuBuildMemoryBackendProps(virJSONValuePtr
*backendProps,
if (discard == VIR_TRISTATE_BOOL_ABSENT)
discard = def->mem.discard;
-if (def->mem.allocation == VIR_DOMAIN_MEMORY_ALLOCATION_IMMEDIATE)
+/* The whole point of free_page_reporting is that as soon as guest frees
+ * any memory it is freed in the host too. Prealloc doesn't make much sense
+ * then. */
+if (def->mem.allocation == VIR_DOMAIN_MEMORY_ALLOCATION_IMMEDIATE &&
+def->memballoon->free_page_reporting != VIR_TRISTATE_SWITCH_ON)
prealloc = true;
IIUC def->mem.allocation is an explicit user-specified configuration, in
such case we should not override the user wish based on a different
configuration.
If they don't make sense together technically, we should reject the
config rather than silently changing it. If it's just semantically wrong
and pre-existing we may leave it be.
Additionally the patch is missing a test case for this scenario.
Yeah, Dan already pointed out that this is not really desired. So I will
leave this hunk out. But to address your point - would it make sense to
add a valiador check? I mean, something like:
if (def->mem.allocation == VIR_DOMAIN_MEMORY_ALLOCATION_IMMEDIATE &&
def->memballoon->free_page_reporting != VIR_TRISTATE_SWITCH_ON) {
virReportError("this combination doesn't make much sense");
return -1;
}
Technically, it is possible to fully allocate memory on domain startup
and then leave QEMU to free pages (which happens as soon virtio_balloon
module is loaded), but IMO it doesn't make much sense. Semantically, at
least to me these two options are mutually exclusive.
Michal
Re: [PATCH] qemu: Relax memory pre-allocation rules
On 11/30/20 11:16 AM, Daniel P. Berrangé wrote:
On Mon, Nov 30, 2020 at 11:06:14AM +0100, Michal Privoznik wrote:
Currently, we configure QEMU to prealloc memory almost by
default. Well, by default for NVDIMMs, hugepages and if user
asked us to (via memoryBacking ).
However, there are two cases where this approach is not the best:
1) in case when guest's NVDIMM is backed by real life NVDIMM. In
this case users should put into the device
, like this:
/dev/pmem0
Instructing QEMU to do prealloc in this case means that each
page of the NVDIMM is "touched" (the first byte is read and
written back - see QEMU commit v2.9.0-rc1~26^2) which cripples
device wear.
2) if free-page-reporting is turned on. While the
free-page-reporting feature might not have a catchy or obvious
name, when enabled it instructs KVM and subsequently QEMU to
free pages no longer used by guest resulting in smaller memory
footprint. And preallocating whole memory goes against this.
The BZ comment 11 mentions another, third case 'virtio-mem' but
that is not implemented in libvirt, yet.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1894053
Signed-off-by: Michal Privoznik
---
src/qemu/qemu_command.c | 11 +--
.../memory-hotplug-nvdimm-pmem.x86_64-latest.args | 2 +-
2 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 479bcc0b0c..3df8b5ac76 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -2977,7 +2977,11 @@ qemuBuildMemoryBackendProps(virJSONValuePtr
*backendProps,
if (discard == VIR_TRISTATE_BOOL_ABSENT)
discard = def->mem.discard;
-if (def->mem.allocation == VIR_DOMAIN_MEMORY_ALLOCATION_IMMEDIATE)
+/* The whole point of free_page_reporting is that as soon as guest frees
+ * any memory it is freed in the host too. Prealloc doesn't make much sense
+ * then. */
+if (def->mem.allocation == VIR_DOMAIN_MEMORY_ALLOCATION_IMMEDIATE &&
+def->memballoon->free_page_reporting != VIR_TRISTATE_SWITCH_ON)
prealloc = true;
If the user asked for allocation == immediate, we should not be
silently ignoring that request. Isn't the scenario described simply
a wierd user configuration scenario and if they don't want that, then
then they can set instead.
Okay.
if (virDomainNumatuneGetMode(def->numa, mem->targetNode, &mode) < 0 &&
@@ -3064,7 +3068,10 @@ qemuBuildMemoryBackendProps(virJSONValuePtr
*backendProps,
if (mem->nvdimmPath) {
memPath = g_strdup(mem->nvdimmPath);
-prealloc = true;
+/* If the NVDIMM is a real device then there's nothing to prealloc.
+ * If anyhing, we would be only wearing off the device. */
+if (!mem->nvdimmPmem)
+prealloc = true;
I wonder if QEMU itself should take this optimization to skip its
allocation logic ?
Also would make sense. This is that kind of bug which lies in between
libvirt and qemu. Although, since we are worried in silently ignoring
user requests, then wouldn't this be exactly what QEMU would be doing? I
mean, if an user/libvirt put both .prealloc=yes and .pmem=yes onto cmd
line then these would cancel out, wouldn't they?
Michal
Re: [PATCH v1 05/26] qemu_domain_address: Reformat qemuDomainAssignS390Addresses()
On Mon, 30 Nov 2020 11:18:20 +0100
Michal Privoznik wrote:
> On 11/30/20 10:38 AM, Thomas Huth wrote:
> > On 27/11/2020 16.02, Michal Privoznik wrote:
> >> Signed-off-by: Michal Privoznik
> >> ---
> >> src/qemu/qemu_domain_address.c | 10 --
> >> 1 file changed, 4 insertions(+), 6 deletions(-)
> >> } else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_S390)) {
> >
> > Not related to your patch, but an idea for a future clean-up: That
> > QEMU_CAPS_VIRTIO_S390 seems to belong to the ancient "s390-virtio" (without
> > ccw) machine that has been removed in QEMU v2.6 already:
> >
> > https://git.qemu.org/?p=qemu.git;a=commitdiff;h=7b3fdbd9a82
> > https://git.qemu.org/?p=qemu.git;a=commitdiff;h=3538fb6f89d
> >
> > IIRC, that machine was already considered as deprecated since a couple of
> > earlier QEMU releases, so I really doubt that anybody is still using that in
> > production today.
> >
> > Thus I think that all code related to QEMU_CAPS_VIRTIO_S390 could likely be
> > removed from libvirt nowadays.
>
> That is even better idea. But currently libvirt supports QEMU-1.5.0 and
> newer. So I think we shouldn't remove that until the minimum version is
> bumped even though we think feature has no users.
>
> https://gitlab.com/libvirt/libvirt/-/commit/c1bc9c662b4
>
> Although, it might be about time to look again what is the oldest QEMU
> we need to support.
Would be great if you could bump it enough to get rid of the old
virtio-s390 transport :)
FWIW, virtio-ccw was introduced in QEMU 1.4, and became the default
with QEMU 2.4, although it had supplanted virtio-s390 well before that.
What are the criteria for possibly removing support for a feature in
libvirt: that nobody would use it in practice, or that nobody would be
able to use it?
Re: [PATCH v1 05/26] qemu_domain_address: Reformat qemuDomainAssignS390Addresses()
On Mon, Nov 30, 2020 at 11:18:20AM +0100, Michal Privoznik wrote:
> On 11/30/20 10:38 AM, Thomas Huth wrote:
> > On 27/11/2020 16.02, Michal Privoznik wrote:
> > > Signed-off-by: Michal Privoznik
> > > ---
> > > src/qemu/qemu_domain_address.c | 10 --
> > > 1 file changed, 4 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/src/qemu/qemu_domain_address.c
> > > b/src/qemu/qemu_domain_address.c
> > > index 2788dc7fb3..d872f75b38 100644
> > > --- a/src/qemu/qemu_domain_address.c
> > > +++ b/src/qemu/qemu_domain_address.c
> > > @@ -408,18 +408,16 @@ qemuDomainAssignS390Addresses(virDomainDefPtr def,
> > > if (qemuDomainIsS390CCW(def) &&
> > > virQEMUCapsGet(qemuCaps, QEMU_CAPS_CCW)) {
> > > if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VFIO_CCW))
> > > -qemuDomainPrimeVfioDeviceAddresses(
> > > -def, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW);
> > > -qemuDomainPrimeVirtioDeviceAddresses(
> > > -def, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW);
> > > +qemuDomainPrimeVfioDeviceAddresses(def,
> > > VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW);
> >
> > Looks fine to me, but docs/coding-style.rst still suggest to format code
> > with "indent -l75", so is this really the right thing to do here?
>
> It's true that we have 80 characters limit, but that is more of a soft limit
> and common sense should be used. Personally, I find
>
>func(
> arg1, arg2
> );
>
> worse than exceeding that 80 char rule. My common sense tells me that the
> rule tries to avoid the following pattern (among others):
>
> func(arg1, arg2, , very_long_list_of_arguments, which, could, easily,
> go_on_multiple_lines, but, didnt);
>
>
> >
> > > +qemuDomainPrimeVirtioDeviceAddresses(def,
> > > VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW);
> > > if (!(addrs = virDomainCCWAddressSetCreateFromDomain(def)))
> > > goto cleanup;
> > > } else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_S390)) {
> >
> > Not related to your patch, but an idea for a future clean-up: That
> > QEMU_CAPS_VIRTIO_S390 seems to belong to the ancient "s390-virtio" (without
> > ccw) machine that has been removed in QEMU v2.6 already:
> >
> > https://git.qemu.org/?p=qemu.git;a=commitdiff;h=7b3fdbd9a82
> > https://git.qemu.org/?p=qemu.git;a=commitdiff;h=3538fb6f89d
> >
> > IIRC, that machine was already considered as deprecated since a couple of
> > earlier QEMU releases, so I really doubt that anybody is still using that in
> > production today.
> >
> > Thus I think that all code related to QEMU_CAPS_VIRTIO_S390 could likely be
> > removed from libvirt nowadays.
>
> That is even better idea. But currently libvirt supports QEMU-1.5.0 and
> newer. So I think we shouldn't remove that until the minimum version is
> bumped even though we think feature has no users.
>
> https://gitlab.com/libvirt/libvirt/-/commit/c1bc9c662b4
>
> Although, it might be about time to look again what is the oldest QEMU we
> need to support.
It was set due to the base RHEL-7 QEMU version. RHEL-7 will fall out of
our support matrix at start of May 2021, so in a few months time we'll
have a massive QEMU version bump we can do, along with a more general
cleanup of RHEL-7 vintage cruft.
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH] qemu: Relax memory pre-allocation rules
On Mon, Nov 30, 2020 at 11:06:14 +0100, Michal Privoznik wrote: > Currently, we configure QEMU to prealloc memory almost by > default. Well, by default for NVDIMMs, hugepages and if user > asked us to (via memoryBacking ). > > However, there are two cases where this approach is not the best: > > 1) in case when guest's NVDIMM is backed by real life NVDIMM. In >this case users should put into the device >, like this: > > > >/dev/pmem0 > > > > >Instructing QEMU to do prealloc in this case means that each >page of the NVDIMM is "touched" (the first byte is read and >written back - see QEMU commit v2.9.0-rc1~26^2) which cripples >device wear. > > 2) if free-page-reporting is turned on. While the >free-page-reporting feature might not have a catchy or obvious >name, when enabled it instructs KVM and subsequently QEMU to >free pages no longer used by guest resulting in smaller memory >footprint. And preallocating whole memory goes against this. > > The BZ comment 11 mentions another, third case 'virtio-mem' but > that is not implemented in libvirt, yet. > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1894053 > Signed-off-by: Michal Privoznik > --- > src/qemu/qemu_command.c | 11 +-- > .../memory-hotplug-nvdimm-pmem.x86_64-latest.args | 2 +- > 2 files changed, 10 insertions(+), 3 deletions(-) > > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > index 479bcc0b0c..3df8b5ac76 100644 > --- a/src/qemu/qemu_command.c > +++ b/src/qemu/qemu_command.c > @@ -2977,7 +2977,11 @@ qemuBuildMemoryBackendProps(virJSONValuePtr > *backendProps, > if (discard == VIR_TRISTATE_BOOL_ABSENT) > discard = def->mem.discard; > > -if (def->mem.allocation == VIR_DOMAIN_MEMORY_ALLOCATION_IMMEDIATE) > +/* The whole point of free_page_reporting is that as soon as guest frees > + * any memory it is freed in the host too. Prealloc doesn't make much > sense > + * then. */ > +if (def->mem.allocation == VIR_DOMAIN_MEMORY_ALLOCATION_IMMEDIATE && > +def->memballoon->free_page_reporting != VIR_TRISTATE_SWITCH_ON) > prealloc = true; IIUC def->mem.allocation is an explicit user-specified configuration, in such case we should not override the user wish based on a different configuration. If they don't make sense together technically, we should reject the config rather than silently changing it. If it's just semantically wrong and pre-existing we may leave it be. Additionally the patch is missing a test case for this scenario.
Re: [PATCH v1 05/26] qemu_domain_address: Reformat qemuDomainAssignS390Addresses()
On 11/30/20 10:38 AM, Thomas Huth wrote:
On 27/11/2020 16.02, Michal Privoznik wrote:
Signed-off-by: Michal Privoznik
---
src/qemu/qemu_domain_address.c | 10 --
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index 2788dc7fb3..d872f75b38 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -408,18 +408,16 @@ qemuDomainAssignS390Addresses(virDomainDefPtr def,
if (qemuDomainIsS390CCW(def) &&
virQEMUCapsGet(qemuCaps, QEMU_CAPS_CCW)) {
if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VFIO_CCW))
-qemuDomainPrimeVfioDeviceAddresses(
-def, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW);
-qemuDomainPrimeVirtioDeviceAddresses(
-def, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW);
+qemuDomainPrimeVfioDeviceAddresses(def,
VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW);
Looks fine to me, but docs/coding-style.rst still suggest to format code
with "indent -l75", so is this really the right thing to do here?
It's true that we have 80 characters limit, but that is more of a soft
limit and common sense should be used. Personally, I find
func(
arg1, arg2
);
worse than exceeding that 80 char rule. My common sense tells me that
the rule tries to avoid the following pattern (among others):
func(arg1, arg2, , very_long_list_of_arguments, which, could,
easily, go_on_multiple_lines, but, didnt);
+qemuDomainPrimeVirtioDeviceAddresses(def,
VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW);
if (!(addrs = virDomainCCWAddressSetCreateFromDomain(def)))
goto cleanup;
} else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_S390)) {
Not related to your patch, but an idea for a future clean-up: That
QEMU_CAPS_VIRTIO_S390 seems to belong to the ancient "s390-virtio" (without
ccw) machine that has been removed in QEMU v2.6 already:
https://git.qemu.org/?p=qemu.git;a=commitdiff;h=7b3fdbd9a82
https://git.qemu.org/?p=qemu.git;a=commitdiff;h=3538fb6f89d
IIRC, that machine was already considered as deprecated since a couple of
earlier QEMU releases, so I really doubt that anybody is still using that in
production today.
Thus I think that all code related to QEMU_CAPS_VIRTIO_S390 could likely be
removed from libvirt nowadays.
That is even better idea. But currently libvirt supports QEMU-1.5.0 and
newer. So I think we shouldn't remove that until the minimum version is
bumped even though we think feature has no users.
https://gitlab.com/libvirt/libvirt/-/commit/c1bc9c662b4
Although, it might be about time to look again what is the oldest QEMU
we need to support.
Michal
Re: [PATCH] qemu: Relax memory pre-allocation rules
On Mon, Nov 30, 2020 at 11:06:14AM +0100, Michal Privoznik wrote:
> Currently, we configure QEMU to prealloc memory almost by
> default. Well, by default for NVDIMMs, hugepages and if user
> asked us to (via memoryBacking ).
>
> However, there are two cases where this approach is not the best:
>
> 1) in case when guest's NVDIMM is backed by real life NVDIMM. In
>this case users should put into the device
>, like this:
>
>
>
>/dev/pmem0
>
>
>
>
>Instructing QEMU to do prealloc in this case means that each
>page of the NVDIMM is "touched" (the first byte is read and
>written back - see QEMU commit v2.9.0-rc1~26^2) which cripples
>device wear.
>
> 2) if free-page-reporting is turned on. While the
>free-page-reporting feature might not have a catchy or obvious
>name, when enabled it instructs KVM and subsequently QEMU to
>free pages no longer used by guest resulting in smaller memory
>footprint. And preallocating whole memory goes against this.
>
> The BZ comment 11 mentions another, third case 'virtio-mem' but
> that is not implemented in libvirt, yet.
>
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1894053
> Signed-off-by: Michal Privoznik
> ---
> src/qemu/qemu_command.c | 11 +--
> .../memory-hotplug-nvdimm-pmem.x86_64-latest.args | 2 +-
> 2 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 479bcc0b0c..3df8b5ac76 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -2977,7 +2977,11 @@ qemuBuildMemoryBackendProps(virJSONValuePtr
> *backendProps,
> if (discard == VIR_TRISTATE_BOOL_ABSENT)
> discard = def->mem.discard;
>
> -if (def->mem.allocation == VIR_DOMAIN_MEMORY_ALLOCATION_IMMEDIATE)
> +/* The whole point of free_page_reporting is that as soon as guest frees
> + * any memory it is freed in the host too. Prealloc doesn't make much
> sense
> + * then. */
> +if (def->mem.allocation == VIR_DOMAIN_MEMORY_ALLOCATION_IMMEDIATE &&
> +def->memballoon->free_page_reporting != VIR_TRISTATE_SWITCH_ON)
> prealloc = true;
If the user asked for allocation == immediate, we should not be
silently ignoring that request. Isn't the scenario described simply
a wierd user configuration scenario and if they don't want that, then
then they can set instead.
> if (virDomainNumatuneGetMode(def->numa, mem->targetNode, &mode) < 0 &&
> @@ -3064,7 +3068,10 @@ qemuBuildMemoryBackendProps(virJSONValuePtr
> *backendProps,
>
> if (mem->nvdimmPath) {
> memPath = g_strdup(mem->nvdimmPath);
> -prealloc = true;
> +/* If the NVDIMM is a real device then there's nothing to
> prealloc.
> + * If anyhing, we would be only wearing off the device. */
> +if (!mem->nvdimmPmem)
> +prealloc = true;
I wonder if QEMU itself should take this optimization to skip its
allocation logic ?
> } else if (useHugepage) {
> if (qemuGetDomainHupageMemPath(priv->driver, def, pagesize,
> &memPath) < 0)
> return -1;
> diff --git
> a/tests/qemuxml2argvdata/memory-hotplug-nvdimm-pmem.x86_64-latest.args
> b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-pmem.x86_64-latest.args
> index cac02a6f6d..fb4ae4b518 100644
> --- a/tests/qemuxml2argvdata/memory-hotplug-nvdimm-pmem.x86_64-latest.args
> +++ b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-pmem.x86_64-latest.args
> @@ -20,7 +20,7 @@ file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes \
> -object memory-backend-ram,id=ram-node0,size=224395264 \
> -numa node,nodeid=0,cpus=0-1,memdev=ram-node0 \
> -object memory-backend-file,id=memnvdimm0,mem-path=/tmp/nvdimm,share=no,\
> -prealloc=yes,size=536870912,pmem=yes \
> +size=536870912,pmem=yes \
> -device nvdimm,node=0,memdev=memnvdimm0,id=nvdimm0,slot=0 \
> -uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
> -display none \
> --
> 2.26.2
>
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH for-6.0 6/6] qapi: Deprecate 'query-kvm'
On Mon, Nov 30, 2020 at 10:21:08 +0100, Markus Armbruster wrote: > Peter Krempa writes: > > > On Fri, Nov 27, 2020 at 16:44:05 +0100, Markus Armbruster wrote: > >> Peter Krempa writes: [...] > > I know it's hard to enforce, but probably the cheapest in terms of > > drawbacks any other solution would be. > > We can and should try. > > Can we flag problematic interface changes automatically? Semantic > changes, no. What about changes visible in query-qmp-schema? I don't know the internals of qemu good enough, but from the perspective of an user of 'query-qmp-schema' it might be possible but not without additional tooling. The output of query-qmp-schema is basically unreviewable when we are updating it. A small change in the schema may trigger a re-numbering of the internal type names so the result is a giant messy piece of JSON where it's impossible to differentiate changes from churn. I've played with generating/expanding all the possibilites and thus stripping the internal numbering for a tool which would simplify writing the query strings (a libvirtism for querying whether the QMP schema has something [1]). This tool could be used in this case to generate a fully expanded and sorted list which should in most cases be append only when new stuff is added. This could be then used to see whether something changed when we'd store the output and compare it against the new one. Unfortunately that would just make query-qmp-schema dumps more reviewable in libvirt though. A change in an interface would be noticed only after it hits qemu upstream. [1] https://gitlab.com/libvirt/libvirt/-/blob/08ae9e5f40f8bae0c3cf48f84181884ddd310fa0/src/qemu/qemu_qapi.c#L392 https://gitlab.com/libvirt/libvirt/-/blob/08ae9e5f40f8bae0c3cf48f84181884ddd310fa0/src/qemu/qemu_capabilities.c#L1512
[PATCH] qemu: Relax memory pre-allocation rules
Currently, we configure QEMU to prealloc memory almost by
default. Well, by default for NVDIMMs, hugepages and if user
asked us to (via memoryBacking ).
However, there are two cases where this approach is not the best:
1) in case when guest's NVDIMM is backed by real life NVDIMM. In
this case users should put into the device
, like this:
/dev/pmem0
Instructing QEMU to do prealloc in this case means that each
page of the NVDIMM is "touched" (the first byte is read and
written back - see QEMU commit v2.9.0-rc1~26^2) which cripples
device wear.
2) if free-page-reporting is turned on. While the
free-page-reporting feature might not have a catchy or obvious
name, when enabled it instructs KVM and subsequently QEMU to
free pages no longer used by guest resulting in smaller memory
footprint. And preallocating whole memory goes against this.
The BZ comment 11 mentions another, third case 'virtio-mem' but
that is not implemented in libvirt, yet.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1894053
Signed-off-by: Michal Privoznik
---
src/qemu/qemu_command.c | 11 +--
.../memory-hotplug-nvdimm-pmem.x86_64-latest.args | 2 +-
2 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 479bcc0b0c..3df8b5ac76 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -2977,7 +2977,11 @@ qemuBuildMemoryBackendProps(virJSONValuePtr
*backendProps,
if (discard == VIR_TRISTATE_BOOL_ABSENT)
discard = def->mem.discard;
-if (def->mem.allocation == VIR_DOMAIN_MEMORY_ALLOCATION_IMMEDIATE)
+/* The whole point of free_page_reporting is that as soon as guest frees
+ * any memory it is freed in the host too. Prealloc doesn't make much sense
+ * then. */
+if (def->mem.allocation == VIR_DOMAIN_MEMORY_ALLOCATION_IMMEDIATE &&
+def->memballoon->free_page_reporting != VIR_TRISTATE_SWITCH_ON)
prealloc = true;
if (virDomainNumatuneGetMode(def->numa, mem->targetNode, &mode) < 0 &&
@@ -3064,7 +3068,10 @@ qemuBuildMemoryBackendProps(virJSONValuePtr
*backendProps,
if (mem->nvdimmPath) {
memPath = g_strdup(mem->nvdimmPath);
-prealloc = true;
+/* If the NVDIMM is a real device then there's nothing to prealloc.
+ * If anyhing, we would be only wearing off the device. */
+if (!mem->nvdimmPmem)
+prealloc = true;
} else if (useHugepage) {
if (qemuGetDomainHupageMemPath(priv->driver, def, pagesize,
&memPath) < 0)
return -1;
diff --git
a/tests/qemuxml2argvdata/memory-hotplug-nvdimm-pmem.x86_64-latest.args
b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-pmem.x86_64-latest.args
index cac02a6f6d..fb4ae4b518 100644
--- a/tests/qemuxml2argvdata/memory-hotplug-nvdimm-pmem.x86_64-latest.args
+++ b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-pmem.x86_64-latest.args
@@ -20,7 +20,7 @@ file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes \
-object memory-backend-ram,id=ram-node0,size=224395264 \
-numa node,nodeid=0,cpus=0-1,memdev=ram-node0 \
-object memory-backend-file,id=memnvdimm0,mem-path=/tmp/nvdimm,share=no,\
-prealloc=yes,size=536870912,pmem=yes \
+size=536870912,pmem=yes \
-device nvdimm,node=0,memdev=memnvdimm0,id=nvdimm0,slot=0 \
-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
-display none \
--
2.26.2
Re: [PATCH 0/3] gitlab: Add issue templates
On Mon, Nov 30, 2020 at 09:51:26 +0100, Pavel Hrdina wrote: > On Fri, Nov 27, 2020 at 06:05:41PM +0100, Peter Krempa wrote: > > On Fri, Nov 27, 2020 at 16:50:01 +, Daniel Berrange wrote: [...] > > > > > ## Software environment > - Operating system: > - Architecture: > - kernel version: > - libvirt version: > - Hypervisor and version: > > ## Description of problem > > ## Steps to reproduce > 1. > 2. > 3. > > ## Expected behavior > > ## Additional information > > > /label ~bug I like this as it explicitly asks for configs. Also a bonus is that it's lean on comments at the top, which is shown first. I think that we should add the note of commit being stripped to the multiline comment though, to be a bit safer.
