Re: [PATCH v2 1/6] qemu_process.c: check migrateURI when setting VIR_QEMU_PROCESS_START_NEW
On 11/18/20 8:58 PM, Daniel Henrique Barboza wrote: qemuProcessCreatePretendCmdPrepare() is setting the VIR_QEMU_PROCESS_START_NEW regardless of whether this is a migration case or not. This behavior differs from what we're doing in qemuProcessStart(), where the flag is set only if !migrate && !snapshot. Fix it by making the flag setting consistent with what we're doing in qemuProcessStart(). Signed-off-by: Daniel Henrique Barboza --- src/qemu/qemu_process.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 579b3c3713..3677da635c 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -7436,7 +7436,10 @@ qemuProcessCreatePretendCmdPrepare(virQEMUDriverPtr driver, VIR_QEMU_PROCESS_START_AUTODESTROY, -1); flags |= VIR_QEMU_PROCESS_START_PRETEND; -flags |= VIR_QEMU_PROCESS_START_NEW; + +if (!migrateURI) +flags |= VIR_QEMU_PROCESS_START_NEW; + if (standalone) flags |= VIR_QEMU_PROCESS_START_STANDALONE; Le sigh, this lead me down the rabbit hole of VIR_QEMU_PROCESS_START_NEW flag and where it's used and what consequences this can have. Then I read the commit message properly and realized this is only for "pretend" case. And of course it's what we do for qemuProcessStart(). Lesson learned :-) Michal
Re: [PATCH v2 0/6] add post parse pSeries mem align, when ABI_UPDATE
On 11/18/20 8:58 PM, Daniel Henrique Barboza wrote: Hi, This is a follow up of [1] after really comprehending what I was messing with and what I couldn't do. This version has a shorter scope, only pSeries guests are being taken care of. I can send a follow up to handle x86 as well depending on the popularity of this work. Patches 1 and 2 moves the existing qemuDomainAlignMemorySizes() from qemu_command.c to qemuProcessPrepareDomain(). They are not related/tied to everything else done here and can be pushed independently if needed. Patches 3, 4 and 5 are reworking the existing code to be consistent to our prerrogative of not aligning memory when migrating or 'snapshotting'. No significant behavioral change is done. Patch 6 is where the bacon is. For new ppc64 guests, without ABI breakage, the mem alignment that are overshooting the intended initial memory is fixed. Test cases were added to help me diagnose and assert what I was changing and what would remain untouched. [1] https://www.redhat.com/archives/libvir-list/2020-November/msg00544.html Daniel Henrique Barboza (6): qemu_process.c: check migrateURI when setting VIR_QEMU_PROCESS_START_NEW qemu: move memory size align to qemuProcessPrepareDomain() Revert "domain_conf.c: auto-align pSeries NVDIMM in virDomainMemoryDefPostParse()" qemuxml2xmltest.c: honor ARG_PARSEFLAGS qemu_domain.c: post parse pSeries NVDIMM align with PARSE_ABI_UPDATE qemu_domain.c: align all pSeries mem modules when PARSE_ABI_UPDATE src/conf/domain_conf.c| 23 + src/qemu/qemu_command.c | 3 -- src/qemu/qemu_domain.c| 39 ++- src/qemu/qemu_process.c | 11 +++- ...memory-hotplug-nvdimm-ppc64-abi-update.xml | 50 +++ ...emory-hotplug-ppc64-nonuma-abi-update.args | 34 + ...memory-hotplug-ppc64-nonuma-abi-update.xml | 32 tests/qemuxml2argvtest.c | 7 +++ ...memory-hotplug-nvdimm-ppc64-abi-update.xml | 50 +++ .../memory-hotplug-nvdimm-ppc64.xml | 2 +- ...memory-hotplug-ppc64-nonuma-abi-update.xml | 45 + tests/qemuxml2xmltest.c | 20 +++- 12 files changed, 286 insertions(+), 30 deletions(-) create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64-abi-update.xml create mode 100644 tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma-abi-update.args create mode 100644 tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma-abi-update.xml create mode 100644 tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-ppc64-abi-update.xml create mode 100644 tests/qemuxml2xmloutdata/memory-hotplug-ppc64-nonuma-abi-update.xml Reviewed-by: Michal Privoznik Michal
Re: [PATCH for 7.0.0 v1 00/26] Introduce virtio memory support
Works for me on qemu-5.2.0-0.7.rc2.fc34.x86_64, libvirt
v6.10.0-61-gd8c9cc3bd0 with this series patched:
guest kernel: kernel-5.10.0-0.rc5.20201125git127c501a03d5.85.fc33.x86_64
recompiled with VIRTIO_PMEM
Steps:
1. Start VM with and :
Domain xml:
pc
bb508b28-d57b-44bd-9e9c-a134cef24b60
20972544
1048576
...
2. Attach the virtio memory device:
➜ ~ cat /tmp/virtio-mem.xml
4194304
0
2048
524288
➜ ~ virsh attach-device pc /tmp/virtio-mem.xml
Device attached successfully
Before memory attached, the VM memory is:
[root@localhost ~]# free -m
totalusedfree shared buff/cache
available
Mem:879 124 601 2 152
615
Swap: 0 0 0
After:
[root@localhost ~]# free -m
totalusedfree shared buff/cache
available
Mem: 1391 1321106 2 153
1074
Swap: 0 0 0
3. Attach the virtio pmem device:
➜ ~ cat /tmp/virtio-pmem.xml
/tmp/virtio_pmem
524288
➜ ~ virsh attach-device pc /tmp/virtio-pmem.xml
Device attached successfully
Check it in VM:
[root@localhost ~]# lsblk
NAME MAJ:MIN RM SIZE RO TYPE MOUNTPOINT
vda252:00 10G 0 disk
└─vda1 252:10 10G 0 part /
pmem0 259:00 512M 0 disk
3. Try to detach them:
➜ ~ virsh detach-device pc /tmp/virtio-mem.xml
error: Failed to detach device from /tmp/virtio-mem.xml
error: internal error: unable to execute QEMU command 'device_del': virtio
based memory devices cannot be unplugged.
➜ ~ virsh detach-device pc /tmp/virtio-pmem.xml
error: Failed to detach device from /tmp/virtio-pmem.xml
error: internal error: unable to execute QEMU command 'device_del': virtio
based memory devices cannot be unplugged.
On Fri, Nov 27, 2020 at 11:05 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!
>
> 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
Re: [PATCH] polkit: Allow libvirt group access to libvirtd ro socket
On 12/1/20 5:15 PM, Neal Gompa wrote: On Tue, Dec 1, 2020 at 4:23 PM Jim Fehlig wrote: On 12/1/20 2:17 AM, Daniel P. Berrangé wrote: On Mon, Nov 30, 2020 at 05:28:16PM -0700, 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. This doesn't make sense - the readonly case should never prompt for a password, since libvirtd.polkit.in grants that permission out of the box. I thought something smelled a bit fishy. I meant to annotate the patch with "It is possible I have a broader polkit config issue", but forgot before sending it last night. And indeed after looking again today with fresh eyes I see the problem is in our polkit-default-privs package -> downstream bug. Ignore this patch. Hah, and I didn't catch this because I rip out the default openSUSE stuff that ruins usability by restricting polkit too much. :) It has been a long time, but I've tripped over default-privs in the past. This time it was the difference between "restricted" (default in SLES) and "standard" (default in openSUSE) rules that got me. See /etc/sysconfig/security. Regards, Jim
Re: [PATCH] target/ppc: Remove "compat" property of server class POWER CPUs
On Tue, Dec 01, 2020 at 02:11:03PM +0100, Greg Kurz wrote:
> This property has been deprecated since QEMU 5.0 by commit 22062e54bb68.
> We only kept a legacy hack that internally converts "compat" into the
> official "max-cpu-compat" property of the pseries machine type.
>
> According to our deprecation policy, we could have removed it for QEMU 5.2
> already. Do it now ; since ppc_cpu_parse_featurestr() now just calls the
> generic parent_parse_features handler, drop it as well.
>
> Users are supposed to use the "max-cpu-compat" property of the pseries
> machine type instead.
Applied, thanks.
>
> Signed-off-by: Greg Kurz
> ---
> docs/system/deprecated.rst | 7
> target/ppc/translate_init.c.inc | 59 -
> 2 files changed, 66 deletions(-)
>
> diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
> index 565389697e84..09c8f380bc82 100644
> --- a/docs/system/deprecated.rst
> +++ b/docs/system/deprecated.rst
> @@ -281,13 +281,6 @@ a future version of QEMU. It's unclear whether anybody
> is still using
> CPU emulation in QEMU, and there are no test images available to make
> sure that the code is still working.
>
> -``compat`` property of server class POWER CPUs (since 5.0)
> -''
> -
> -The ``compat`` property used to set backwards compatibility modes for
> -the processor has been deprecated. The ``max-cpu-compat`` property of
> -the ``pseries`` machine type should be used instead.
> -
> ``lm32`` CPUs (since 5.2.0)
> '''
>
> diff --git a/target/ppc/translate_init.c.inc b/target/ppc/translate_init.c.inc
> index 78cc8f043b92..e4082cfde746 100644
> --- a/target/ppc/translate_init.c.inc
> +++ b/target/ppc/translate_init.c.inc
> @@ -10470,63 +10470,6 @@ static ObjectClass *ppc_cpu_class_by_name(const char
> *name)
> return oc;
> }
>
> -static void ppc_cpu_parse_featurestr(const char *type, char *features,
> - Error **errp)
> -{
> -Object *machine = qdev_get_machine();
> -const PowerPCCPUClass *pcc =
> POWERPC_CPU_CLASS(object_class_by_name(type));
> -
> -if (!features) {
> -return;
> -}
> -
> -if (object_property_find(machine, "max-cpu-compat")) {
> -int i;
> -char **inpieces;
> -char *s = features;
> -Error *local_err = NULL;
> -char *compat_str = NULL;
> -
> -/*
> - * Backwards compatibility hack:
> - *
> - * CPUs had a "compat=" property which didn't make sense for
> - * anything except pseries. It was replaced by "max-cpu-compat"
> - * machine option. This supports old command lines like
> - * -cpu POWER8,compat=power7
> - * By stripping the compat option and applying it to the machine
> - * before passing it on to the cpu level parser.
> - */
> -inpieces = g_strsplit(features, ",", 0);
> -*s = '\0';
> -for (i = 0; inpieces[i]; i++) {
> -if (g_str_has_prefix(inpieces[i], "compat=")) {
> -warn_report_once("CPU 'compat' property is deprecated; "
> -"use max-cpu-compat machine property instead");
> -compat_str = inpieces[i];
> -continue;
> -}
> -if ((i != 0) && (s != features)) {
> -s = g_stpcpy(s, ",");
> -}
> -s = g_stpcpy(s, inpieces[i]);
> -}
> -
> -if (compat_str) {
> -char *v = compat_str + strlen("compat=");
> -object_property_set_str(machine, "max-cpu-compat", v,
> &local_err);
> -}
> -g_strfreev(inpieces);
> -if (local_err) {
> -error_propagate(errp, local_err);
> -return;
> -}
> -}
> -
> -/* do property processing with generic handler */
> -pcc->parent_parse_features(type, features, errp);
> -}
> -
> PowerPCCPUClass *ppc_cpu_get_family_class(PowerPCCPUClass *pcc)
> {
> ObjectClass *oc = OBJECT_CLASS(pcc);
> @@ -10905,8 +10848,6 @@ static void ppc_cpu_class_init(ObjectClass *oc, void
> *data)
> device_class_set_parent_reset(dc, ppc_cpu_reset, &pcc->parent_reset);
>
> cc->class_by_name = ppc_cpu_class_by_name;
> -pcc->parent_parse_features = cc->parse_features;
> -cc->parse_features = ppc_cpu_parse_featurestr;
> cc->has_work = ppc_cpu_has_work;
> cc->do_interrupt = ppc_cpu_do_interrupt;
> cc->cpu_exec_interrupt = ppc_cpu_exec_interrupt;
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [PATCH] polkit: Allow libvirt group access to libvirtd ro socket
On 12/1/20 2:17 AM, Daniel P. Berrangé wrote: On Mon, Nov 30, 2020 at 05:28:16PM -0700, 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. This doesn't make sense - the readonly case should never prompt for a password, since libvirtd.polkit.in grants that permission out of the box. I thought something smelled a bit fishy. I meant to annotate the patch with "It is possible I have a broader polkit config issue", but forgot before sending it last night. And indeed after looking again today with fresh eyes I see the problem is in our polkit-default-privs package -> downstream bug. Ignore this patch. Regards, Jim
Re: [PATCH] polkit: Allow libvirt group access to libvirtd ro socket
On Tue, Dec 1, 2020 at 4:23 PM Jim Fehlig wrote: > > On 12/1/20 2:17 AM, Daniel P. Berrangé wrote: > > On Mon, Nov 30, 2020 at 05:28:16PM -0700, 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. > > > > This doesn't make sense - the readonly case should never prompt for > > a password, since libvirtd.polkit.in grants that permission out of > > the box. > > I thought something smelled a bit fishy. I meant to annotate the patch with > "It > is possible I have a broader polkit config issue", but forgot before sending > it > last night. > > And indeed after looking again today with fresh eyes I see the problem is in > our > polkit-default-privs package -> downstream bug. Ignore this patch. > Hah, and I didn't catch this because I rip out the default openSUSE stuff that ruins usability by restricting polkit too much. :) Shame on me for not double checking my environment. :) -- 真実はいつも一つ！/ Always, there's only one truth!
Re: [PATCH 01/21] domain_conf.c: move NVDIMM 'labelsize' check to post parse
On 12/1/20 6:16 PM, Michal Privoznik wrote: On 12/1/20 10:03 PM, Daniel Henrique Barboza wrote: On 12/1/20 5:20 PM, Michal Privoznik wrote: On 12/1/20 8:58 PM, Daniel Henrique Barboza wrote: On 12/1/20 3:46 PM, Michal Privoznik wrote: On 11/24/20 8:20 PM, Daniel Henrique Barboza wrote: Move 'labelsize' validation to virDomainMemoryDefPostParse(). Signed-off-by: Daniel Henrique Barboza --- src/conf/domain_conf.c | 43 +- 1 file changed, 22 insertions(+), 21 deletions(-) [...] + /* Although only the QEMU driver implements PPC64 support, this + * code is related to the platform specification (PAPR), i.e. it + * is hypervisor agnostic, and any future PPC64 hypervisor driver + * will have the same restriction. + */ + if (ARCH_IS_PPC64(def->os.arch) && + virDomainNVDimmAlignSizePseries(mem) < 0) + return -1; + } For this and the rest of patches - shouldn't changes like this go into validator callback? I view post parse callbacks as "fill missing values" not a place to check if configuration makes sense/is valid. You mean these callbacks? domain_conf.h /* validation callbacks */ virDomainDefValidateCallback domainValidateCallback; virDomainDeviceDefValidateCallback deviceValidateCallback; I mean virDomainDefValidate() and more specifically virDomainDefValidateInternal(). Driver specific callbacks are out of question - exactly for the reason you pointed out. Got it. I'll not overload the PostParse() functions and, instead, use virDomainDefValidateInternal() and virDomainDeviceDefValidateInternal() for these cases. Let's try it again in v2. Yeah, you can merge those cleanup patches to which I replied with my reviewed-by. Just did. Thanks for the reviews! I vaguely recall that I might merge some patches of your that did something similar - moved checks from parser to post parse, do you remember? If so, I'm sorry that I misled you. Nah don't worry about it. It's all learning experience. Besides, the only one instance I'm recalling doing that ATM is with a NVDIMM code that wasn't you who merged. (and this particular code can be moved to the proper place like we discussed above. I'll keep that in mind when sending the v2). Thanks, DHB Michal
Re: [PATCH 00/18] qapi/qom: QAPIfy object-add
On Tue, Dec 01, 2020 at 10:23:57PM +0100, Paolo Bonzini wrote: > On 01/12/20 20:35, Kevin Wolf wrote: > > Am 01.12.2020 um 18:16 hat Paolo Bonzini geschrieben: > > I don't think this is actually a new things. We already have types and > > commands declared with things like 'if': 'defined(TARGET_S390X)'. > > As far as I understand, QAPI generated files are already built per > > target, so not compiling things into all binaries should be entirely > > possible. > > There is some complication due to having different discriminators per > target. So yes it should be possible. But probably best left after objects > because it's so much bigger a task and because objects have a bit more > freedom for experimentation (less ties to other qdev-specific concepts, e.g. > the magic "bus" property). > > > So maybe only the abstract base class that actually defines the machine > > properties (like generic-pc-machine) should be described in QAPI, and > > then the concrete machine types would inherit from it without being > > described in QAPI themselves? > > Yes, maybe. > > > > 1) whether to generate _all_ boilerplate or only properties > > > > I would like to generate as much boilerplate as possible. That is, I > > don't want to constrain us to only properties, but at the same time, I'm > > not sure if it's possible to get rid of all boilerplate. > > > > Basically, the vision I have in mind is that QAPI would generate code > > that is the same for most instances, and then provide an option that > > prevents code generation for a specific part for more complicated cases, > > so that the respective function can (and must) be provided in the C > > source. > > Ok, so that's a bit different from what I am thinking of. I don't care very > much about the internal boilerplate, only the external interface for > configuration. So I don't care about type registration, dynamic cast macros > etc., only essentially the part that leads to ucc->complete. > > > > 2) whether we want to introduce a separation between configuration > > > schema and run-time state > > > > You mean the internal run-time state? How is this separation not already > > present with getter/setter functions for each property? In many cases > > they just directly access the run-time state, but there are other cases > > where they actually do things. > > I mean moving more towards the blockdev/chardev way of doing things, > increasing the separation very much by having separate configuration structs > that have (potentially) no link to the run-time state struct. > > > > 3) in the latter case, whether properties will survive at all---iothread > > > and > > > throttle-groups don't really need them even if they're writable after > > > creation. > > > > How do you define properties, i.e. at which point would they stop > > existing and what would be a non-property alternative? > > Properties are only a useful concept if they have a use. If > -object/object_add/object-add can do the same job without properties, > properties are not needed anymore. Do you mean "not needed for -object anymore"? Properties are still used by internal C code (esp. board code), -device/device_add, -machine, -cpu, and debugging commands (like "info qtree" and qom-list/qom-get/qom-set). > > Right now QOM is all about exposing properties, and having multiple > interfaces to set them (by picking a different visitor). But in practice > most QOM objects have a lifetime that consists of 1) set properties 2) flip > a switch (realized/complete/open) 3) let the object live on its own. 1+2 > are a single monitor command or CLI option; during 3 you access the object > through monitor commands, not properties. I agree with this, except for the word "all" in "QOM is all about". QOM is also an extensively used internal QEMU API, including internal usage of the QOM property system. > > > So in summary, it seems to me that the QOM way is more flexible because > > you can get both models out of it. Whether we actually need this > > flexibility I can't say. > > I'm thinking there's no need for it, but maybe I'm overly optimistic. > > > * Configuration options are described in the QAPI schema. This is mainly > >for object creation, but runtime modifiable properties are a subset of > >this. > > > > * Properties are generated for each option. By default, the getter > >just returns the value from the configuration at creation time, though > >generation of it can be disabled so that it can be overridden. Also, > >setters just return an error by default. > > > > * Property setters aren't called for object creation. Instead, the > >relevant ObjectOptions branch is made available to some init method. > > > > * Runtime modifiable properties (declared as such in the schema) don't > >get the default setter, so you have to provide an implementation for > >them. > > I wouldn't bother with properties at all in the QAPI schema. Just do the > first and third bullet
Re: [PATCH 00/18] qapi/qom: QAPIfy object-add
On 01/12/20 20:35, Kevin Wolf wrote: Am 01.12.2020 um 18:16 hat Paolo Bonzini geschrieben: I don't think this is actually a new things. We already have types and commands declared with things like 'if': 'defined(TARGET_S390X)'. As far as I understand, QAPI generated files are already built per target, so not compiling things into all binaries should be entirely possible. There is some complication due to having different discriminators per target. So yes it should be possible. But probably best left after objects because it's so much bigger a task and because objects have a bit more freedom for experimentation (less ties to other qdev-specific concepts, e.g. the magic "bus" property). So maybe only the abstract base class that actually defines the machine properties (like generic-pc-machine) should be described in QAPI, and then the concrete machine types would inherit from it without being described in QAPI themselves? Yes, maybe. 1) whether to generate _all_ boilerplate or only properties I would like to generate as much boilerplate as possible. That is, I don't want to constrain us to only properties, but at the same time, I'm not sure if it's possible to get rid of all boilerplate. Basically, the vision I have in mind is that QAPI would generate code that is the same for most instances, and then provide an option that prevents code generation for a specific part for more complicated cases, so that the respective function can (and must) be provided in the C source. Ok, so that's a bit different from what I am thinking of. I don't care very much about the internal boilerplate, only the external interface for configuration. So I don't care about type registration, dynamic cast macros etc., only essentially the part that leads to ucc->complete. 2) whether we want to introduce a separation between configuration schema and run-time state You mean the internal run-time state? How is this separation not already present with getter/setter functions for each property? In many cases they just directly access the run-time state, but there are other cases where they actually do things. I mean moving more towards the blockdev/chardev way of doing things, increasing the separation very much by having separate configuration structs that have (potentially) no link to the run-time state struct. 3) in the latter case, whether properties will survive at all---iothread and throttle-groups don't really need them even if they're writable after creation. How do you define properties, i.e. at which point would they stop existing and what would be a non-property alternative? Properties are only a useful concept if they have a use. If -object/object_add/object-add can do the same job without properties, properties are not needed anymore. Right now QOM is all about exposing properties, and having multiple interfaces to set them (by picking a different visitor). But in practice most QOM objects have a lifetime that consists of 1) set properties 2) flip a switch (realized/complete/open) 3) let the object live on its own. 1+2 are a single monitor command or CLI option; during 3 you access the object through monitor commands, not properties. So in summary, it seems to me that the QOM way is more flexible because you can get both models out of it. Whether we actually need this flexibility I can't say. I'm thinking there's no need for it, but maybe I'm overly optimistic. * Configuration options are described in the QAPI schema. This is mainly for object creation, but runtime modifiable properties are a subset of this. * Properties are generated for each option. By default, the getter just returns the value from the configuration at creation time, though generation of it can be disabled so that it can be overridden. Also, setters just return an error by default. * Property setters aren't called for object creation. Instead, the relevant ObjectOptions branch is made available to some init method. * Runtime modifiable properties (declared as such in the schema) don't get the default setter, so you have to provide an implementation for them. I wouldn't bother with properties at all in the QAPI schema. Just do the first and third bullet. Declaring read-only QOM properties is trivial. So while this series is doing only one part of the whole solution, that the second part is missing doesn't make the first part wrong. Yeah, I think it's clear that for the long term we're not really disagreeing (or perhaps I'm even more radical than you :)). I'm just worried about having yet another incomplete transition. One possibly nasty detail to consider there is that we sometimes declare the USER_CREATABLE interface in the base class, so ucc->complete is for the base class rather than the actually instantiated class. If we only instantiate leaf classes (I think this is true), we can move USER_CREATABLE there. You can also use a while loop covering ea
Re: [PATCH 01/21] domain_conf.c: move NVDIMM 'labelsize' check to post parse
On 12/1/20 10:03 PM, Daniel Henrique Barboza wrote:
On 12/1/20 5:20 PM, Michal Privoznik wrote:
On 12/1/20 8:58 PM, Daniel Henrique Barboza wrote:
On 12/1/20 3:46 PM, Michal Privoznik wrote:
On 11/24/20 8:20 PM, Daniel Henrique Barboza wrote:
Move 'labelsize' validation to virDomainMemoryDefPostParse().
Signed-off-by: Daniel Henrique Barboza
---
src/conf/domain_conf.c | 43
+-
1 file changed, 22 insertions(+), 21 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index b1534dcc1e..5e5905f483 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -5363,15 +5363,28 @@ static int
virDomainMemoryDefPostParse(virDomainMemoryDefPtr mem,
const virDomainDef *def)
{
- /* Although only the QEMU driver implements PPC64 support, this
- * code is related to the platform specification (PAPR), i.e. it
- * is hypervisor agnostic, and any future PPC64 hypervisor driver
- * will have the same restriction.
- */
- if (ARCH_IS_PPC64(def->os.arch) &&
- mem->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM &&
- virDomainNVDimmAlignSizePseries(mem) < 0)
- return -1;
+ if (mem->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM) {
+ if (mem->labelsize && mem->labelsize < 128) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("nvdimm label must be at least
128KiB"));
+ return -1;
+ }
+
+ if (mem->labelsize >= mem->size) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("label size must be smaller than
NVDIMM size"));
+ return -1;
+ }
+
+ /* Although only the QEMU driver implements PPC64 support,
this
+ * code is related to the platform specification (PAPR),
i.e. it
+ * is hypervisor agnostic, and any future PPC64 hypervisor
driver
+ * will have the same restriction.
+ */
+ if (ARCH_IS_PPC64(def->os.arch) &&
+ virDomainNVDimmAlignSizePseries(mem) < 0)
+ return -1;
+ }
For this and the rest of patches - shouldn't changes like this go
into validator callback? I view post parse callbacks as "fill
missing values" not a place to check if configuration makes sense/is
valid.
You mean these callbacks?
domain_conf.h
/* validation callbacks */
virDomainDefValidateCallback domainValidateCallback;
virDomainDeviceDefValidateCallback deviceValidateCallback;
I mean virDomainDefValidate() and more specifically
virDomainDefValidateInternal(). Driver specific callbacks are out of
question - exactly for the reason you pointed out.
Got it. I'll not overload the PostParse() functions and, instead, use
virDomainDefValidateInternal() and virDomainDeviceDefValidateInternal()
for these cases.
Let's try it again in v2.
Yeah, you can merge those cleanup patches to which I replied with my
reviewed-by.
I vaguely recall that I might merge some patches of your that did
something similar - moved checks from parser to post parse, do you
remember? If so, I'm sorry that I misled you.
Michal
Re: [PATCH 01/21] domain_conf.c: move NVDIMM 'labelsize' check to post parse
On 12/1/20 5:20 PM, Michal Privoznik wrote:
On 12/1/20 8:58 PM, Daniel Henrique Barboza wrote:
On 12/1/20 3:46 PM, Michal Privoznik wrote:
On 11/24/20 8:20 PM, Daniel Henrique Barboza wrote:
Move 'labelsize' validation to virDomainMemoryDefPostParse().
Signed-off-by: Daniel Henrique Barboza
---
src/conf/domain_conf.c | 43 +-
1 file changed, 22 insertions(+), 21 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index b1534dcc1e..5e5905f483 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -5363,15 +5363,28 @@ static int
virDomainMemoryDefPostParse(virDomainMemoryDefPtr mem,
const virDomainDef *def)
{
- /* Although only the QEMU driver implements PPC64 support, this
- * code is related to the platform specification (PAPR), i.e. it
- * is hypervisor agnostic, and any future PPC64 hypervisor driver
- * will have the same restriction.
- */
- if (ARCH_IS_PPC64(def->os.arch) &&
- mem->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM &&
- virDomainNVDimmAlignSizePseries(mem) < 0)
- return -1;
+ if (mem->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM) {
+ if (mem->labelsize && mem->labelsize < 128) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("nvdimm label must be at least 128KiB"));
+ return -1;
+ }
+
+ if (mem->labelsize >= mem->size) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("label size must be smaller than NVDIMM size"));
+ return -1;
+ }
+
+ /* Although only the QEMU driver implements PPC64 support, this
+ * code is related to the platform specification (PAPR), i.e. it
+ * is hypervisor agnostic, and any future PPC64 hypervisor driver
+ * will have the same restriction.
+ */
+ if (ARCH_IS_PPC64(def->os.arch) &&
+ virDomainNVDimmAlignSizePseries(mem) < 0)
+ return -1;
+ }
For this and the rest of patches - shouldn't changes like this go into validator
callback? I view post parse callbacks as "fill missing values" not a place to
check if configuration makes sense/is valid.
You mean these callbacks?
domain_conf.h
/* validation callbacks */
virDomainDefValidateCallback domainValidateCallback;
virDomainDeviceDefValidateCallback deviceValidateCallback;
I mean virDomainDefValidate() and more specifically
virDomainDefValidateInternal(). Driver specific callbacks are out of question -
exactly for the reason you pointed out.
Got it. I'll not overload the PostParse() functions and, instead, use
virDomainDefValidateInternal() and virDomainDeviceDefValidateInternal()
for these cases.
Let's try it again in v2.
Thanks,
DHB
Michal
Re: [PATCH 01/21] domain_conf.c: move NVDIMM 'labelsize' check to post parse
On 12/1/20 8:58 PM, Daniel Henrique Barboza wrote:
On 12/1/20 3:46 PM, Michal Privoznik wrote:
On 11/24/20 8:20 PM, Daniel Henrique Barboza wrote:
Move 'labelsize' validation to virDomainMemoryDefPostParse().
Signed-off-by: Daniel Henrique Barboza
---
src/conf/domain_conf.c | 43 +-
1 file changed, 22 insertions(+), 21 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index b1534dcc1e..5e5905f483 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -5363,15 +5363,28 @@ static int
virDomainMemoryDefPostParse(virDomainMemoryDefPtr mem,
const virDomainDef *def)
{
- /* Although only the QEMU driver implements PPC64 support, this
- * code is related to the platform specification (PAPR), i.e. it
- * is hypervisor agnostic, and any future PPC64 hypervisor driver
- * will have the same restriction.
- */
- if (ARCH_IS_PPC64(def->os.arch) &&
- mem->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM &&
- virDomainNVDimmAlignSizePseries(mem) < 0)
- return -1;
+ if (mem->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM) {
+ if (mem->labelsize && mem->labelsize < 128) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("nvdimm label must be at least 128KiB"));
+ return -1;
+ }
+
+ if (mem->labelsize >= mem->size) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("label size must be smaller than NVDIMM
size"));
+ return -1;
+ }
+
+ /* Although only the QEMU driver implements PPC64 support, this
+ * code is related to the platform specification (PAPR),
i.e. it
+ * is hypervisor agnostic, and any future PPC64 hypervisor
driver
+ * will have the same restriction.
+ */
+ if (ARCH_IS_PPC64(def->os.arch) &&
+ virDomainNVDimmAlignSizePseries(mem) < 0)
+ return -1;
+ }
For this and the rest of patches - shouldn't changes like this go into
validator callback? I view post parse callbacks as "fill missing
values" not a place to check if configuration makes sense/is valid.
You mean these callbacks?
domain_conf.h
/* validation callbacks */
virDomainDefValidateCallback domainValidateCallback;
virDomainDeviceDefValidateCallback deviceValidateCallback;
I mean virDomainDefValidate() and more specifically
virDomainDefValidateInternal(). Driver specific callbacks are out of
question - exactly for the reason you pointed out.
Michal
Re: [PATCH 01/21] domain_conf.c: move NVDIMM 'labelsize' check to post parse
On 12/1/20 3:46 PM, Michal Privoznik wrote:
On 11/24/20 8:20 PM, Daniel Henrique Barboza wrote:
Move 'labelsize' validation to virDomainMemoryDefPostParse().
Signed-off-by: Daniel Henrique Barboza
---
src/conf/domain_conf.c | 43 +-
1 file changed, 22 insertions(+), 21 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index b1534dcc1e..5e5905f483 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -5363,15 +5363,28 @@ static int
virDomainMemoryDefPostParse(virDomainMemoryDefPtr mem,
const virDomainDef *def)
{
- /* Although only the QEMU driver implements PPC64 support, this
- * code is related to the platform specification (PAPR), i.e. it
- * is hypervisor agnostic, and any future PPC64 hypervisor driver
- * will have the same restriction.
- */
- if (ARCH_IS_PPC64(def->os.arch) &&
- mem->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM &&
- virDomainNVDimmAlignSizePseries(mem) < 0)
- return -1;
+ if (mem->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM) {
+ if (mem->labelsize && mem->labelsize < 128) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("nvdimm label must be at least 128KiB"));
+ return -1;
+ }
+
+ if (mem->labelsize >= mem->size) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("label size must be smaller than NVDIMM size"));
+ return -1;
+ }
+
+ /* Although only the QEMU driver implements PPC64 support, this
+ * code is related to the platform specification (PAPR), i.e. it
+ * is hypervisor agnostic, and any future PPC64 hypervisor driver
+ * will have the same restriction.
+ */
+ if (ARCH_IS_PPC64(def->os.arch) &&
+ virDomainNVDimmAlignSizePseries(mem) < 0)
+ return -1;
+ }
For this and the rest of patches - shouldn't changes like this go into validator
callback? I view post parse callbacks as "fill missing values" not a place to
check if configuration makes sense/is valid.
You mean these callbacks?
domain_conf.h
/* validation callbacks */
virDomainDefValidateCallback domainValidateCallback;
virDomainDeviceDefValidateCallback deviceValidateCallback;
These callbacks makes sense for driver specific validations, but for what
we're doing here is driver agnostic (well, most of it anyway - probably
there are QEMU specific stuff mixed in).
If we move this logic to say qemuValidateDomainDeviceDef(), then we'll need
to compensate the other drivers that won't have access to these validations
(git grep tells me it's bhyve and vz_driver). Granted, we can put these in
an unique function and use them in the callback for all the drivers, if that's
the case.
Thanks,
DHB
Michal
Re: [PATCH 00/18] qapi/qom: QAPIfy object-add
Am 01.12.2020 um 18:16 hat Paolo Bonzini geschrieben: > On 01/12/20 17:20, Kevin Wolf wrote: > > Am 30.11.2020 um 20:35 hat Paolo Bonzini geschrieben: > > > 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. > > > > Sure, the number of devices means that we can't convert all of them at > > once. And we don't need to, we can make the change incrementally. > > There's also the question that most devices are not present in all binaries. > So QAPI introspection would tell you what properties are supported but not > what devices are. Also the marshaling/unmarshaling code for hundreds of > devices would bloat the QEMU executables unnecessarily (it'd all be > reachable from visit_DeviceOptions so it'd not be possible to compile it > out, even with LTO). I don't think this is actually a new things. We already have types and commands declared with things like 'if': 'defined(TARGET_S390X)'. As far as I understand, QAPI generated files are already built per target, so not compiling things into all binaries should be entirely possible. What probably needs a solution is that an explicit 'if' in the schema would duplicate information that is actually defined in the build system configuration. So we would somehow need a common source for both. > Plus the above issue with machine types. As I said, I don't know the machine type code well, so I'm not really sure about the best way there. But maybe QAPI isn't for version-specific machine types. I think they never have additional properties, but only different init functions. So their description in QAPI would be essentially empty anyway. So maybe only the abstract base class that actually defines the machine properties (like generic-pc-machine) should be described in QAPI, and then the concrete machine types would inherit from it without being described in QAPI themselves? > > > > > 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. > > > > Well, we all know that this series duplicates things. But at the same > > time, we also know that this isn't going to be the final state. > > > > Once QAPI learns about QOM inheritance (which it has to if it should > > generate the boilerplate), it will know which objects are user creatable > > without a an explicitly defined separate enum. > > > > I think it will still need to have the enum internally, but as long as > > it's automatically generated, that shouldn't be a big deal. > > Right, I don't want to have the final state now but I'd like to have a > rough idea of the plan: > > 1) whether to generate _all_ boilerplate or only properties I would like to generate as much boilerplate as possible. That is, I don't want to constrain us to only properties, but at the same time, I'm not sure if it's possible to get rid of all boilerplate. Basically, the vision I have in mind is that QAPI would generate code that is the same for most instances, and then provide an option that prevents code generation for a specific part for more complicated cases, so that the respective function can (and must) be provided in the C source. > 2) whether we want to introduce a separation between configuration > schema and run-time state You mean the internal run-time state? How is this separation not already present with getter/setter functions for each property? In many cases they just directly access the run-time state, but there are other cases where they actually do things. Or do you mean between create-time options and properties that can be written at runtime? In that case, yes, I think that would be very desirable because mashing them together has resulted in lots of bugs. > 3) in the latter case, whether properties will survive at all---iothread and > throttle-groups don't really need them even if they're writable after > creation. How do you define properties, i.e. at which point would they stop existing and what would be a non-property alternative? Outside of QOM, usually have a query-* command that returns the whole state rather than a single property, and possibly individual QMP commands to update the state. blockdev-reopen takes a new configuration (the same structure as blockdev-add) and then applies that
Re: qemu no-shutdown feature in libvirt
On Tue, Dec 01, 2020 at 07:38:51PM +0100, Jiri Denemark wrote: > On Tue, Dec 01, 2020 at 17:53:30 +, Daniel P. Berrangé wrote: > > On Tue, Dec 01, 2020 at 05:09:26PM +0100, Jiri Denemark wrote: > > > On Tue, Dec 01, 2020 at 15:34:05 +0100, Peter Krempa wrote: > > > > On Mon, Nov 30, 2020 at 14:45:18 -0500, Luna Xu wrote: > > > > > Hi developers, > > > > > > > > Hi, > > > > > > > > [...] > > > > > > > > > When I start a VM with the qemu command I can specify the -no-shutdown > > > > > flag so that my qemu process doesn't quit even if I shutdown the VM > > > > > from the inside (issue shutdown or halt command inside VM). The VM is > > > > > shutdown but the qemu process is not killed so that the jobs I > > > > > submitted before for example backup (drive-backup) can continue to run > > > > > even after VM gets shutdown by the user, and I'm able to check the > > > > > status of the job through qmp commands (by communicating with the qemu > > > > > process). > > > > > > > > As I've noted in my reply on libvirt-users, we currently don't support > > > > this on the shutdown part of the VM lifecycle. > > > > > > > > On the startup part you can use VIR_DOMAIN_START_PAUSED for a VM which > > > > didn't yet excecute any guest instructions. > > > > > > > > > Is anyone aware of any project that is currently implementing this > > > > > functionality? Thanks for reading. > > > > > > > > I don't think that there's anybody working on it. > > > > > > > > If you are interested in implementing the feature the idea would be to > > > > add a 'pause' action for pause along with a > > > > new 'virDomainPausedReason' such as VIR_DOMAIN_PAUSED_POWEROFF and > > > > > > > implement the lifecycle transition from PAUSED -> RUNNING via > > > > qemuDomainResume, which would in this case have to issue a > > > > 'system-reset' qmp command and emit the appropriate events. > > > > > > This is weird, the domain was paused just before being shut down so > > > resuming it should logically just resume the paused process and finish > > > the shutdown. On the other hand, domain shutdown as a result of calling > > > resume is weird too. > > > > > > So how about just adding a check to qemuDomainResume to report an error > > > if you try to resume a domain which was paused for shutdown? > > > > Note we have a virDomainReset() API that does a full machine/cpu reset, > > so if we're in the puased state after shutdown, then a reset followed > > by a resume is a valid way to boot the guest OS fresh. This is in fact > > how we fake graceful reboots. > > Sure, I just didn't want virDomainResume to automagically do the reset > part. I guess you're suggesting that we should not report an error > because virDomainReset would not change the domain state (or would it?) > and thus reporting an error would block this scenario. So in that case, > we should not report any error and in just try to resume the CPUs. In > other words, this would mean no change is really needed in > qemuDomainResume. We should be in the "SHUTDOWN" state when paused at the end of shutdown. Normally you'd never see this stsate as we immediately go into SHUTOFF. A virDomainReset should likely transition back to "RUNNING", but "PAUSED". So we can block on the state in virDomainResume if we're in SHUTDOWN. 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 10/21] domain_conf: modernize virDomainDiskDefParseXML()
On 11/24/20 8:20 PM, Daniel Henrique Barboza wrote: Register an AUTOPTR_CLEANUP_FUNC for virDomainDiskDefPtr, then use g_autoptr() in virDomainDiskDef and virStorageEncryption pointers to get rid of the 'cleanup' and 'error' labels. Signed-off-by: Daniel Henrique Barboza --- src/conf/domain_conf.c | 95 +++--- src/conf/domain_conf.h | 1 + 2 files changed, 45 insertions(+), 51 deletions(-) Reviewed-by: Michal Privoznik Michal
Re: [PATCH 14/21] domain_conf.c: remove 'error' label in virDomainDefTunablesParse()
On 11/24/20 8:20 PM, Daniel Henrique Barboza wrote: The 'error' label is just doing a 'return -1'. There's also a couple of 'VIR_FREE(nodes)' calls that are happening right before exiting on error, but 'nodes' is already set for autocleanup. These calls can also be removed. Signed-off-by: Daniel Henrique Barboza --- src/conf/domain_conf.c | 77 -- 1 file changed, 36 insertions(+), 41 deletions(-) Reviewed-by: Michal Privoznik Michal
Re: [PATCH 07/21] domain_conf.c: use g_autoptr() with virDomainVideoDefPtr
On 11/24/20 8:20 PM, Daniel Henrique Barboza wrote: This will modernize virDomainVideoDefParseXML() and virDomainDefAddImplicitVideo() by removing unneeded cleanup labels. Signed-off-by: Daniel Henrique Barboza --- src/conf/domain_conf.c | 48 ++ 1 file changed, 20 insertions(+), 28 deletions(-) Reviewed-by: Michal Privoznik Michal
Re: [PATCH 18/21] domain_conf.c: modernize virDomainControllerDefParseXML()
On 11/24/20 8:20 PM, Daniel Henrique Barboza wrote: Let's register AUTOPTR_CLEANUP_FUNC for virDomainControllerDefPtr and modernize this function, removing the 'error' label using g_autoptr(). Signed-off-by: Daniel Henrique Barboza --- src/conf/domain_conf.c | 60 -- src/conf/domain_conf.h | 1 + 2 files changed, 29 insertions(+), 32 deletions(-) Reviewed-by: Michal Privoznik Michal
Re: [PATCH 19/21] domain_conf.c: modernize virDomainDefControllersParse()
On 11/24/20 8:20 PM, Daniel Henrique Barboza wrote: The 'error' label is just returning -1, so let's 'return -1' directly. Use g_autoptr() with virDomainControllerDefPtr to remove the need to call virDomainControllerDefFree() in the error path. There is no need to VIR_FREE(nodes) explictly since 'nodes' is using g_autofree. Signed-off-by: Daniel Henrique Barboza --- src/conf/domain_conf.c | 26 ++ 1 file changed, 10 insertions(+), 16 deletions(-) Reviewed-by: Michal Privoznik Michal
Re: [PATCH 13/21] domain_conf.c: modernize virDomainSmartcardDefParseXML
On 11/24/20 8:20 PM, Daniel Henrique Barboza wrote: Register a AUTOPTR_CLEANUP_FUNC for virDomainSmartcardDef and use g_autoptr() to eliminate the 'error' label. Signed-off-by: Daniel Henrique Barboza --- src/conf/domain_conf.c | 34 +++--- src/conf/domain_conf.h | 1 + 2 files changed, 16 insertions(+), 19 deletions(-) Reviewed-by: Michal Privoznik Michal
Re: [PATCH 09/21] virstorageencryption.h: add AUTOPTR_CLEANUP_FUNC for virStorageEncryptionPtr
On 11/24/20 8:20 PM, Daniel Henrique Barboza wrote: This will open an opportunity to modernize virDomainDiskDefParseXML() in the next patch. Signed-off-by: Daniel Henrique Barboza --- src/util/virstorageencryption.h | 1 + 1 file changed, 1 insertion(+) Reviewed-by: Michal Privoznik Michal
Re: [PATCH 05/21] domain_conf.c: do not leak 'video' in virDomainDefParseXML()
On 11/24/20 8:20 PM, Daniel Henrique Barboza wrote: The 'video' pointer is only being freed on error path, meaning that we're leaking it after each loop restart. There are more opportunities for auto cleanups of virDomainVideoDef pointers, so let's register AUTOPTR_CLEANUP_FUNC for it to use g_autoptr() later on. Signed-off-by: Daniel Henrique Barboza --- src/conf/domain_conf.c | 4 +--- src/conf/domain_conf.h | 1 + 2 files changed, 2 insertions(+), 3 deletions(-) Reviewed-by: Michal Privoznik Michal
Re: [PATCH 01/21] domain_conf.c: move NVDIMM 'labelsize' check to post parse
On 11/24/20 8:20 PM, Daniel Henrique Barboza wrote:
Move 'labelsize' validation to virDomainMemoryDefPostParse().
Signed-off-by: Daniel Henrique Barboza
---
src/conf/domain_conf.c | 43 +-
1 file changed, 22 insertions(+), 21 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index b1534dcc1e..5e5905f483 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -5363,15 +5363,28 @@ static int
virDomainMemoryDefPostParse(virDomainMemoryDefPtr mem,
const virDomainDef *def)
{
-/* Although only the QEMU driver implements PPC64 support, this
- * code is related to the platform specification (PAPR), i.e. it
- * is hypervisor agnostic, and any future PPC64 hypervisor driver
- * will have the same restriction.
- */
-if (ARCH_IS_PPC64(def->os.arch) &&
-mem->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM &&
-virDomainNVDimmAlignSizePseries(mem) < 0)
-return -1;
+if (mem->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM) {
+if (mem->labelsize && mem->labelsize < 128) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("nvdimm label must be at least 128KiB"));
+return -1;
+}
+
+if (mem->labelsize >= mem->size) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("label size must be smaller than NVDIMM size"));
+return -1;
+}
+
+/* Although only the QEMU driver implements PPC64 support, this
+ * code is related to the platform specification (PAPR), i.e. it
+ * is hypervisor agnostic, and any future PPC64 hypervisor driver
+ * will have the same restriction.
+ */
+if (ARCH_IS_PPC64(def->os.arch) &&
+virDomainNVDimmAlignSizePseries(mem) < 0)
+return -1;
+}
For this and the rest of patches - shouldn't changes like this go into
validator callback? I view post parse callbacks as "fill missing values"
not a place to check if configuration makes sense/is valid.
Michal
Re: [PATCH 02/21] domain_conf.c: use g_autofree in 'dev' in virDomainDefParseBootXML()
On 11/24/20 8:20 PM, Daniel Henrique Barboza wrote: This spares us of 2 explicit VIR_FREE() calls. Signed-off-by: Daniel Henrique Barboza --- src/conf/domain_conf.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) Reviewed-by: Michal Privoznik Michal
Re: [PATCH 03/21] domain_conf.c: modernize virDomainDefBootOrderPostParse()
On 11/24/20 8:20 PM, Daniel Henrique Barboza wrote: Use g_autoptr() with the hash and remove the 'cleanup' label. Signed-off-by: Daniel Henrique Barboza --- src/conf/domain_conf.c | 15 +-- 1 file changed, 5 insertions(+), 10 deletions(-) Reviewed-by: Michal Privoznik Michal
Re: qemu no-shutdown feature in libvirt
On Tue, Dec 01, 2020 at 17:53:30 +, Daniel P. Berrangé wrote: > On Tue, Dec 01, 2020 at 05:09:26PM +0100, Jiri Denemark wrote: > > On Tue, Dec 01, 2020 at 15:34:05 +0100, Peter Krempa wrote: > > > On Mon, Nov 30, 2020 at 14:45:18 -0500, Luna Xu wrote: > > > > Hi developers, > > > > > > Hi, > > > > > > [...] > > > > > > > When I start a VM with the qemu command I can specify the -no-shutdown > > > > flag so that my qemu process doesn't quit even if I shutdown the VM > > > > from the inside (issue shutdown or halt command inside VM). The VM is > > > > shutdown but the qemu process is not killed so that the jobs I > > > > submitted before for example backup (drive-backup) can continue to run > > > > even after VM gets shutdown by the user, and I'm able to check the > > > > status of the job through qmp commands (by communicating with the qemu > > > > process). > > > > > > As I've noted in my reply on libvirt-users, we currently don't support > > > this on the shutdown part of the VM lifecycle. > > > > > > On the startup part you can use VIR_DOMAIN_START_PAUSED for a VM which > > > didn't yet excecute any guest instructions. > > > > > > > Is anyone aware of any project that is currently implementing this > > > > functionality? Thanks for reading. > > > > > > I don't think that there's anybody working on it. > > > > > > If you are interested in implementing the feature the idea would be to > > > add a 'pause' action for pause along with a > > > new 'virDomainPausedReason' such as VIR_DOMAIN_PAUSED_POWEROFF and > > > > > implement the lifecycle transition from PAUSED -> RUNNING via > > > qemuDomainResume, which would in this case have to issue a > > > 'system-reset' qmp command and emit the appropriate events. > > > > This is weird, the domain was paused just before being shut down so > > resuming it should logically just resume the paused process and finish > > the shutdown. On the other hand, domain shutdown as a result of calling > > resume is weird too. > > > > So how about just adding a check to qemuDomainResume to report an error > > if you try to resume a domain which was paused for shutdown? > > Note we have a virDomainReset() API that does a full machine/cpu reset, > so if we're in the puased state after shutdown, then a reset followed > by a resume is a valid way to boot the guest OS fresh. This is in fact > how we fake graceful reboots. Sure, I just didn't want virDomainResume to automagically do the reset part. I guess you're suggesting that we should not report an error because virDomainReset would not change the domain state (or would it?) and thus reporting an error would block this scenario. So in that case, we should not report any error and in just try to resume the CPUs. In other words, this would mean no change is really needed in qemuDomainResume. Jirka
Re: [PATCH 00/18] qapi/qom: QAPIfy object-add
On Tue, Dec 01, 2020 at 06:16:14PM +0100, Paolo Bonzini wrote: > On 01/12/20 17:20, Kevin Wolf wrote: [...] > > BlockdevOptions is about external interfaces, not about > > implementation details. Same thing as QOM properties are external > > interfaces, not implementation details. There may be fields in the > > internal state that correspond 1:1 to the externally visible QAPI > > type/QOM property, but it's not necessarily the case. > > Right. It may well be that we decide that, as a result of this series, > QOM's property interface is declared essentially a failed experiment. I > wouldn't be surprised, and that's why I want to understand better where we > want to go. > > For example, Eduardo is focusing specifically on external interfaces that > correspond 1:1 to the internal implementation. If we decide that > non-1:1-mappings and checks on mandatory properties are an important part of > QOM, that may make large parts of his work moot. [...] Whatever we decide, my first and biggest worry is to have a reasonable migration path for any new API. We could describe in detail what's the API we _want_, but we also need a reasonable migration path for the code we already _have_. If a new API that requires manual conversion of existing devices, it will probably coexist with the existing qdev property API for years. (Note that I haven't read this whole thread yet.) >[...] If we decide that most QOM > objects need no properties at all, then we don't want to move more > qdev-specific stuff from to QOM. I don't understand what "move more qdev-specific stuff to QOM" means. I consider the QOM field property API valuable even if we decide the only user of the API will be legacy qdev code, because it separates the core mechanism (that's code that already existed) from qdev-specific policies. > > > QAPI is already here and it's going to stay. QOM doesn't have to > > duplicate input validation that existing code can already perform. > > > > I'm not sure which complexity you think I'm introducing: QAPI is already > > there. I'm adding the schema, which you agree is valuable documentation, > > so we want to have it either case. The actual change to QOM that we have > > in this series is this: > > The complexity is that properties used to be split in two places, and now > they're split in three places. > > It may very well be that this is a good first step to at least have classes > described in the QAPI schema. But since _in the short term_ there are > things that the series makes worse (and has a risk of bringing things out of > sync), I'd like to understand the long term plan and ensure that the QAPI > maintainers are on board with it. > > Can you at least add a comment to all UserCreatable classes that says "if > you add a property, remember to modify ... as well in the QAPI schema"? > > > > 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). > > > > Yes, I found bugs that the QAPI schema would have prevented. They were > > generally about not checking whether mandatory propertes are actually > > set. > > Okay, I found your series at > https://patchew.org/QEMU/20201130105615.21799-1-kw...@redhat.com/ too, good > to know. > > So that's another useful thing that can be chalked to this series at least > if -object and object_add are converted (and also, another thing against QOM > properties and 1:1 mappings between configuration schema and run-time > state). > > Paolo > -- Eduardo
Re: Adding an nftables backend in addition to iptables?
Hi, > 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. I see, thank you! Based on the error message I wrongly assumed that this was an intentionally forced transition from iptables to nft... I confirmed that the same invocation works fine on my Kali machine, so it certainly looks like a Debian specific bug. Out of curiosity, I built the same version that I tried on Kali (v1.8.5) directly from the Netfilter git repo which gives me the same error. But it is linked to the same libnftnl library, so a wild guess would be that there's a bug in the Debian Testing version of libnftnl. Anyway, that is clearly off-topic for this list, I will file a bug report for the Debian package. Thanks again, Aljoscha
Re: qemu no-shutdown feature in libvirt
Hi Daniel, Thanks for replying. Yes a reset will do if the VM gets restarted. In this case, it makes sense to just need to implement the pause function on shutdown in libvirt for qemu. Thanks for pointing out. Thanks, Luna On Tue, Dec 1, 2020 at 12:53 PM Daniel P. Berrangé wrote: > > On Tue, Dec 01, 2020 at 05:09:26PM +0100, Jiri Denemark wrote: > > On Tue, Dec 01, 2020 at 15:34:05 +0100, Peter Krempa wrote: > > > On Mon, Nov 30, 2020 at 14:45:18 -0500, Luna Xu wrote: > > > > Hi developers, > > > > > > Hi, > > > > > > [...] > > > > > > > When I start a VM with the qemu command I can specify the -no-shutdown > > > > flag so that my qemu process doesn't quit even if I shutdown the VM > > > > from the inside (issue shutdown or halt command inside VM). The VM is > > > > shutdown but the qemu process is not killed so that the jobs I > > > > submitted before for example backup (drive-backup) can continue to run > > > > even after VM gets shutdown by the user, and I'm able to check the > > > > status of the job through qmp commands (by communicating with the qemu > > > > process). > > > > > > As I've noted in my reply on libvirt-users, we currently don't support > > > this on the shutdown part of the VM lifecycle. > > > > > > On the startup part you can use VIR_DOMAIN_START_PAUSED for a VM which > > > didn't yet excecute any guest instructions. > > > > > > > Is anyone aware of any project that is currently implementing this > > > > functionality? Thanks for reading. > > > > > > I don't think that there's anybody working on it. > > > > > > If you are interested in implementing the feature the idea would be to > > > add a 'pause' action for pause along with a > > > new 'virDomainPausedReason' such as VIR_DOMAIN_PAUSED_POWEROFF and > > > > > implement the lifecycle transition from PAUSED -> RUNNING via > > > qemuDomainResume, which would in this case have to issue a > > > 'system-reset' qmp command and emit the appropriate events. > > > > This is weird, the domain was paused just before being shut down so > > resuming it should logically just resume the paused process and finish > > the shutdown. On the other hand, domain shutdown as a result of calling > > resume is weird too. > > > > So how about just adding a check to qemuDomainResume to report an error > > if you try to resume a domain which was paused for shutdown? > > Note we have a virDomainReset() API that does a full machine/cpu reset, > so if we're in the puased state after shutdown, then a reset followed > by a resume is a valid way to boot the guest OS fresh. This is in fact > how we fake graceful reboots. > > 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: qemu no-shutdown feature in libvirt
On Tue, Dec 01, 2020 at 05:09:26PM +0100, Jiri Denemark wrote: > On Tue, Dec 01, 2020 at 15:34:05 +0100, Peter Krempa wrote: > > On Mon, Nov 30, 2020 at 14:45:18 -0500, Luna Xu wrote: > > > Hi developers, > > > > Hi, > > > > [...] > > > > > When I start a VM with the qemu command I can specify the -no-shutdown > > > flag so that my qemu process doesn't quit even if I shutdown the VM > > > from the inside (issue shutdown or halt command inside VM). The VM is > > > shutdown but the qemu process is not killed so that the jobs I > > > submitted before for example backup (drive-backup) can continue to run > > > even after VM gets shutdown by the user, and I'm able to check the > > > status of the job through qmp commands (by communicating with the qemu > > > process). > > > > As I've noted in my reply on libvirt-users, we currently don't support > > this on the shutdown part of the VM lifecycle. > > > > On the startup part you can use VIR_DOMAIN_START_PAUSED for a VM which > > didn't yet excecute any guest instructions. > > > > > Is anyone aware of any project that is currently implementing this > > > functionality? Thanks for reading. > > > > I don't think that there's anybody working on it. > > > > If you are interested in implementing the feature the idea would be to > > add a 'pause' action for pause along with a > > new 'virDomainPausedReason' such as VIR_DOMAIN_PAUSED_POWEROFF and > > > implement the lifecycle transition from PAUSED -> RUNNING via > > qemuDomainResume, which would in this case have to issue a > > 'system-reset' qmp command and emit the appropriate events. > > This is weird, the domain was paused just before being shut down so > resuming it should logically just resume the paused process and finish > the shutdown. On the other hand, domain shutdown as a result of calling > resume is weird too. > > So how about just adding a check to qemuDomainResume to report an error > if you try to resume a domain which was paused for shutdown? Note we have a virDomainReset() API that does a full machine/cpu reset, so if we're in the puased state after shutdown, then a reset followed by a resume is a valid way to boot the guest OS fresh. This is in fact how we fake graceful reboots. 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: qemu no-shutdown feature in libvirt
Hi Jiri, Thanks for replying. Actually the no-shutdown flag behavior in qemu is not pausing the VM shutdown, the VM shuts down normally (like you see the linux message saying it's powered off in the tty console, and you can't log in again), but the qemu process stays to finish the async (coroutine) jobs in the background. If the libvirt could take the no-shutdown flag and pass it to qemu, qemu would handle this automatically but somehow libvirt shouldn't kill the qemu process but wait for the process to finish working and exit normally itself. If the VM is started again before the qemu process quits itself, we would need to find a way maybe wait for the last qemu process to quit before launching a new one, or reuse the qmeu process just start the VM virtualization coroutine again inside the same qemu process. Thanks, Luna On Tue, Dec 1, 2020 at 11:09 AM Jiri Denemark wrote: > > On Tue, Dec 01, 2020 at 15:34:05 +0100, Peter Krempa wrote: > > On Mon, Nov 30, 2020 at 14:45:18 -0500, Luna Xu wrote: > > > Hi developers, > > > > Hi, > > > > [...] > > > > > When I start a VM with the qemu command I can specify the -no-shutdown > > > flag so that my qemu process doesn't quit even if I shutdown the VM > > > from the inside (issue shutdown or halt command inside VM). The VM is > > > shutdown but the qemu process is not killed so that the jobs I > > > submitted before for example backup (drive-backup) can continue to run > > > even after VM gets shutdown by the user, and I'm able to check the > > > status of the job through qmp commands (by communicating with the qemu > > > process). > > > > As I've noted in my reply on libvirt-users, we currently don't support > > this on the shutdown part of the VM lifecycle. > > > > On the startup part you can use VIR_DOMAIN_START_PAUSED for a VM which > > didn't yet excecute any guest instructions. > > > > > Is anyone aware of any project that is currently implementing this > > > functionality? Thanks for reading. > > > > I don't think that there's anybody working on it. > > > > If you are interested in implementing the feature the idea would be to > > add a 'pause' action for pause along with a > > new 'virDomainPausedReason' such as VIR_DOMAIN_PAUSED_POWEROFF and > > > implement the lifecycle transition from PAUSED -> RUNNING via > > qemuDomainResume, which would in this case have to issue a > > 'system-reset' qmp command and emit the appropriate events. > > This is weird, the domain was paused just before being shut down so > resuming it should logically just resume the paused process and finish > the shutdown. On the other hand, domain shutdown as a result of calling > resume is weird too. > > So how about just adding a check to qemuDomainResume to report an error > if you try to resume a domain which was paused for shutdown? > > Jirka
Re: [PATCH 00/18] qapi/qom: QAPIfy object-add
On 01/12/20 17:20, Kevin Wolf wrote: Am 30.11.2020 um 20:35 hat Paolo Bonzini geschrieben: 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. Sure, the number of devices means that we can't convert all of them at once. And we don't need to, we can make the change incrementally. There's also the question that most devices are not present in all binaries. So QAPI introspection would tell you what properties are supported but not what devices are. Also the marshaling/unmarshaling code for hundreds of devices would bloat the QEMU executables unnecessarily (it'd all be reachable from visit_DeviceOptions so it'd not be possible to compile it out, even with LTO). Plus the above issue with machine types. 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. Well, we all know that this series duplicates things. But at the same time, we also know that this isn't going to be the final state. Once QAPI learns about QOM inheritance (which it has to if it should generate the boilerplate), it will know which objects are user creatable without a an explicitly defined separate enum. I think it will still need to have the enum internally, but as long as it's automatically generated, that shouldn't be a big deal. Right, I don't want to have the final state now but I'd like to have a rough idea of the plan: 1) whether to generate _all_ boilerplate or only properties 2) whether we want to introduce a separation between configuration schema and run-time state 3) in the latter case, whether properties will survive at all---iothread and throttle-groups don't really need them even if they're writable after creation. These questions have a substantial effect on how to handle this series. Without answers to this questions I cannot understand the long term and its interaction with other long term plans for QOM. 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. QAPI is first and foremost about user-visible parts, and in fact most of the problems I listed are about external interfaces. Yes, but QAPI is also about interfacing with existing code. Also, QAPI does not generate only structs, it also generate C function prototypes. I'm not sure whether a QOM object's more similar to the struct case (what you do here) or to the QMP command case: - there's a 1:1 correspondence between ObjectOptions cases and ucc->complete implementations - there's a registry of object types just like there's one for QMP commands. So another possibility could be that the QAPI generator essentially generated the user_creatable_add_type function that called out to user-provided functions qom_scsi_pr_helper_complete, qom_throttle_group_complete, etc. The arguments to those functions would be the configuration. That is a very interesting prospect (though one that would require changes to the QAPI code generator). BlockdevOptions is about external interfaces, not about implementation details. Same thing as QOM properties are external interfaces, not implementation details. There may be fields in the internal state that correspond 1:1 to the externally visible QAPI type/QOM property, but it's not necessarily the case. Right. It may well be that we decide that, as a result of this series, QOM's property interface is declared essentially a failed experiment. I wouldn't be surprised, and that's why I want to understand better where we want to go. For example, Eduardo is focusing specifically on external interfaces that correspond 1:1 to the internal implementation. If we decide that non-1:1-mappings and checks on mandatory properties are an important part of QOM, that may make large parts of his work moot. If we decide that most QOM objects need no properties at all, then we don't want to move more qdev-specific stuff from to QOM. QAPI is already here and it's going to stay. QOM doesn't have to duplicate input validation that existing
Re: [PATCH 00/21] move checks from parse functions to post parse
Ping On 11/24/20 4:20 PM, Daniel Henrique Barboza wrote: Hi, This started as a simple NVDIMM change, then I realized there is a Gitlab work item for it [1], so I took the extra mile and did a bit more. I'll copy/paste here the motivation for this kind of change, provided by Cole in [1]: - The code that handles domain/VM XML parsing (virDomainDefParseXML in src/domain/domain_conf.c) has various validation checks sprinkled within it. Many of these checks should be moved out of the parse step and into virDomainDefPostParse, so they can be shared by various places in the code that build a domain definition without XML, such as converting from native vmware/virtualbox/xen formats. - There are still checks to be moved after this work. If this is accepted I'll update [1] with more details/tips about how to proceed with the remaining checks. Some g_auto* cleanups were done along the way. [1] https://gitlab.com/libvirt/libvirt/-/issues/7 Daniel Henrique Barboza (21): domain_conf.c: move NVDIMM 'labelsize' check to post parse domain_conf.c: use g_autofree in 'dev' in virDomainDefParseBootXML() domain_conf.c: modernize virDomainDefBootOrderPostParse() domain_conf.c: move boot related timeouts check to post parse domain_conf.c: do not leak 'video' in virDomainDefParseXML() domain_conf.c: move primary video check to virDomainDefPostParseVideo() domain_conf.c: use g_autoptr() with virDomainVideoDefPtr domain_conf.c: move QXL attributes check to virDomainVideoDefPostParse() virstorageencryption.h: add AUTOPTR_CLEANUP_FUNC for virStorageEncryptionPtr domain_conf: modernize virDomainDiskDefParseXML() domain_conf.c: move vendor, product and tray checks to post parse domain_conf.c: move smartcard address check to post parse domain_conf.c: modernize virDomainSmartcardDefParseXML domain_conf.c: remove 'error' label in virDomainDefTunablesParse() domain_conf.c: move duplicate blkio path check to post parse domain_conf.c: move virDomainPCIControllerOpts checks to post parse domain_conf.c: move pci-root/pcie-root address check to post parse domain_conf.c: modernize virDomainControllerDefParseXML() domain_conf.c: modernize virDomainDefControllersParse() domain_conf.c: use VIR_ERR_CONFIG_UNSUPPORTED in post parse domain_conf.c: move idmapEntry checks to post parse src/conf/domain_conf.c| 762 ++ src/conf/domain_conf.h| 4 + src/util/virstorageencryption.h | 1 + tests/qemuxml2argvdata/pci-root-address.err | 2 +- .../pseries-default-phb-numa-node.err | 2 +- .../video-multiple-primaries.err | 1 + .../video-multiple-primaries.xml | 32 + tests/qemuxml2argvtest.c | 5 + 8 files changed, 451 insertions(+), 358 deletions(-) create mode 100644 tests/qemuxml2argvdata/video-multiple-primaries.err create mode 100644 tests/qemuxml2argvdata/video-multiple-primaries.xml
[PATCH 1/2] lib: Replace VIR_AUTOSTRINGLIST with GStrv
Glib provides g_auto(GStrv) which is in-place replacement of our
VIR_AUTOSTRINGLIST.
Signed-off-by: Michal Privoznik
---
src/conf/cpu_conf.c| 2 +-
src/conf/domain_conf.c | 2 +-
src/cpu/cpu_arm.c | 2 +-
src/libxl/xen_common.c | 10 +++
src/libxl/xen_xl.c | 2 +-
src/lxc/lxc_process.c | 2 +-
src/qemu/qemu_capabilities.c | 6 ++---
src/qemu/qemu_cgroup.c | 2 +-
src/qemu/qemu_conf.c | 10 +++
src/qemu/qemu_firmware.c | 4 +--
src/qemu/qemu_monitor_json.c | 4 +--
src/qemu/qemu_namespace.c | 28 ++--
src/qemu/qemu_process.c| 4 +--
src/qemu/qemu_qapi.c | 2 +-
src/qemu/qemu_vhost_user.c | 2 +-
src/rpc/virnetsocket.c | 4 +--
src/storage/storage_backend_sheepdog.c | 4 +--
src/storage/storage_backend_zfs.c | 12 -
src/util/vircgroup.c | 2 +-
src/util/vircommand.c | 2 +-
src/util/virdevmapper.c| 6 ++---
src/util/virfile.c | 2 +-
src/util/virfirewall.c | 2 +-
src/util/virhook.c | 2 +-
src/util/virjson.c | 2 +-
src/util/virprocess.c | 2 +-
src/util/virstoragefile.c | 10 +++
src/util/virsystemd.c | 2 +-
src/vmx/vmx.c | 2 +-
tests/qemufirmwaretest.c | 2 +-
tests/qemusecuritytest.c | 2 +-
tests/qemuvhostusertest.c | 2 +-
tests/qemuxml2argvtest.c | 4 +--
tests/virfirewalltest.c| 2 +-
tools/virsh-completer-domain.c | 36 +-
tools/virsh-completer-host.c | 6 ++---
tools/virsh-completer-interface.c | 2 +-
tools/virsh-completer-network.c| 8 +++---
tools/virsh-completer-nodedev.c| 6 ++---
tools/virsh-completer-nwfilter.c | 4 +--
tools/virsh-completer-pool.c | 6 ++---
tools/virsh-completer-secret.c | 4 +--
tools/virsh-completer-snapshot.c | 2 +-
tools/virsh-completer-volume.c | 2 +-
tools/virsh-completer.c| 4 +--
tools/virsh-domain.c | 4 +--
46 files changed, 116 insertions(+), 116 deletions(-)
diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c
index 5ff87cb41c..a2d88ba818 100644
--- a/src/conf/cpu_conf.c
+++ b/src/conf/cpu_conf.c
@@ -985,7 +985,7 @@ virCPUDefCheckFeatures(virCPUDefPtr cpu,
void *opaque,
char ***features)
{
-VIR_AUTOSTRINGLIST list = NULL;
+g_auto(GStrv) list = NULL;
size_t n = 0;
size_t i;
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index b1534dcc1e..cef3b2b9f1 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -32595,7 +32595,7 @@ static int
virDomainDiskAddISCSIPoolSourceHost(virStorageSourcePtr src,
virStoragePoolDefPtr pooldef)
{
-VIR_AUTOSTRINGLIST tokens = NULL;
+g_auto(GStrv) tokens = NULL;
size_t ntokens;
/* Only support one host */
diff --git a/src/cpu/cpu_arm.c b/src/cpu/cpu_arm.c
index 8a408a324a..6ba5bf0724 100644
--- a/src/cpu/cpu_arm.c
+++ b/src/cpu/cpu_arm.c
@@ -550,7 +550,7 @@ virCPUarmCpuDataFromRegs(virCPUarmData *data)
{
unsigned long cpuid;
unsigned long hwcaps;
-VIR_AUTOSTRINGLIST features = NULL;
+g_auto(GStrv) features = NULL;
int cpu_feature_index = 0;
size_t i;
diff --git a/src/libxl/xen_common.c b/src/libxl/xen_common.c
index c82e487d80..407d28aaa5 100644
--- a/src/libxl/xen_common.c
+++ b/src/libxl/xen_common.c
@@ -370,8 +370,8 @@ static virDomainHostdevDefPtr
xenParsePCI(char *entry)
{
virDomainHostdevDefPtr hostdev = NULL;
-VIR_AUTOSTRINGLIST tokens = NULL;
-VIR_AUTOSTRINGLIST options = NULL;
+g_auto(GStrv) tokens = NULL;
+g_auto(GStrv) options = NULL;
size_t ntokens = 0;
size_t nexttoken = 0;
char *str;
@@ -482,7 +482,7 @@ xenHandleConfGetValueStringListErrors(int ret)
static int
xenParsePCIList(virConfPtr conf, virDomainDefPtr def)
{
-VIR_AUTOSTRINGLIST pcis = NULL;
+g_auto(GStrv) pcis = NULL;
char **entries = NULL;
int rc;
@@ -709,7 +709,7 @@ xenParseVfb(virConfPtr conf, virDomainDefPtr def)
}
if (!hvm && def->graphics == NULL) { /* New PV guests use this format */
-VIR_AUTOSTRINGLIST vfbs = NULL;
+g_auto(GStrv) vfbs = NULL;
int rc;
if ((rc = virConfGetValueStringList(conf, "vfb", false, &vfbs)) == 1) {
@@ -941,7 +941,7 @@ xenParseSxprChar(const char *value,
static int
xenParseCharDev(virConfPtr conf, virDomainDefPtr def, const char *nativeFormat)
{
-VIR_AUTOSTRINGLIST serials = NULL;
+g_auto(GStrv) seria
[PATCH 2/2] virstring: Drop VIR_AUTOSTRINGLIST
Now that no one uses VIR_AUTOSTRINGLIST it can be dropped.
Signed-off-by: Michal Privoznik
---
src/libvirt_private.syms | 1 -
src/util/virstring.c | 10 --
src/util/virstring.h | 9 -
3 files changed, 20 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 179dcecb0a..2f640ef1c4 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -3227,7 +3227,6 @@ virStringHasSuffix;
virStringIsEmpty;
virStringIsPrintable;
virStringListAdd;
-virStringListAutoFree;
virStringListFreeCount;
virStringListGetFirstWithPrefix;
virStringListHasString;
diff --git a/src/util/virstring.c b/src/util/virstring.c
index 5c49b56f75..5578a5545b 100644
--- a/src/util/virstring.c
+++ b/src/util/virstring.c
@@ -259,16 +259,6 @@ virStringListMerge(char ***dst,
}
-void virStringListAutoFree(char ***strings)
-{
-if (!*strings)
-return;
-
-g_strfreev(*strings);
-*strings = NULL;
-}
-
-
/**
* virStringListFreeCount:
* @strings: array of strings to free
diff --git a/src/util/virstring.h b/src/util/virstring.h
index 561ce0cbc0..210e43a953 100644
--- a/src/util/virstring.h
+++ b/src/util/virstring.h
@@ -45,7 +45,6 @@ void virStringListRemove(char ***strings,
int virStringListMerge(char ***dst,
char ***src);
-void virStringListAutoFree(char ***strings);
void virStringListFreeCount(char **strings,
size_t count);
@@ -179,11 +178,3 @@ int virStringParsePort(const char *str,
int virStringParseYesNo(const char *str,
bool *result)
G_GNUC_WARN_UNUSED_RESULT;
-/**
- * VIR_AUTOSTRINGLIST:
- *
- * Declares a NULL-terminated list of strings which will be automatically freed
- * when the pointer goes out of scope.
- */
-#define VIR_AUTOSTRINGLIST \
-__attribute__((cleanup(virStringListAutoFree))) char **
--
2.26.2
[PATCH 0/2] Replace VIR_AUTOSTRINGLIST with GStrv
It was only recently that I learned about g_auto(GStrv). It's just like our VIR_AUTOSTRINGLIST. Michal Prívozník (2): lib: Replace VIR_AUTOSTRINGLIST with GStrv virstring: Drop VIR_AUTOSTRINGLIST src/conf/cpu_conf.c| 2 +- src/conf/domain_conf.c | 2 +- src/cpu/cpu_arm.c | 2 +- src/libvirt_private.syms | 1 - src/libxl/xen_common.c | 10 +++ src/libxl/xen_xl.c | 2 +- src/lxc/lxc_process.c | 2 +- src/qemu/qemu_capabilities.c | 6 ++--- src/qemu/qemu_cgroup.c | 2 +- src/qemu/qemu_conf.c | 10 +++ src/qemu/qemu_firmware.c | 4 +-- src/qemu/qemu_monitor_json.c | 4 +-- src/qemu/qemu_namespace.c | 28 ++-- src/qemu/qemu_process.c| 4 +-- src/qemu/qemu_qapi.c | 2 +- src/qemu/qemu_vhost_user.c | 2 +- src/rpc/virnetsocket.c | 4 +-- src/storage/storage_backend_sheepdog.c | 4 +-- src/storage/storage_backend_zfs.c | 12 - src/util/vircgroup.c | 2 +- src/util/vircommand.c | 2 +- src/util/virdevmapper.c| 6 ++--- src/util/virfile.c | 2 +- src/util/virfirewall.c | 2 +- src/util/virhook.c | 2 +- src/util/virjson.c | 2 +- src/util/virprocess.c | 2 +- src/util/virstoragefile.c | 10 +++ src/util/virstring.c | 10 --- src/util/virstring.h | 9 --- src/util/virsystemd.c | 2 +- src/vmx/vmx.c | 2 +- tests/qemufirmwaretest.c | 2 +- tests/qemusecuritytest.c | 2 +- tests/qemuvhostusertest.c | 2 +- tests/qemuxml2argvtest.c | 4 +-- tests/virfirewalltest.c| 2 +- tools/virsh-completer-domain.c | 36 +- tools/virsh-completer-host.c | 6 ++--- tools/virsh-completer-interface.c | 2 +- tools/virsh-completer-network.c| 8 +++--- tools/virsh-completer-nodedev.c| 6 ++--- tools/virsh-completer-nwfilter.c | 4 +-- tools/virsh-completer-pool.c | 6 ++--- tools/virsh-completer-secret.c | 4 +-- tools/virsh-completer-snapshot.c | 2 +- tools/virsh-completer-volume.c | 2 +- tools/virsh-completer.c| 4 +-- tools/virsh-domain.c | 4 +-- 49 files changed, 116 insertions(+), 136 deletions(-) -- 2.26.2
[PATCH v2] qemuDomainGetGuestInfo: Exit early if getting info fails
If there is an error getting info from guest agent, then the
control on qemuDomainGetGuestInfo() jumps onto 'exitagent' label
and subsequently continues on 'endagentjob'. Both labels are hit
also in success case too. The control then continues by
attempting to match fetched info (e.g. disk addresses) with
domain def. But this is needless - the API will return error
regardless.
To return early from the function move both 'exitagent' and
'endagentjob' labels at the end of the function and jump straight
onto 'cleanup' afterwards. This allows us to set 'ret = 0' later
- only when we know we succeeded.
Signed-off-by: Michal Privoznik
---
v2 of:
https://www.redhat.com/archives/libvir-list/2020-December/msg00020.html
src/qemu/qemu_driver.c | 14 +-
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 8eaa3ce68f..bca1c84630 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -20116,12 +20116,7 @@ qemuDomainGetGuestInfo(virDomainPtr dom,
}
}
-ret = 0;
-
- exitagent:
qemuDomainObjExitAgent(vm, agent);
-
- endagentjob:
qemuDomainObjEndAgentJob(vm);
if (nfs > 0 || ndisks > 0) {
@@ -20143,6 +20138,8 @@ qemuDomainGetGuestInfo(virDomainPtr dom,
qemuDomainObjEndJob(driver, vm);
}
+ret = 0;
+
cleanup:
for (i = 0; i < nfs; i++)
qemuAgentFSInfoFree(agentfsinfo[i]);
@@ -20153,6 +20150,13 @@ qemuDomainGetGuestInfo(virDomainPtr dom,
virDomainObjEndAPI(&vm);
return ret;
+
+ exitagent:
+qemuDomainObjExitAgent(vm, agent);
+
+ endagentjob:
+qemuDomainObjEndAgentJob(vm);
+goto cleanup;
}
--
2.26.2
Re: [PATCH 1/3] qemuDomainGetGuestInfo: Exit early if getting info fails
On 12/1/20 4:39 PM, Ján Tomko wrote: On a Tuesday in 2020, Michal Privoznik wrote: If there is an error getting info from guest agent, then the control on qemuDomainGetGuestInfo() jumps onto 'exitagent' label and subsequently continues on 'endagentjob'. Both labels are hit also in success case, which is why there is a code that tries to match info obtained from the guest agent with domain definition. I'm confused by 'exitagent' and 'exitagentjob' being above code that is only done (or only makes sense) on success. And ret being set to zero so early - I guess that's due to the nature of the best-effort information gathering here. But I think it would be perfectly fine to error out if we fail to get a query job or the domain dies in the meantime. Moving the exitagent and endagentjob labels after the cleanup block would remove the need to check ret. (i.e. duplicating ExitAgent and EndAgentJob calls - one pair that would be exectued on success and one pair only on failure) Fair enough. Will post v2. Michal
Re: [PATCH 00/18] qapi/qom: QAPIfy object-add
Am 30.11.2020 um 20:35 hat Paolo Bonzini geschrieben: > 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. Sure, the number of devices means that we can't convert all of them at once. And we don't need to, we can make the change incrementally. The only thing we can't do during this incremental phase is switching device_add from 'gen': false to actually using QAPI types. Doing that would be the very last step in the conversion. > > > 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. Well, we all know that this series duplicates things. But at the same time, we also know that this isn't going to be the final state. Once QAPI learns about QOM inheritance (which it has to if it should generate the boilerplate), it will know which objects are user creatable without a an explicitly defined separate enum. I think it will still need to have the enum internally, but as long as it's automatically generated, that shouldn't be a big deal. > > 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. QAPI is first and foremost about user-visible parts, and in fact most of the problems I listed are about external interfaces. It seems to make a lot of sense to me to start there. > > > 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. I'm not sure what you mean with run-time state. Yes, BlockdevOptions is about external interfaces, not about implementation details. Same thing as QOM properties are external interfaces, not implementation details. There may be fields in the internal state that correspond 1:1 to the externally visible QAPI type/QOM property, but it's not necessarily the case. > > > > 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 b
Re: qemu no-shutdown feature in libvirt
On Tue, Dec 01, 2020 at 15:34:05 +0100, Peter Krempa wrote: > On Mon, Nov 30, 2020 at 14:45:18 -0500, Luna Xu wrote: > > Hi developers, > > Hi, > > [...] > > > When I start a VM with the qemu command I can specify the -no-shutdown > > flag so that my qemu process doesn't quit even if I shutdown the VM > > from the inside (issue shutdown or halt command inside VM). The VM is > > shutdown but the qemu process is not killed so that the jobs I > > submitted before for example backup (drive-backup) can continue to run > > even after VM gets shutdown by the user, and I'm able to check the > > status of the job through qmp commands (by communicating with the qemu > > process). > > As I've noted in my reply on libvirt-users, we currently don't support > this on the shutdown part of the VM lifecycle. > > On the startup part you can use VIR_DOMAIN_START_PAUSED for a VM which > didn't yet excecute any guest instructions. > > > Is anyone aware of any project that is currently implementing this > > functionality? Thanks for reading. > > I don't think that there's anybody working on it. > > If you are interested in implementing the feature the idea would be to > add a 'pause' action for pause along with a > new 'virDomainPausedReason' such as VIR_DOMAIN_PAUSED_POWEROFF and > implement the lifecycle transition from PAUSED -> RUNNING via > qemuDomainResume, which would in this case have to issue a > 'system-reset' qmp command and emit the appropriate events. This is weird, the domain was paused just before being shut down so resuming it should logically just resume the paused process and finish the shutdown. On the other hand, domain shutdown as a result of calling resume is weird too. So how about just adding a check to qemuDomainResume to report an error if you try to resume a domain which was paused for shutdown? Jirka
Re: [libvirt PATCH] docs: Update language bindings spotlight
On a Tuesday in 2020, Andrea Bolognani wrote: We should highlight the language bindings that are actively maintained, keep up with the core library's development pace, have good API coverage and are relevant to people looking to integrate libvirt into their projects in $currentyear: based Consider %Y from strftime(3) instead of that unreadable bashism. on these criteria, it makes sense to highlight the Go binding instead of the Java one. Signed-off-by: Andrea Bolognani --- docs/index.html.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH 3/3] qemu: Use virJSONValueObjectGetStringArray() more
On a Tuesday in 2020, Michal Privoznik wrote: In a few commit back (v6.10.0-5-gb3dad96972) a new helper for obtaining string arrays from a virJSONObject was introduced: virJSONValueObjectGetStringArray(). I've identified three places where it can be used instead of open coding it: qemuAgentSSHGetAuthorizedKeys(), qemuMonitorJSONGetStringListProperty() and qemuMonitorJSONGetCPUDefinitions(). Signed-off-by: Michal Privoznik --- src/qemu/qemu_agent.c| 30 +++ src/qemu/qemu_monitor_json.c | 56 +--- 2 files changed, 11 insertions(+), 75 deletions(-) @@ -6788,9 +6764,6 @@ qemuMonitorJSONGetStringListProperty(qemuMonitorPtr mon, g_autoptr(virJSONValue) cmd = NULL; g_autoptr(virJSONValue) reply = NULL; VIR_AUTOSTRINGLIST list = NULL; list is now unused: ../src/qemu/qemu_monitor_json.c:6766:24: error: unused variable 'list' [-Werror,-Wunused-variable] VIR_AUTOSTRINGLIST list = NULL; ^ 1 error generated. -virJSONValuePtr data; -size_t n; -size_t i; *strList = NULL; with that fixed: Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: qemu no-shutdown feature in libvirt
Hi Peter, Thank you again for replying here. These are great instructions. I will take a look into these. Thank you! Thanks, Luna On Tue, Dec 1, 2020 at 9:34 AM Peter Krempa wrote: > > On Mon, Nov 30, 2020 at 14:45:18 -0500, Luna Xu wrote: > > Hi developers, > > Hi, > > [...] > > > When I start a VM with the qemu command I can specify the -no-shutdown > > flag so that my qemu process doesn't quit even if I shutdown the VM > > from the inside (issue shutdown or halt command inside VM). The VM is > > shutdown but the qemu process is not killed so that the jobs I > > submitted before for example backup (drive-backup) can continue to run > > even after VM gets shutdown by the user, and I'm able to check the > > status of the job through qmp commands (by communicating with the qemu > > process). > > As I've noted in my reply on libvirt-users, we currently don't support > this on the shutdown part of the VM lifecycle. > > On the startup part you can use VIR_DOMAIN_START_PAUSED for a VM which > didn't yet excecute any guest instructions. > > > Is anyone aware of any project that is currently implementing this > > functionality? Thanks for reading. > > I don't think that there's anybody working on it. > > If you are interested in implementing the feature the idea would be to > add a 'pause' action for pause along with a > new 'virDomainPausedReason' such as VIR_DOMAIN_PAUSED_POWEROFF and > implement the lifecycle transition from PAUSED -> RUNNING via > qemuDomainResume, which would in this case have to issue a > 'system-reset' qmp command and emit the appropriate events. >
Re: [PATCH 2/3] virJSONValueObjectGetStringArray: Report error if @key is not an array
On a Tuesday in 2020, Michal Privoznik wrote:
The virJSONValueObjectGetStringArray() function is given a @key
which is supposed to be an array inside given @object. Well, if
it's not then an error state is returned (NULL), but no error
message is set.
Signed-off-by: Michal Privoznik
---
src/util/virjson.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/src/util/virjson.c b/src/util/virjson.c
index d471923732..160f6172d2 100644
--- a/src/util/virjson.c
+++ b/src/util/virjson.c
@@ -1472,8 +1472,12 @@ virJSONValueObjectGetStringArray(virJSONValuePtr object,
const char *key)
size_t i;
data = virJSONValueObjectGetArray(object, key);
-if (!data)
+if (!data) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("%s is missing not an array"),
is missing or not an array
Unless I'm missing something.
Reviewed-by: Ján Tomko
Jano
+ key);
return NULL;
+}
n = virJSONValueArraySize(data);
ret = g_new0(char *, n + 1);
--
2.26.2
signature.asc
Description: PGP signature
Re: [PATCH 1/3] qemuDomainGetGuestInfo: Exit early if getting info fails
On a Tuesday in 2020, Michal Privoznik wrote:
If there is an error getting info from guest agent, then the
control on qemuDomainGetGuestInfo() jumps onto 'exitagent' label
and subsequently continues on 'endagentjob'. Both labels are hit
also in success case, which is why there is a code that tries to
match info obtained from the guest agent with domain definition.
I'm confused by 'exitagent' and 'exitagentjob' being above code
that is only done (or only makes sense) on success. And ret being
set to zero so early - I guess that's due to the nature of the
best-effort information gathering here. But I think it would be
perfectly fine to error out if we fail to get a query job or
the domain dies in the meantime.
Moving the exitagent and endagentjob labels after the cleanup
block would remove the need to check ret.
(i.e. duplicating ExitAgent and EndAgentJob calls - one
pair that would be exectued on success and one pair only on failure)
However, if we know that we've reached this area because of an
error (ret = -1) there is no reason for us to attempt finding the
match as the API as whole will end up with an error.
Signed-off-by: Michal Privoznik
---
src/qemu/qemu_driver.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 2d4b5a8b99..338c609854 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -20129,6 +20129,9 @@ qemuDomainGetGuestInfo(virDomainPtr dom,
endagentjob:
qemuDomainObjEndAgentJob(vm);
+if (ret < 0)
+goto cleanup;
+
if (nfs > 0 || ndisks > 0) {
if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0)
goto cleanup;
--
2.26.2
signature.asc
Description: PGP signature
[libvirt PATCH] docs: Update language bindings spotlight
We should highlight the language bindings that are actively maintained, keep up with the core library's development pace, have good API coverage and are relevant to people looking to integrate libvirt into their projects in $currentyear: based on these criteria, it makes sense to highlight the Go binding instead of the Java one. Signed-off-by: Andrea Bolognani --- docs/index.html.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/index.html.in b/docs/index.html.in index df182c27b6..f44e055174 100644 --- a/docs/index.html.in +++ b/docs/index.html.in @@ -18,7 +18,7 @@ is a toolkit to manage virtualization platforms -is accessible from C, Python, Perl, Java and more +is accessible from C, Python, Perl, Go and more is licensed under open source licenses supports KVM, QEMU, Xen, -- 2.26.2
[libvirt PATCH 1/3] gitlab: re-generate container images from lcitool
This introduces Fedora 33 and removes some redundant packages.
Signed-off-by: Daniel P. Berrangé
---
.gitlab-ci.yml| 20 +--
ci/containers/libvirt-centos-7.Dockerfile | 15 +-
ci/containers/libvirt-centos-8.Dockerfile | 16 ++-
.../libvirt-centos-stream.Dockerfile | 16 ++-
...libvirt-debian-10-cross-aarch64.Dockerfile | 16 +--
.../libvirt-debian-10-cross-armv6l.Dockerfile | 16 +--
.../libvirt-debian-10-cross-armv7l.Dockerfile | 16 +--
.../libvirt-debian-10-cross-i686.Dockerfile | 16 +--
.../libvirt-debian-10-cross-mips.Dockerfile | 16 +--
...ibvirt-debian-10-cross-mips64el.Dockerfile | 16 +--
.../libvirt-debian-10-cross-mipsel.Dockerfile | 16 +--
...libvirt-debian-10-cross-ppc64le.Dockerfile | 16 +--
.../libvirt-debian-10-cross-s390x.Dockerfile | 16 +--
ci/containers/libvirt-debian-10.Dockerfile| 16 +--
...ibvirt-debian-sid-cross-aarch64.Dockerfile | 16 +--
...libvirt-debian-sid-cross-armv6l.Dockerfile | 16 +--
...libvirt-debian-sid-cross-armv7l.Dockerfile | 16 +--
.../libvirt-debian-sid-cross-i686.Dockerfile | 16 +--
...bvirt-debian-sid-cross-mips64el.Dockerfile | 16 +--
...libvirt-debian-sid-cross-mipsel.Dockerfile | 16 +--
...ibvirt-debian-sid-cross-ppc64le.Dockerfile | 16 +--
.../libvirt-debian-sid-cross-s390x.Dockerfile | 16 +--
ci/containers/libvirt-debian-sid.Dockerfile | 16 +--
ci/containers/libvirt-fedora-32.Dockerfile| 15 +-
...ockerfile => libvirt-fedora-33.Dockerfile} | 19 ++
...rt-fedora-rawhide-cross-mingw32.Dockerfile | 15 +-
...rt-fedora-rawhide-cross-mingw64.Dockerfile | 15 +-
.../libvirt-fedora-rawhide.Dockerfile | 15 +-
ci/containers/libvirt-opensuse-151.Dockerfile | 17 ++--
ci/containers/libvirt-ubuntu-1804.Dockerfile | 16 +--
ci/containers/libvirt-ubuntu-2004.Dockerfile | 16 +--
31 files changed, 44 insertions(+), 455 deletions(-)
rename ci/containers/{libvirt-fedora-31.Dockerfile =>
libvirt-fedora-33.Dockerfile} (88%)
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 6792accf8f..f4f6aa8ee9 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -145,15 +145,15 @@ x64-debian-sid-container:
variables:
NAME: debian-sid
-x64-fedora-31-container:
+x64-fedora-32-container:
<<: *container_job_definition
variables:
-NAME: fedora-31
+NAME: fedora-32
-x64-fedora-32-container:
+x64-fedora-33-container:
<<: *container_job_definition
variables:
-NAME: fedora-32
+NAME: fedora-33
x64-fedora-rawhide-container:
<<: *container_job_definition
@@ -334,20 +334,20 @@ x64-centos-stream:
NAME: centos-stream
RPM: skip
-x64-fedora-31:
+x64-fedora-32:
<<: *native_build_job_definition
needs:
-- x64-fedora-31-container
+- x64-fedora-32-container
variables:
-NAME: fedora-31
+NAME: fedora-32
RPM: skip
-x64-fedora-32:
+x64-fedora-33:
<<: *native_build_job_definition
needs:
-- x64-fedora-32-container
+- x64-fedora-33-container
variables:
-NAME: fedora-32
+NAME: fedora-33
x64-fedora-rawhide:
<<: *native_build_job_definition
diff --git a/ci/containers/libvirt-centos-7.Dockerfile
b/ci/containers/libvirt-centos-7.Dockerfile
index cff225ccc7..ea460f76c9 100644
--- a/ci/containers/libvirt-centos-7.Dockerfile
+++ b/ci/containers/libvirt-centos-7.Dockerfile
@@ -1,4 +1,4 @@
-FROM centos:7
+FROM registry.centos.org/centos:7
RUN echo -e '[openvz]\n\
name=OpenVZ addons\n\
@@ -35,14 +35,11 @@ WEiJKtQrZDJloqtyi/mmRa1VsV7RYR0VPJjhK/R8EQ7Ysshy\n\
yum install -y \
audit-libs-devel \
augeas \
-autoconf \
-automake \
avahi-devel \
bash \
bash-completion \
ca-certificates \
ccache \
-chrony \
clang \
cyrus-sasl-devel \
dbus-devel \
@@ -53,9 +50,7 @@ WEiJKtQrZDJloqtyi/mmRa1VsV7RYR0VPJjhK/R8EQ7Ysshy\n\
firewalld-filesystem \
fuse-devel \
gcc \
-gdb \
gettext \
-gettext-devel \
git \
glib2-devel \
glibc-common \
@@ -80,16 +75,13 @@ WEiJKtQrZDJloqtyi/mmRa1VsV7RYR0VPJjhK/R8EQ7Ysshy\n\
libssh-devel \
libssh2-devel \
libtirpc-devel \
-libtool \
libudev-devel \
libwsman-devel \
libxml2 \
libxml2-devel \
libxslt \
-lsof \
lvm2 \
make \
-net-tools \
netcf-devel \
nfs-utils \
ninja-build \
@@ -112,15 +104,10 @@ WEiJKtQrZDJloqtyi/mmRa1VsV7RYR0VPJjhK/R8EQ7Ysshy\n\
readline-devel \
rpm-build \
[libvirt PATCH 2/3] gitlab: refresh containers with lcitool for fully minimized base
Signed-off-by: Daniel P. Berrangé --- ci/containers/libvirt-centos-7.Dockerfile| 6 +- ci/containers/libvirt-centos-8.Dockerfile| 6 +- ci/containers/libvirt-centos-stream.Dockerfile | 6 +- ci/containers/libvirt-debian-10-cross-aarch64.Dockerfile | 6 +- ci/containers/libvirt-debian-10-cross-armv6l.Dockerfile | 6 +- ci/containers/libvirt-debian-10-cross-armv7l.Dockerfile | 6 +- ci/containers/libvirt-debian-10-cross-i686.Dockerfile| 6 +- ci/containers/libvirt-debian-10-cross-mips.Dockerfile| 6 +- .../libvirt-debian-10-cross-mips64el.Dockerfile | 6 +- ci/containers/libvirt-debian-10-cross-mipsel.Dockerfile | 6 +- ci/containers/libvirt-debian-10-cross-ppc64le.Dockerfile | 6 +- ci/containers/libvirt-debian-10-cross-s390x.Dockerfile | 6 +- ci/containers/libvirt-debian-10.Dockerfile | 6 +- .../libvirt-debian-sid-cross-aarch64.Dockerfile | 9 + ci/containers/libvirt-debian-sid-cross-armv6l.Dockerfile | 9 + ci/containers/libvirt-debian-sid-cross-armv7l.Dockerfile | 9 + ci/containers/libvirt-debian-sid-cross-i686.Dockerfile | 9 + .../libvirt-debian-sid-cross-mips64el.Dockerfile | 9 + ci/containers/libvirt-debian-sid-cross-mipsel.Dockerfile | 9 + .../libvirt-debian-sid-cross-ppc64le.Dockerfile | 9 + ci/containers/libvirt-debian-sid-cross-s390x.Dockerfile | 9 + ci/containers/libvirt-debian-sid.Dockerfile | 9 + ci/containers/libvirt-fedora-32.Dockerfile | 9 + ci/containers/libvirt-fedora-33.Dockerfile | 9 + .../libvirt-fedora-rawhide-cross-mingw32.Dockerfile | 9 + .../libvirt-fedora-rawhide-cross-mingw64.Dockerfile | 9 + ci/containers/libvirt-fedora-rawhide.Dockerfile | 9 + ci/containers/libvirt-opensuse-151.Dockerfile| 6 +- ci/containers/libvirt-ubuntu-1804.Dockerfile | 6 +- ci/containers/libvirt-ubuntu-2004.Dockerfile | 6 +- 30 files changed, 30 insertions(+), 192 deletions(-) diff --git a/ci/containers/libvirt-centos-7.Dockerfile b/ci/containers/libvirt-centos-7.Dockerfile index ea460f76c9..d94f335c9d 100644 --- a/ci/containers/libvirt-centos-7.Dockerfile +++ b/ci/containers/libvirt-centos-7.Dockerfile @@ -36,7 +36,6 @@ WEiJKtQrZDJloqtyi/mmRa1VsV7RYR0VPJjhK/R8EQ7Ysshy\n\ audit-libs-devel \ augeas \ avahi-devel \ -bash \ bash-completion \ ca-certificates \ ccache \ @@ -44,6 +43,7 @@ WEiJKtQrZDJloqtyi/mmRa1VsV7RYR0VPJjhK/R8EQ7Ysshy\n\ cyrus-sasl-devel \ dbus-devel \ device-mapper-devel \ +diffutils \ dnsmasq \ dwarves \ ebtables \ @@ -89,9 +89,7 @@ WEiJKtQrZDJloqtyi/mmRa1VsV7RYR0VPJjhK/R8EQ7Ysshy\n\ numad \ parted \ parted-devel \ -patch \ perl \ -perl-App-cpanminus \ pkgconfig \ polkit \ python3 \ @@ -119,9 +117,7 @@ RUN pip3 install \ meson==0.54.0 ENV LANG "en_US.UTF-8" - ENV MAKE "/usr/bin/make" ENV NINJA "/usr/bin/ninja-build" ENV PYTHON "/usr/bin/python3" - ENV CCACHE_WRAPPERSDIR "/usr/libexec/ccache-wrappers" diff --git a/ci/containers/libvirt-centos-8.Dockerfile b/ci/containers/libvirt-centos-8.Dockerfile index 633a60dcd4..568e23af76 100644 --- a/ci/containers/libvirt-centos-8.Dockerfile +++ b/ci/containers/libvirt-centos-8.Dockerfile @@ -9,7 +9,6 @@ RUN dnf install 'dnf-command(config-manager)' -y && \ audit-libs-devel \ augeas \ avahi-devel \ -bash \ bash-completion \ ca-certificates \ ccache \ @@ -17,6 +16,7 @@ RUN dnf install 'dnf-command(config-manager)' -y && \ cyrus-sasl-devel \ dbus-devel \ device-mapper-devel \ +diffutils \ dnsmasq \ dwarves \ ebtables \ @@ -62,9 +62,7 @@ RUN dnf install 'dnf-command(config-manager)' -y && \ numad \ parted \ parted-devel \ -patch \ perl \ -perl-App-cpanminus \ pkgconfig \ polkit \ python3 \ @@ -94,9 +92,7 @@ RUN pip3 install \ meson==0.54.0 ENV LANG "en_US.UTF-8" - ENV MAKE "/usr/bin/make" ENV NINJA "/usr/bin/ninja" ENV PYTHON "/usr/bin/python3" - ENV CCACHE_WRAPPERSDIR "/usr/libexec/ccache-wrappers" diff --git a/ci/containers/libvirt-centos-stream.Dockerfile b/ci/containers/libvirt-centos-stream.Dockerfile index 8256e26944..f1ecab837a 100644 --- a/ci/containers/libvirt-centos-stream.Dockerfile +++ b/ci/containers/libvirt-centos-stream.Dockerfile @@ -10,7 +10,6 @@ RUN dnf install -y centos-release-stream && \ audit-libs-devel \ augeas \ avahi-devel \ -bash \ bash-completion \ ca-c
[libvirt PATCH 0/3] gitlab: refresh containers
This syncs with latest state of libvirt-ci.git
Daniel P. Berrang=C3=A9 (3):
gitlab: re-generate container images from lcitool
gitlab: refresh containers with lcitool for fully minimized base
ci: replace "libvirt-" prefix with "ci-" in dockerfiles
.gitlab-ci.yml| 22 +++
...os-7.Dockerfile =3D> ci-centos-7.Dockerfile} | 21 ++
...os-8.Dockerfile =3D> ci-centos-8.Dockerfile} | 22 ++-
...Dockerfile =3D> ci-centos-stream.Dockerfile} | 22 ++-
... =3D> ci-debian-10-cross-aarch64.Dockerfile} | 22 ++-
...e =3D> ci-debian-10-cross-armv6l.Dockerfile} | 22 ++-
...e =3D> ci-debian-10-cross-armv7l.Dockerfile} | 22 ++-
...ile =3D> ci-debian-10-cross-i686.Dockerfile} | 22 ++-
...ile =3D> ci-debian-10-cross-mips.Dockerfile} | 22 ++-
...=3D> ci-debian-10-cross-mips64el.Dockerfile} | 22 ++-
...e =3D> ci-debian-10-cross-mipsel.Dockerfile} | 22 ++-
... =3D> ci-debian-10-cross-ppc64le.Dockerfile} | 22 ++-
...le =3D> ci-debian-10-cross-s390x.Dockerfile} | 22 ++-
...-10.Dockerfile =3D> ci-debian-10.Dockerfile} | 22 ++-
...=3D> ci-debian-sid-cross-aarch64.Dockerfile} | 25 ++---
... =3D> ci-debian-sid-cross-armv6l.Dockerfile} | 25 ++---
... =3D> ci-debian-sid-cross-armv7l.Dockerfile} | 25 ++---
...le =3D> ci-debian-sid-cross-i686.Dockerfile} | 25 ++---
...> ci-debian-sid-cross-mips64el.Dockerfile} | 25 ++---
... =3D> ci-debian-sid-cross-mipsel.Dockerfile} | 25 ++---
...=3D> ci-debian-sid-cross-ppc64le.Dockerfile} | 25 ++---
...e =3D> ci-debian-sid-cross-s390x.Dockerfile} | 25 ++---
...id.Dockerfile =3D> ci-debian-sid.Dockerfile} | 25 ++---
...-32.Dockerfile =3D> ci-fedora-32.Dockerfile} | 24 ++--
...-31.Dockerfile =3D> ci-fedora-33.Dockerfile} | 28 ++-
...i-fedora-rawhide-cross-mingw32.Dockerfile} | 24 ++--
...i-fedora-rawhide-cross-mingw64.Dockerfile} | 24 ++--
...ockerfile =3D> ci-fedora-rawhide.Dockerfile} | 24 ++--
Dockerfile =3D> ci-opensuse-151.Dockerfile} | 23 ++-
...4.Dockerfile =3D> ci-ubuntu-1804.Dockerfile} | 22 ++-
...4.Dockerfile =3D> ci-ubuntu-2004.Dockerfile} | 22 ++-
ci/containers/refresh | 14 +-
32 files changed, 82 insertions(+), 655 deletions(-)
rename ci/containers/{libvirt-centos-7.Dockerfile =3D> ci-centos-7.Dockerfil=
e} (92%)
rename ci/containers/{libvirt-centos-8.Dockerfile =3D> ci-centos-8.Dockerfil=
e} (88%)
rename ci/containers/{libvirt-centos-stream.Dockerfile =3D> ci-centos-stream=
.Dockerfile} (88%)
rename ci/containers/{libvirt-debian-10-cross-aarch64.Dockerfile =3D> ci-deb=
ian-10-cross-aarch64.Dockerfile} (90%)
rename ci/containers/{libvirt-debian-10-cross-armv6l.Dockerfile =3D> ci-debi=
an-10-cross-armv6l.Dockerfile} (90%)
rename ci/containers/{libvirt-debian-10-cross-armv7l.Dockerfile =3D> ci-debi=
an-10-cross-armv7l.Dockerfile} (90%)
rename ci/containers/{libvirt-debian-10-cross-i686.Dockerfile =3D> ci-debian=
-10-cross-i686.Dockerfile} (90%)
rename ci/containers/{libvirt-debian-10-cross-mips.Dockerfile =3D> ci-debian=
-10-cross-mips.Dockerfile} (90%)
rename ci/containers/{libvirt-debian-10-cross-mips64el.Dockerfile =3D> ci-de=
bian-10-cross-mips64el.Dockerfile} (91%)
rename ci/containers/{libvirt-debian-10-cross-mipsel.Dockerfile =3D> ci-debi=
an-10-cross-mipsel.Dockerfile} (90%)
rename ci/containers/{libvirt-debian-10-cross-ppc64le.Dockerfile =3D> ci-deb=
ian-10-cross-ppc64le.Dockerfile} (90%)
rename ci/containers/{libvirt-debian-10-cross-s390x.Dockerfile =3D> ci-debia=
n-10-cross-s390x.Dockerfile} (90%)
rename ci/containers/{libvirt-debian-10.Dockerfile =3D> ci-debian-10.Dockerf=
ile} (87%)
rename ci/containers/{libvirt-debian-sid-cross-aarch64.Dockerfile =3D> ci-de=
bian-sid-cross-aarch64.Dockerfile} (88%)
rename ci/containers/{libvirt-debian-sid-cross-armv6l.Dockerfile =3D> ci-deb=
ian-sid-cross-armv6l.Dockerfile} (88%)
rename ci/containers/{libvirt-debian-sid-cross-armv7l.Dockerfile =3D> ci-deb=
ian-sid-cross-armv7l.Dockerfile} (88%)
rename ci/containers/{libvirt-debian-sid-cross-i686.Dockerfile =3D> ci-debia=
n-sid-cross-i686.Dockerfile} (88%)
rename ci/containers/{libvirt-debian-sid-cross-mips64el.Dockerfile =3D> ci-d=
ebian-sid-cross-mips64el.Dockerfile} (88%)
rename ci/containers/{libvirt-debian-sid-cross-mipsel.Dockerfile =3D> ci-deb=
ian-sid-cross-mipsel.Dockerfile} (88%)
rename ci/containers/{libvirt-debian-sid-cross-ppc64le.Dockerfile =3D> ci-de=
bian-sid-cross-ppc64le.Dockerfile} (88%)
rename ci/containers/{libvirt-debian-sid-cross-s390x.Dockerfile =3D> ci-debi=
an-sid-cross-s390x.Dockerfile} (88%)
rename ci/containers/{libvirt-debian-sid.Dockerfile =3D> ci-debian-sid.Docke=
rfile} (8
[libvirt PATCH 3/3] ci: replace "libvirt-" prefix with "ci-" in dockerfiles
This makes the dockerfile name match the output container name
Signed-off-by: Daniel P. Berrangé
---
.gitlab-ci.yml | 2 +-
...-centos-7.Dockerfile => ci-centos-7.Dockerfile} | 0
...-centos-8.Dockerfile => ci-centos-8.Dockerfile} | 0
...ream.Dockerfile => ci-centos-stream.Dockerfile} | 0
...rfile => ci-debian-10-cross-aarch64.Dockerfile} | 0
...erfile => ci-debian-10-cross-armv6l.Dockerfile} | 0
...erfile => ci-debian-10-cross-armv7l.Dockerfile} | 0
...ckerfile => ci-debian-10-cross-i686.Dockerfile} | 0
...ckerfile => ci-debian-10-cross-mips.Dockerfile} | 0
...file => ci-debian-10-cross-mips64el.Dockerfile} | 0
...erfile => ci-debian-10-cross-mipsel.Dockerfile} | 0
...rfile => ci-debian-10-cross-ppc64le.Dockerfile} | 0
...kerfile => ci-debian-10-cross-s390x.Dockerfile} | 0
...ebian-10.Dockerfile => ci-debian-10.Dockerfile} | 0
...file => ci-debian-sid-cross-aarch64.Dockerfile} | 0
...rfile => ci-debian-sid-cross-armv6l.Dockerfile} | 0
...rfile => ci-debian-sid-cross-armv7l.Dockerfile} | 0
...kerfile => ci-debian-sid-cross-i686.Dockerfile} | 0
...ile => ci-debian-sid-cross-mips64el.Dockerfile} | 0
...rfile => ci-debian-sid-cross-mipsel.Dockerfile} | 0
...file => ci-debian-sid-cross-ppc64le.Dockerfile} | 0
...erfile => ci-debian-sid-cross-s390x.Dockerfile} | 0
...ian-sid.Dockerfile => ci-debian-sid.Dockerfile} | 0
...edora-32.Dockerfile => ci-fedora-32.Dockerfile} | 0
...edora-33.Dockerfile => ci-fedora-33.Dockerfile} | 0
... => ci-fedora-rawhide-cross-mingw32.Dockerfile} | 0
... => ci-fedora-rawhide-cross-mingw64.Dockerfile} | 0
...ide.Dockerfile => ci-fedora-rawhide.Dockerfile} | 0
...e-151.Dockerfile => ci-opensuse-151.Dockerfile} | 0
...u-1804.Dockerfile => ci-ubuntu-1804.Dockerfile} | 0
...u-2004.Dockerfile => ci-ubuntu-2004.Dockerfile} | 0
ci/containers/refresh | 14 +++---
32 files changed, 8 insertions(+), 8 deletions(-)
rename ci/containers/{libvirt-centos-7.Dockerfile => ci-centos-7.Dockerfile}
(100%)
rename ci/containers/{libvirt-centos-8.Dockerfile => ci-centos-8.Dockerfile}
(100%)
rename ci/containers/{libvirt-centos-stream.Dockerfile =>
ci-centos-stream.Dockerfile} (100%)
rename ci/containers/{libvirt-debian-10-cross-aarch64.Dockerfile =>
ci-debian-10-cross-aarch64.Dockerfile} (100%)
rename ci/containers/{libvirt-debian-10-cross-armv6l.Dockerfile =>
ci-debian-10-cross-armv6l.Dockerfile} (100%)
rename ci/containers/{libvirt-debian-10-cross-armv7l.Dockerfile =>
ci-debian-10-cross-armv7l.Dockerfile} (100%)
rename ci/containers/{libvirt-debian-10-cross-i686.Dockerfile =>
ci-debian-10-cross-i686.Dockerfile} (100%)
rename ci/containers/{libvirt-debian-10-cross-mips.Dockerfile =>
ci-debian-10-cross-mips.Dockerfile} (100%)
rename ci/containers/{libvirt-debian-10-cross-mips64el.Dockerfile =>
ci-debian-10-cross-mips64el.Dockerfile} (100%)
rename ci/containers/{libvirt-debian-10-cross-mipsel.Dockerfile =>
ci-debian-10-cross-mipsel.Dockerfile} (100%)
rename ci/containers/{libvirt-debian-10-cross-ppc64le.Dockerfile =>
ci-debian-10-cross-ppc64le.Dockerfile} (100%)
rename ci/containers/{libvirt-debian-10-cross-s390x.Dockerfile =>
ci-debian-10-cross-s390x.Dockerfile} (100%)
rename ci/containers/{libvirt-debian-10.Dockerfile => ci-debian-10.Dockerfile}
(100%)
rename ci/containers/{libvirt-debian-sid-cross-aarch64.Dockerfile =>
ci-debian-sid-cross-aarch64.Dockerfile} (100%)
rename ci/containers/{libvirt-debian-sid-cross-armv6l.Dockerfile =>
ci-debian-sid-cross-armv6l.Dockerfile} (100%)
rename ci/containers/{libvirt-debian-sid-cross-armv7l.Dockerfile =>
ci-debian-sid-cross-armv7l.Dockerfile} (100%)
rename ci/containers/{libvirt-debian-sid-cross-i686.Dockerfile =>
ci-debian-sid-cross-i686.Dockerfile} (100%)
rename ci/containers/{libvirt-debian-sid-cross-mips64el.Dockerfile =>
ci-debian-sid-cross-mips64el.Dockerfile} (100%)
rename ci/containers/{libvirt-debian-sid-cross-mipsel.Dockerfile =>
ci-debian-sid-cross-mipsel.Dockerfile} (100%)
rename ci/containers/{libvirt-debian-sid-cross-ppc64le.Dockerfile =>
ci-debian-sid-cross-ppc64le.Dockerfile} (100%)
rename ci/containers/{libvirt-debian-sid-cross-s390x.Dockerfile =>
ci-debian-sid-cross-s390x.Dockerfile} (100%)
rename ci/containers/{libvirt-debian-sid.Dockerfile =>
ci-debian-sid.Dockerfile} (100%)
rename ci/containers/{libvirt-fedora-32.Dockerfile => ci-fedora-32.Dockerfile}
(100%)
rename ci/containers/{libvirt-fedora-33.Dockerfile => ci-fedora-33.Dockerfile}
(100%)
rename ci/containers/{libvirt-fedora-rawhide-cross-mingw32.Dockerfile =>
ci-fedora-rawhide-cross-mingw32.Dockerfile} (100%)
rename ci/containers/{libvirt-fedora-rawhide-cross-mingw64.Dockerfile =>
ci-fedora-rawhide-cross-mingw64.Dockerfile} (100%)
rename ci/containers/{libvirt-fedora-rawhide.Dockerfile =>
ci-fedora-rawhide.Dockerfile} (100%)
rename ci/containers/{libvirt-opensuse-151.Dockerfile =>
ci-opensuse-151.Do
Re: [PATCH] qemu: Recreate NUMA caps when creating connect capabilities
On 12/1/20 3:17 PM, Peter Krempa wrote: On Tue, Dec 01, 2020 at 13:54:29 +0100, Michal Privoznik wrote: In v6.0.0-rc1~439 (and friends) we tried to cache NUMA capabilities because we assumed they are immutable. And to some extent they are (NUMA hotplug is not a thing, is it). However, our capabilities contain also some runtime info that can change, e.g. hugepages pool allocation sizes or total amount of memory per node (host side memory hotplug might change the value). When rebuilding virCaps (e.g. for 'virsh capabilities') we have to dropped the cached info and rebuild it from scratch so that the correct runtime info is reported. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1819058 Fixes: 1a1d848694f6c2f1d98a371124928375bc3bb4a3 Signed-off-by: Michal Privoznik --- After this, we are still caching CPU caps, but I'm not sure if that is a problem. Perhaps if CPU was hotplugged/unplugged? I don't know. src/qemu/qemu_conf.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index d6615ca0dd..89a16349bf 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1380,6 +1380,12 @@ virCapsPtr virQEMUDriverCreateCapabilities(virQEMUDriverPtr driver) "DOI \"%s\"", model, doi); } +/* Forcibly recreate NUMA caps. They are not static + * (e.g. size of hugepages pools can change). */ +qemuDriverLock(driver); +g_clear_pointer(&driver->hostnuma, virCapabilitiesHostNUMAUnref); +qemuDriverUnlock(driver); On a second look, this looks really fishy here. virQEMUDriverCreateCapabilities is meant to convert 'driver' internals into a virCaps. It has no business modifying 'driver'. Except it already does that. Like you pointed out in the previous e-mail, the driver->hostnuma is lazily initialized on the first use. I can move this hunk into virQEMUDriverGetCapabilities() under refresh=true branch, if that's more acceptable. + caps->host.numa = virQEMUDriverGetHostNUMACaps(driver); If we always require new numa data, why don't we fetch it here instead of having the driver code generate it? Also if it needs to be refreshed on use, maybe caching it is not the best approach in the first place. We are using NUMA part when starting up a guest to construct a list of CPUs belonging to the nodes on which numad wants to place the guest. And I guess that part is static, so caching it makes sense. I see two ways out: 1) drop caching completely, 2) keep caching for guest startup sake, and in virQEMUDriverCreateCapabilities() don't overwrite driver->hostnuma, but call virCapabilitiesHostNUMANewHost() to create fresh NUMA caps. Michal
Re: [PATCH] target/ppc: Remove "compat" property of server class POWER CPUs
Just to clarify, this is for 6.0.
On Tue, 1 Dec 2020 14:11:03 +0100
Greg Kurz wrote:
> This property has been deprecated since QEMU 5.0 by commit 22062e54bb68.
> We only kept a legacy hack that internally converts "compat" into the
> official "max-cpu-compat" property of the pseries machine type.
>
> According to our deprecation policy, we could have removed it for QEMU 5.2
> already. Do it now ; since ppc_cpu_parse_featurestr() now just calls the
> generic parent_parse_features handler, drop it as well.
>
> Users are supposed to use the "max-cpu-compat" property of the pseries
> machine type instead.
>
> Signed-off-by: Greg Kurz
> ---
> docs/system/deprecated.rst | 7
> target/ppc/translate_init.c.inc | 59 -
> 2 files changed, 66 deletions(-)
>
> diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
> index 565389697e84..09c8f380bc82 100644
> --- a/docs/system/deprecated.rst
> +++ b/docs/system/deprecated.rst
> @@ -281,13 +281,6 @@ a future version of QEMU. It's unclear whether anybody
> is still using
> CPU emulation in QEMU, and there are no test images available to make
> sure that the code is still working.
>
> -``compat`` property of server class POWER CPUs (since 5.0)
> -''
> -
> -The ``compat`` property used to set backwards compatibility modes for
> -the processor has been deprecated. The ``max-cpu-compat`` property of
> -the ``pseries`` machine type should be used instead.
> -
> ``lm32`` CPUs (since 5.2.0)
> '''
>
> diff --git a/target/ppc/translate_init.c.inc b/target/ppc/translate_init.c.inc
> index 78cc8f043b92..e4082cfde746 100644
> --- a/target/ppc/translate_init.c.inc
> +++ b/target/ppc/translate_init.c.inc
> @@ -10470,63 +10470,6 @@ static ObjectClass *ppc_cpu_class_by_name(const char
> *name)
> return oc;
> }
>
> -static void ppc_cpu_parse_featurestr(const char *type, char *features,
> - Error **errp)
> -{
> -Object *machine = qdev_get_machine();
> -const PowerPCCPUClass *pcc =
> POWERPC_CPU_CLASS(object_class_by_name(type));
> -
> -if (!features) {
> -return;
> -}
> -
> -if (object_property_find(machine, "max-cpu-compat")) {
> -int i;
> -char **inpieces;
> -char *s = features;
> -Error *local_err = NULL;
> -char *compat_str = NULL;
> -
> -/*
> - * Backwards compatibility hack:
> - *
> - * CPUs had a "compat=" property which didn't make sense for
> - * anything except pseries. It was replaced by "max-cpu-compat"
> - * machine option. This supports old command lines like
> - * -cpu POWER8,compat=power7
> - * By stripping the compat option and applying it to the machine
> - * before passing it on to the cpu level parser.
> - */
> -inpieces = g_strsplit(features, ",", 0);
> -*s = '\0';
> -for (i = 0; inpieces[i]; i++) {
> -if (g_str_has_prefix(inpieces[i], "compat=")) {
> -warn_report_once("CPU 'compat' property is deprecated; "
> -"use max-cpu-compat machine property instead");
> -compat_str = inpieces[i];
> -continue;
> -}
> -if ((i != 0) && (s != features)) {
> -s = g_stpcpy(s, ",");
> -}
> -s = g_stpcpy(s, inpieces[i]);
> -}
> -
> -if (compat_str) {
> -char *v = compat_str + strlen("compat=");
> -object_property_set_str(machine, "max-cpu-compat", v,
> &local_err);
> -}
> -g_strfreev(inpieces);
> -if (local_err) {
> -error_propagate(errp, local_err);
> -return;
> -}
> -}
> -
> -/* do property processing with generic handler */
> -pcc->parent_parse_features(type, features, errp);
> -}
> -
> PowerPCCPUClass *ppc_cpu_get_family_class(PowerPCCPUClass *pcc)
> {
> ObjectClass *oc = OBJECT_CLASS(pcc);
> @@ -10905,8 +10848,6 @@ static void ppc_cpu_class_init(ObjectClass *oc, void
> *data)
> device_class_set_parent_reset(dc, ppc_cpu_reset, &pcc->parent_reset);
>
> cc->class_by_name = ppc_cpu_class_by_name;
> -pcc->parent_parse_features = cc->parse_features;
> -cc->parse_features = ppc_cpu_parse_featurestr;
> cc->has_work = ppc_cpu_has_work;
> cc->do_interrupt = ppc_cpu_do_interrupt;
> cc->cpu_exec_interrupt = ppc_cpu_exec_interrupt;
Re: [PATCH] qemu_conf: Fix double free problem for cfg->firmwares
On a Tuesday in 2020, Michal Privoznik wrote: On 12/1/20 2:50 AM, Tuguoyi wrote: -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? Oh yeah, sorry. I've pushed it now: Thank you, Jano https://gitlab.com/libvirt/libvirt/-/commit/c4f4e195a14c86b7daff2c45f1cbfd23ac16aaa8 Congratulations on your first libvirt contribution! Michal signature.asc Description: PGP signature
Re: [PATCH] target/ppc: Remove "compat" property of server class POWER CPUs
On a Tuesday in 2020, Greg Kurz wrote: This property has been deprecated since QEMU 5.0 by commit 22062e54bb68. We only kept a legacy hack that internally converts "compat" into the official "max-cpu-compat" property of the pseries machine type. According to our deprecation policy, we could have removed it for QEMU 5.2 already. Do it now ; since ppc_cpu_parse_featurestr() now just calls the generic parent_parse_features handler, drop it as well. Users are supposed to use the "max-cpu-compat" property of the pseries machine type instead. For libvirt: Reviewed-by: Ján Tomko We use the new option as of libvirt commit: commit 2b041dc8c7b70e762d99b6bd7805daa9961740f6 Author: Shivaprasad G Bhat AuthorDate: 2018-01-05 19:18:00 +0530 Commit: Andrea Bolognani CommitDate: 2018-01-05 17:12:14 +0100 qemu: Add support for pseries machine's max-cpu-compat= parameter Jano Signed-off-by: Greg Kurz --- docs/system/deprecated.rst | 7 target/ppc/translate_init.c.inc | 59 - 2 files changed, 66 deletions(-) signature.asc Description: PGP signature
Re: qemu no-shutdown feature in libvirt
On Mon, Nov 30, 2020 at 14:45:18 -0500, Luna Xu wrote: > Hi developers, Hi, [...] > When I start a VM with the qemu command I can specify the -no-shutdown > flag so that my qemu process doesn't quit even if I shutdown the VM > from the inside (issue shutdown or halt command inside VM). The VM is > shutdown but the qemu process is not killed so that the jobs I > submitted before for example backup (drive-backup) can continue to run > even after VM gets shutdown by the user, and I'm able to check the > status of the job through qmp commands (by communicating with the qemu > process). As I've noted in my reply on libvirt-users, we currently don't support this on the shutdown part of the VM lifecycle. On the startup part you can use VIR_DOMAIN_START_PAUSED for a VM which didn't yet excecute any guest instructions. > Is anyone aware of any project that is currently implementing this > functionality? Thanks for reading. I don't think that there's anybody working on it. If you are interested in implementing the feature the idea would be to add a 'pause' action for pause along with a new 'virDomainPausedReason' such as VIR_DOMAIN_PAUSED_POWEROFF and implement the lifecycle transition from PAUSED -> RUNNING via qemuDomainResume, which would in this case have to issue a 'system-reset' qmp command and emit the appropriate events.
Re: [PATCH] qemu: Recreate NUMA caps when creating connect capabilities
On Tue, Dec 01, 2020 at 13:54:29 +0100, Michal Privoznik wrote: > In v6.0.0-rc1~439 (and friends) we tried to cache NUMA > capabilities because we assumed they are immutable. And to some > extent they are (NUMA hotplug is not a thing, is it). However, > our capabilities contain also some runtime info that can change, > e.g. hugepages pool allocation sizes or total amount of memory > per node (host side memory hotplug might change the value). > > When rebuilding virCaps (e.g. for 'virsh capabilities') we have > to dropped the cached info and rebuild it from scratch so that > the correct runtime info is reported. > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1819058 > Fixes: 1a1d848694f6c2f1d98a371124928375bc3bb4a3 > Signed-off-by: Michal Privoznik > --- > > After this, we are still caching CPU caps, but I'm not sure if that is a > problem. Perhaps if CPU was hotplugged/unplugged? I don't know. > > src/qemu/qemu_conf.c | 6 ++ > 1 file changed, 6 insertions(+) > > diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c > index d6615ca0dd..89a16349bf 100644 > --- a/src/qemu/qemu_conf.c > +++ b/src/qemu/qemu_conf.c > @@ -1380,6 +1380,12 @@ virCapsPtr > virQEMUDriverCreateCapabilities(virQEMUDriverPtr driver) >"DOI \"%s\"", model, doi); > } > > +/* Forcibly recreate NUMA caps. They are not static > + * (e.g. size of hugepages pools can change). */ > +qemuDriverLock(driver); > +g_clear_pointer(&driver->hostnuma, virCapabilitiesHostNUMAUnref); > +qemuDriverUnlock(driver); On a second look, this looks really fishy here. virQEMUDriverCreateCapabilities is meant to convert 'driver' internals into a virCaps. It has no business modifying 'driver'. > + > caps->host.numa = virQEMUDriverGetHostNUMACaps(driver); If we always require new numa data, why don't we fetch it here instead of having the driver code generate it? Also if it needs to be refreshed on use, maybe caching it is not the best approach in the first place.
Re: [PATCH] qemu: Recreate NUMA caps when creating connect capabilities
On Tue, Dec 01, 2020 at 13:54:29 +0100, Michal Privoznik wrote: > In v6.0.0-rc1~439 (and friends) we tried to cache NUMA > capabilities because we assumed they are immutable. And to some > extent they are (NUMA hotplug is not a thing, is it). However, > our capabilities contain also some runtime info that can change, > e.g. hugepages pool allocation sizes or total amount of memory > per node (host side memory hotplug might change the value). > > When rebuilding virCaps (e.g. for 'virsh capabilities') we have > to dropped the cached info and rebuild it from scratch so that > the correct runtime info is reported. > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1819058 > Fixes: 1a1d848694f6c2f1d98a371124928375bc3bb4a3 > Signed-off-by: Michal Privoznik > --- > > After this, we are still caching CPU caps, but I'm not sure if that is a > problem. Perhaps if CPU was hotplugged/unplugged? I don't know. > > src/qemu/qemu_conf.c | 6 ++ > 1 file changed, 6 insertions(+) > > diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c > index d6615ca0dd..89a16349bf 100644 > --- a/src/qemu/qemu_conf.c > +++ b/src/qemu/qemu_conf.c > @@ -1380,6 +1380,12 @@ virCapsPtr > virQEMUDriverCreateCapabilities(virQEMUDriverPtr driver) >"DOI \"%s\"", model, doi); > } > > +/* Forcibly recreate NUMA caps. They are not static > + * (e.g. size of hugepages pools can change). */ > +qemuDriverLock(driver); > +g_clear_pointer(&driver->hostnuma, virCapabilitiesHostNUMAUnref); > +qemuDriverUnlock(driver); The documentation for 'driver->hostnuma' says: /* Lazy initialized on first use, immutable thereafter. * Require lock to get the pointer & do optional initialization */ virCapsHostNUMAPtr hostnuma; This patch invalidates that. A very very quick glance at the code suggests that it's okay, since the accessor returns a reference, so the docs must be adjusted.
Re: [PATCH v2] lxc: Cleanup after failed startup
On 11/11/20 1:57 PM, Michal Privoznik wrote: > Polite ping. Michal
Re: [PATCH] qemu_conf: Fix double free problem for cfg->firmwares
On 12/1/20 2:50 AM, Tuguoyi wrote: -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? Oh yeah, sorry. I've pushed it now: https://gitlab.com/libvirt/libvirt/-/commit/c4f4e195a14c86b7daff2c45f1cbfd23ac16aaa8 Congratulations on your first libvirt contribution! Michal
[PATCH] target/ppc: Remove "compat" property of server class POWER CPUs
This property has been deprecated since QEMU 5.0 by commit 22062e54bb68.
We only kept a legacy hack that internally converts "compat" into the
official "max-cpu-compat" property of the pseries machine type.
According to our deprecation policy, we could have removed it for QEMU 5.2
already. Do it now ; since ppc_cpu_parse_featurestr() now just calls the
generic parent_parse_features handler, drop it as well.
Users are supposed to use the "max-cpu-compat" property of the pseries
machine type instead.
Signed-off-by: Greg Kurz
---
docs/system/deprecated.rst | 7
target/ppc/translate_init.c.inc | 59 -
2 files changed, 66 deletions(-)
diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
index 565389697e84..09c8f380bc82 100644
--- a/docs/system/deprecated.rst
+++ b/docs/system/deprecated.rst
@@ -281,13 +281,6 @@ a future version of QEMU. It's unclear whether anybody is
still using
CPU emulation in QEMU, and there are no test images available to make
sure that the code is still working.
-``compat`` property of server class POWER CPUs (since 5.0)
-''
-
-The ``compat`` property used to set backwards compatibility modes for
-the processor has been deprecated. The ``max-cpu-compat`` property of
-the ``pseries`` machine type should be used instead.
-
``lm32`` CPUs (since 5.2.0)
'''
diff --git a/target/ppc/translate_init.c.inc b/target/ppc/translate_init.c.inc
index 78cc8f043b92..e4082cfde746 100644
--- a/target/ppc/translate_init.c.inc
+++ b/target/ppc/translate_init.c.inc
@@ -10470,63 +10470,6 @@ static ObjectClass *ppc_cpu_class_by_name(const char
*name)
return oc;
}
-static void ppc_cpu_parse_featurestr(const char *type, char *features,
- Error **errp)
-{
-Object *machine = qdev_get_machine();
-const PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(object_class_by_name(type));
-
-if (!features) {
-return;
-}
-
-if (object_property_find(machine, "max-cpu-compat")) {
-int i;
-char **inpieces;
-char *s = features;
-Error *local_err = NULL;
-char *compat_str = NULL;
-
-/*
- * Backwards compatibility hack:
- *
- * CPUs had a "compat=" property which didn't make sense for
- * anything except pseries. It was replaced by "max-cpu-compat"
- * machine option. This supports old command lines like
- * -cpu POWER8,compat=power7
- * By stripping the compat option and applying it to the machine
- * before passing it on to the cpu level parser.
- */
-inpieces = g_strsplit(features, ",", 0);
-*s = '\0';
-for (i = 0; inpieces[i]; i++) {
-if (g_str_has_prefix(inpieces[i], "compat=")) {
-warn_report_once("CPU 'compat' property is deprecated; "
-"use max-cpu-compat machine property instead");
-compat_str = inpieces[i];
-continue;
-}
-if ((i != 0) && (s != features)) {
-s = g_stpcpy(s, ",");
-}
-s = g_stpcpy(s, inpieces[i]);
-}
-
-if (compat_str) {
-char *v = compat_str + strlen("compat=");
-object_property_set_str(machine, "max-cpu-compat", v, &local_err);
-}
-g_strfreev(inpieces);
-if (local_err) {
-error_propagate(errp, local_err);
-return;
-}
-}
-
-/* do property processing with generic handler */
-pcc->parent_parse_features(type, features, errp);
-}
-
PowerPCCPUClass *ppc_cpu_get_family_class(PowerPCCPUClass *pcc)
{
ObjectClass *oc = OBJECT_CLASS(pcc);
@@ -10905,8 +10848,6 @@ static void ppc_cpu_class_init(ObjectClass *oc, void
*data)
device_class_set_parent_reset(dc, ppc_cpu_reset, &pcc->parent_reset);
cc->class_by_name = ppc_cpu_class_by_name;
-pcc->parent_parse_features = cc->parse_features;
-cc->parse_features = ppc_cpu_parse_featurestr;
cc->has_work = ppc_cpu_has_work;
cc->do_interrupt = ppc_cpu_do_interrupt;
cc->cpu_exec_interrupt = ppc_cpu_exec_interrupt;
--
2.26.2
[PATCH] qemu: Recreate NUMA caps when creating connect capabilities
In v6.0.0-rc1~439 (and friends) we tried to cache NUMA capabilities because we assumed they are immutable. And to some extent they are (NUMA hotplug is not a thing, is it). However, our capabilities contain also some runtime info that can change, e.g. hugepages pool allocation sizes or total amount of memory per node (host side memory hotplug might change the value). When rebuilding virCaps (e.g. for 'virsh capabilities') we have to dropped the cached info and rebuild it from scratch so that the correct runtime info is reported. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1819058 Fixes: 1a1d848694f6c2f1d98a371124928375bc3bb4a3 Signed-off-by: Michal Privoznik --- After this, we are still caching CPU caps, but I'm not sure if that is a problem. Perhaps if CPU was hotplugged/unplugged? I don't know. src/qemu/qemu_conf.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index d6615ca0dd..89a16349bf 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1380,6 +1380,12 @@ virCapsPtr virQEMUDriverCreateCapabilities(virQEMUDriverPtr driver) "DOI \"%s\"", model, doi); } +/* Forcibly recreate NUMA caps. They are not static + * (e.g. size of hugepages pools can change). */ +qemuDriverLock(driver); +g_clear_pointer(&driver->hostnuma, virCapabilitiesHostNUMAUnref); +qemuDriverUnlock(driver); + caps->host.numa = virQEMUDriverGetHostNUMACaps(driver); caps->host.cpu = virQEMUDriverGetHostCPU(driver); return g_steal_pointer(&caps); -- 2.26.2
[PATCH 3/3] qemu: Use virJSONValueObjectGetStringArray() more
In a few commit back (v6.10.0-5-gb3dad96972) a new helper for
obtaining string arrays from a virJSONObject was introduced:
virJSONValueObjectGetStringArray(). I've identified three places
where it can be used instead of open coding it:
qemuAgentSSHGetAuthorizedKeys(),
qemuMonitorJSONGetStringListProperty() and
qemuMonitorJSONGetCPUDefinitions().
Signed-off-by: Michal Privoznik
---
src/qemu/qemu_agent.c| 30 +++
src/qemu/qemu_monitor_json.c | 56 +---
2 files changed, 11 insertions(+), 75 deletions(-)
diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
index 1796ade6e2..d4530dcd7a 100644
--- a/src/qemu/qemu_agent.c
+++ b/src/qemu/qemu_agent.c
@@ -2533,9 +2533,6 @@ qemuAgentSSHGetAuthorizedKeys(qemuAgentPtr agent,
g_autoptr(virJSONValue) cmd = NULL;
g_autoptr(virJSONValue) reply = NULL;
virJSONValuePtr data = NULL;
-size_t ndata;
-size_t i;
-char **keys_ret = NULL;
if (!(cmd = qemuAgentMakeCommand("guest-ssh-get-authorized-keys",
"s:username", user,
@@ -2545,35 +2542,16 @@ qemuAgentSSHGetAuthorizedKeys(qemuAgentPtr agent,
if (qemuAgentCommand(agent, cmd, &reply, agent->timeout) < 0)
return -1;
-if (!(data = virJSONValueObjectGetObject(reply, "return")) ||
-!(data = virJSONValueObjectGetArray(data, "keys"))) {
+if (!(data = virJSONValueObjectGetObject(reply, "return"))) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("qemu agent didn't return an array of keys"));
return -1;
}
-ndata = virJSONValueArraySize(data);
+if (!(*keys = virJSONValueObjectGetStringArray(data, "keys")))
+return -1;
-keys_ret = g_new0(char *, ndata + 1);
-
-for (i = 0; i < ndata; i++) {
-virJSONValuePtr entry = virJSONValueArrayGet(data, i);
-
-if (!entry) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("array element missing in
guest-ssh-get-authorized-keys return value"));
-goto error;
-}
-
-keys_ret[i] = g_strdup(virJSONValueGetString(entry));
-}
-
-*keys = g_steal_pointer(&keys_ret);
-return ndata;
-
- error:
-virStringListFreeCount(keys_ret, ndata);
-return -1;
+return g_strv_length(*keys);
}
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index e7aa6b6ffd..ea65d6bee7 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -5931,41 +5931,17 @@ qemuMonitorJSONGetCPUDefinitions(qemuMonitorPtr mon,
cpu->type = g_strdup(tmp);
if (virJSONValueObjectHasKey(child, "unavailable-features")) {
-virJSONValuePtr blockers;
-size_t j;
-size_t len;
-
-blockers = virJSONValueObjectGetArray(child,
- "unavailable-features");
-if (!blockers) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("unavailable-features in
query-cpu-definitions "
- "reply data was not an array"));
+if (!(cpu->blockers = virJSONValueObjectGetStringArray(child,
+
"unavailable-features")))
return -1;
-}
-len = virJSONValueArraySize(blockers);
-
-if (len == 0) {
+if (g_strv_length(cpu->blockers) == 0) {
cpu->usable = VIR_DOMCAPS_CPU_USABLE_YES;
+g_clear_pointer(&cpu->blockers, g_strfreev);
continue;
}
cpu->usable = VIR_DOMCAPS_CPU_USABLE_NO;
-cpu->blockers = g_new0(char *, len + 1);
-
-for (j = 0; j < len; j++) {
-virJSONValuePtr blocker = virJSONValueArrayGet(blockers, j);
-
-if (virJSONValueGetType(blocker) != VIR_JSON_TYPE_STRING) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("unexpected value in unavailable-features
"
- "array"));
-return -1;
-}
-
-cpu->blockers[j] = g_strdup(virJSONValueGetString(blocker));
-}
}
}
@@ -6788,9 +6764,6 @@ qemuMonitorJSONGetStringListProperty(qemuMonitorPtr mon,
g_autoptr(virJSONValue) cmd = NULL;
g_autoptr(virJSONValue) reply = NULL;
VIR_AUTOSTRINGLIST list = NULL;
-virJSONValuePtr data;
-size_t n;
-size_t i;
*strList = NULL;
@@ -6806,25 +6779,10 @@ qemuMonitorJSONGetStringListProperty(qemuMonitorPtr mon,
if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_ARRAY) < 0)
return -1;
-data = virJSONValueObjectGetArray(reply, "return");
-n = virJSONValueArraySize(data);
+if (!(*strList = virJSONValueObjectGetStringArray(r
[PATCH 2/3] virJSONValueObjectGetStringArray: Report error if @key is not an array
The virJSONValueObjectGetStringArray() function is given a @key
which is supposed to be an array inside given @object. Well, if
it's not then an error state is returned (NULL), but no error
message is set.
Signed-off-by: Michal Privoznik
---
src/util/virjson.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/src/util/virjson.c b/src/util/virjson.c
index d471923732..160f6172d2 100644
--- a/src/util/virjson.c
+++ b/src/util/virjson.c
@@ -1472,8 +1472,12 @@ virJSONValueObjectGetStringArray(virJSONValuePtr object,
const char *key)
size_t i;
data = virJSONValueObjectGetArray(object, key);
-if (!data)
+if (!data) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("%s is missing not an array"),
+ key);
return NULL;
+}
n = virJSONValueArraySize(data);
ret = g_new0(char *, n + 1);
--
2.26.2
[PATCH 1/3] qemuDomainGetGuestInfo: Exit early if getting info fails
If there is an error getting info from guest agent, then the
control on qemuDomainGetGuestInfo() jumps onto 'exitagent' label
and subsequently continues on 'endagentjob'. Both labels are hit
also in success case, which is why there is a code that tries to
match info obtained from the guest agent with domain definition.
However, if we know that we've reached this area because of an
error (ret = -1) there is no reason for us to attempt finding the
match as the API as whole will end up with an error.
Signed-off-by: Michal Privoznik
---
src/qemu/qemu_driver.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 2d4b5a8b99..338c609854 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -20129,6 +20129,9 @@ qemuDomainGetGuestInfo(virDomainPtr dom,
endagentjob:
qemuDomainObjEndAgentJob(vm);
+if (ret < 0)
+goto cleanup;
+
if (nfs > 0 || ndisks > 0) {
if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0)
goto cleanup;
--
2.26.2
[PATCH 0/3] qemu: Couple of qemuDomainGetGuestInfo() related cleanups
While reviewing Marc-André's patches I've noticed couple of cleanup possible. Michal Prívozník (3): qemuDomainGetGuestInfo: Exit early if getting info fails virJSONValueObjectGetStringArray: Report error if @key is not an array qemu: Use virJSONValueObjectGetStringArray() more src/qemu/qemu_agent.c| 30 +++ src/qemu/qemu_driver.c | 3 ++ src/qemu/qemu_monitor_json.c | 56 +--- src/util/virjson.c | 6 +++- 4 files changed, 19 insertions(+), 76 deletions(-) -- 2.26.2
Re: [libvirt PATCH 0/9] qemu: report guest disks informations
On 12/1/20 8:07 AM, Marc-André Lureau wrote: Hi Tentative: Reviewed-by: Michal Privoznik This is now pushed. Michal
Re: [PATCH v2 0/6] add post parse pSeries mem align, when ABI_UPDATE
Ping On 11/18/20 4:58 PM, Daniel Henrique Barboza wrote: Hi, This is a follow up of [1] after really comprehending what I was messing with and what I couldn't do. This version has a shorter scope, only pSeries guests are being taken care of. I can send a follow up to handle x86 as well depending on the popularity of this work. Patches 1 and 2 moves the existing qemuDomainAlignMemorySizes() from qemu_command.c to qemuProcessPrepareDomain(). They are not related/tied to everything else done here and can be pushed independently if needed. Patches 3, 4 and 5 are reworking the existing code to be consistent to our prerrogative of not aligning memory when migrating or 'snapshotting'. No significant behavioral change is done. Patch 6 is where the bacon is. For new ppc64 guests, without ABI breakage, the mem alignment that are overshooting the intended initial memory is fixed. Test cases were added to help me diagnose and assert what I was changing and what would remain untouched. [1] https://www.redhat.com/archives/libvir-list/2020-November/msg00544.html Daniel Henrique Barboza (6): qemu_process.c: check migrateURI when setting VIR_QEMU_PROCESS_START_NEW qemu: move memory size align to qemuProcessPrepareDomain() Revert "domain_conf.c: auto-align pSeries NVDIMM in virDomainMemoryDefPostParse()" qemuxml2xmltest.c: honor ARG_PARSEFLAGS qemu_domain.c: post parse pSeries NVDIMM align with PARSE_ABI_UPDATE qemu_domain.c: align all pSeries mem modules when PARSE_ABI_UPDATE src/conf/domain_conf.c| 23 + src/qemu/qemu_command.c | 3 -- src/qemu/qemu_domain.c| 39 ++- src/qemu/qemu_process.c | 11 +++- ...memory-hotplug-nvdimm-ppc64-abi-update.xml | 50 +++ ...emory-hotplug-ppc64-nonuma-abi-update.args | 34 + ...memory-hotplug-ppc64-nonuma-abi-update.xml | 32 tests/qemuxml2argvtest.c | 7 +++ ...memory-hotplug-nvdimm-ppc64-abi-update.xml | 50 +++ .../memory-hotplug-nvdimm-ppc64.xml | 2 +- ...memory-hotplug-ppc64-nonuma-abi-update.xml | 45 + tests/qemuxml2xmltest.c | 20 +++- 12 files changed, 286 insertions(+), 30 deletions(-) create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64-abi-update.xml create mode 100644 tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma-abi-update.args create mode 100644 tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma-abi-update.xml create mode 100644 tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-ppc64-abi-update.xml create mode 100644 tests/qemuxml2xmloutdata/memory-hotplug-ppc64-nonuma-abi-update.xml
Re: [PATCH] polkit: Allow libvirt group access to libvirtd ro socket
On Tue, Dec 01, 2020 at 09:17:01AM +, Daniel P. Berrangé wrote: > On Mon, Nov 30, 2020 at 05:28:16PM -0700, 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. > > This doesn't make sense - the readonly case should never prompt for > a password, since libvirtd.polkit.in grants that permission out of > the box. The libvirtd.rules file should just be extending what is > defined in the main libvirtd.polkit file. In fact, this caught my eye and it works as expected on Fedora, can you share a bit more on what setup this fails for you? Erik
Re: [PATCH v2 1/2] gitlab: Add issue template for reporting a generic bug
On Tue, Dec 01, 2020 at 09:19:20 +, Daniel Berrange wrote: > On Tue, Dec 01, 2020 at 06:52:16AM +0100, Peter Krempa wrote: > > 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 > > I wonder if we need this - I've always found the expected/actual > behaviour sections in bugzilla's template redundant, as the description > should already cover it. Hmm, yeah. In many cases it's just "$THING is happening" in description and expected behaviour: "$THING should not happen" > Regardless of answer, though it can have I'll drop it. Bugzilla had it maybe to cover the "Feature request" use case where the "current behaviour" field is redundant.
Re: [PATCH v2 2/2] gitlab: Add issue template for a feature request
On Tue, Dec 01, 2020 at 06:52:17AM +0100, Peter Krempa wrote: > 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 Reviewed-by: Daniel P. Berrangé 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 v2 1/2] gitlab: Add issue template for reporting a generic bug
On Tue, Dec 01, 2020 at 06:52:16AM +0100, Peter Krempa wrote: > 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 I wonder if we need this - I've always found the expected/actual behaviour sections in bugzilla's template redundant, as the description should already cover it. Regardless of answer, though it can have Reviewed-by: Daniel P. Berrangé > + > +## Additional information > + > + > + > + > + > + > +/label ~bug > -- > 2.28.0 > 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] polkit: Allow libvirt group access to libvirtd ro socket
On Mon, Nov 30, 2020 at 05:28:16PM -0700, 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.
This doesn't make sense - the readonly case should never prompt for
a password, since libvirtd.polkit.in grants that permission out of
the box. The libvirtd.rules file should just be extending what is
defined in the main libvirtd.polkit file.
>
> 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
>
>
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 :|
qemu no-shutdown feature in libvirt
Hi developers, Thanks for developing and maintaining such a great tool. I'm new and I'm trying to use the no-shutdown feature of qemu with libvirt. I've confirmed with the user list that currently there's no such feature in libvirt that supports what I want to do, so I'm posting here and see if anyone is aware of any existing project that can achieve this or anything under development to avoid re-implementing the wheel. Here is what I'm doing on qemu: When I start a VM with the qemu command I can specify the -no-shutdown flag so that my qemu process doesn't quit even if I shutdown the VM from the inside (issue shutdown or halt command inside VM). The VM is shutdown but the qemu process is not killed so that the jobs I submitted before for example backup (drive-backup) can continue to run even after VM gets shutdown by the user, and I'm able to check the status of the job through qmp commands (by communicating with the qemu process). Is anyone aware of any project that is currently implementing this functionality? Thanks for reading. Thanks, Luna
Release of libvirt-6.10.0
The 6.10.0 release of both libvirt and libvirt-python is tagged and signed tarballs and source RPMs are available at https://libvirt.org/sources/ https://libvirt.org/sources/python/ Thanks everybody who helped with this release by sending patches, reviewing, testing, or providing any other feedback. Your work is greatly appreciated. * Security * qemu: Enable client TLS certificate validation by default for ``chardev``, ``migration``, and ``backup`` servers. The default value if qemu.conf options ``chardev_tls_x509_verify``, ``migrate_tls_x509_verify``, or ``backup_tls_x509_verify`` are not specified explicitly in the config file and also the ``default_tls_x509_verify`` config option is missing are now '1'. This ensures that only legitimate clients access servers, which don't have any additional form of authentication. * New features * qemu: Implement OpenSSH authorized key file management APIs New APIs (``virDomainAuthorizedSSHKeysGet()`` and ``virDomainAuthorizedSSHKeysSet()``) and virsh commands (``get-user-sshkeys`` and ``set-user-sshkeys``) are added to manage authorized_keys SSH file for user. * hyperv: implement new APIs The ``virDomainGetMaxMemory()``, ``virDomainSetMaxMemory()``, ``virDomainGetSchedulerType()``, ``virDomainGetSchedulerParameters()``, ``virDomainGetSchedulerParametersFlags()``, ``virDomainGetVcpus()``, ``virDomainGetVcpusFlags()``, ``virDomainGetMaxVcpus()``, ``virDomainSetVcpus()``, and ``virDomainSetVcpusFlags()`` APIs have been implemented in the Hyper-V driver. * Improvements * virsh: Support network disks in ``virsh attach-disk`` The ``virsh attach-disk`` helper command which simplifies attaching of disks without the need for the user to formulate the disk XML manually now supports network-backed images. Users can specify the protocol and host specification with new command line arguments. Please refer to the man page of virsh for further information. * Bug fixes * remote: fixed performance regression in SSH tunnelling The ``virt-ssh-helper`` binary introduced in 6.8.0 had very poor scalability which impacted libvirt tunnelled migration and storage volume upload/download in particular. It has been updated and now has performance on par with netcat. * Removed features * hyperv: removed support for the Hyper-V V1 WMI API This drops support for Windows Server 2008R2 and 2012. The earliest supported version is now Windows 2012R2. Enjoy. Jirka
Re: [libvirt PATCH 0/9] qemu: report guest disks informations
On Sat, Nov 21, 2020 at 2:11 AM 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(-) > > -- > 2.29.0 > > > Works for me on qemu-5.2.0-0.7.rc2.fc34.x86_64: For the guest with qemu-guest-agent-5.2.0-0.7.rc2.fc34.x86_64: ➜ ~ virsh guestinfo pc --disks disks.count : 2 disks.0.name: /dev/vda1 disks.0.partition : yes disks.0.dependencies.count: 1 disks.0.dependencies.0.name: /dev/vda disks.1.name: /dev/vda disks.1.partition : no disks.1.alias : vda For the guest with qemu-guest-agent-4.1.1-1.fc31.x86_64: ➜ ~ virsh guestinfo new --disks error: internal error: unable to execute QEMU agent command 'guest-get-disks': The command guest-get-disks has not been found -- Tested-by: Han Han
Re: [PATCH 00/18] qapi/qom: QAPIfy object-add
Paolo Bonzini writes:
> 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.
It should be, because it's what we've done elsewhere, isn't it?
Yes, we can extend QAPI to let us embed opaques outside the QAPI
schema's scope ("state"), along with means to create, destroy, and
clone. This is new.
But we can also invert: put the QAPI-generated C type ("config") in a
handwritten C type that marries it to "state".
Code to create and destroy is handwritten, and uses QAPI-generated code
for the "config" part.
A generic interface to handwritten creation is possible: Take the
QAPI-generated "config" type as argument, extract enough information to
call the appropriate constructor, return its value.
Generic destruction is also possible: all it needs is a map from
instance to destructor function.
None of this is new in QEMU.
