Re: RFC: deprecating/obsoleting netcf package and libvirt virInterface*() APIs
On 12/2/20 8:47 PM, Laine Stump wrote: So, that's my piece to speak. I'm looking for opinions and ideas on a few different fronts: 1) Does this generally sound like a good direction? Or is there something I'm ignoring that renders my points moot? Yes, it sounds good. A few years back I wanted to adapt netcf for my distro, but never gotten around it, since there was no real consumer, only my motivation to build libvirt with everything possible. 2) If we are going to do it, how should we proceed? We obviously can't simply *remove* the virInterface API from libvirt (since that would destroy backward compatibility guarantees), but could immediately begin logging some sort of "this API is deprecated" message when any of the functions are called, and then in a later release change the APIs to return an error (while simultaneously removing netcf from the build and dependency lists). At the same time, we would need to decide if the "interface status" functionality needs to be maintained within appropriate virInterface*() APIs, reproduced in virNodeDeviceGetXMLDesc(), or just dropped altogether. What we usually do is we remove the driver implementation and let public API report unsupported error (since driver->XXX callback is going to be NULL), for instance: 464a41bc0de. Before that, we reported a deprecated error (6f532d7ffc3 and 5cc402a9b4c). This was paired with documentation: a8073797ce174f6eaa622104e4fd33ee88cb6fad Michal
Re: [PATCH libvirt v3 01/11] nodedev: detect AP card device
On Wed, Dec 02, 2020 at 05:04:29PM +0100, Shalini Chellathurai Saroja wrote: > Hello Erik, > > I will make changes based on your comments in here and patch 03. Thank you > very much for the review. I also responded to one of the patches in the v2 series, so it'd be cool if you could respond there as well. Erik
[PATCH 2/2] security: Avoid calling virSecurityManagerCheckModel with NULL model
Attempting to create a domain with results in
virsh --connect lxc:/// create distro_nosec.xml
error: Failed to create domain from distro_nosec.xml
error: unsupported configuration: Security driver model '(null)' is not
available
With , the model field of virSecurityLabelDef will
be NULL, causing virSecurityManagerCheckModel() to fail with the above
error. Avoid calling virSecurityManagerCheckModel() when they seclabel
type is VIR_DOMAIN_SECLABEL_NONE.
Signed-off-by: Jim Fehlig
---
This could also be fixed by checking for a NULL secmodel in
virSecurityManagerCheckModel, but it seems more appropriate to check for
a valid seclabel type before checking the model.
src/security/security_manager.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/security/security_manager.c b/src/security/security_manager.c
index be81ee5e44..789e24d273 100644
--- a/src/security/security_manager.c
+++ b/src/security/security_manager.c
@@ -781,6 +781,9 @@ virSecurityManagerCheckDomainLabel(virSecurityManagerPtr
mgr,
size_t i;
for (i = 0; i < def->nseclabels; i++) {
+if (def->seclabels[i]->type == VIR_DOMAIN_SECLABEL_NONE)
+continue;
+
if (virSecurityManagerCheckModel(mgr, def->seclabels[i]->model) < 0)
return -1;
}
--
2.29.2
[PATCH 0/2] security: A couple of fixes
I finally got around to investigating some apparmor-related test failures. The first patch is apparmor specific. The second patch fixes a bug that should affect all security drivers. Jim Fehlig (2): apparmor: Allow lxc processes to receive signals from libvirt security: Avoid calling virSecurityManagerCheckModel with NULL model src/security/apparmor/libvirt-lxc | 4 src/security/security_manager.c | 3 +++ 2 files changed, 7 insertions(+) -- 2.29.2
[PATCH 1/2] apparmor: Allow lxc processes to receive signals from libvirt
LXC processes confined by apparmor are not permitted to receive signals from libvirtd. Attempting to destroy such a process fails virsh --connect lxc:/// destroy distro_apparmor error: Failed to destroy domain distro_apparmor error: Failed to kill process 29491: Permission denied And from /var/log/audit/audit.log type=AVC msg=audit(1606949706.142:6345): apparmor="DENIED" operation="signal" profile="libvirt-314b7109-fdce-48dc-ad28-7c47958a27c1" pid=29390 comm="libvirtd" requested_mask="receive" denied_mask="receive" signal=term peer="libvirtd" Similar to the libvirt-qemu abstraction, add a rule to the libvirt-lxc abstraction allowing reception of signals from libvirtd. Signed-off-by: Jim Fehlig --- src/security/apparmor/libvirt-lxc | 4 1 file changed, 4 insertions(+) diff --git a/src/security/apparmor/libvirt-lxc b/src/security/apparmor/libvirt-lxc index e556f2a7bd..0c8b812743 100644 --- a/src/security/apparmor/libvirt-lxc +++ b/src/security/apparmor/libvirt-lxc @@ -1,5 +1,9 @@ #include + # Allow receiving signals from libvirtd + signal (receive) peer=libvirtd, + signal (receive) peer=/usr/sbin/libvirtd, + umount, # ignore DENIED message on / remount -- 2.29.2
Re: [PATCH 00/18] qapi/qom: QAPIfy object-add
Hi,
> It would be much nicer to do the wrapper the other way round, i.e.
> setting properties before the device is realized would update a
> configuration struct and realize would then call .create() with that
> struct. To me, this sounds much harder, though also a more useful state.
Well, in some places we already have separate config structs. We have
NICConf for example, which is typically used like this:
struct USBNetState {
USBDevice dev;
[ ... ]
NICConf conf;
[ ... ]
};
and
static Property net_properties[] = {
DEFINE_NIC_PROPERTIES(USBNetState, conf),
DEFINE_PROP_END_OF_LIST(),
};
So I think we could:
(1) move *all* properties into structs.
(2) generate those structs from qapi schemas.
(3) generate Property lists (or functions with
object_class_property_add_*() calls) from qapi
schema.
We could then convert devices one-by-one without breaking anything
or needing two code paths essentially doing the same thing in two
different ways.
take care,
Gerd
RE: [PATCH] qemu_conf: Fix double free problem for cfg->firmwares
> -Original Message- > From: Michal Privoznik [mailto:mpriv...@redhat.com] > Sent: Tuesday, December 01, 2020 9:28 PM > To: tuguoyi (Cloud) ; Ján Tomko > Cc: libvir-list@redhat.com > Subject: 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/c4f4e195a14c86b7daff2c45f1cbfd2 > 3ac16aaa8 > > Congratulations on your first libvirt contribution! > > Michal Thanks a lot
Re: [PATCH 00/18] qapi/qom: QAPIfy object-add
On Wed, Dec 02, 2020 at 06:35:06PM +0100, Kevin Wolf wrote: > Am 02.12.2020 um 17:05 hat Eduardo Habkost geschrieben: > > > > Looks nice as end goal. Then, these are a few questions I would > > > > have about the transition plan: > > > > > > > > Would it require changing both device implementation and device > > > > users in lockstep? Should we have a compatibility layer to allow > > > > existing qdev_new()+qdev_prop_set_*() code to keep working after > > > > the devices are converted to the new system? If not, why not? > > > > > > Technically, it doesn't strictly require a lockstep update. You can have > > > two code paths leading to a fully initialised device, one being the > > > traditional way of setting properties and finally calling dc->realize, > > > the other being a new method that takes the configuration in its > > > parameters and also sets dev->realized = true at the end. > > > > > > If at all possible, I would however prefer a lockstep update because > > > having two paths is a weird intermediate state and the code paths could > > > easily diverge. Keeping the old way around for a device also means that > > > property setters are still doing two different jobs (initial > > > configuration and updates at runtime). > > > > I'd like to understand better how that intermediate state would > > look like and why there's risk of separate code paths diverging. > > > > Could we have an intermediate state that doesn't require any > > duplication and thus have no separate code paths that could > > diverge? > > The one requirement we have for an intermediate state is that it > supports both interfaces: The well-know create/set properties/realize > dance, and a new DeviceClass method, say .create(), that takes the > configuration in parameters instead of relying on previously set > properties. I agree completely. > > I assumed two separate implementations of transferring the configuration > into the internal state. On second thought, this assumption is maybe > wrong. > > You can implement the new method as wrapper around the old way: It could > just set all the properties and call realize. Of course, you don't win > much in terms of improving the class implementation this way, but just > support the new interface, but I guess it can be a reasonable > intermediate step to resolve complicated dependencies etc. > > It would be much nicer to do the wrapper the other way round, i.e. > setting properties before the device is realized would update a > configuration struct and realize would then call .create() with that > struct. To me, this sounds much harder, though also a more useful state. Comment about this below (look for [1]). > > As you have worked a lot with properties recently, maybe you have a good > idea how we could get an intermediate state closer to this? I'd have to re-read this whole thread and think about it. > > > > > If we add a compatibility layer, is the end goal to convert all > > > > existing qdev_new() users to the new system? If yes, why? If > > > > not, why not? > > > > > > My personal goal is covering -object and -device, i.e. the external > > > interfaces. Converting purely internally created devices is not as > > > interesting (especially as long as we focus only on object creation), > > > but might be desirable for consistency. > > > > I wonder how much consistency we will lose and how much confusion > > we'll cause if we end up with two completely separate methods for > > creating devices. > > I do think we should follow through and convert everything. It's just > not my main motivation, and if the people who work more with qdev think > it's better to leave that part unchanged (or that it won't make much of > a difference), I won't insist. This worries me. Converting thousands of lines of code that don't involve user-visible interfaces seems complicated and maybe pointless. On the other hand, having two separate APIs for creating objects internally would cause confusion. Maybe we should accept the fact that the 2 APIs will exist, and address the confusion part: we should guarantee the two APIs to be 100% equivalent, except for the fact that the newer one gives us type safety in the C code. I'd like to avoid a case like qdev vs QOM APIs, where they have similar but slightly different features, and nobody knows which one to use. > > > > > What about subclasses? Would base classes need to be converted > > > > in lockstep with all subclasses? How would the transition > > > > process of (e.g.) PCI devices look like? > > > > > > I don't think so. > > > > > > If you want to convert base classes first, you may need to take the > > > path shown above where both initialisation paths coexist while the > > > children are converted because instantiation of a child class involves > > > setting properties of the base class. So you can only restrict these > > > properties to runtime-only after all children have been converted. > > > > > > The other way around
RFC: deprecating/obsoleting netcf package and libvirt virInterface*() APIs
netcf (the backend of libvirt's virInterface*() APIs) hasn't been modified in over 2 years, and the last time there was a change significant enough for an upstream release was in 2015 (!). It has never been possible to reliably translate back and forth between native and netcf/libvirt XML config for interfaces without losing some information, and impossible to keep up with new functionality being added to host network configuration in NetworkManager (especially since the modelling was slightly different - netcf is based on the idea of each physical interface having one configuration, while NetworkManager has potentially several different "Connections" for any given hardware interface, and at most one of these connections can be active at a time for the interface. Or something like that.) The libvirt virInterface*() API (and netcf behind it) originally arose out of a request from the oVirt project that we (libvirt) provide a way to provision the networking config on a compute node (I *think* this is the case - I started working on libvirt when they were in the middle of these discussions; netcf was originally implemented mostly by David Lutterkort, and then handed off to me when he moved on to other pastures). oVirt network provisioning usually meant adding a bridge, assigning it an IP address, and attaching an ethernet (or two via a bond) to that bridge. They wanted libvirt to provide this functionality because (I guess?) they wanted to have a single connection to the node that could perform all the setup they needed. Although netcf could do that, in the end it didn't provide exactly what they needed, so they "rolled their own" host network interface config and didn't use netcf. In the meantime, the idea of using a virtualization API to configure host network interfaces never took off (Who'da thunk?). netcf was designed so that a single C API + XML frontend could be compiled with multiple different backends, for different network configuration paradigms. A few other drivers were (partially) written (e.g. SuSE and Windows), but in the real world, the only backends that have been used have been the one that uses ifcfg files in Fedora/RHEL/CentOS, and the one that uses the /etc/network/interfaces file on debian/ubuntu. In both cases, the backend understands a subset of what is possible in those files, so some information from the config doesn't show up in the XML, and thus won't be preserved if netcf is used to modify (i.e. redefine) the interface. In addition, since a functionally identical configuration can be represented in multiple ways in both those formats, sometimes a config file is deemed unreadable by netcf, or it is read and interpreted correctly, but the modified config is written back using the "other" method. Since ovirt chose not to use it, the only users of its interface configuration capabilities that I've ever noticed in the wild were the occasional user wanting to use the "virsh iface-bridge" command to put a bridge behind their ethernet - certainly none of the higher level virtualization management applications (that I'm aware of) use any of the libvirt APIs that call to netcf (that means "anything starting with "virInterface"). The small part of the virInterface APIs that have been used with some regularity are just the functions that list current interfaces and get their current live status (i.e. based on sending/receiving netlink messages, not on the contents of the host network config files). Even on that front, the one commonly used example that I know know of is virt-manager, which had used virInterface*() to get a list of bridges/ethernets available for guest network connections, but that functionality was removed from virt-manager-3.0, which was released in September of this year. In spite of this sporadic use, there are occasional netcf BZes that get filed complaining about certain device options missing, or erroneous config resulting in a confusing error message. These are usually filed by Red Hat virt QE, simply because it's a part of their test plan (i.e. these reports generally don't reflect a failure on a production system). Because the "fix" wouldn't provide any gain in the real world, these BZes just sit in the queue and server only to make me (the netcf maintainer) feel like even more of a procrastinator that I actually am. (A sidebar: netcf was originally made into a separate library, rather than just a few files within libvirt itself, because there was at least shrugging verbal agreement that it would be used in places other than libvirt (and thus there would be a community benefit in eliminated duplicate code in the multiple projects); this also never materialized, so in the end, it is a separate library that is only consumed by libvirt, but because it's a separate library the "barrier to entry" for anyone to make any changes to it is very high, and so it (effectively) never sees any
[libvirt PATCH] conf: Fix segfault when parsing mdev types
Commit f1b0890 introduced a potential crash due to incorrect operator
precedence when accessing an element from a pointer to an array.
Backtrace below:
#0 virNodeDeviceGetMdevTypesCaps (sysfspath=0x7fff801661e0
"/sys/devices/pci:00/:00:02.0", mdev_types=0x7fff801c9b40,
nmdev_types=0x7fff801c9b48) at ../src/conf/node_device_conf.c:2676
#1 0x77caf53d in virNodeDeviceGetPCIDynamicCaps
(sysfsPath=0x7fff801661e0 "/sys/devices/pci:00/:00:02.0",
pci_dev=0x7fff801c9ac8) at ../src/conf/node_device_conf.c:2705
#2 0x77cae38f in virNodeDeviceUpdateCaps (def=0x7fff80168a10) at
../src/conf/node_device_conf.c:2342
#3 0x77cb11c0 in virNodeDeviceObjMatch (obj=0x7fff84002e50, flags=0)
at ../src/conf/virnodedeviceobj.c:850
#4 0x77cb153d in virNodeDeviceObjListExportCallback
(payload=0x7fff84002e50, name=0x7fff801cbc20 "pci__00_02_0",
opaque=0x7fffe2ffc6a0) at ../src/conf/virnodedeviceobj.c:909
#5 0x77b69146 in virHashForEach (table=0x7fff9814b700 = {...},
iter=0x77cb149e ,
opaque=0x7fffe2ffc6a0) at ../src/util/virhash.c:394
#6 0x77cb1694 in virNodeDeviceObjListExport (conn=0x7fff98013170,
devs=0x7fff98154430, devices=0x7fffe2ffc798, filter=0x77cf47a1
, flags=0)
at ../src/conf/virnodedeviceobj.c:943
#7 0x7fffe00694b2 in nodeConnectListAllNodeDevices (conn=0x7fff98013170,
devices=0x7fffe2ffc798, flags=0) at ../src/node_device/node_device_driver.c:228
#8 0x77e703aa in virConnectListAllNodeDevices (conn=0x7fff98013170,
devices=0x7fffe2ffc798, flags=0) at ../src/libvirt-nodedev.c:130
#9 0x5557f796 in remoteDispatchConnectListAllNodeDevices
(server=0x55627080, client=0x556bf050, msg=0x556c,
rerr=0x7fffe2ffc8a0, args=0x7fffd4008470, ret=0x7fffd40084e0)
at src/remote/remote_daemon_dispatch_stubs.h:1613
#10 0x5557f6f9 in remoteDispatchConnectListAllNodeDevicesHelper
(server=0x55627080, client=0x556bf050, msg=0x556c,
rerr=0x7fffe2ffc8a0, args=0x7fffd4008470, ret=0x7fffd40084e0)
at src/remote/remote_daemon_dispatch_stubs.h:1591
#11 0x77ce9542 in virNetServerProgramDispatchCall
(prog=0x55690c10, server=0x55627080, client=0x556bf050,
msg=0x556c) at ../src/rpc/virnetserverprogram.c:428
#12 0x77ce90bd in virNetServerProgramDispatch (prog=0x55690c10,
server=0x55627080, client=0x556bf050, msg=0x556c) at
../src/rpc/virnetserverprogram.c:302
#13 0x77cf042b in virNetServerProcessMsg (srv=0x55627080,
client=0x556bf050, prog=0x55690c10, msg=0x556c) at
../src/rpc/virnetserver.c:137
#14 0x77cf04eb in virNetServerHandleJob (jobOpaque=0x556b66b0,
opaque=0x55627080) at ../src/rpc/virnetserver.c:154
#15 0x77bd912f in virThreadPoolWorker (opaque=0x5562bc70) at
../src/util/virthreadpool.c:163
#16 0x77bd8645 in virThreadHelper (data=0x5562bc90) at
../src/util/virthread.c:233
#17 0x76d90432 in start_thread () at /lib64/libpthread.so.0
#18 0x775c5913 in clone () at /lib64/libc.so.6
Signed-off-by: Jonathon Jongsma
---
src/conf/node_device_conf.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
index 4e2837c1cd..cac4243b50 100644
--- a/src/conf/node_device_conf.c
+++ b/src/conf/node_device_conf.c
@@ -2673,7 +2673,7 @@ virNodeDeviceGetMdevTypesCaps(const char *sysfspath,
/* this could be a refresh, so clear out the old data */
for (i = 0; i < *nmdev_types; i++)
- virMediatedDeviceTypeFree(*mdev_types[i]);
+ virMediatedDeviceTypeFree((*mdev_types)[i]);
VIR_FREE(*mdev_types);
*nmdev_types = 0;
--
2.26.2
Re: [PATCH 00/18] qapi/qom: QAPIfy object-add
Am 02.12.2020 um 17:05 hat Eduardo Habkost geschrieben: > > > Looks nice as end goal. Then, these are a few questions I would > > > have about the transition plan: > > > > > > Would it require changing both device implementation and device > > > users in lockstep? Should we have a compatibility layer to allow > > > existing qdev_new()+qdev_prop_set_*() code to keep working after > > > the devices are converted to the new system? If not, why not? > > > > Technically, it doesn't strictly require a lockstep update. You can have > > two code paths leading to a fully initialised device, one being the > > traditional way of setting properties and finally calling dc->realize, > > the other being a new method that takes the configuration in its > > parameters and also sets dev->realized = true at the end. > > > > If at all possible, I would however prefer a lockstep update because > > having two paths is a weird intermediate state and the code paths could > > easily diverge. Keeping the old way around for a device also means that > > property setters are still doing two different jobs (initial > > configuration and updates at runtime). > > I'd like to understand better how that intermediate state would > look like and why there's risk of separate code paths diverging. > > Could we have an intermediate state that doesn't require any > duplication and thus have no separate code paths that could > diverge? The one requirement we have for an intermediate state is that it supports both interfaces: The well-know create/set properties/realize dance, and a new DeviceClass method, say .create(), that takes the configuration in parameters instead of relying on previously set properties. I assumed two separate implementations of transferring the configuration into the internal state. On second thought, this assumption is maybe wrong. You can implement the new method as wrapper around the old way: It could just set all the properties and call realize. Of course, you don't win much in terms of improving the class implementation this way, but just support the new interface, but I guess it can be a reasonable intermediate step to resolve complicated dependencies etc. It would be much nicer to do the wrapper the other way round, i.e. setting properties before the device is realized would update a configuration struct and realize would then call .create() with that struct. To me, this sounds much harder, though also a more useful state. As you have worked a lot with properties recently, maybe you have a good idea how we could get an intermediate state closer to this? > > > If we add a compatibility layer, is the end goal to convert all > > > existing qdev_new() users to the new system? If yes, why? If > > > not, why not? > > > > My personal goal is covering -object and -device, i.e. the external > > interfaces. Converting purely internally created devices is not as > > interesting (especially as long as we focus only on object creation), > > but might be desirable for consistency. > > I wonder how much consistency we will lose and how much confusion > we'll cause if we end up with two completely separate methods for > creating devices. I do think we should follow through and convert everything. It's just not my main motivation, and if the people who work more with qdev think it's better to leave that part unchanged (or that it won't make much of a difference), I won't insist. > > > What about subclasses? Would base classes need to be converted > > > in lockstep with all subclasses? How would the transition > > > process of (e.g.) PCI devices look like? > > > > I don't think so. > > > > If you want to convert base classes first, you may need to take the > > path shown above where both initialisation paths coexist while the > > children are converted because instantiation of a child class involves > > setting properties of the base class. So you can only restrict these > > properties to runtime-only after all children have been converted. > > > > The other way around might be easier: You will need to describe the > > properties of base classes in the QAPI schema from the beginning, but > > that doesn't mean that their initialisation code has to change just yet. > > The child classes will need to forward the part of their configuration > > that belongs to the base class. The downside is that this code will have > > to be changed again when the base class is finally converted. > > > > So we have options there, and we can decide case by case which one is > > most appropriate for the specific class to be converted (depending on > > how many child classes exist, how many properties are inherited, etc.) > > Right now it's hard for me to understand what those intermediate > states would look like. It sounds like it requires too many > complicated manual changes to be done by humans, and lots of room > for mistakes when maintaining two parallel code paths. I'd > prefer to delegate the translation job to a
[PATCH v2] qemu: Pass / fill niothreads for qemuMonitorGetIOThreads
Let's pass along / fill @niothreads rather than trying to make dual
use as a return value and thread count.
This resolves a Coverity issue detected in qemuDomainGetIOThreadsMon
where if qemuDomainObjExitMonitor failed, then a -1 was returned and
overwrite @niothreads causing a memory leak.
Signed-off-by: John Ferlan
---
Since v1, updated the logic to pass @niothreads around rather than
rely on the dual meaning. Took the full plunge.
src/qemu/qemu_driver.c | 23 +++
src/qemu/qemu_monitor.c | 8 +---
src/qemu/qemu_monitor.h | 3 ++-
src/qemu/qemu_monitor_json.c | 6 --
src/qemu/qemu_monitor_json.h | 3 ++-
src/qemu/qemu_process.c | 4 ++--
tests/qemumonitorjsontest.c | 4 ++--
7 files changed, 28 insertions(+), 23 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index bca1c84630..65725b2ef2 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4972,17 +4972,18 @@ qemuDomainGetMaxVcpus(virDomainPtr dom)
static int
qemuDomainGetIOThreadsMon(virQEMUDriverPtr driver,
virDomainObjPtr vm,
- qemuMonitorIOThreadInfoPtr **iothreads)
+ qemuMonitorIOThreadInfoPtr **iothreads,
+ int *niothreads)
{
qemuDomainObjPrivatePtr priv = vm->privateData;
-int niothreads = 0;
+int ret = -1;
qemuDomainObjEnterMonitor(driver, vm);
-niothreads = qemuMonitorGetIOThreads(priv->mon, iothreads);
-if (qemuDomainObjExitMonitor(driver, vm) < 0 || niothreads < 0)
+ret = qemuMonitorGetIOThreads(priv->mon, iothreads, niothreads);
+if (qemuDomainObjExitMonitor(driver, vm) < 0)
return -1;
-return niothreads;
+return ret;
}
@@ -5014,7 +5015,7 @@ qemuDomainGetIOThreadsLive(virQEMUDriverPtr driver,
goto endjob;
}
-if ((niothreads = qemuDomainGetIOThreadsMon(driver, vm, )) < 0)
+if ((ret = qemuDomainGetIOThreadsMon(driver, vm, , ))
< 0)
goto endjob;
/* Nothing to do */
@@ -5314,8 +5315,7 @@ qemuDomainHotplugAddIOThread(virQEMUDriverPtr driver,
* IOThreads thread_id's, adjust the cgroups, thread affinity,
* and add the thread_id to the vm->def->iothreadids list.
*/
-if ((new_niothreads = qemuMonitorGetIOThreads(priv->mon,
- _iothreads)) < 0)
+if (qemuMonitorGetIOThreads(priv->mon, _iothreads, _niothreads) <
0)
goto exit_monitor;
if (qemuDomainObjExitMonitor(driver, vm) < 0)
@@ -5425,8 +5425,7 @@ qemuDomainHotplugDelIOThread(virQEMUDriverPtr driver,
if (rc < 0)
goto exit_monitor;
-if ((new_niothreads = qemuMonitorGetIOThreads(priv->mon,
- _iothreads)) < 0)
+if (qemuMonitorGetIOThreads(priv->mon, _iothreads, _niothreads) <
0)
goto exit_monitor;
if (qemuDomainObjExitMonitor(driver, vm) < 0)
@@ -18507,7 +18506,7 @@ qemuDomainGetStatsIOThread(virQEMUDriverPtr driver,
qemuDomainObjPrivatePtr priv = dom->privateData;
size_t i;
qemuMonitorIOThreadInfoPtr *iothreads = NULL;
-int niothreads;
+int niothreads = 0;
int ret = -1;
if (!HAVE_JOB(privflags) || !virDomainObjIsActive(dom))
@@ -18516,7 +18515,7 @@ qemuDomainGetStatsIOThread(virQEMUDriverPtr driver,
if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD))
return 0;
-if ((niothreads = qemuDomainGetIOThreadsMon(driver, dom, )) < 0)
+if (qemuDomainGetIOThreadsMon(driver, dom, , ) < 0)
return -1;
/* qemuDomainGetIOThreadsMon returns a NULL-terminated list, so we must
free
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index ce1a06c4c8..551b65e778 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -4211,22 +4211,24 @@ qemuMonitorRTCResetReinjection(qemuMonitorPtr mon)
* qemuMonitorGetIOThreads:
* @mon: Pointer to the monitor
* @iothreads: Location to return array of IOThreadInfo data
+ * @niothreads: Count of the number of IOThreads in the array
*
* Issue query-iothreads command.
* Retrieve the list of iothreads defined/running for the machine
*
- * Returns count of IOThreadInfo structures on success
+ * Returns 0 on success
*-1 on error.
*/
int
qemuMonitorGetIOThreads(qemuMonitorPtr mon,
-qemuMonitorIOThreadInfoPtr **iothreads)
+qemuMonitorIOThreadInfoPtr **iothreads,
+int *niothreads)
{
VIR_DEBUG("iothreads=%p", iothreads);
QEMU_CHECK_MONITOR(mon);
-return qemuMonitorJSONGetIOThreads(mon, iothreads);
+return qemuMonitorJSONGetIOThreads(mon, iothreads, niothreads);
}
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index 8bc092870b..49be2d5412 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -1365,7 +1365,8 @@ struct _qemuMonitorIOThreadInfo {
[PATCH v2 1/2] qemu: support append param on live attaching file chardev
Currently it is simply ignored. Signed-off-by: Nikolay Shirokovskiy --- src/qemu/qemu_monitor_json.c | 4 1 file changed, 4 insertions(+) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 47ee1ff..ff03a5a 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -7497,6 +7497,10 @@ qemuMonitorJSONAttachCharDevCommand(const char *chrID, backend_type = "file"; if (virJSONValueObjectAppendString(data, "out", chr->data.file.path) < 0) goto cleanup; +if (virJSONValueObjectAdd(data, + "T:append", chr->data.file.append, + NULL) < 0) +goto cleanup; break; case VIR_DOMAIN_CHR_TYPE_DEV: -- 1.8.3.1
[PATCH v2 0/2] qemu: support some missing params on live attaching chardev
Specifying chardev logfile and file backend append param is ignored yet. Diff from v1: - use existing virJSONValueObjectAdd instead of introducing virJSONValueObjectAppendBooleanTristate. Nikolay Shirokovskiy (2): qemu: support append param on live attaching file chardev qemu: support logfile on live attaching chardev src/qemu/qemu_monitor_json.c | 11 +++ 1 file changed, 11 insertions(+) -- 1.8.3.1
[PATCH v2 2/2] qemu: support logfile on live attaching chardev
Currently it is simply ignored. Signed-off-by: Nikolay Shirokovskiy --- src/qemu/qemu_monitor_json.c | 7 +++ 1 file changed, 7 insertions(+) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index ff03a5a..0745717 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -7611,6 +7611,13 @@ qemuMonitorJSONAttachCharDevCommand(const char *chrID, goto cleanup; } +if (chr->logfile && +virJSONValueObjectAdd(data, + "s:logfile", chr->logfile, + "T:logappend", chr->logappend, + NULL) < 0) +goto cleanup; + if (virJSONValueObjectAppendString(backend, "type", backend_type) < 0 || virJSONValueObjectAppend(backend, "data", data) < 0) goto cleanup; -- 1.8.3.1
Re: [PATCH 4/7] qemu: Fix resource leak and coding error in qemuDomainGetIOThreadsMon
On 12/2/20 9:44 AM, Ján Tomko wrote:
> The commit summary uses 'and' which makes me think there are two
> issues, but the commit message and code only seem to fix one.
>
True - I changed my mind multiple times on this one w/r/t how involved
the fix should be. Originally I did have cleanup added, but then changed
my mind to allow the caller to do it and pass instead.
I can rework this one - thanks for the reviews/pushes. I'll also drop
the pidfilefd one and keep it in my false positive list.
Tks -
John
> On a Wednesday in 2020, John Ferlan wrote:
>> If qemuDomainGetIOThreadsMon fails because qemuDomainObjExitMonitor
>> fails, then a -1 is returned which overwrites @niothreads causing a
>> memory leak. Let's pass @niothreads instead.
>>
>> Found by Coverity.
>>
>> Signed-off-by: John Ferlan
>> ---
>> src/qemu/qemu_driver.c | 19 +--
>> 1 file changed, 9 insertions(+), 10 deletions(-)
>>
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index 8eaa3ce68f..870159de47 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -4972,17 +4972,16 @@ qemuDomainGetMaxVcpus(virDomainPtr dom)
>> static int
>> qemuDomainGetIOThreadsMon(virQEMUDriverPtr driver,
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â virDomainObjPtr vm,
>> -Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â
>> qemuMonitorIOThreadInfoPtr **iothreads)
>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â
>> qemuMonitorIOThreadInfoPtr **iothreads,
>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â int *niothreads)
>
> Returning niothreads separately from success/error might clear things
> up,
>
>> {
>> Â Â Â qemuDomainObjPrivatePtr priv = vm->privateData;
>> -Â Â Â int niothreads = 0;
>>
>> Â Â Â qemuDomainObjEnterMonitor(driver, vm);
>> -Â Â Â niothreads = qemuMonitorGetIOThreads(priv->mon, iothreads);
>> -Â Â Â if (qemuDomainObjExitMonitor(driver, vm) < 0 || niothreads < 0)
>> +Â Â Â *niothreads = qemuMonitorGetIOThreads(priv->mon, iothreads);
>
> but qemuMonitorGetIOThreads can also return -1. In that case we
> should not:
> a) return 0 in this function
> b) compare the return value against unsigned size_t in the cleanup
> Â section of qemuDomainGetStatsIOThread
>
>> +Â Â Â if (qemuDomainObjExitMonitor(driver, vm) < 0)
>
> I think that now that we take a job even for destroying the domain
> in processMonitorEOFEvent, this should not happen.
>
> But if it still can, it would be more consistent if
> qemuDomainGetIOThreadsMon
> cleaned up after itself if it returns -1, instead of leaving it up
> to the caller.
>
> (Other callers of qemuMonitorGetIOThreads and qemuDomainGetIOThreadsMon
> Â seem to handle it properly and check if (iothreads) in their cleanup
> Â sections, it's only qemuDomainGetStatsIOThread that relies on
> Â niothreads being right)
>
> Jano
>
>> Â Â Â Â Â Â Â return -1;
>> -
>> -Â Â Â return niothreads;
>> +Â Â Â return 0;
>> }
>>
>>
>> @@ -5014,7 +5013,7 @@ qemuDomainGetIOThreadsLive(virQEMUDriverPtr driver,
>> Â Â Â Â Â Â Â goto endjob;
>> Â Â Â }
>>
>> -Â Â Â if ((niothreads = qemuDomainGetIOThreadsMon(driver, vm,
>> )) < 0)
>> +Â Â Â if (qemuDomainGetIOThreadsMon(driver, vm, ,
>> ) < 0)
>> Â Â Â Â Â Â Â goto endjob;
>>
>> Â Â Â /* Nothing to do */
>> @@ -18507,7 +18506,7 @@ qemuDomainGetStatsIOThread(virQEMUDriverPtr
>> driver,
>> Â Â Â qemuDomainObjPrivatePtr priv = dom->privateData;
>> Â Â Â size_t i;
>> Â Â Â qemuMonitorIOThreadInfoPtr *iothreads = NULL;
>> -Â Â Â int niothreads;
>> +Â Â Â int niothreads = 0;
>> Â Â Â int ret = -1;
>>
>> Â Â Â if (!HAVE_JOB(privflags) || !virDomainObjIsActive(dom))
>> @@ -18516,8 +18515,8 @@ qemuDomainGetStatsIOThread(virQEMUDriverPtr
>> driver,
>> Â Â Â if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD))
>> Â Â Â Â Â Â Â return 0;
>>
>> -Â Â Â if ((niothreads = qemuDomainGetIOThreadsMon(driver, dom,
>> )) < 0)
>> -Â Â Â Â Â Â Â return -1;
>> +Â Â Â if (qemuDomainGetIOThreadsMon(driver, dom, ,
>> ) < 0)
>> +Â Â Â Â Â Â Â goto cleanup;
>>
>> Â Â Â /* qemuDomainGetIOThreadsMon returns a NULL-terminated list, so we
>> must free
>> Â Â Â Â * it even if it returns 0 */
>> --Â
>> 2.28.0
>>
Re: [PATCH 00/18] qapi/qom: QAPIfy object-add
On Wed, Dec 02, 2020 at 04:17:13PM +0100, Kevin Wolf wrote:
> Am 02.12.2020 um 14:54 hat Eduardo Habkost geschrieben:
> > On Wed, Dec 02, 2020 at 02:26:44PM +0100, Paolo Bonzini wrote:
> > > On 02/12/20 13:51, Eduardo Habkost wrote:
> > > > > > I'm liking the direction this is taking. However, I would still
> > > > > > like to have a clearer and feasible plan that would work for
> > > > > > -device, -machine, and -cpu.
> > > > >
> > > > > -cpu is not a problem since it's generally created with a static
> > > > > configuration (now done with global properties, in the future it
> > > > > could be a
> > > > > struct).
> > > >
> > > > It is a problem if it requires manually converting existing code
> > > > defining CPU properties and we don't have a transition plan.
> > >
> > > We do not have to convert everything _if_ for some objects there are good
> > > reasons to do programmatically-generated properties. CPUs might be one of
> > > those cases (or if we decide to convert them, they might endure some more
> > > code duplication than other devices because they have so many properties).
> >
> > OK, we just need to agree on what the transition will look like
> > when we do it. I think we should put as much care into
> > transition/glue infrastructure as we put into the new
> > infrastructure.
> >
> >
> > >
> > > > Would a -device conversion also involve non-user-creatable
> > > > devices, or would we keep existing internal usage of QOM
> > > > properties?
> > > >
> > > > Even if it's just for user-creatable devices, getting rid of QOM
> > > > property usage in devices sounds like a very ambitious goal. I'd
> > > > like us to have a good transition plan, in addition to declaring
> > > > what's our ideal end goal.
> > >
> > > For user-creatable objects Kevin is doing work in lockstep on all classes;
> > > but once we have the infrastructure for QAPI object configuration schemas
> > > we
> > > can proceed in the other direction and operate on one device at a time.
> > >
> > > With some handwaving, something like (see create_unimplemented_device)
> > >
> > > DeviceState *dev = qdev_new(TYPE_UNIMPLEMENTED_DEVICE);
> > >
> > > qdev_prop_set_string(dev, "name", name);
> > > qdev_prop_set_uint64(dev, "size", size);
> > > sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), _fatal);
> > >
> > > might become something like
> > >
> > > { 'object': 'unimplemented-device',
> > > 'config': {
> > > 'name': 'str',
> > > 'size': 'size'
> > > },
> > > }
> > >
> > > DeviceState *dev = qapi_Unimplemented_new(&(
> > > (UnimplementedDeviceOptions) {
> > > .name = name,
> > > .size = size
> > > }, _fatal);
> > > object_unref(dev);
> > >
> > > (i.e. all typesafe!) and the underlying code would be something like
> > [...]
> > >
> >
> > Looks nice as end goal. Then, these are a few questions I would
> > have about the transition plan:
> >
> > Would it require changing both device implementation and device
> > users in lockstep? Should we have a compatibility layer to allow
> > existing qdev_new()+qdev_prop_set_*() code to keep working after
> > the devices are converted to the new system? If not, why not?
>
> Technically, it doesn't strictly require a lockstep update. You can have
> two code paths leading to a fully initialised device, one being the
> traditional way of setting properties and finally calling dc->realize,
> the other being a new method that takes the configuration in its
> parameters and also sets dev->realized = true at the end.
>
> If at all possible, I would however prefer a lockstep update because
> having two paths is a weird intermediate state and the code paths could
> easily diverge. Keeping the old way around for a device also means that
> property setters are still doing two different jobs (initial
> configuration and updates at runtime).
I'd like to understand better how that intermediate state would
look like and why there's risk of separate code paths diverging.
Could we have an intermediate state that doesn't require any
duplication and thus have no separate code paths that could
diverge?
>
> > If we add a compatibility layer, is the end goal to convert all
> > existing qdev_new() users to the new system? If yes, why? If
> > not, why not?
>
> My personal goal is covering -object and -device, i.e. the external
> interfaces. Converting purely internally created devices is not as
> interesting (especially as long as we focus only on object creation),
> but might be desirable for consistency.
I wonder how much consistency we will lose and how much confusion
we'll cause if we end up with two completely separate methods for
creating devices.
>
> > What about subclasses? Would base classes need to be converted
> > in lockstep with all subclasses? How would the transition
> > process of (e.g.) PCI devices look like?
>
> I don't think so.
>
> If you want to convert base
Re: [PATCH libvirt v3 01/11] nodedev: detect AP card device
Hello Erik,
I will make changes based on your comments in here and patch 03. Thank
you very much for the review.
On 11/30/20 3:20 PM, Erik Skultety wrote:
On Tue, Nov 17, 2020 at 01:10:55PM +0100, Shalini Chellathurai Saroja wrote:
Introduce support for the Adjunct Processor (AP) crypto card device.
Udev already detects the device, so add support for libvirt nodedev
driver.
Signed-off-by: Farhan Ali
Signed-off-by: Shalini Chellathurai Saroja
Reviewed-by: Bjoern Walk
Reviewed-by: Boris Fiuczynski
---
docs/formatnode.html.in| 8 ++
docs/schemas/nodedev.rng | 10
src/conf/node_device_conf.c| 41 ++
src/conf/node_device_conf.h| 8 ++
src/conf/virnodedeviceobj.c| 1 +
src/node_device/node_device_udev.c | 29 +
tools/virsh-nodedev.c | 1 +
7 files changed, 98 insertions(+)
...
+static int
+virNodeDevCapAPCardParseXML(xmlXPathContextPtr ctxt,
+virNodeDeviceDefPtr def,
+xmlNodePtr node,
+virNodeDevCapAPCardPtr ap_card)
+{
+xmlNodePtr orig;
+g_autofree char *adapter = NULL;
+
+orig = ctxt->node;
+ctxt->node = node;
+
+if (!(adapter = virXPathString("string(./ap-adapter[1])", ctxt))) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("missing ap-adapter value for '%s'"), def->name);
+return -1;
+}
+
+if (virStrToLong_uip(adapter, NULL, 0, _card->ap_adapter) < 0) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("invalid ap-adapter value '%s' for '%s'"),
+ adapter, def->name);
+return -1;
+}
+
+ctxt->node = orig;
^this restore will not happen if any of the above fails. I'd suggest using the
VIR_XPATH_NODE_AUTORESTORE(ctxt) macro which already adopts our latest g_auto*
standard.
In context of patch 3, I'd also suggest to extract the adapter parsing logic
into another static helper function e.g. virNodeDevAPAdapterParseXML which
could be reused later from virNodeDevCapAPQueueParseXML to avoid duplication.
Erik
--
Kind regards
Shalini Chellathurai Saroja
Linux on Z and Virtualization Development
Vorsitzende des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
Re: [PATCH 00/11] virDomainCheckpointAlignDisks: Clean up similarly to virDomainSnapshotAlignDisks
On a Wednesday in 2020, Peter Krempa wrote: I've refactored virDomainSnapshotAlignDisks and virDomainCheckpointAlignDisks was heavily inspired by it. Clean it up too. Peter Krempa (11): virDomainCheckpointAlignDisks: Refactor cleanup virDomainCheckpointAlignDisks: Unbreak error message virDomainCheckpointAlignDisks: Use 'domdef' for domain definition virDomainCheckpointAlignDisks: rename 'def' to 'chkdef' virDomainCheckpointAlignDisks: Use 'chkdisk' instead of 'disk' virDomainCheckpointAlignDisks: Extract domain disk def pointer to 'domdisk' virDomainCheckpointAlignDisks: refactor extension to all disks virDomainCheckpointDiskDef: Remove unused 'idx' field virDomainDiskByName: Remove ternary operator virDomainCheckpointAlignDisks: Use virDomainDiskByName virDomainSnapshotAlignDisks: Use virDomainDiskByName src/conf/checkpoint_conf.c | 123 - src/conf/checkpoint_conf.h | 1 - src/conf/domain_conf.c | 6 +- src/conf/snapshot_conf.c | 7 +-- 4 files changed, 61 insertions(+), 76 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
[PATCH 0/3] qemu: support some missing params on live attaching chardev
Specifying chardev logfile and file backend append param is ignored yet. Nikolay Shirokovskiy (3): util: add virJSONValueObjectAppendBooleanTristate qemu: support append param on live attaching file chardev qemu: support logfile on live attaching chardev src/libvirt_private.syms | 1 + src/qemu/qemu_monitor_json.c | 12 src/util/virjson.c | 25 + src/util/virjson.h | 1 + 4 files changed, 39 insertions(+) -- 1.8.3.1
[PATCH 1/3] util: add virJSONValueObjectAppendBooleanTristate
It appends boolean from virTristateBool/virTristateSwitch value.
Signed-off-by: Nikolay Shirokovskiy
---
src/libvirt_private.syms | 1 +
src/util/virjson.c | 25 +
src/util/virjson.h | 1 +
3 files changed, 27 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 179dcec..f7f133c 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2386,6 +2386,7 @@ virJSONValueObjectAdd;
virJSONValueObjectAddVArgs;
virJSONValueObjectAppend;
virJSONValueObjectAppendBoolean;
+virJSONValueObjectAppendBooleanTristate;
virJSONValueObjectAppendNull;
virJSONValueObjectAppendNumberDouble;
virJSONValueObjectAppendNumberInt;
diff --git a/src/util/virjson.c b/src/util/virjson.c
index 4f92464..78ddc4b 100644
--- a/src/util/virjson.c
+++ b/src/util/virjson.c
@@ -767,6 +767,31 @@ virJSONValueObjectAppendBoolean(virJSONValuePtr object,
int
+virJSONValueObjectAppendBooleanTristate(virJSONValuePtr object,
+const char *key,
+int value)
+{
+g_autoptr(virJSONValue) jvalue = NULL;
+int v;
+
+if (value == VIR_TRISTATE_SWITCH_ABSENT)
+return 0;
+
+if (value == VIR_TRISTATE_SWITCH_OFF)
+v = 0;
+else
+v = 1;
+
+jvalue = virJSONValueNewBoolean(v);
+if (virJSONValueObjectAppend(object, key, jvalue) < 0)
+return -1;
+jvalue = NULL;
+
+return 0;
+}
+
+
+int
virJSONValueObjectAppendNull(virJSONValuePtr object,
const char *key)
{
diff --git a/src/util/virjson.h b/src/util/virjson.h
index 4dc7ed1..aa4ae82 100644
--- a/src/util/virjson.h
+++ b/src/util/virjson.h
@@ -137,6 +137,7 @@ int virJSONValueObjectAppendNumberLong(virJSONValuePtr
object, const char *key,
int virJSONValueObjectAppendNumberUlong(virJSONValuePtr object, const char
*key, unsigned long long number);
int virJSONValueObjectAppendNumberDouble(virJSONValuePtr object, const char
*key, double number);
int virJSONValueObjectAppendBoolean(virJSONValuePtr object, const char *key,
int boolean);
+int virJSONValueObjectAppendBooleanTristate(virJSONValuePtr object, const char
*key, int value);
int virJSONValueObjectAppendNull(virJSONValuePtr object, const char *key);
int virJSONValueObjectRemoveKey(virJSONValuePtr object, const char *key,
--
1.8.3.1
[PATCH 2/3] qemu: support append param on live attaching file chardev
Currently it is simply ignored. Signed-off-by: Nikolay Shirokovskiy --- src/qemu/qemu_monitor_json.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 47ee1ff..df95a4a 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -7497,6 +7497,9 @@ qemuMonitorJSONAttachCharDevCommand(const char *chrID, backend_type = "file"; if (virJSONValueObjectAppendString(data, "out", chr->data.file.path) < 0) goto cleanup; +if (virJSONValueObjectAppendBooleanTristate(data, "append", +chr->data.file.append) < 0) +goto cleanup; break; case VIR_DOMAIN_CHR_TYPE_DEV: -- 1.8.3.1
[PATCH 3/3] qemu: support logfile on live attaching chardev
Currently it is simply ignored.
Signed-off-by: Nikolay Shirokovskiy
---
src/qemu/qemu_monitor_json.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index df95a4a..6e96232 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -7610,6 +7610,15 @@ qemuMonitorJSONAttachCharDevCommand(const char *chrID,
goto cleanup;
}
+if (chr->logfile) {
+if (virJSONValueObjectAppendString(data, "logfile",
+ chr->logfile) < 0)
+goto cleanup;
+if (virJSONValueObjectAppendBooleanTristate(data, "logappend",
+chr->logappend) < 0)
+goto cleanup;
+}
+
if (virJSONValueObjectAppendString(backend, "type", backend_type) < 0 ||
virJSONValueObjectAppend(backend, "data", data) < 0)
goto cleanup;
--
1.8.3.1
Re: [PATCH v2] coding-style: Document 100 chars limit for line length
On Wed, Dec 02, 2020 at 03:54:28PM +0100, Michal Privoznik wrote: > The idea is to have it like a soft limit: if possible then break > lines, if not then have a long line instead of some creative > approach. > > Signed-off-by: Michal Privoznik > --- > > v2 of: > > https://www.redhat.com/archives/libvir-list/2020-November/msg01600.html 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 00/18] qapi/qom: QAPIfy object-add
Am 02.12.2020 um 14:54 hat Eduardo Habkost geschrieben:
> On Wed, Dec 02, 2020 at 02:26:44PM +0100, Paolo Bonzini wrote:
> > On 02/12/20 13:51, Eduardo Habkost wrote:
> > > > > I'm liking the direction this is taking. However, I would still
> > > > > like to have a clearer and feasible plan that would work for
> > > > > -device, -machine, and -cpu.
> > > >
> > > > -cpu is not a problem since it's generally created with a static
> > > > configuration (now done with global properties, in the future it could
> > > > be a
> > > > struct).
> > >
> > > It is a problem if it requires manually converting existing code
> > > defining CPU properties and we don't have a transition plan.
> >
> > We do not have to convert everything _if_ for some objects there are good
> > reasons to do programmatically-generated properties. CPUs might be one of
> > those cases (or if we decide to convert them, they might endure some more
> > code duplication than other devices because they have so many properties).
>
> OK, we just need to agree on what the transition will look like
> when we do it. I think we should put as much care into
> transition/glue infrastructure as we put into the new
> infrastructure.
>
>
> >
> > > Would a -device conversion also involve non-user-creatable
> > > devices, or would we keep existing internal usage of QOM
> > > properties?
> > >
> > > Even if it's just for user-creatable devices, getting rid of QOM
> > > property usage in devices sounds like a very ambitious goal. I'd
> > > like us to have a good transition plan, in addition to declaring
> > > what's our ideal end goal.
> >
> > For user-creatable objects Kevin is doing work in lockstep on all classes;
> > but once we have the infrastructure for QAPI object configuration schemas we
> > can proceed in the other direction and operate on one device at a time.
> >
> > With some handwaving, something like (see create_unimplemented_device)
> >
> > DeviceState *dev = qdev_new(TYPE_UNIMPLEMENTED_DEVICE);
> >
> > qdev_prop_set_string(dev, "name", name);
> > qdev_prop_set_uint64(dev, "size", size);
> > sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), _fatal);
> >
> > might become something like
> >
> > { 'object': 'unimplemented-device',
> > 'config': {
> > 'name': 'str',
> > 'size': 'size'
> > },
> > }
> >
> > DeviceState *dev = qapi_Unimplemented_new(&(
> > (UnimplementedDeviceOptions) {
> > .name = name,
> > .size = size
> > }, _fatal);
> > object_unref(dev);
> >
> > (i.e. all typesafe!) and the underlying code would be something like
> [...]
> >
>
> Looks nice as end goal. Then, these are a few questions I would
> have about the transition plan:
>
> Would it require changing both device implementation and device
> users in lockstep? Should we have a compatibility layer to allow
> existing qdev_new()+qdev_prop_set_*() code to keep working after
> the devices are converted to the new system? If not, why not?
Technically, it doesn't strictly require a lockstep update. You can have
two code paths leading to a fully initialised device, one being the
traditional way of setting properties and finally calling dc->realize,
the other being a new method that takes the configuration in its
parameters and also sets dev->realized = true at the end.
If at all possible, I would however prefer a lockstep update because
having two paths is a weird intermediate state and the code paths could
easily diverge. Keeping the old way around for a device also means that
property setters are still doing two different jobs (initial
configuration and updates at runtime).
> If we add a compatibility layer, is the end goal to convert all
> existing qdev_new() users to the new system? If yes, why? If
> not, why not?
My personal goal is covering -object and -device, i.e. the external
interfaces. Converting purely internally created devices is not as
interesting (especially as long as we focus only on object creation),
but might be desirable for consistency.
> What about subclasses? Would base classes need to be converted
> in lockstep with all subclasses? How would the transition
> process of (e.g.) PCI devices look like?
I don't think so.
If you want to convert base classes first, you may need to take the
path shown above where both initialisation paths coexist while the
children are converted because instantiation of a child class involves
setting properties of the base class. So you can only restrict these
properties to runtime-only after all children have been converted.
The other way around might be easier: You will need to describe the
properties of base classes in the QAPI schema from the beginning, but
that doesn't mean that their initialisation code has to change just yet.
The child classes will need to forward the part of their configuration
that belongs to the base class. The downside is that this code will have
to be changed again
Re: [PATCH 7/7] qemu: Fix some issues in virQEMUDriverConfigLoadNVRAMEntry
On a Wednesday in 2020, John Ferlan wrote: Commit c4f4e195 fixed a double free, but if the code returns before we realloc the list and virFirmwareFreeList was called with cfg->nfirmwares 0 (e.g. during virQEMUDriverConfigDispose), then it would be rather disasterous. So let's reinitialze that too to indicate the list is empty. *disastrous *reinitialize Coverity pointed out that using nvram[0] as a guard to reallocating the list could lead to a possible NULL deref. While nvram[0] may always be true in this case, if it wasn't then the subsequent for loop would fail. Just reallocate always regardless - even if nfirmwares == 0 as virFirmwareFreeList will free it for us anyway. Signed-off-by: John Ferlan --- src/qemu/qemu_conf.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH] qemu: Drop qemuMonitorGetVirtType()
On 12/2/20 3:53 PM, Ján Tomko wrote: On a Wednesday in 2020, Michal Privoznik wrote: It's unused since v5.5.0-rc1~113. $ git show show v5.5.0-rc1~113. fatal: ambiguous argument 'show': unknown revision or path not in the working tree. Just drop the dot, it's marking the end of the sentence, not part of the commit ID. fatal: ambiguous argument 'v5.5.0-rc1~113.': unknown revision or path not in the working tree. Signed-off-by: Michal Privoznik --- src/qemu/qemu_monitor.c     | 10 --- src/qemu/qemu_monitor.h     | 2 -- src/qemu/qemu_monitor_json.c | 40 --- src/qemu/qemu_monitor_json.h | 2 -- tests/qemumonitorjsontest.c | 52 5 files changed, 106 deletions(-) Reviewed-by: Ján Tomko Pushed thanks. Michal
Re: [PATCH] spec: keep existing nwfilters uuid on update
Polite ping
On 26.10.2020 12:21, Nikolay Shirokovskiy wrote:
> Now on every nwfilter config package update we overwrite existing filters
> entirely. It is desired to bring new version of filters on update but we'd
> better keep their uuids I guess.
>
> Actually patch primarily address noise in logs on update. If both libvirtd and
> firewalld are running and libvirt is using firewalld backend then on firewalld
> restart we reload all nwfilters. So if node is updated and we have update for
> both firewalld and libvirt then in the process of update first new nwfilters
> of
> libvirt package are copied to /etc/libvirt/nwfilters then firewalld is
> restarted and then libvirtd is restarted. In this process firewalld restart
> cause log messages like [1]. The issue is libvirt brings nwfilters without
> in definition and on handling firewalld restart libvirt generates
> missing uuid and then fail to update filter definition because it is already
> present in filters list with different uuid.
>
> [1] virNWFilterObjListAssignDef:337 : operation failed: filter
> 'no-ip-spoofing'
> already exists with uuid c302edf9-8a48-40d8-a652-f70b2c563ad1
>
> Signed-off-by: Nikolay Shirokovskiy
> ---
> libvirt.spec.in | 13 -
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/libvirt.spec.in b/libvirt.spec.in
> index 2a4324b..6a31440 100644
> --- a/libvirt.spec.in
> +++ b/libvirt.spec.in
> @@ -1438,7 +1438,18 @@ fi
> rm -rf %{_localstatedir}/lib/rpm-state/libvirt || :
>
> %post daemon-config-nwfilter
> -cp %{_datadir}/libvirt/nwfilter/*.xml %{_sysconfdir}/libvirt/nwfilter/
> +# keep existing filters uuid on update
> +for dfile in %{_datadir}/libvirt/nwfilter/*.xml; do
> +sfile=%{_sysconfdir}/libvirt/nwfilter/`basename $dfile`
> +if [ -f "$sfile" ]; then
> + uuidstr=`sed -n '/.*<\/uuid>/p' "$sfile"`
> + if [ ! -z "$uuidstr" ]; then
> +sed -e "s,,&\n$uuidstr," "$dfile" > "$sfile"
> +continue
> + fi
> +fi
> +cp "$dfile" "$sfile"
> +done
> # libvirt saves these files with mode 600
> chmod 600 %{_sysconfdir}/libvirt/nwfilter/*.xml
> # Make sure libvirt picks up the new nwfilter defininitons
>
[PATCH v2] coding-style: Document 100 chars limit for line length
The idea is to have it like a soft limit: if possible then break
lines, if not then have a long line instead of some creative
approach.
Signed-off-by: Michal Privoznik
---
v2 of:
https://www.redhat.com/archives/libvir-list/2020-November/msg01600.html
docs/coding-style.rst | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/docs/coding-style.rst b/docs/coding-style.rst
index cfd7b16638..b3ac070fac 100644
--- a/docs/coding-style.rst
+++ b/docs/coding-style.rst
@@ -131,7 +131,7 @@ around operators and keywords:
indent-libvirt()
{
-indent -bad -bap -bbb -bli4 -br -ce -brs -cs -i4 -l75 -lc75 \
+indent -bad -bap -bbb -bli4 -br -ce -brs -cs -i4 -l100 -lc100 \
-sbi4 -psl -saf -sai -saw -sbi4 -ss -sc -cdw -cli4 -npcs -nbc \
--no-tabs "$@"
}
@@ -141,6 +141,9 @@ further, by piping it through ``expand -i``, since some
leading
TABs can get through. Usually they're in macro definitions or
strings, and should be converted anyhow.
+The maximum permitted line length is 100 characters, but lines
+should aim to be approximately 80 characters.
+
Libvirt requires a C99 compiler for various reasons. However, most
of the code base prefers to stick to C89 syntax unless there is a
compelling reason otherwise. For example, it is preferable to use
--
2.26.2
Re: [PATCH] qemu: Drop qemuMonitorGetVirtType()
On a Wednesday in 2020, Michal Privoznik wrote: It's unused since v5.5.0-rc1~113. $ git show show v5.5.0-rc1~113. fatal: ambiguous argument 'v5.5.0-rc1~113.': unknown revision or path not in the working tree. Signed-off-by: Michal Privoznik --- src/qemu/qemu_monitor.c | 10 --- src/qemu/qemu_monitor.h | 2 -- src/qemu/qemu_monitor_json.c | 40 --- src/qemu/qemu_monitor_json.h | 2 -- tests/qemumonitorjsontest.c | 52 5 files changed, 106 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH 6/7] locking: Resolve mem leak in virLockDaemonPreExecRestart
On a Wednesday in 2020, John Ferlan wrote: Initialize and free @magic since virJSONValueObjectAppendString does not free it for us eventually. Signed-off-by: John Ferlan --- src/locking/lock_daemon.c | 7 +++ 1 file changed, 3 insertions(+), 4 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH 5/7] logging: Resolve mem leak in virLogDaemonPreExecRestart
On a Wednesday in 2020, John Ferlan wrote: Initialize and free @magic since virJSONValueObjectAppendString does not free it for us eventually. Signed-off-by: John Ferlan --- src/logging/log_daemon.c | 7 +++ 1 file changed, 3 insertions(+), 4 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
[PATCH] qemu: Drop qemuMonitorGetVirtType()
It's unused since v5.5.0-rc1~113.
Signed-off-by: Michal Privoznik
---
src/qemu/qemu_monitor.c | 10 ---
src/qemu/qemu_monitor.h | 2 --
src/qemu/qemu_monitor_json.c | 40 ---
src/qemu/qemu_monitor_json.h | 2 --
tests/qemumonitorjsontest.c | 52
5 files changed, 106 deletions(-)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index c333fc1364..ce1a06c4c8 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -1964,16 +1964,6 @@ qemuMonitorSetLink(qemuMonitorPtr mon,
}
-int
-qemuMonitorGetVirtType(qemuMonitorPtr mon,
- virDomainVirtType *virtType)
-{
-QEMU_CHECK_MONITOR(mon);
-
-return qemuMonitorJSONGetVirtType(mon, virtType);
-}
-
-
/**
* Returns: 0 if balloon not supported, +1 if balloon query worked
* or -1 on failure
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index 3dcceffef8..8bc092870b 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -656,8 +656,6 @@ virBitmapPtr qemuMonitorGetCpuHalted(qemuMonitorPtr mon,
size_t maxvcpus,
bool fast);
-int qemuMonitorGetVirtType(qemuMonitorPtr mon,
- virDomainVirtType *virtType);
int qemuMonitorGetBalloonInfo(qemuMonitorPtr mon,
unsigned long long *currmem);
int qemuMonitorGetMemoryStats(qemuMonitorPtr mon,
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index ab83abf6b3..5acc1a10aa 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -2052,46 +2052,6 @@ qemuMonitorJSONQueryCPUs(qemuMonitorPtr mon,
}
-int qemuMonitorJSONGetVirtType(qemuMonitorPtr mon,
- virDomainVirtType *virtType)
-{
-int ret = -1;
-virJSONValuePtr cmd = qemuMonitorJSONMakeCommand("query-kvm",
- NULL);
-virJSONValuePtr reply = NULL;
-virJSONValuePtr data;
-bool val = false;
-
-*virtType = VIR_DOMAIN_VIRT_QEMU;
-
-if (!cmd)
-return -1;
-
-if (qemuMonitorJSONCommand(mon, cmd, ) < 0)
-goto cleanup;
-
-if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_OBJECT) < 0)
-goto cleanup;
-
-data = virJSONValueObjectGetObject(reply, "return");
-
-if (virJSONValueObjectGetBoolean(data, "enabled", ) < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("info kvm reply missing 'enabled' field"));
-goto cleanup;
-}
-
-if (val)
-*virtType = VIR_DOMAIN_VIRT_KVM;
-
-ret = 0;
- cleanup:
-virJSONValueFree(cmd);
-virJSONValueFree(reply);
-return ret;
-}
-
-
/**
* Loads correct video memory size values from QEMU and update the video
* definition.
diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
index da988f0d41..d2928b0ffc 100644
--- a/src/qemu/qemu_monitor_json.h
+++ b/src/qemu/qemu_monitor_json.h
@@ -57,8 +57,6 @@ int qemuMonitorJSONQueryCPUs(qemuMonitorPtr mon,
size_t *nentries,
bool force,
bool fast);
-int qemuMonitorJSONGetVirtType(qemuMonitorPtr mon,
- virDomainVirtType *virtType);
int qemuMonitorJSONUpdateVideoMemorySize(qemuMonitorPtr mon,
virDomainVideoDefPtr video,
char *path);
diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c
index d22a92d3e1..79ef2a545e 100644
--- a/tests/qemumonitorjsontest.c
+++ b/tests/qemumonitorjsontest.c
@@ -1445,57 +1445,6 @@ testQemuMonitorJSONqemuMonitorJSONGetBalloonInfo(const
void *opaque)
return 0;
}
-static int
-testQemuMonitorJSONqemuMonitorJSONGetVirtType(const void *opaque)
-{
-const testGenericData *data = opaque;
-virDomainXMLOptionPtr xmlopt = data->xmlopt;
-virDomainVirtType virtType;
-g_autoptr(qemuMonitorTest) test = NULL;
-
-if (!(test = qemuMonitorTestNewSchema(xmlopt, data->schema)))
-return -1;
-
-if (qemuMonitorTestAddItem(test, "query-kvm",
- "{"
- "\"return\": {"
- "\"enabled\": true,"
- "\"present\": true"
- "},"
- "\"id\": \"libvirt-8\""
- "}") < 0 ||
-qemuMonitorTestAddItem(test, "query-kvm",
- "{"
- "\"return\": {"
- "\"enabled\": false,"
- "\"present\": true"
- "},"
- "\"id\": \"libvirt-7\""
-
Re: [libvirt][PATCH v1 1/1] support system default memory policy with numatune
Apologies for the late review. First, 'ninja -C build test' complained about long lines in the numatune-memory-migratable.x86_64-latest.args file. I amended it here and the tests passed: $ git diff diff --git a/tests/qemuxml2argvdata/numatune-memory-migratable.x86_64-latest.args b/tests/qemuxml2argvdata/numatune-memory-migratable.x86_64-latest.args index 6a99540792..1f15c4396e 100644 --- a/tests/qemuxml2argvdata/numatune-memory-migratable.x86_64-latest.args +++ b/tests/qemuxml2argvdata/numatune-memory-migratable.x86_64-latest.args @@ -6,12 +6,16 @@ LOGNAME=test \ XDG_DATA_HOME=/tmp/lib/domain--1-QEMUGuest/.local/share \ XDG_CACHE_HOME=/tmp/lib/domain--1-QEMUGuest/.cache \ XDG_CONFIG_HOME=/tmp/lib/domain--1-QEMUGuest/.config \ -QEMU_AUDIO_DRV=none /usr/bin/qemu-system-x86_64 \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-x86_64 \ -name guest=QEMUGuest,debug-threads=on \ -S \ --object secret,id=masterKey0,format=raw,file=/tmp/lib/domain--1-QEMUGuest/master-key.aes \ +-object secret,id=masterKey0,format=raw,\ +file=/tmp/lib/domain--1-QEMUGuest/master-key.aes \ -machine pc,accel=tcg,usb=off,dump-guest-core=off \ --cpu qemu64 -m 24105 -overcommit mem-lock=off \ +-cpu qemu64 \ +-m 24105 \ +-overcommit mem-lock=off \ -smp 32,sockets=32,cores=1,threads=1 \ -object memory-backend-ram,id=ram-node0,size=20971520 \ -numa node,nodeid=0,cpus=0,memdev=ram-node0 \ @@ -31,5 +35,6 @@ QEMU_AUDIO_DRV=none /usr/bin/qemu-system-x86_64 \ -boot strict=on \ -device piix3-usb-uhci,id=usb,bus=pci.0,addr=0x1.0x2 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x2 \ --sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \ +-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\ +resourcecontrol=deny \ -msg timestamp=on So there's already something to be fixed in the next version. More comments below: On 11/3/20 8:15 AM, Luyao Zhong wrote: This patch seeks the support of system default memory policy when numatune is configured. Before this patch, numatune only has three memory modes: static, interleave and prefered. These memory policies are ultimately set by mbind() system call. Memory policy could be 'hard coded' into the kernel, but none of above policies fit our requirment under this case. mbind() support s/requirment/requirement default memory policy, but it requires a NULL nodemask. So obviously setting allowed memory nodes is cgroups' mission. That means if 'default' mode is specified in numatune, numatune config will be completely cgroups setting, which restricting the memory nodes allowed for each vcpu thread in different cells. If you want to set default memory policy, please set migratable to "yes" and mode to "default" at the same time. Some implementation details are not determained, so this patch s/determained/determined is not split into logical blocks yet. IIUC 'migratable' always implies mode='default' for all memnode cells. In this case I find it a bit redundant to have both 'migratable' and 'default' settings to care about. At the same time I was not able to come up with something clever/better: having just the 'migratable' attribute and simply assume 'mode=default' for all memnodes would be confusing. I guess the extra verbosity of having to set both is justified in this case. --- docs/formatdomain.rst | 12 - docs/schemas/domaincommon.rng | 7 +++ include/libvirt/libvirt-domain.h | 1 + src/conf/numa_conf.c | 50 ++- src/conf/numa_conf.h | 2 + src/libvirt_private.syms | 1 + src/qemu/qemu_command.c | 7 ++- src/qemu/qemu_process.c | 25 ++ src/util/virnuma.c| 3 ++ .../numatune-memory-migratable.args | 34 + ...atune-memory-migratable.x86_64-latest.args | 35 + .../numatune-memory-migratable.xml| 33 tests/qemuxml2argvtest.c | 2 + ...matune-memory-migratable.x86_64-latest.xml | 40 +++ .../numatune-memory-migratable.xml| 39 +++ tests/qemuxml2xmltest.c | 1 + 16 files changed, 288 insertions(+), 4 deletions(-) create mode 100644 tests/qemuxml2argvdata/numatune-memory-migratable.args create mode 100644 tests/qemuxml2argvdata/numatune-memory-migratable.x86_64-latest.args create mode 100644 tests/qemuxml2argvdata/numatune-memory-migratable.xml create mode 100644 tests/qemuxml2xmloutdata/numatune-memory-migratable.x86_64-latest.xml create mode 100644 tests/qemuxml2xmloutdata/numatune-memory-migratable.xml diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index ae635bedff..4ab9873c32 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -1086,8 +1086,9 @@ NUMA Node Tuning ``memory`` The
Re: [PATCH 4/7] qemu: Fix resource leak and coding error in qemuDomainGetIOThreadsMon
The commit summary uses 'and' which makes me think there are two
issues, but the commit message and code only seem to fix one.
On a Wednesday in 2020, John Ferlan wrote:
If qemuDomainGetIOThreadsMon fails because qemuDomainObjExitMonitor
fails, then a -1 is returned which overwrites @niothreads causing a
memory leak. Let's pass @niothreads instead.
Found by Coverity.
Signed-off-by: John Ferlan
---
src/qemu/qemu_driver.c | 19 +--
1 file changed, 9 insertions(+), 10 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 8eaa3ce68f..870159de47 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4972,17 +4972,16 @@ qemuDomainGetMaxVcpus(virDomainPtr dom)
static int
qemuDomainGetIOThreadsMon(virQEMUDriverPtr driver,
virDomainObjPtr vm,
- qemuMonitorIOThreadInfoPtr **iothreads)
+ qemuMonitorIOThreadInfoPtr **iothreads,
+ int *niothreads)
Returning niothreads separately from success/error might clear things
up,
{
qemuDomainObjPrivatePtr priv = vm->privateData;
-int niothreads = 0;
qemuDomainObjEnterMonitor(driver, vm);
-niothreads = qemuMonitorGetIOThreads(priv->mon, iothreads);
-if (qemuDomainObjExitMonitor(driver, vm) < 0 || niothreads < 0)
+*niothreads = qemuMonitorGetIOThreads(priv->mon, iothreads);
but qemuMonitorGetIOThreads can also return -1. In that case we
should not:
a) return 0 in this function
b) compare the return value against unsigned size_t in the cleanup
section of qemuDomainGetStatsIOThread
+if (qemuDomainObjExitMonitor(driver, vm) < 0)
I think that now that we take a job even for destroying the domain
in processMonitorEOFEvent, this should not happen.
But if it still can, it would be more consistent if qemuDomainGetIOThreadsMon
cleaned up after itself if it returns -1, instead of leaving it up
to the caller.
(Other callers of qemuMonitorGetIOThreads and qemuDomainGetIOThreadsMon
seem to handle it properly and check if (iothreads) in their cleanup
sections, it's only qemuDomainGetStatsIOThread that relies on
niothreads being right)
Jano
return -1;
-
-return niothreads;
+return 0;
}
@@ -5014,7 +5013,7 @@ qemuDomainGetIOThreadsLive(virQEMUDriverPtr driver,
goto endjob;
}
-if ((niothreads = qemuDomainGetIOThreadsMon(driver, vm, )) < 0)
+if (qemuDomainGetIOThreadsMon(driver, vm, , ) < 0)
goto endjob;
/* Nothing to do */
@@ -18507,7 +18506,7 @@ qemuDomainGetStatsIOThread(virQEMUDriverPtr driver,
qemuDomainObjPrivatePtr priv = dom->privateData;
size_t i;
qemuMonitorIOThreadInfoPtr *iothreads = NULL;
-int niothreads;
+int niothreads = 0;
int ret = -1;
if (!HAVE_JOB(privflags) || !virDomainObjIsActive(dom))
@@ -18516,8 +18515,8 @@ qemuDomainGetStatsIOThread(virQEMUDriverPtr driver,
if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD))
return 0;
-if ((niothreads = qemuDomainGetIOThreadsMon(driver, dom, )) < 0)
-return -1;
+if (qemuDomainGetIOThreadsMon(driver, dom, , ) < 0)
+goto cleanup;
/* qemuDomainGetIOThreadsMon returns a NULL-terminated list, so we must free
* it even if it returns 0 */
--
2.28.0
signature.asc
Description: PGP signature
Re: [PATCH v2] src: use singular form instead of plural, for guest disk info
On Wed, Dec 2, 2020 at 4:47 PM Daniel P. Berrangé wrote: > Existing practice with the filesystem fields reported for the > virDomainGetGuestInfo API is to use the singular form for > field names. Ensure the disk info follows this practice. > > Fixes > > commit 05a75ca2ce743bc0bb119fb8d532ff84646fafa3 > Author: Marc-André Lureau > Date: Fri Nov 20 22:09:46 2020 +0400 > > domain: add disk informations to virDomainGetGuestInfo > > commit 0cb2d9f05d00497a715352f6ea28cf8fb6921731 > Author: Marc-André Lureau > Date: Fri Nov 20 22:09:47 2020 +0400 > > qemu_driver: report guest disk informations > > commit 172b8304352b1945e328394e61290a24446280dd > Author: Marc-André Lureau > Date: Fri Nov 20 22:09:48 2020 +0400 > > virsh: add --disk informations to guestinfo command > > Signed-off-by: Daniel P. Berrangé > Reviewed-by: Marc-André Lureau --- > > In v2: also update docs and virsh > > docs/manpages/virsh.rst | 20 ++-- > src/libvirt-domain.c| 14 +++--- > src/qemu/qemu_driver.c | 14 +++--- > tools/virsh-domain.c| 6 +++--- > 4 files changed, 27 insertions(+), 27 deletions(-) > > diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst > index 9ef6b68422..aa54bc21ef 100644 > --- a/docs/manpages/virsh.rst > +++ b/docs/manpages/virsh.rst > @@ -2679,7 +2679,7 @@ guestinfo > :: > > guestinfo domain [--user] [--os] [--timezone] [--hostname] > [--filesystem] > - [--disks] > + [--disk] > > Print information about the guest from the point of view of the guest > agent. > Note that this command requires a guest agent to be configured and > running in > @@ -2690,7 +2690,7 @@ are supported by the guest agent. You can limit the > types of information that > are returned by specifying one or more flags. If a requested information > type is not supported, the processes will provide an exit code of 1. > Available information types flags are *--user*, *--os*, > -*--timezone*, *--hostname*, *--filesystem* and *--disks*. > +*--timezone*, *--hostname*, *--filesystem* and *--disk*. > > Note that depending on the hypervisor type and the version of the guest > agent > running within the domain, not all of the following information may be > @@ -2747,15 +2747,15 @@ returned: > * ``fs..disk..serial`` - the serial number of disk > * ``fs..disk..device`` - the device node of disk > > -*--disks* returns: > +*--disk* returns: > > -* ``disks.count`` - the number of disks defined on this domain > -* ``disks..name`` - device node (Linux) or device UNC (Windows) > -* ``disks..partition`` - whether this is a partition or disk > -* ``disks..dependencies.count`` - the number of device dependencies > -* ``disks..dependencies..name`` - a dependency name > -* ``disks..alias`` - the device alias of the disk (e.g. sda) > -* ``disks..guest_alias`` - optional alias assigned to the disk > +* ``disk.count`` - the number of disks defined on this domain > +* ``disk..name`` - device node (Linux) or device UNC (Windows) > +* ``disk..partition`` - whether this is a partition or disk > +* ``disk..dependency.count`` - the number of device dependencies > +* ``disk..dependency..name`` - a dependency name > +* ``disk..alias`` - the device alias of the disk (e.g. sda) > +* ``disk..guest_alias`` - optional alias assigned to the disk > > > guestvcpus > diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c > index 35e95e5395..f5cd43ecea 100644 > --- a/src/libvirt-domain.c > +++ b/src/libvirt-domain.c > @@ -12345,17 +12345,17 @@ virDomainSetVcpu(virDomainPtr domain, > * Returns information about the disks within the domain. The typed > * parameter keys are in this format: > * > - * "disks.count" - the number of disks defined on this domain > + * "disk.count" - the number of disks defined on this domain > * as an unsigned int > - * "disks..name" - device node (Linux) or device UNC (Windows) > - * "disks..partition" - whether this is a partition or disk > - * "disks..dependencies.count" - the number of device > dependencies > + * "disk..name" - device node (Linux) or device UNC (Windows) > + * "disk..partition" - whether this is a partition or disk > + * "disk..dependency.count" - the number of device dependencies > * e.g. for LVs of the LVM this will > * hold the list of PVs, for LUKS encrypted volume > this will > * contain the disk where the volume is placed. > (Linux) > - * "disks..dependencies..name" - a dependency > - * "disks..alias" - the device alias of the disk (e.g. sda) > - * "disks..guest_alias" - optional alias assigned to the disk, > on Linux > + * "disk..dependency..name" - a dependency > + * "disk..alias" - the device alias of the disk (e.g. sda) > + * "disk..guest_alias" - optional alias assigned to the disk, > on Linux > * this is a name assigned
[PATCH 4/5] conf: checkpoint: Prepare internals for missing domain definition
Coniditonalize code which assumes that the domain definition stored in
the checkpoint is present.
Signed-off-by: Peter Krempa
---
src/conf/checkpoint_conf.c | 31 +
tests/qemudomaincheckpointxml2xmltest.c | 5
2 files changed, 16 insertions(+), 20 deletions(-)
diff --git a/src/conf/checkpoint_conf.c b/src/conf/checkpoint_conf.c
index 73fdb78e7a..2071494d52 100644
--- a/src/conf/checkpoint_conf.c
+++ b/src/conf/checkpoint_conf.c
@@ -475,10 +475,10 @@ virDomainCheckpointDefFormatInternal(virBufferPtr buf,
virBufferAddLit(buf, "\n");
}
-if (!(flags & VIR_DOMAIN_CHECKPOINT_FORMAT_NO_DOMAIN) &&
-virDomainDefFormatInternal(def->parent.dom, xmlopt,
- buf, domainflags) < 0)
-return -1;
+if (def->parent.dom && !(flags & VIR_DOMAIN_CHECKPOINT_FORMAT_NO_DOMAIN)) {
+if (virDomainDefFormatInternal(def->parent.dom, xmlopt, buf,
domainflags) < 0)
+return -1;
+}
virBufferAdjustIndent(buf, -2);
virBufferAddLit(buf, "\n");
@@ -510,23 +510,24 @@ virDomainCheckpointRedefinePrep(virDomainObjPtr vm,
virDomainCheckpointDefPtr def,
bool *update_current)
{
-char uuidstr[VIR_UUID_STRING_BUFLEN];
virDomainMomentObjPtr parent = NULL;
-virUUIDFormat(vm->def->uuid, uuidstr);
-
if (virDomainCheckpointCheckCycles(vm->checkpoints, def, vm->def->name) <
0)
return -1;
-if (!def->parent.dom ||
-memcmp(def->parent.dom->uuid, vm->def->uuid, VIR_UUID_BUFLEN)) {
-virReportError(VIR_ERR_INVALID_ARG,
- _("definition for checkpoint %s must use uuid %s"),
- def->parent.name, uuidstr);
-return -1;
+if (def->parent.dom) {
+if (memcmp(def->parent.dom->uuid, vm->def->uuid, VIR_UUID_BUFLEN)) {
+char uuidstr[VIR_UUID_STRING_BUFLEN];
+virUUIDFormat(vm->def->uuid, uuidstr);
+virReportError(VIR_ERR_INVALID_ARG,
+ _("definition for checkpoint %s must use uuid %s"),
+ def->parent.name, uuidstr);
+return -1;
+}
+
+if (virDomainCheckpointAlignDisks(def) < 0)
+return -1;
}
-if (virDomainCheckpointAlignDisks(def) < 0)
-return -1;
if (def->parent.parent_name &&
(parent = virDomainCheckpointFindByName(vm->checkpoints,
diff --git a/tests/qemudomaincheckpointxml2xmltest.c
b/tests/qemudomaincheckpointxml2xmltest.c
index a5a5b59205..8b4b75d753 100644
--- a/tests/qemudomaincheckpointxml2xmltest.c
+++ b/tests/qemudomaincheckpointxml2xmltest.c
@@ -87,11 +87,6 @@ testCompareXMLToXMLFiles(const char *inxml,
formatflags |= VIR_DOMAIN_CHECKPOINT_FORMAT_SIZE;
}
-/* Parsing XML does not populate the domain definition; work
- * around that by not requesting domain on output */
-if (!def->parent.dom)
-formatflags |= VIR_DOMAIN_CHECKPOINT_FORMAT_NO_DOMAIN;
-
if (!(actual = virDomainCheckpointDefFormat(def,
driver.xmlopt,
formatflags)))
--
2.28.0
[PATCH 2/5] virDomainCheckpointDefParse: Use 'unsigned int' for flags
Fix the type for a variable holding flags to the usual 'unsigned int'
and change the name to be more appropriate to it's use.
Signed-off-by: Peter Krempa
---
src/conf/checkpoint_conf.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/conf/checkpoint_conf.c b/src/conf/checkpoint_conf.c
index 33b6699be7..8744ac83e1 100644
--- a/src/conf/checkpoint_conf.c
+++ b/src/conf/checkpoint_conf.c
@@ -157,12 +157,12 @@ virDomainCheckpointDefParse(xmlXPathContextPtr ctxt,
def->parent.parent_name = virXPathString("string(./parent/name)",
ctxt);
if ((domainNode = virXPathNode("./domain", ctxt))) {
-int domainflags = VIR_DOMAIN_DEF_PARSE_INACTIVE |
- VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE;
+unsigned int domainParseFlags = VIR_DOMAIN_DEF_PARSE_INACTIVE |
+VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE;
def->parent.dom = virDomainDefParseNode(ctxt->node->doc,
domainNode,
xmlopt, parseOpaque,
-domainflags);
+domainParseFlags);
if (!def->parent.dom)
return NULL;
} else {
--
2.28.0
[PATCH 1/5] virDomainCheckpointDefParse: Don't extract unused domain type
We can extract './domain' directly and let the parser deal with the
type.
Signed-off-by: Peter Krempa
---
src/conf/checkpoint_conf.c | 12 +++-
1 file changed, 3 insertions(+), 9 deletions(-)
diff --git a/src/conf/checkpoint_conf.c b/src/conf/checkpoint_conf.c
index a8d18928de..33b6699be7 100644
--- a/src/conf/checkpoint_conf.c
+++ b/src/conf/checkpoint_conf.c
@@ -126,7 +126,6 @@ virDomainCheckpointDefParse(xmlXPathContextPtr ctxt,
virDomainCheckpointDefPtr ret = NULL;
size_t i;
int n;
-char *tmp;
g_autofree xmlNodePtr *nodes = NULL;
g_autoptr(virDomainCheckpointDef) def = NULL;
@@ -146,6 +145,8 @@ virDomainCheckpointDefParse(xmlXPathContextPtr ctxt,
def->parent.description = virXPathString("string(./description)", ctxt);
if (flags & VIR_DOMAIN_CHECKPOINT_PARSE_REDEFINE) {
+xmlNodePtr domainNode;
+
if (virXPathLongLong("string(./creationTime)", ctxt,
>parent.creationTime) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
@@ -155,17 +156,10 @@ virDomainCheckpointDefParse(xmlXPathContextPtr ctxt,
def->parent.parent_name = virXPathString("string(./parent/name)",
ctxt);
-if ((tmp = virXPathString("string(./domain/@type)", ctxt))) {
+if ((domainNode = virXPathNode("./domain", ctxt))) {
int domainflags = VIR_DOMAIN_DEF_PARSE_INACTIVE |
VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE;
-xmlNodePtr domainNode = virXPathNode("./domain", ctxt);
-VIR_FREE(tmp);
-if (!domainNode) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("missing domain in checkpoint"));
-return NULL;
-}
def->parent.dom = virDomainDefParseNode(ctxt->node->doc,
domainNode,
xmlopt, parseOpaque,
domainflags);
--
2.28.0
[PATCH 5/5] conf: checkpoint: Don't require when redefining checkpoints
The domain definition stored with a checkpoint isn't used currently
apart from matching disks when creating a new checkpoints.
As some users of the incremental backup API want to provide backups in
offline mode under their control (obviously while compying with our
documentation on how the on-disk state should be handled) and then want
to define the checkpoint for live use, supplying a sub-element
is overly complex and not actually needed by the code.
Relax the restriction when re-defining a checkpoint so that is
not necessary and add (alibistic) documentation saying that future
actions may not work if it's missing.
Signed-off-by: Peter Krempa
---
docs/formatcheckpoint.rst | 12 +---
src/conf/checkpoint_conf.c | 4
2 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/docs/formatcheckpoint.rst b/docs/formatcheckpoint.rst
index f159f2a7a3..ff3f1e8c00 100644
--- a/docs/formatcheckpoint.rst
+++ b/docs/formatcheckpoint.rst
@@ -1,3 +1,5 @@
+.. role:: since
+
Checkpoint XML format
=
@@ -103,12 +105,16 @@ The top-level ``domaincheckpoint`` element may contain
the following elements:
A readonly representation of the inactive `domain
configuration `__ at the time the checkpoint was created.
This element may be omitted for output brevity by supplying the
- ``VIR_DOMAIN_CHECKPOINT_XML_NO_DOMAIN`` flag, but the resulting XML is no
- longer viable for use with the ``VIR_DOMAIN_CHECKPOINT_CREATE_REDEFINE``
flag
- of ``virDomainCheckpointCreateXML()``. The domain will have
+ ``VIR_DOMAIN_CHECKPOINT_XML_NO_DOMAIN`` flag. The domain will have
security-sensitive information omitted unless the flag
``VIR_DOMAIN_CHECKPOINT_XML_SECURE`` is provided on a read-write connection.
+ ``virDomainCheckpointCreateXML()`` requires that the is present
+ when used with ``VIR_DOMAIN_CHECKPOINT_CREATE_REDEFINE``.
+ :since:`Since 7.0.0` the element can be omitted when redefining
+ a checkpoint, but hypervisors may not support certain operations if it's
+ missing.
+
Examples
diff --git a/src/conf/checkpoint_conf.c b/src/conf/checkpoint_conf.c
index 2071494d52..2df5e41495 100644
--- a/src/conf/checkpoint_conf.c
+++ b/src/conf/checkpoint_conf.c
@@ -165,10 +165,6 @@ virDomainCheckpointDefParse(xmlXPathContextPtr ctxt,
domainParseFlags);
if (!def->parent.dom)
return NULL;
-} else {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("missing domain in checkpoint redefine"));
-return NULL;
}
} else if (virDomainXMLOptionRunMomentPostParse(xmlopt, >parent) < 0)
{
return NULL;
--
2.28.0
[PATCH 3/5] virDomainCheckpointRedefineCommit: Don't check ABI of definition in checkpoint
Checking the definition ABI when redefining checkpoints doesn't make
much sense for the following reasons:
* the domain definition in the checkpoint is mostly unused (a relic
adopted from the snapshot code)
* can be very easily overriden by deleting the checkpoint metadata
before redefinition
Rather than complicating the logic when we'll be taking into account
that the domain definition may be missing, let's just remove the check.
Signed-off-by: Peter Krempa
---
src/conf/checkpoint_conf.c | 7 +--
src/conf/checkpoint_conf.h | 3 +--
src/qemu/qemu_checkpoint.c | 7 +++
src/test/test_driver.c | 2 +-
4 files changed, 6 insertions(+), 13 deletions(-)
diff --git a/src/conf/checkpoint_conf.c b/src/conf/checkpoint_conf.c
index 8744ac83e1..73fdb78e7a 100644
--- a/src/conf/checkpoint_conf.c
+++ b/src/conf/checkpoint_conf.c
@@ -545,8 +545,7 @@ virDomainCheckpointRedefinePrep(virDomainObjPtr vm,
virDomainMomentObjPtr
virDomainCheckpointRedefineCommit(virDomainObjPtr vm,
- virDomainCheckpointDefPtr *defptr,
- virDomainXMLOptionPtr xmlopt)
+ virDomainCheckpointDefPtr *defptr)
{
virDomainCheckpointDefPtr def = *defptr;
virDomainMomentObjPtr other = NULL;
@@ -556,10 +555,6 @@ virDomainCheckpointRedefineCommit(virDomainObjPtr vm,
other = virDomainCheckpointFindByName(vm->checkpoints, def->parent.name);
if (other) {
otherdef = virDomainCheckpointObjGetDef(other);
-if (!virDomainDefCheckABIStability(otherdef->parent.dom,
- def->parent.dom, xmlopt))
-return NULL;
-
/* Drop and rebuild the parent relationship, but keep all
* child relations by reusing chk. */
virDomainMomentDropParent(other);
diff --git a/src/conf/checkpoint_conf.h b/src/conf/checkpoint_conf.h
index 631f863151..4508f61b29 100644
--- a/src/conf/checkpoint_conf.h
+++ b/src/conf/checkpoint_conf.h
@@ -97,7 +97,6 @@ virDomainCheckpointRedefinePrep(virDomainObjPtr vm,
virDomainMomentObjPtr
virDomainCheckpointRedefineCommit(virDomainObjPtr vm,
- virDomainCheckpointDefPtr *defptr,
- virDomainXMLOptionPtr xmlopt);
+ virDomainCheckpointDefPtr *defptr);
VIR_ENUM_DECL(virDomainCheckpoint);
diff --git a/src/qemu/qemu_checkpoint.c b/src/qemu/qemu_checkpoint.c
index e8d18b2e02..1740cadabf 100644
--- a/src/qemu/qemu_checkpoint.c
+++ b/src/qemu/qemu_checkpoint.c
@@ -439,8 +439,7 @@ qemuCheckpointRedefineValidateBitmaps(virDomainObjPtr vm,
static virDomainMomentObjPtr
-qemuCheckpointRedefine(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
+qemuCheckpointRedefine(virDomainObjPtr vm,
virDomainCheckpointDefPtr *def,
bool *update_current,
bool validate_bitmaps)
@@ -452,7 +451,7 @@ qemuCheckpointRedefine(virQEMUDriverPtr driver,
qemuCheckpointRedefineValidateBitmaps(vm, *def) < 0)
return NULL;
-return virDomainCheckpointRedefineCommit(vm, def, driver->xmlopt);
+return virDomainCheckpointRedefineCommit(vm, def);
}
@@ -605,7 +604,7 @@ qemuCheckpointCreateXML(virDomainPtr domain,
return NULL;
if (redefine) {
-chk = qemuCheckpointRedefine(driver, vm, , _current,
validate_bitmaps);
+chk = qemuCheckpointRedefine(vm, , _current,
validate_bitmaps);
} else {
chk = qemuCheckpointCreate(driver, vm, );
}
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 136269de3d..29c4c86b1d 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -8991,7 +8991,7 @@ testDomainCheckpointCreateXML(virDomainPtr domain,
if (virDomainCheckpointRedefinePrep(vm, def, _current) < 0)
goto cleanup;
-if (!(chk = virDomainCheckpointRedefineCommit(vm, ,
privconn->xmlopt)))
+if (!(chk = virDomainCheckpointRedefineCommit(vm, )))
goto cleanup;
} else {
if (!(def->parent.dom = virDomainDefCopy(vm->def,
--
2.28.0
[PATCH 0/5] qemu: checkpoint: Don't require when redefining a checkpoint
I've got a request from the oVirt devs integrating incremental backup whether there's a simpler way to redefine checkpoints since they need to support cases when the VM isn't running. Since we don't really use the def we can stop requiring it. Peter Krempa (5): virDomainCheckpointDefParse: Don't extract unused domain type virDomainCheckpointDefParse: Use 'unsigned int' for flags virDomainCheckpointRedefineCommit: Don't check ABI of definition in checkpoint conf: checkpoint: Prepare internals for missing domain definition conf: checkpoint: Don't require when redefining checkpoints docs/formatcheckpoint.rst | 12 +++-- src/conf/checkpoint_conf.c | 60 ++--- src/conf/checkpoint_conf.h | 3 +- src/qemu/qemu_checkpoint.c | 7 ++- src/test/test_driver.c | 2 +- tests/qemudomaincheckpointxml2xmltest.c | 5 --- 6 files changed, 37 insertions(+), 52 deletions(-) -- 2.28.0
Re: [PATCH v2 0/6] add post parse pSeries mem align, when ABI_UPDATE
On Wed, 2020-12-02 at 09:14 -0300, Daniel Henrique Barboza wrote: > On 12/2/20 4:17 AM, Michal Privoznik wrote: > > On 11/18/20 8:58 PM, Daniel Henrique Barboza wrote: > > > 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 > > > > > Reviewed-by: Michal Privoznik > > Thanks for the review! > > Andrea/Peter, do you want to take a look again to see if there's > something that I missed? Yeah, sorry for not being very responsive, and thanks a lot Michal for picking up the slack! I'll try to give it at least a quick look by the end of the day. -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH 3/7] docs: Fix link for virConnectGetStoragePoolCapabilities
On a Wednesday in 2020, John Ferlan wrote: The API is in the storage family not the domain family Signed-off-by: John Ferlan --- docs/formatstoragecaps.html.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH 2/7] util: Resolve resource leak in virExec error path
On a Wednesday in 2020, John Ferlan wrote:
On error, the @pidfilefd was not released
Found by Coverity
The pidfile code was introduced by:
commit d146105f1e4a9e0ab179f0b78c070ea38b9d5334
Author: Michal Prívozník
CommitDate: 2020-03-24 15:44:23 +0100
virCommand: Actually acquire pidfile instead of just writing it
further changed by:
commit be00118d5d9a3afb41e0edcddec823dff63a7ae1
Author: Marc-André Lureau
CommitDate: 2020-03-25 09:04:49 +0100
util: keep the pidfile locked
then some code movement added more error paths after the pidfile code:
commit 0bb796bda31103ebf54eefc11c471586c87b95fd
Author: Daniel Henrique Barboza
CommitDate: 2020-10-02 14:32:57 -0300
vircommand.c: write child pidfile before process tuning in virExec()
IIUC the point of leaking the pidfilefd is to make sure the child holds a
lock on the pidfile - so strictly speaking we should close it in all
code paths that don't end up in a successful execv. Practically,
this already happens because the fork_error calls _exit.
Jano
Signed-off-by: John Ferlan
---
src/util/vircommand.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/util/vircommand.c b/src/util/vircommand.c
index e47dd6b932..1716225aeb 100644
--- a/src/util/vircommand.c
+++ b/src/util/vircommand.c
@@ -792,12 +792,14 @@ virExec(virCommandPtr cmd)
if (virSetInherit(pidfilefd, true) < 0) {
virReportSystemError(errno, "%s",
_("Cannot disable close-on-exec flag"));
+virPidFileReleasePath(cmd->pidfile, pidfilefd);
goto fork_error;
}
c = '1';
if (safewrite(pipesync[1], , sizeof(c)) != sizeof(c)) {
virReportSystemError(errno, "%s", _("Unable to notify child
process"));
+virPidFileReleasePath(cmd->pidfile, pidfilefd);
goto fork_error;
}
VIR_FORCE_CLOSE(pipesync[0]);
--
2.28.0
signature.asc
Description: PGP signature
Re: [PATCH v2] qemu: Don't cache NUMA caps
On 12/2/20 5:26 AM, 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).
Because of the caching we might not be reporting the correct
runtime info in 'virsh capabilities'.
The NUMA caps are used in three places:
1) 'virsh capabilities'
2) domain startup, when parsing numad reply
3) parsing domain private data XML
In cases 2) and 3) we need NUMA caps to construct list of
physical CPUs that belong to NUMA nodes from numad reply. And
while this may seem static, it's not really because of possible
CPU hotplug on physical host.
There are two possible approaches:
1) build a validation mechanism that would invalidate the
cached NUMA caps, or
2) drop the caching and construct NUMA caps from scratch on
each use.
In this commit, the latter approach is implemented, because it's
easier.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1819058
Fixes: 1a1d848694f6c2f1d98a371124928375bc3bb4a3
Signed-off-by: Michal Privoznik
---
Reviewed-by: Daniel Henrique Barboza
v2 of:
https://www.redhat.com/archives/libvir-list/2020-December/msg00023.html
diff to v1:
- instead of fixing cached capabilities, drop caching completely
src/qemu/qemu_conf.c| 23 +--
src/qemu/qemu_conf.h| 6 --
src/qemu/qemu_domain.c | 7 +++
src/qemu/qemu_driver.c | 1 -
src/qemu/qemu_process.c | 7 +++
5 files changed, 7 insertions(+), 37 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index cbdde0c0dc..0ae86e5468 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -1289,27 +1289,6 @@ virQEMUDriverCreateXMLConf(virQEMUDriverPtr driver,
}
-virCapsHostNUMAPtr
-virQEMUDriverGetHostNUMACaps(virQEMUDriverPtr driver)
-{
-virCapsHostNUMAPtr hostnuma;
-
-qemuDriverLock(driver);
-
-if (!driver->hostnuma)
-driver->hostnuma = virCapabilitiesHostNUMANewHost();
-
-hostnuma = driver->hostnuma;
-
-qemuDriverUnlock(driver);
-
-if (hostnuma)
-virCapabilitiesHostNUMARef(hostnuma);
-
-return hostnuma;
-}
-
-
virCPUDefPtr
virQEMUDriverGetHostCPU(virQEMUDriverPtr driver)
{
@@ -1381,7 +1360,7 @@ virCapsPtr
virQEMUDriverCreateCapabilities(virQEMUDriverPtr driver)
"DOI \"%s\"", model, doi);
}
-caps->host.numa = virQEMUDriverGetHostNUMACaps(driver);
+caps->host.numa = virCapabilitiesHostNUMANewHost();
caps->host.cpu = virQEMUDriverGetHostCPU(driver);
return g_steal_pointer();
}
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index 411d08db36..7025b5222e 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -270,11 +270,6 @@ struct _virQEMUDriver {
*/
virCapsPtr caps;
-/* Lazy initialized on first use, immutable thereafter.
- * Require lock to get the pointer & do optional initialization
- */
-virCapsHostNUMAPtr hostnuma;
-
/* Lazy initialized on first use, immutable thereafter.
* Require lock to get the pointer & do optional initialization
*/
@@ -337,7 +332,6 @@ virQEMUDriverConfigSetDefaults(virQEMUDriverConfigPtr cfg);
virQEMUDriverConfigPtr virQEMUDriverGetConfig(virQEMUDriverPtr driver);
-virCapsHostNUMAPtr virQEMUDriverGetHostNUMACaps(virQEMUDriverPtr driver);
virCPUDefPtr virQEMUDriverGetHostCPU(virQEMUDriverPtr driver);
virCapsPtr virQEMUDriverCreateCapabilities(virQEMUDriverPtr driver);
virCapsPtr virQEMUDriverGetCapabilities(virQEMUDriverPtr driver,
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 663c0af867..8e9c0224e4 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -2498,8 +2498,7 @@ qemuDomainObjPrivateXMLParseVcpu(xmlNodePtr node,
static int
qemuDomainObjPrivateXMLParseAutomaticPlacement(xmlXPathContextPtr ctxt,
- qemuDomainObjPrivatePtr priv,
- virQEMUDriverPtr driver)
+ qemuDomainObjPrivatePtr priv)
{
g_autoptr(virCapsHostNUMA) caps = NULL;
g_autofree char *nodeset = NULL;
@@ -2513,7 +2512,7 @@
qemuDomainObjPrivateXMLParseAutomaticPlacement(xmlXPathContextPtr ctxt,
if (!nodeset && !cpuset)
return 0;
-if (!(caps = virQEMUDriverGetHostNUMACaps(driver)))
+if (!(caps = virCapabilitiesHostNUMANewHost()))
return -1;
/* Figure out how big the nodeset bitmap needs to be.
@@ -3114,7 +3113,7 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt,
}
VIR_FREE(nodes);
-if (qemuDomainObjPrivateXMLParseAutomaticPlacement(ctxt, priv, driver) < 0)
Re: [PATCH v2 1/2] qemu: support append param on live attaching file chardev
On 12/2/20 9:25 AM, Nikolay Shirokovskiy wrote: Currently it is simply ignored. Signed-off-by: Nikolay Shirokovskiy --- Reviewed-by: Daniel Henrique Barboza src/qemu/qemu_monitor_json.c | 4 1 file changed, 4 insertions(+) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 47ee1ff..ff03a5a 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -7497,6 +7497,10 @@ qemuMonitorJSONAttachCharDevCommand(const char *chrID, backend_type = "file"; if (virJSONValueObjectAppendString(data, "out", chr->data.file.path) < 0) goto cleanup; +if (virJSONValueObjectAdd(data, + "T:append", chr->data.file.append, + NULL) < 0) +goto cleanup; break; case VIR_DOMAIN_CHR_TYPE_DEV:
Re: [PATCH v2 2/2] qemu: support logfile on live attaching chardev
On 12/2/20 9:25 AM, Nikolay Shirokovskiy wrote: Currently it is simply ignored. Signed-off-by: Nikolay Shirokovskiy --- Reviewed-by: Daniel Henrique Barboza src/qemu/qemu_monitor_json.c | 7 +++ 1 file changed, 7 insertions(+) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index ff03a5a..0745717 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -7611,6 +7611,13 @@ qemuMonitorJSONAttachCharDevCommand(const char *chrID, goto cleanup; } +if (chr->logfile && +virJSONValueObjectAdd(data, + "s:logfile", chr->logfile, + "T:logappend", chr->logappend, + NULL) < 0) +goto cleanup; + if (virJSONValueObjectAppendString(backend, "type", backend_type) < 0 || virJSONValueObjectAppend(backend, "data", data) < 0) goto cleanup;
Re: [PATCH 00/18] qapi/qom: QAPIfy object-add
On Wed, Dec 02, 2020 at 02:26:44PM +0100, Paolo Bonzini wrote:
> On 02/12/20 13:51, Eduardo Habkost wrote:
> > > > I'm liking the direction this is taking. However, I would still
> > > > like to have a clearer and feasible plan that would work for
> > > > -device, -machine, and -cpu.
> > >
> > > -cpu is not a problem since it's generally created with a static
> > > configuration (now done with global properties, in the future it could be
> > > a
> > > struct).
> >
> > It is a problem if it requires manually converting existing code
> > defining CPU properties and we don't have a transition plan.
>
> We do not have to convert everything _if_ for some objects there are good
> reasons to do programmatically-generated properties. CPUs might be one of
> those cases (or if we decide to convert them, they might endure some more
> code duplication than other devices because they have so many properties).
OK, we just need to agree on what the transition will look like
when we do it. I think we should put as much care into
transition/glue infrastructure as we put into the new
infrastructure.
>
> > Would a -device conversion also involve non-user-creatable
> > devices, or would we keep existing internal usage of QOM
> > properties?
> >
> > Even if it's just for user-creatable devices, getting rid of QOM
> > property usage in devices sounds like a very ambitious goal. I'd
> > like us to have a good transition plan, in addition to declaring
> > what's our ideal end goal.
>
> For user-creatable objects Kevin is doing work in lockstep on all classes;
> but once we have the infrastructure for QAPI object configuration schemas we
> can proceed in the other direction and operate on one device at a time.
>
> With some handwaving, something like (see create_unimplemented_device)
>
> DeviceState *dev = qdev_new(TYPE_UNIMPLEMENTED_DEVICE);
>
> qdev_prop_set_string(dev, "name", name);
> qdev_prop_set_uint64(dev, "size", size);
> sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), _fatal);
>
> might become something like
>
> { 'object': 'unimplemented-device',
> 'config': {
> 'name': 'str',
> 'size': 'size'
> },
> }
>
> DeviceState *dev = qapi_Unimplemented_new(&(
> (UnimplementedDeviceOptions) {
> .name = name,
> .size = size
> }, _fatal);
> object_unref(dev);
>
> (i.e. all typesafe!) and the underlying code would be something like
[...]
>
Looks nice as end goal. Then, these are a few questions I would
have about the transition plan:
Would it require changing both device implementation and device
users in lockstep? Should we have a compatibility layer to allow
existing qdev_new()+qdev_prop_set_*() code to keep working after
the devices are converted to the new system? If not, why not?
If we add a compatibility layer, is the end goal to convert all
existing qdev_new() users to the new system? If yes, why? If
not, why not?
What about subclasses? Would base classes need to be converted
in lockstep with all subclasses? How would the transition
process of (e.g.) PCI devices look like?
--
Eduardo
Re: [PATCH 1/7] util: Fix memory leak in virNetDevOpenvswitchInterfaceGetMaster
On a Wednesday in 2020, John Ferlan wrote: Since 032548c4 @cmd was never autofree'd. Perhaps as a result of VIR_AUTOPTR type changes occurring at roughly the same time so the copy pasta missed this. Found by Coverity. Signed-off-by: John Ferlan --- src/util/virnetdevopenvswitch.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH 00/18] qapi/qom: QAPIfy object-add
On 02/12/20 13:51, Eduardo Habkost wrote:
I'm liking the direction this is taking. However, I would still
like to have a clearer and feasible plan that would work for
-device, -machine, and -cpu.
-cpu is not a problem since it's generally created with a static
configuration (now done with global properties, in the future it could be a
struct).
It is a problem if it requires manually converting existing code
defining CPU properties and we don't have a transition plan.
We do not have to convert everything _if_ for some objects there are
good reasons to do programmatically-generated properties. CPUs might be
one of those cases (or if we decide to convert them, they might endure
some more code duplication than other devices because they have so many
properties).
Would a -device conversion also involve non-user-creatable
devices, or would we keep existing internal usage of QOM
properties?
Even if it's just for user-creatable devices, getting rid of QOM
property usage in devices sounds like a very ambitious goal. I'd
like us to have a good transition plan, in addition to declaring
what's our ideal end goal.
For user-creatable objects Kevin is doing work in lockstep on all
classes; but once we have the infrastructure for QAPI object
configuration schemas we can proceed in the other direction and operate
on one device at a time.
With some handwaving, something like (see create_unimplemented_device)
DeviceState *dev = qdev_new(TYPE_UNIMPLEMENTED_DEVICE);
qdev_prop_set_string(dev, "name", name);
qdev_prop_set_uint64(dev, "size", size);
sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), _fatal);
might become something like
{ 'object': 'unimplemented-device',
'config': {
'name': 'str',
'size': 'size'
},
}
DeviceState *dev = qapi_Unimplemented_new(&(
(UnimplementedDeviceOptions) {
.name = name,
.size = size
}, _fatal);
object_unref(dev);
(i.e. all typesafe!) and the underlying code would be something like
void (*init_properties)(Object *obj, Visitor *v, Error **errp);
...
// QAPI generated constructor
UnimplementedDevice *qapi_Unimplemented_new(
UnimplementedDeviceOptions *opt, Error **errp)
{
Object *obj = object_new(TYPE_UNIMPLEMENTED_DEVICE);
if (!qapi_Unimplemented_init_properties(obj, opt, errp)) {
object_unref(obj);
return NULL;
}
return obj;
}
// QAPI generated Visitor->struct adapter
bool qapi_Unimplemented_init_properties(
Object *obj, Visitor *v, Error **errp)
{
UnimplementedDeviceOptions opt;
Error *local_err = NULL;
visit_type_UnimplementedDeviceOptions(v, NULL,
, _err);
if (local_err) {
error_propagate(errp, local_err);
return false;
}
unimplemented_init_properties(UNIMPLEMENTED_DEVICE(obj),
, _err);
if (local_err) {
error_propagate(errp, local_err);
return false;
}
return true;
}
// Hand-written (?) initializer:
void unimplemented_init_properties(Object *obj,
UnimplementedDeviceOptions *opt, Error **errp)
{
obj->name = name;
obj->size = size;
device_realize(obj, errp);
}
// class_init code:
oc->init_properties = qapi_Unimplemented_init_properties;
and all read-only-after-realize QOM properties could be made *really*
read-only.
Paolo
Re: [PATCH 00/18] qapi/qom: QAPIfy object-add
On Wed, Dec 02, 2020 at 10:30:11AM +0100, Paolo Bonzini wrote:
> On 01/12/20 23:08, Eduardo Habkost wrote:
> > > 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).
>
> Yes.
>
> > > 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.
>
> Yeah, "all about exposing properties" includes internal usage. And you're
> right that some "phase 3" monitor commands do work at the property level
> (mostly "info qtree", but also "qom-get" because there are some cases of
> public run-time properties).
I still disagree on the "all about" part even for internal usage.
But this shouldn't really matter for this discussion, I guess.
>
> > I'm liking the direction this is taking. However, I would still
> > like to have a clearer and feasible plan that would work for
> > -device, -machine, and -cpu.
>
> -cpu is not a problem since it's generally created with a static
> configuration (now done with global properties, in the future it could be a
> struct).
It is a problem if it requires manually converting existing code
defining CPU properties and we don't have a transition plan.
>
> -machine and -device in principle could be done the same way as -object,
> just through a different registry (_not_ a huge struct; that's an acceptable
> stopgap for -object but that's it). The static aka field properties would
> remain as read-only, with defaults moved to instance_init or realize. But
> there would be again "triplication" with a trivial conversion:
>
> 1) in the QAPI schema, e.g. 'num_queues': 'int16'
>
> 2) in the struct, "int16_t num_queues;"
>
> 3) in the realize function,
>
> s->num_queues = cfg->has_num_queues ? cfg->num_queues : 8;
>
> So having a mechanism for defaults in the QAPI schema would be good. Maybe
> 'num_queues': { 'type': 'int16', 'default': '8' }?
>
Would a -device conversion also involve non-user-creatable
devices, or would we keep existing internal usage of QOM
properties?
Even if it's just for user-creatable devices, getting rid of QOM
property usage in devices sounds like a very ambitious goal. I'd
like us to have a good transition plan, in addition to declaring
what's our ideal end goal.
> I also need to review more the part of this code with respect to the
> application of global properties. I wonder if there are visitor tricks that
> we can do, so that global properties keep working but correspond to QAPI
> fields instead of QOM properties.
>
> Paolo
>
--
Eduardo
[PATCH v2] src: use singular form instead of plural, for guest disk info
Existing practice with the filesystem fields reported for the virDomainGetGuestInfo API is to use the singular form for field names. Ensure the disk info follows this practice. Fixes commit 05a75ca2ce743bc0bb119fb8d532ff84646fafa3 Author: Marc-André Lureau Date: Fri Nov 20 22:09:46 2020 +0400 domain: add disk informations to virDomainGetGuestInfo commit 0cb2d9f05d00497a715352f6ea28cf8fb6921731 Author: Marc-André Lureau Date: Fri Nov 20 22:09:47 2020 +0400 qemu_driver: report guest disk informations commit 172b8304352b1945e328394e61290a24446280dd Author: Marc-André Lureau Date: Fri Nov 20 22:09:48 2020 +0400 virsh: add --disk informations to guestinfo command Signed-off-by: Daniel P. Berrangé --- In v2: also update docs and virsh docs/manpages/virsh.rst | 20 ++-- src/libvirt-domain.c| 14 +++--- src/qemu/qemu_driver.c | 14 +++--- tools/virsh-domain.c| 6 +++--- 4 files changed, 27 insertions(+), 27 deletions(-) diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index 9ef6b68422..aa54bc21ef 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -2679,7 +2679,7 @@ guestinfo :: guestinfo domain [--user] [--os] [--timezone] [--hostname] [--filesystem] - [--disks] + [--disk] Print information about the guest from the point of view of the guest agent. Note that this command requires a guest agent to be configured and running in @@ -2690,7 +2690,7 @@ are supported by the guest agent. You can limit the types of information that are returned by specifying one or more flags. If a requested information type is not supported, the processes will provide an exit code of 1. Available information types flags are *--user*, *--os*, -*--timezone*, *--hostname*, *--filesystem* and *--disks*. +*--timezone*, *--hostname*, *--filesystem* and *--disk*. Note that depending on the hypervisor type and the version of the guest agent running within the domain, not all of the following information may be @@ -2747,15 +2747,15 @@ returned: * ``fs..disk..serial`` - the serial number of disk * ``fs..disk..device`` - the device node of disk -*--disks* returns: +*--disk* returns: -* ``disks.count`` - the number of disks defined on this domain -* ``disks..name`` - device node (Linux) or device UNC (Windows) -* ``disks..partition`` - whether this is a partition or disk -* ``disks..dependencies.count`` - the number of device dependencies -* ``disks..dependencies..name`` - a dependency name -* ``disks..alias`` - the device alias of the disk (e.g. sda) -* ``disks..guest_alias`` - optional alias assigned to the disk +* ``disk.count`` - the number of disks defined on this domain +* ``disk..name`` - device node (Linux) or device UNC (Windows) +* ``disk..partition`` - whether this is a partition or disk +* ``disk..dependency.count`` - the number of device dependencies +* ``disk..dependency..name`` - a dependency name +* ``disk..alias`` - the device alias of the disk (e.g. sda) +* ``disk..guest_alias`` - optional alias assigned to the disk guestvcpus diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 35e95e5395..f5cd43ecea 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -12345,17 +12345,17 @@ virDomainSetVcpu(virDomainPtr domain, * Returns information about the disks within the domain. The typed * parameter keys are in this format: * - * "disks.count" - the number of disks defined on this domain + * "disk.count" - the number of disks defined on this domain * as an unsigned int - * "disks..name" - device node (Linux) or device UNC (Windows) - * "disks..partition" - whether this is a partition or disk - * "disks..dependencies.count" - the number of device dependencies + * "disk..name" - device node (Linux) or device UNC (Windows) + * "disk..partition" - whether this is a partition or disk + * "disk..dependency.count" - the number of device dependencies * e.g. for LVs of the LVM this will * hold the list of PVs, for LUKS encrypted volume this will * contain the disk where the volume is placed. (Linux) - * "disks..dependencies..name" - a dependency - * "disks..alias" - the device alias of the disk (e.g. sda) - * "disks..guest_alias" - optional alias assigned to the disk, on Linux + * "disk..dependency..name" - a dependency + * "disk..alias" - the device alias of the disk (e.g. sda) + * "disk..guest_alias" - optional alias assigned to the disk, on Linux * this is a name assigned by device mapper * * VIR_DOMAIN_GUEST_INFO_HOSTNAME: diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 8eaa3ce68f..548df6ae68 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -19876,20 +19876,20 @@ qemuAgentDiskInfoFormatParams(qemuAgentDiskInfoPtr *info,
[PATCH 7/7] qemu: Fix some issues in virQEMUDriverConfigLoadNVRAMEntry
Commit c4f4e195 fixed a double free, but if the code returns before
we realloc the list and virFirmwareFreeList was called with cfg->nfirmwares
> 0 (e.g. during virQEMUDriverConfigDispose), then it would be rather
disasterous. So let's reinitialze that too to indicate the list is empty.
Coverity pointed out that using nvram[0] as a guard to reallocating the
list could lead to a possible NULL deref. While nvram[0] may always be
true in this case, if it wasn't then the subsequent for loop would fail.
Just reallocate always regardless - even if nfirmwares == 0 as
virFirmwareFreeList will free it for us anyway.
Signed-off-by: John Ferlan
---
src/qemu/qemu_conf.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index cbdde0c0dc..690cfd39f9 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -835,6 +835,7 @@ virQEMUDriverConfigLoadNVRAMEntry(virQEMUDriverConfigPtr
cfg,
virFirmwareFreeList(cfg->firmwares, cfg->nfirmwares);
cfg->firmwares = NULL;
+cfg->nfirmwares = 0;
if (qemuFirmwareFetchConfigs(, privileged) < 0)
return -1;
@@ -843,13 +844,11 @@ virQEMUDriverConfigLoadNVRAMEntry(virQEMUDriverConfigPtr
cfg,
VIR_WARN("Obsolete nvram variable is set while firmware metadata "
"files found. Note that the nvram config file variable is
"
"going to be ignored.");
-cfg->nfirmwares = 0;
return 0;
}
cfg->nfirmwares = virStringListLength((const char *const *)nvram);
-if (nvram[0])
-cfg->firmwares = g_new0(virFirmwarePtr, cfg->nfirmwares);
+cfg->firmwares = g_new0(virFirmwarePtr, cfg->nfirmwares);
for (i = 0; nvram[i] != NULL; i++) {
cfg->firmwares[i] = g_new0(virFirmware, 1);
--
2.28.0
[PATCH 3/7] docs: Fix link for virConnectGetStoragePoolCapabilities
The API is in the storage family not the domain family Signed-off-by: John Ferlan --- docs/formatstoragecaps.html.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/formatstoragecaps.html.in b/docs/formatstoragecaps.html.in index 900303aef7..a9ecc371fa 100644 --- a/docs/formatstoragecaps.html.in +++ b/docs/formatstoragecaps.html.in @@ -20,7 +20,7 @@ (Since 5.2.0): -virConnectGetStoragePoolCapabilities +virConnectGetStoragePoolCapabilities The root element that emulator capability XML document starts with is -- 2.28.0
[PATCH 5/7] logging: Resolve mem leak in virLogDaemonPreExecRestart
Initialize and free @magic since virJSONValueObjectAppendString
does not free it for us eventually.
Signed-off-by: John Ferlan
---
src/logging/log_daemon.c | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/src/logging/log_daemon.c b/src/logging/log_daemon.c
index be93c63eb5..6b8f3b6fe5 100644
--- a/src/logging/log_daemon.c
+++ b/src/logging/log_daemon.c
@@ -508,7 +508,7 @@ virLogDaemonPreExecRestart(const char *state_file,
virJSONValuePtr child;
char *state = NULL;
virJSONValuePtr object = virJSONValueNewObject();
-char *magic;
+char *magic = NULL;
VIR_DEBUG("Running pre-restart exec");
@@ -523,10 +523,8 @@ virLogDaemonPreExecRestart(const char *state_file,
if (!(magic = virLogDaemonGetExecRestartMagic()))
goto cleanup;
-if (virJSONValueObjectAppendString(object, "magic", magic) < 0) {
-VIR_FREE(magic);
+if (virJSONValueObjectAppendString(object, "magic", magic) < 0)
goto cleanup;
-}
if (!(child = virLogHandlerPreExecRestart(logDaemon->handler)))
goto cleanup;
@@ -559,6 +557,7 @@ virLogDaemonPreExecRestart(const char *state_file,
abort(); /* This should be impossible to reach */
cleanup:
+VIR_FREE(magic);
VIR_FREE(state);
virJSONValueFree(object);
return -1;
--
2.28.0
[PATCH 1/7] util: Fix memory leak in virNetDevOpenvswitchInterfaceGetMaster
Since 032548c4 @cmd was never autofree'd. Perhaps as a result of
VIR_AUTOPTR type changes occurring at roughly the same time so the
copy pasta missed this.
Found by Coverity.
Signed-off-by: John Ferlan
---
src/util/virnetdevopenvswitch.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c
index 7452527f49..d380b0cf22 100644
--- a/src/util/virnetdevopenvswitch.c
+++ b/src/util/virnetdevopenvswitch.c
@@ -428,7 +428,7 @@ virNetDevOpenvswitchInterfaceStats(const char *ifname,
int
virNetDevOpenvswitchInterfaceGetMaster(const char *ifname, char **master)
{
-virCommandPtr cmd = virNetDevOpenvswitchCreateCmd();
+g_autoptr(virCommand) cmd = virNetDevOpenvswitchCreateCmd();
int exitstatus;
*master = NULL;
--
2.28.0
[PATCH 2/7] util: Resolve resource leak in virExec error path
On error, the @pidfilefd was not released
Found by Coverity
Signed-off-by: John Ferlan
---
src/util/vircommand.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/util/vircommand.c b/src/util/vircommand.c
index e47dd6b932..1716225aeb 100644
--- a/src/util/vircommand.c
+++ b/src/util/vircommand.c
@@ -792,12 +792,14 @@ virExec(virCommandPtr cmd)
if (virSetInherit(pidfilefd, true) < 0) {
virReportSystemError(errno, "%s",
_("Cannot disable close-on-exec flag"));
+virPidFileReleasePath(cmd->pidfile, pidfilefd);
goto fork_error;
}
c = '1';
if (safewrite(pipesync[1], , sizeof(c)) != sizeof(c)) {
virReportSystemError(errno, "%s", _("Unable to notify child
process"));
+virPidFileReleasePath(cmd->pidfile, pidfilefd);
goto fork_error;
}
VIR_FORCE_CLOSE(pipesync[0]);
--
2.28.0
Re: [PATCH 00/18] qapi/qom: QAPIfy object-add
On 02/12/20 11:27, Kevin Wolf wrote: Declaring read-only QOM properties is trivial. Trivial sounds like it's something the computer should be doing. Possibly, but not necessarily. There's always a cost to automatic code generation. If things are _too_ trivial and easy to get right, the cost of automatic code generation exceeds the cost of doing things by hand. You pretty much proved that ucc->complete is hard enough to get right, that it benefits from code generation. But I am a bit more conservative about extending that to the rest of QOM. I'm just worried about having yet another incomplete transition. Would you really feel at home in a QEMU without incomplete transitions? One can always dream. :) But since you had already posted RFC patches a while ago to address this, I considered it solved. Yup, I posted the non-RFC today. 1. This series (mostly for documentation and introspection) 2. -object and HMP object_add (so that we get QAPI's validation, and to make sure that forgetting to update the schema means that the new options just doesn't work) 3. Create a separate 'object' entity in QAPI, generate ucc->complete implementations that call a create function in the C source code with type-specific parameters (like QMP command handlers) If we agree on all of these that's already a pretty good place to be in. The decreasing length of the replies to the thread suggests that we are. I wouldn't mind an example of (3) that is hand-coded and may only work for one object type (even a simple one such as iothread), to show what the generated QAPI code would look like. It's not a blocker as far as I am concerned, but it would be nice. More important, Markus and John should agree with the plan before (1) is committed. That may also require the aforementioned example, but it's up to them. What's still open: Should QAPI cover run-time properties, too? Should run-time properties even exist at all in the final state? What is the exact QAPI representation of everything? (The last one includes that I need to have a closer look at how QAPI can best be taught about inheritance.) Run-time properties will exist but they will probably be cut down substantially, and most of them will be read-only (they already are as far as external code is concerned, i.e. they have a setter but qom-set always fails because it's called too late). Paolo
[PATCH 6/7] locking: Resolve mem leak in virLockDaemonPreExecRestart
Initialize and free @magic since virJSONValueObjectAppendString
does not free it for us eventually.
Signed-off-by: John Ferlan
---
src/locking/lock_daemon.c | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c
index 57c7fb088f..851e9fc6f0 100644
--- a/src/locking/lock_daemon.c
+++ b/src/locking/lock_daemon.c
@@ -700,7 +700,7 @@ virLockDaemonPreExecRestart(const char *state_file,
virJSONValuePtr child;
char *state = NULL;
virJSONValuePtr object = virJSONValueNewObject();
-char *magic;
+char *magic = NULL;
virHashKeyValuePairPtr pairs = NULL, tmp;
virJSONValuePtr lockspaces;
@@ -748,10 +748,8 @@ virLockDaemonPreExecRestart(const char *state_file,
if (!(magic = virLockDaemonGetExecRestartMagic()))
goto cleanup;
-if (virJSONValueObjectAppendString(object, "magic", magic) < 0) {
-VIR_FREE(magic);
+if (virJSONValueObjectAppendString(object, "magic", magic) < 0)
goto cleanup;
-}
if (!(state = virJSONValueToString(object, true)))
goto cleanup;
@@ -775,6 +773,7 @@ virLockDaemonPreExecRestart(const char *state_file,
abort(); /* This should be impossible to reach */
cleanup:
+VIR_FREE(magic);
VIR_FREE(pairs);
VIR_FREE(state);
virJSONValueFree(object);
--
2.28.0
[PATCH 4/7] qemu: Fix resource leak and coding error in qemuDomainGetIOThreadsMon
If qemuDomainGetIOThreadsMon fails because qemuDomainObjExitMonitor
fails, then a -1 is returned which overwrites @niothreads causing a
memory leak. Let's pass @niothreads instead.
Found by Coverity.
Signed-off-by: John Ferlan
---
src/qemu/qemu_driver.c | 19 +--
1 file changed, 9 insertions(+), 10 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 8eaa3ce68f..870159de47 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4972,17 +4972,16 @@ qemuDomainGetMaxVcpus(virDomainPtr dom)
static int
qemuDomainGetIOThreadsMon(virQEMUDriverPtr driver,
virDomainObjPtr vm,
- qemuMonitorIOThreadInfoPtr **iothreads)
+ qemuMonitorIOThreadInfoPtr **iothreads,
+ int *niothreads)
{
qemuDomainObjPrivatePtr priv = vm->privateData;
-int niothreads = 0;
qemuDomainObjEnterMonitor(driver, vm);
-niothreads = qemuMonitorGetIOThreads(priv->mon, iothreads);
-if (qemuDomainObjExitMonitor(driver, vm) < 0 || niothreads < 0)
+*niothreads = qemuMonitorGetIOThreads(priv->mon, iothreads);
+if (qemuDomainObjExitMonitor(driver, vm) < 0)
return -1;
-
-return niothreads;
+return 0;
}
@@ -5014,7 +5013,7 @@ qemuDomainGetIOThreadsLive(virQEMUDriverPtr driver,
goto endjob;
}
-if ((niothreads = qemuDomainGetIOThreadsMon(driver, vm, )) < 0)
+if (qemuDomainGetIOThreadsMon(driver, vm, , ) < 0)
goto endjob;
/* Nothing to do */
@@ -18507,7 +18506,7 @@ qemuDomainGetStatsIOThread(virQEMUDriverPtr driver,
qemuDomainObjPrivatePtr priv = dom->privateData;
size_t i;
qemuMonitorIOThreadInfoPtr *iothreads = NULL;
-int niothreads;
+int niothreads = 0;
int ret = -1;
if (!HAVE_JOB(privflags) || !virDomainObjIsActive(dom))
@@ -18516,8 +18515,8 @@ qemuDomainGetStatsIOThread(virQEMUDriverPtr driver,
if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD))
return 0;
-if ((niothreads = qemuDomainGetIOThreadsMon(driver, dom, )) < 0)
-return -1;
+if (qemuDomainGetIOThreadsMon(driver, dom, , ) < 0)
+goto cleanup;
/* qemuDomainGetIOThreadsMon returns a NULL-terminated list, so we must
free
* it even if it returns 0 */
--
2.28.0
[PATCH 0/7] A few Coverity related issues
Fix some of the issues that have built up. Also a docs link fix that I tripped across at some point in time. Again as a reminder, I no longer have push access ;-) John Ferlan (7): util: Fix memory leak in virNetDevOpenvswitchInterfaceGetMaster util: Resolve resource leak in virExec error path docs: Fix link for virConnectGetStoragePoolCapabilities qemu: Fix resource leak and coding error in qemuDomainGetIOThreadsMon logging: Resolve mem leak in virLogDaemonPreExecRestart locking: Resolve mem leak in virLockDaemonPreExecRestart qemu: Fix some issues in virQEMUDriverConfigLoadNVRAMEntry docs/formatstoragecaps.html.in | 2 +- src/locking/lock_daemon.c | 7 +++ src/logging/log_daemon.c| 7 +++ src/qemu/qemu_conf.c| 5 ++--- src/qemu/qemu_driver.c | 19 +-- src/util/vircommand.c | 2 ++ src/util/virnetdevopenvswitch.c | 2 +- 7 files changed, 21 insertions(+), 23 deletions(-) -- 2.28.0
Re: [PATCH 00/18] qapi/qom: QAPIfy object-add
On 02/12/20 11:38, Kevin Wolf wrote: Am 02.12.2020 um 10:30 hat Paolo Bonzini geschrieben: On 01/12/20 23:08, Eduardo Habkost wrote: 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). Yes. Are internal uses mostly just right after object creation, or do we make a lot of use of them during runtime? Not "a lot" but enough to be nasty. There are a couple uses of "well-known" QOM properties (e.g. to access the RTC time or the balloon stats), plus "info qtree". Paolo
Re: [libvirt PATCH] src: use singular form instead of plural, for guest disk info
On Wed, Dec 02, 2020 at 12:12:21 +, Daniel Berrange wrote: > Existing practice with the filesystem fields reported for the > virDomainGetGuestInfo API is to use the singular form for > field names. Ensure the disk info follows this practice. > > Fixes > > commit 05a75ca2ce743bc0bb119fb8d532ff84646fafa3 > Author: Marc-André Lureau > Date: Fri Nov 20 22:09:46 2020 +0400 > > domain: add disk informations to virDomainGetGuestInfo > > commit 0cb2d9f05d00497a715352f6ea28cf8fb6921731 > Author: Marc-André Lureau > Date: Fri Nov 20 22:09:47 2020 +0400 > > qemu_driver: report guest disk informations > > Signed-off-by: Daniel P. Berrangé > --- > src/libvirt-domain.c | 14 +++--- > src/qemu/qemu_driver.c | 14 +++--- > 2 files changed, 14 insertions(+), 14 deletions(-) Reviewed-by: Peter Krempa
[PATCH 09/11] virDomainDiskByName: Remove ternary operator
Signed-off-by: Peter Krempa
---
src/conf/domain_conf.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 49b68c2c68..6922ebf407 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -17649,7 +17649,11 @@ virDomainDiskByName(virDomainDefPtr def,
bool allow_ambiguous)
{
int idx = virDomainDiskIndexByName(def, name, allow_ambiguous);
-return idx < 0 ? NULL : def->disks[idx];
+
+if (idx < 0)
+return NULL;
+
+return def->disks[idx];
}
--
2.28.0
[PATCH 06/11] virDomainCheckpointAlignDisks: Extract domain disk def pointer to 'domdisk'
Add a local variable holding the pointer instead of indexing the array
multiple times.
Signed-off-by: Peter Krempa
---
src/conf/checkpoint_conf.c | 12
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/src/conf/checkpoint_conf.c b/src/conf/checkpoint_conf.c
index 22757d148f..3213097f4f 100644
--- a/src/conf/checkpoint_conf.c
+++ b/src/conf/checkpoint_conf.c
@@ -328,6 +328,7 @@ virDomainCheckpointAlignDisks(virDomainCheckpointDefPtr
chkdef)
for (i = 0; i < chkdef->ndisks; i++) {
virDomainCheckpointDiskDefPtr chkdisk = >disks[i];
int idx = virDomainDiskIndexByName(domdef, chkdisk->name, false);
+virDomainDiskDefPtr domdisk;
if (idx < 0) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
@@ -335,14 +336,17 @@ virDomainCheckpointAlignDisks(virDomainCheckpointDefPtr
chkdef)
return -1;
}
+domdisk = domdef->disks[idx];
+
if (virBitmapIsBitSet(map, idx)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("disk '%s' specified twice"),
chkdisk->name);
return -1;
}
-if ((virStorageSourceIsEmpty(domdef->disks[idx]->src) ||
- domdef->disks[idx]->src->readonly) &&
+
+if ((virStorageSourceIsEmpty(domdisk->src) ||
+ domdisk->src->readonly) &&
chkdisk->type != VIR_DOMAIN_CHECKPOINT_TYPE_NONE) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("disk '%s' is empty or readonly"),
@@ -352,9 +356,9 @@ virDomainCheckpointAlignDisks(virDomainCheckpointDefPtr
chkdef)
ignore_value(virBitmapSetBit(map, idx));
chkdisk->idx = idx;
-if (STRNEQ(chkdisk->name, domdef->disks[idx]->dst)) {
+if (STRNEQ(chkdisk->name, domdisk->dst)) {
VIR_FREE(chkdisk->name);
-chkdisk->name = g_strdup(domdef->disks[idx]->dst);
+chkdisk->name = g_strdup(domdisk->dst);
}
}
--
2.28.0
[PATCH 11/11] virDomainSnapshotAlignDisks: Use virDomainDiskByName
We don't need the index that virDomainDiskIndexByName returns.
Signed-off-by: Peter Krempa
---
src/conf/snapshot_conf.c | 7 ++-
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index 8ef9708c72..f896fd1cf2 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -663,17 +663,14 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr
snapdef,
/* Double check requested disks. */
for (i = 0; i < snapdef->ndisks; i++) {
virDomainSnapshotDiskDefPtr snapdisk = >disks[i];
-int idx = virDomainDiskIndexByName(domdef, snapdisk->name, false);
-virDomainDiskDefPtr domdisk = NULL;
+virDomainDiskDefPtr domdisk = virDomainDiskByName(domdef,
snapdisk->name, false);
-if (idx < 0) {
+if (!domdisk) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("no disk named '%s'"), snapdisk->name);
return -1;
}
-domdisk = domdef->disks[idx];
-
if (virHashHasEntry(map, domdisk->dst)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("disk '%s' specified twice"),
--
2.28.0
[PATCH 10/11] virDomainCheckpointAlignDisks: Use virDomainDiskByName
We don't need the index that virDomainDiskIndexByName returns.
Signed-off-by: Peter Krempa
---
src/conf/checkpoint_conf.c | 7 ++-
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/src/conf/checkpoint_conf.c b/src/conf/checkpoint_conf.c
index bd0a673757..089488fbc6 100644
--- a/src/conf/checkpoint_conf.c
+++ b/src/conf/checkpoint_conf.c
@@ -315,17 +315,14 @@ virDomainCheckpointAlignDisks(virDomainCheckpointDefPtr
chkdef)
/* Double check requested disks. */
for (i = 0; i < chkdef->ndisks; i++) {
virDomainCheckpointDiskDefPtr chkdisk = >disks[i];
-int idx = virDomainDiskIndexByName(domdef, chkdisk->name, false);
-virDomainDiskDefPtr domdisk;
+virDomainDiskDefPtr domdisk = virDomainDiskByName(domdef,
chkdisk->name, false);
-if (idx < 0) {
+if (!domdisk) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("no disk named '%s'"), chkdisk->name);
return -1;
}
-domdisk = domdef->disks[idx];
-
if (virHashHasEntry(map, domdisk->dst)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("disk '%s' specified twice"),
--
2.28.0
[PATCH 08/11] virDomainCheckpointDiskDef: Remove unused 'idx' field
Signed-off-by: Peter Krempa
---
src/conf/checkpoint_conf.h | 1 -
1 file changed, 1 deletion(-)
diff --git a/src/conf/checkpoint_conf.h b/src/conf/checkpoint_conf.h
index 631f863151..5997e07ff9 100644
--- a/src/conf/checkpoint_conf.h
+++ b/src/conf/checkpoint_conf.h
@@ -42,7 +42,6 @@ typedef struct _virDomainCheckpointDiskDef
virDomainCheckpointDiskDef;
typedef virDomainCheckpointDiskDef *virDomainCheckpointDiskDefPtr;
struct _virDomainCheckpointDiskDef {
char *name; /* name matching the dom->disks that matches name */
int type; /* virDomainCheckpointType */
char *bitmap; /* bitmap name, if type is bitmap */
unsigned long long size; /* current checkpoint size in bytes */
--
2.28.0
[PATCH 07/11] virDomainCheckpointAlignDisks: refactor extension to all disks
Similarly to d3c029bb105 where we've refactored
virDomainSnapshotAlignDisks, modify the extension algorithm to avoid use
of the 'idx' variable and sorting of the array.
Signed-off-by: Peter Krempa
---
src/conf/checkpoint_conf.c | 53 +++---
1 file changed, 21 insertions(+), 32 deletions(-)
diff --git a/src/conf/checkpoint_conf.c b/src/conf/checkpoint_conf.c
index 3213097f4f..bd0a673757 100644
--- a/src/conf/checkpoint_conf.c
+++ b/src/conf/checkpoint_conf.c
@@ -274,16 +274,6 @@
virDomainCheckpointDefAssignBitmapNames(virDomainCheckpointDefPtr def)
}
-static int
-virDomainCheckpointCompareDiskIndex(const void *a, const void *b)
-{
-const virDomainCheckpointDiskDef *diska = a;
-const virDomainCheckpointDiskDef *diskb = b;
-
-/* Integer overflow shouldn't be a problem here. */
-return diska->idx - diskb->idx;
-}
-
/* Align def->disks to def->domain. Sort the list of def->disks,
* filling in any missing disks with appropriate default. Convert
* paths to disk targets for uniformity. Issue an error and return -1
@@ -293,9 +283,9 @@ int
virDomainCheckpointAlignDisks(virDomainCheckpointDefPtr chkdef)
{
virDomainDefPtr domdef = chkdef->parent.dom;
-g_autoptr(virBitmap) map = NULL;
+g_autoptr(GHashTable) map = virHashNew(NULL);
+g_autofree virDomainCheckpointDiskDefPtr olddisks = NULL;
size_t i;
-int ndisks;
int checkpoint_default = VIR_DOMAIN_CHECKPOINT_TYPE_NONE;
if (!domdef) {
@@ -322,8 +312,6 @@ virDomainCheckpointAlignDisks(virDomainCheckpointDefPtr
chkdef)
if (!chkdef->ndisks)
checkpoint_default = VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP;
-map = virBitmapNew(domdef->ndisks);
-
/* Double check requested disks. */
for (i = 0; i < chkdef->ndisks; i++) {
virDomainCheckpointDiskDefPtr chkdisk = >disks[i];
@@ -338,13 +326,16 @@ virDomainCheckpointAlignDisks(virDomainCheckpointDefPtr
chkdef)
domdisk = domdef->disks[idx];
-if (virBitmapIsBitSet(map, idx)) {
+if (virHashHasEntry(map, domdisk->dst)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("disk '%s' specified twice"),
chkdisk->name);
return -1;
}
+if (virHashAddEntry(map, domdisk->dst, chkdisk) < 0)
+return -1;
+
if ((virStorageSourceIsEmpty(domdisk->src) ||
domdisk->src->readonly) &&
chkdisk->type != VIR_DOMAIN_CHECKPOINT_TYPE_NONE) {
@@ -353,8 +344,6 @@ virDomainCheckpointAlignDisks(virDomainCheckpointDefPtr
chkdef)
chkdisk->name);
return -1;
}
-ignore_value(virBitmapSetBit(map, idx));
-chkdisk->idx = idx;
if (STRNEQ(chkdisk->name, domdisk->dst)) {
VIR_FREE(chkdisk->name);
@@ -362,32 +351,32 @@ virDomainCheckpointAlignDisks(virDomainCheckpointDefPtr
chkdef)
}
}
-/* Provide defaults for all remaining disks. */
-ndisks = chkdef->ndisks;
-if (VIR_EXPAND_N(chkdef->disks, chkdef->ndisks,
- domdef->ndisks - chkdef->ndisks) < 0)
-return -1;
+olddisks = g_steal_pointer(>disks);
+chkdef->disks = g_new0(virDomainCheckpointDiskDef, domdef->ndisks);
+chkdef->ndisks = domdef->ndisks;
for (i = 0; i < domdef->ndisks; i++) {
-virDomainCheckpointDiskDefPtr chkdisk;
+virDomainDiskDefPtr domdisk = domdef->disks[i];
+virDomainCheckpointDiskDefPtr chkdisk = chkdef->disks + i;
+virDomainCheckpointDiskDefPtr existing;
-if (virBitmapIsBitSet(map, i))
+/* copy existing disks */
+if ((existing = virHashLookup(map, domdisk->dst))) {
+memcpy(chkdisk, existing, sizeof(*chkdisk));
continue;
-chkdisk = >disks[ndisks++];
-chkdisk->name = g_strdup(domdef->disks[i]->dst);
-chkdisk->idx = i;
+}
+
+/* Provide defaults for all remaining disks. */
+chkdisk->name = g_strdup(domdisk->dst);
/* Don't checkpoint empty or readonly drives */
-if (virStorageSourceIsEmpty(domdef->disks[i]->src) ||
-domdef->disks[i]->src->readonly)
+if (virStorageSourceIsEmpty(domdisk->src) ||
+domdisk->src->readonly)
chkdisk->type = VIR_DOMAIN_CHECKPOINT_TYPE_NONE;
else
chkdisk->type = checkpoint_default;
}
-qsort(>disks[0], chkdef->ndisks, sizeof(chkdef->disks[0]),
- virDomainCheckpointCompareDiskIndex);
-
/* Generate default bitmap names for checkpoint */
if (virDomainCheckpointDefAssignBitmapNames(chkdef) < 0)
return -1;
--
2.28.0
[PATCH 03/11] virDomainCheckpointAlignDisks: Use 'domdef' for domain definition
Extract the pointer and use a local variable throughout the function.
Signed-off-by: Peter Krempa
---
src/conf/checkpoint_conf.c | 29 +++--
1 file changed, 15 insertions(+), 14 deletions(-)
diff --git a/src/conf/checkpoint_conf.c b/src/conf/checkpoint_conf.c
index 914deb41f2..2375c78b92 100644
--- a/src/conf/checkpoint_conf.c
+++ b/src/conf/checkpoint_conf.c
@@ -292,25 +292,26 @@ virDomainCheckpointCompareDiskIndex(const void *a, const
void *b)
int
virDomainCheckpointAlignDisks(virDomainCheckpointDefPtr def)
{
+virDomainDefPtr domdef = def->parent.dom;
g_autoptr(virBitmap) map = NULL;
size_t i;
int ndisks;
int checkpoint_default = VIR_DOMAIN_CHECKPOINT_TYPE_NONE;
-if (!def->parent.dom) {
+if (!domdef) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("missing domain in checkpoint"));
return -1;
}
-if (def->ndisks > def->parent.dom->ndisks) {
+if (def->ndisks > domdef->ndisks) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("too many disk checkpoint requests for domain"));
return -1;
}
/* Unlikely to have a guest without disks but technically possible. */
-if (!def->parent.dom->ndisks) {
+if (!domdef->ndisks) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("domain must have at least one disk to perform
checkpoints"));
return -1;
@@ -321,12 +322,12 @@ virDomainCheckpointAlignDisks(virDomainCheckpointDefPtr
def)
if (!def->ndisks)
checkpoint_default = VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP;
-map = virBitmapNew(def->parent.dom->ndisks);
+map = virBitmapNew(domdef->ndisks);
/* Double check requested disks. */
for (i = 0; i < def->ndisks; i++) {
virDomainCheckpointDiskDefPtr disk = >disks[i];
-int idx = virDomainDiskIndexByName(def->parent.dom, disk->name, false);
+int idx = virDomainDiskIndexByName(domdef, disk->name, false);
if (idx < 0) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
@@ -340,8 +341,8 @@ virDomainCheckpointAlignDisks(virDomainCheckpointDefPtr def)
disk->name);
return -1;
}
-if ((virStorageSourceIsEmpty(def->parent.dom->disks[idx]->src) ||
- def->parent.dom->disks[idx]->src->readonly) &&
+if ((virStorageSourceIsEmpty(domdef->disks[idx]->src) ||
+ domdef->disks[idx]->src->readonly) &&
disk->type != VIR_DOMAIN_CHECKPOINT_TYPE_NONE) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("disk '%s' is empty or readonly"),
@@ -351,30 +352,30 @@ virDomainCheckpointAlignDisks(virDomainCheckpointDefPtr
def)
ignore_value(virBitmapSetBit(map, idx));
disk->idx = idx;
-if (STRNEQ(disk->name, def->parent.dom->disks[idx]->dst)) {
+if (STRNEQ(disk->name, domdef->disks[idx]->dst)) {
VIR_FREE(disk->name);
-disk->name = g_strdup(def->parent.dom->disks[idx]->dst);
+disk->name = g_strdup(domdef->disks[idx]->dst);
}
}
/* Provide defaults for all remaining disks. */
ndisks = def->ndisks;
if (VIR_EXPAND_N(def->disks, def->ndisks,
- def->parent.dom->ndisks - def->ndisks) < 0)
+ domdef->ndisks - def->ndisks) < 0)
return -1;
-for (i = 0; i < def->parent.dom->ndisks; i++) {
+for (i = 0; i < domdef->ndisks; i++) {
virDomainCheckpointDiskDefPtr disk;
if (virBitmapIsBitSet(map, i))
continue;
disk = >disks[ndisks++];
-disk->name = g_strdup(def->parent.dom->disks[i]->dst);
+disk->name = g_strdup(domdef->disks[i]->dst);
disk->idx = i;
/* Don't checkpoint empty or readonly drives */
-if (virStorageSourceIsEmpty(def->parent.dom->disks[i]->src) ||
-def->parent.dom->disks[i]->src->readonly)
+if (virStorageSourceIsEmpty(domdef->disks[i]->src) ||
+domdef->disks[i]->src->readonly)
disk->type = VIR_DOMAIN_CHECKPOINT_TYPE_NONE;
else
disk->type = checkpoint_default;
--
2.28.0
[PATCH 04/11] virDomainCheckpointAlignDisks: rename 'def' to 'chkdef'
In most cases 'def' is used for the domain definition. Rename it to
chkdef to prevent confusion.
Signed-off-by: Peter Krempa
---
src/conf/checkpoint_conf.c | 24
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/src/conf/checkpoint_conf.c b/src/conf/checkpoint_conf.c
index 2375c78b92..1881c93e09 100644
--- a/src/conf/checkpoint_conf.c
+++ b/src/conf/checkpoint_conf.c
@@ -290,9 +290,9 @@ virDomainCheckpointCompareDiskIndex(const void *a, const
void *b)
* if any def->disks[n]->name appears more than once or does not map
* to dom->disks. */
int
-virDomainCheckpointAlignDisks(virDomainCheckpointDefPtr def)
+virDomainCheckpointAlignDisks(virDomainCheckpointDefPtr chkdef)
{
-virDomainDefPtr domdef = def->parent.dom;
+virDomainDefPtr domdef = chkdef->parent.dom;
g_autoptr(virBitmap) map = NULL;
size_t i;
int ndisks;
@@ -304,7 +304,7 @@ virDomainCheckpointAlignDisks(virDomainCheckpointDefPtr def)
return -1;
}
-if (def->ndisks > domdef->ndisks) {
+if (chkdef->ndisks > domdef->ndisks) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("too many disk checkpoint requests for domain"));
return -1;
@@ -319,14 +319,14 @@ virDomainCheckpointAlignDisks(virDomainCheckpointDefPtr
def)
/* If omitted, do bitmap on all writeable disks;
* otherwise, do nothing for omitted disks */
-if (!def->ndisks)
+if (!chkdef->ndisks)
checkpoint_default = VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP;
map = virBitmapNew(domdef->ndisks);
/* Double check requested disks. */
-for (i = 0; i < def->ndisks; i++) {
-virDomainCheckpointDiskDefPtr disk = >disks[i];
+for (i = 0; i < chkdef->ndisks; i++) {
+virDomainCheckpointDiskDefPtr disk = >disks[i];
int idx = virDomainDiskIndexByName(domdef, disk->name, false);
if (idx < 0) {
@@ -359,9 +359,9 @@ virDomainCheckpointAlignDisks(virDomainCheckpointDefPtr def)
}
/* Provide defaults for all remaining disks. */
-ndisks = def->ndisks;
-if (VIR_EXPAND_N(def->disks, def->ndisks,
- domdef->ndisks - def->ndisks) < 0)
+ndisks = chkdef->ndisks;
+if (VIR_EXPAND_N(chkdef->disks, chkdef->ndisks,
+ domdef->ndisks - chkdef->ndisks) < 0)
return -1;
for (i = 0; i < domdef->ndisks; i++) {
@@ -369,7 +369,7 @@ virDomainCheckpointAlignDisks(virDomainCheckpointDefPtr def)
if (virBitmapIsBitSet(map, i))
continue;
-disk = >disks[ndisks++];
+disk = >disks[ndisks++];
disk->name = g_strdup(domdef->disks[i]->dst);
disk->idx = i;
@@ -381,11 +381,11 @@ virDomainCheckpointAlignDisks(virDomainCheckpointDefPtr
def)
disk->type = checkpoint_default;
}
-qsort(>disks[0], def->ndisks, sizeof(def->disks[0]),
+qsort(>disks[0], chkdef->ndisks, sizeof(chkdef->disks[0]),
virDomainCheckpointCompareDiskIndex);
/* Generate default bitmap names for checkpoint */
-if (virDomainCheckpointDefAssignBitmapNames(def) < 0)
+if (virDomainCheckpointDefAssignBitmapNames(chkdef) < 0)
return -1;
return 0;
--
2.28.0
[PATCH 05/11] virDomainCheckpointAlignDisks: Use 'chkdisk' instead of 'disk'
Clarify that the variable refers to the definition of the disk from the
checkpoint definition.
Signed-off-by: Peter Krempa
---
src/conf/checkpoint_conf.c | 32
1 file changed, 16 insertions(+), 16 deletions(-)
diff --git a/src/conf/checkpoint_conf.c b/src/conf/checkpoint_conf.c
index 1881c93e09..22757d148f 100644
--- a/src/conf/checkpoint_conf.c
+++ b/src/conf/checkpoint_conf.c
@@ -326,35 +326,35 @@ virDomainCheckpointAlignDisks(virDomainCheckpointDefPtr
chkdef)
/* Double check requested disks. */
for (i = 0; i < chkdef->ndisks; i++) {
-virDomainCheckpointDiskDefPtr disk = >disks[i];
-int idx = virDomainDiskIndexByName(domdef, disk->name, false);
+virDomainCheckpointDiskDefPtr chkdisk = >disks[i];
+int idx = virDomainDiskIndexByName(domdef, chkdisk->name, false);
if (idx < 0) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("no disk named '%s'"), disk->name);
+ _("no disk named '%s'"), chkdisk->name);
return -1;
}
if (virBitmapIsBitSet(map, idx)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("disk '%s' specified twice"),
- disk->name);
+ chkdisk->name);
return -1;
}
if ((virStorageSourceIsEmpty(domdef->disks[idx]->src) ||
domdef->disks[idx]->src->readonly) &&
-disk->type != VIR_DOMAIN_CHECKPOINT_TYPE_NONE) {
+chkdisk->type != VIR_DOMAIN_CHECKPOINT_TYPE_NONE) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("disk '%s' is empty or readonly"),
- disk->name);
+ chkdisk->name);
return -1;
}
ignore_value(virBitmapSetBit(map, idx));
-disk->idx = idx;
+chkdisk->idx = idx;
-if (STRNEQ(disk->name, domdef->disks[idx]->dst)) {
-VIR_FREE(disk->name);
-disk->name = g_strdup(domdef->disks[idx]->dst);
+if (STRNEQ(chkdisk->name, domdef->disks[idx]->dst)) {
+VIR_FREE(chkdisk->name);
+chkdisk->name = g_strdup(domdef->disks[idx]->dst);
}
}
@@ -365,20 +365,20 @@ virDomainCheckpointAlignDisks(virDomainCheckpointDefPtr
chkdef)
return -1;
for (i = 0; i < domdef->ndisks; i++) {
-virDomainCheckpointDiskDefPtr disk;
+virDomainCheckpointDiskDefPtr chkdisk;
if (virBitmapIsBitSet(map, i))
continue;
-disk = >disks[ndisks++];
-disk->name = g_strdup(domdef->disks[i]->dst);
-disk->idx = i;
+chkdisk = >disks[ndisks++];
+chkdisk->name = g_strdup(domdef->disks[i]->dst);
+chkdisk->idx = i;
/* Don't checkpoint empty or readonly drives */
if (virStorageSourceIsEmpty(domdef->disks[i]->src) ||
domdef->disks[i]->src->readonly)
-disk->type = VIR_DOMAIN_CHECKPOINT_TYPE_NONE;
+chkdisk->type = VIR_DOMAIN_CHECKPOINT_TYPE_NONE;
else
-disk->type = checkpoint_default;
+chkdisk->type = checkpoint_default;
}
qsort(>disks[0], chkdef->ndisks, sizeof(chkdef->disks[0]),
--
2.28.0
[PATCH 02/11] virDomainCheckpointAlignDisks: Unbreak error message
Signed-off-by: Peter Krempa
---
src/conf/checkpoint_conf.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/src/conf/checkpoint_conf.c b/src/conf/checkpoint_conf.c
index 795c6f93c4..914deb41f2 100644
--- a/src/conf/checkpoint_conf.c
+++ b/src/conf/checkpoint_conf.c
@@ -312,8 +312,7 @@ virDomainCheckpointAlignDisks(virDomainCheckpointDefPtr def)
/* Unlikely to have a guest without disks but technically possible. */
if (!def->parent.dom->ndisks) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("domain must have at least one disk to perform "
- "checkpoints"));
+ _("domain must have at least one disk to perform
checkpoints"));
return -1;
}
--
2.28.0
[PATCH 01/11] virDomainCheckpointAlignDisks: Refactor cleanup
Use g_autoptr for virBitmap and get rid of the 'cleanup:' label and ret
variable.
Signed-off-by: Peter Krempa
---
src/conf/checkpoint_conf.c | 25 ++---
1 file changed, 10 insertions(+), 15 deletions(-)
diff --git a/src/conf/checkpoint_conf.c b/src/conf/checkpoint_conf.c
index a8d18928de..795c6f93c4 100644
--- a/src/conf/checkpoint_conf.c
+++ b/src/conf/checkpoint_conf.c
@@ -292,8 +292,7 @@ virDomainCheckpointCompareDiskIndex(const void *a, const
void *b)
int
virDomainCheckpointAlignDisks(virDomainCheckpointDefPtr def)
{
-int ret = -1;
-virBitmapPtr map = NULL;
+g_autoptr(virBitmap) map = NULL;
size_t i;
int ndisks;
int checkpoint_default = VIR_DOMAIN_CHECKPOINT_TYPE_NONE;
@@ -301,13 +300,13 @@ virDomainCheckpointAlignDisks(virDomainCheckpointDefPtr
def)
if (!def->parent.dom) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("missing domain in checkpoint"));
-goto cleanup;
+return -1;
}
if (def->ndisks > def->parent.dom->ndisks) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("too many disk checkpoint requests for domain"));
-goto cleanup;
+return -1;
}
/* Unlikely to have a guest without disks but technically possible. */
@@ -315,7 +314,7 @@ virDomainCheckpointAlignDisks(virDomainCheckpointDefPtr def)
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("domain must have at least one disk to perform "
"checkpoints"));
-goto cleanup;
+return -1;
}
/* If omitted, do bitmap on all writeable disks;
@@ -333,14 +332,14 @@ virDomainCheckpointAlignDisks(virDomainCheckpointDefPtr
def)
if (idx < 0) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("no disk named '%s'"), disk->name);
-goto cleanup;
+return -1;
}
if (virBitmapIsBitSet(map, idx)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("disk '%s' specified twice"),
disk->name);
-goto cleanup;
+return -1;
}
if ((virStorageSourceIsEmpty(def->parent.dom->disks[idx]->src) ||
def->parent.dom->disks[idx]->src->readonly) &&
@@ -348,7 +347,7 @@ virDomainCheckpointAlignDisks(virDomainCheckpointDefPtr def)
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("disk '%s' is empty or readonly"),
disk->name);
-goto cleanup;
+return -1;
}
ignore_value(virBitmapSetBit(map, idx));
disk->idx = idx;
@@ -363,7 +362,7 @@ virDomainCheckpointAlignDisks(virDomainCheckpointDefPtr def)
ndisks = def->ndisks;
if (VIR_EXPAND_N(def->disks, def->ndisks,
def->parent.dom->ndisks - def->ndisks) < 0)
-goto cleanup;
+return -1;
for (i = 0; i < def->parent.dom->ndisks; i++) {
virDomainCheckpointDiskDefPtr disk;
@@ -387,13 +386,9 @@ virDomainCheckpointAlignDisks(virDomainCheckpointDefPtr
def)
/* Generate default bitmap names for checkpoint */
if (virDomainCheckpointDefAssignBitmapNames(def) < 0)
-goto cleanup;
-
-ret = 0;
+return -1;
- cleanup:
-virBitmapFree(map);
-return ret;
+return 0;
}
--
2.28.0
[PATCH 00/11] virDomainCheckpointAlignDisks: Clean up similarly to virDomainSnapshotAlignDisks
I've refactored virDomainSnapshotAlignDisks and virDomainCheckpointAlignDisks was heavily inspired by it. Clean it up too. Peter Krempa (11): virDomainCheckpointAlignDisks: Refactor cleanup virDomainCheckpointAlignDisks: Unbreak error message virDomainCheckpointAlignDisks: Use 'domdef' for domain definition virDomainCheckpointAlignDisks: rename 'def' to 'chkdef' virDomainCheckpointAlignDisks: Use 'chkdisk' instead of 'disk' virDomainCheckpointAlignDisks: Extract domain disk def pointer to 'domdisk' virDomainCheckpointAlignDisks: refactor extension to all disks virDomainCheckpointDiskDef: Remove unused 'idx' field virDomainDiskByName: Remove ternary operator virDomainCheckpointAlignDisks: Use virDomainDiskByName virDomainSnapshotAlignDisks: Use virDomainDiskByName src/conf/checkpoint_conf.c | 123 - src/conf/checkpoint_conf.h | 1 - src/conf/domain_conf.c | 6 +- src/conf/snapshot_conf.c | 7 +-- 4 files changed, 61 insertions(+), 76 deletions(-) -- 2.28.0
Re: [PATCH v2 0/6] add post parse pSeries mem align, when ABI_UPDATE
On 12/2/20 4:17 AM, Michal Privoznik wrote: 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 Thanks for the review! Andrea/Peter, do you want to take a look again to see if there's something that I missed? DHB Michal
[libvirt PATCH] src: use singular form instead of plural, for guest disk info
Existing practice with the filesystem fields reported for the
virDomainGetGuestInfo API is to use the singular form for
field names. Ensure the disk info follows this practice.
Fixes
commit 05a75ca2ce743bc0bb119fb8d532ff84646fafa3
Author: Marc-André Lureau
Date: Fri Nov 20 22:09:46 2020 +0400
domain: add disk informations to virDomainGetGuestInfo
commit 0cb2d9f05d00497a715352f6ea28cf8fb6921731
Author: Marc-André Lureau
Date: Fri Nov 20 22:09:47 2020 +0400
qemu_driver: report guest disk informations
Signed-off-by: Daniel P. Berrangé
---
src/libvirt-domain.c | 14 +++---
src/qemu/qemu_driver.c | 14 +++---
2 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index 35e95e5395..f5cd43ecea 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -12345,17 +12345,17 @@ virDomainSetVcpu(virDomainPtr domain,
* Returns information about the disks within the domain. The typed
* parameter keys are in this format:
*
- * "disks.count" - the number of disks defined on this domain
+ * "disk.count" - the number of disks defined on this domain
* as an unsigned int
- * "disks..name" - device node (Linux) or device UNC (Windows)
- * "disks..partition" - whether this is a partition or disk
- * "disks..dependencies.count" - the number of device dependencies
+ * "disk..name" - device node (Linux) or device UNC (Windows)
+ * "disk..partition" - whether this is a partition or disk
+ * "disk..dependency.count" - the number of device dependencies
* e.g. for LVs of the LVM this will
* hold the list of PVs, for LUKS encrypted volume this
will
* contain the disk where the volume is placed. (Linux)
- * "disks..dependencies..name" - a dependency
- * "disks..alias" - the device alias of the disk (e.g. sda)
- * "disks..guest_alias" - optional alias assigned to the disk, on
Linux
+ * "disk..dependency..name" - a dependency
+ * "disk..alias" - the device alias of the disk (e.g. sda)
+ * "disk..guest_alias" - optional alias assigned to the disk, on
Linux
* this is a name assigned by device mapper
*
* VIR_DOMAIN_GUEST_INFO_HOSTNAME:
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 8eaa3ce68f..548df6ae68 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -19876,20 +19876,20 @@ qemuAgentDiskInfoFormatParams(qemuAgentDiskInfoPtr
*info,
size_t i, j, ndeps;
if (virTypedParamsAddUInt(params, nparams, maxparams,
- "disks.count", ndisks) < 0)
+ "disk.count", ndisks) < 0)
return;
for (i = 0; i < ndisks; i++) {
char param_name[VIR_TYPED_PARAM_FIELD_LENGTH];
g_snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
- "disks.%zu.name", i);
+ "disk.%zu.name", i);
if (virTypedParamsAddString(params, nparams, maxparams,
param_name, info[i]->name) < 0)
return;
g_snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
- "disks.%zu.partition", i);
+ "disk.%zu.partition", i);
if (virTypedParamsAddBoolean(params, nparams, maxparams,
param_name, info[i]->partition) < 0)
return;
@@ -19897,14 +19897,14 @@ qemuAgentDiskInfoFormatParams(qemuAgentDiskInfoPtr
*info,
if (info[i]->dependencies) {
ndeps = g_strv_length(info[i]->dependencies);
g_snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
- "disks.%zu.dependencies.count", i);
+ "disk.%zu.dependency.count", i);
if (ndeps &&
virTypedParamsAddUInt(params, nparams, maxparams,
param_name, ndeps) < 0)
return;
for (j = 0; j < ndeps; j++) {
g_snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
- "disks.%zu.dependencies.%zu.name", i, j);
+ "disk.%zu.dependency.%zu.name", i, j);
if (virTypedParamsAddString(params, nparams, maxparams,
param_name,
info[i]->dependencies[j]) < 0)
return;
@@ -19922,7 +19922,7 @@ qemuAgentDiskInfoFormatParams(qemuAgentDiskInfoPtr
*info,
info[i]->address->unit);
if (diskdef) {
g_snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
- "disks.%zu.alias", i);
+ "disk.%zu.alias", i);
if (diskdef->dst &&
virTypedParamsAddString(params, nparams, maxparams,
Re: [libvirt PATCH] meson: add winsock2 library on windows builds
On Wed, Dec 02, 2020 at 11:06:26AM +, Daniel P. Berrangé wrote: > If building for windows with curl disabled we get build failures due to > missing ws2_32 library needed for winsock2. > > Signed-off-by: Daniel P. Berrangé > --- > meson.build | 2 ++ > 1 file changed, 2 insertions(+) Reviewed-by: Pavel Hrdina signature.asc Description: PGP signature
Re: [PATCH 2/3] qemu: support append param on live attaching file chardev
On Wed, Dec 02, 2020 at 14:55:08 +0300, Nikolay Shirokovskiy wrote: > Currently it is simply ignored. > > Signed-off-by: Nikolay Shirokovskiy > --- > src/qemu/qemu_monitor_json.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c > index 47ee1ff..df95a4a 100644 > --- a/src/qemu/qemu_monitor_json.c > +++ b/src/qemu/qemu_monitor_json.c > @@ -7497,6 +7497,9 @@ qemuMonitorJSONAttachCharDevCommand(const char *chrID, > backend_type = "file"; > if (virJSONValueObjectAppendString(data, "out", chr->data.file.path) > < 0) > goto cleanup; > +if (virJSONValueObjectAppendBooleanTristate(data, "append", > +chr->data.file.append) < > 0) > +goto cleanup; You can use: if (virJSONValueObjectAdd(data, "T:append", chr->data.file.append, NULL) < 0) goto cleanup; instead of adding the new function at all.
Re: [PATCH] accel/tcg: Remove deprecated '-tb-size' option
On a Wednesday in 2020, Philippe Mathieu-Daudé wrote: The '-tb-size' option (replaced by '-accel tcg,tb-size') is deprecated since 5.0 (commit fe174132478). Remove it. Signed-off-by: Philippe Mathieu-Daudé --- docs/system/deprecated.rst | 12 +--- accel/tcg/translate-all.c | 2 +- softmmu/vl.c | 8 qemu-options.hx| 8 4 files changed, 6 insertions(+), 24 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH v2] 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 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(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH 0/2] Replace VIR_AUTOSTRINGLIST with GStrv
On a Tuesday in 2020, Michal Privoznik wrote: 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 +- [...] 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(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
[Call for Presentations] FOSDEM 2021: Virt & IaaS Devroom
[Cross-posting to KVM, QEMU, and libvirt lists] The Call For Papers for FOSDEM's Virt & IaaS Devroom went out yesterday[1]. Here's the text (slightly formatted for readability): === We are excited to announce that the call for proposals is now open for the Virtualization & IaaS devroom at the upcoming FOSDEM 2021, to be hosted virtually on February 6th 2021. This year will mark FOSDEM's 21th anniversary as one of the longest-running free and open source software developer events, attracting thousands of developers and users from all over the world. Due to Covid-19, FOSDEM will be held virtually this year on February 6th & 7th, 2021. Important Dates --- * Submission deadline: 20th of December * Acceptance notifications: 25th of December * Final schedule announcement: 31st of December * Recorded presentations upload deadline: 15th of January * Devroom: 6th February 2021 About the Devroom - The Virtualization & IaaS devroom will feature session topics such as open source hypervisors and virtual machine managers such as Xen Project, KVM, bhyve, and VirtualBox, and Infrastructure-as-a-Service projects such as KubeVirt, Apache CloudStack, Foreman, OpenStack, oVirt, QEMU and OpenNebula. This devroom will host presentations that focus on topics of shared interest, such as KVM; libvirt; shared storage; virtualized networking; cloud security; clustering and high availability; interfacing with multiple hypervisors; hyperconverged deployments; and scaling across hundreds or thousands of servers. Presentations in this devroom will be aimed at users or developers working on these platforms who are looking to collaborate and improve shared infrastructure or solve common problems. We seek topics that encourage dialog between projects and continued work post-FOSDEM. Submit Your Proposal All submissions must be made via the Pentabarf event planning site[1]. If you have not used Pentabarf before, you will need to create an account. If you submitted proposals for FOSDEM in previous years, you can use your existing account. After creating the account, select Create Event to start the submission process. Make sure to select Virtualization and IaaS devroom from the Track list. Please fill out all the required fields, and provide a meaningful abstract and description of your proposed session. Submission Guidelines - We expect more proposals than we can possibly accept, so it is vitally important that you submit your proposal on or before the deadline. Late submissions are unlikely to be considered. All presentation slots are 30 minutes, with 20 minutes planned for presentations, and 10 minutes for Q All presentations will need to be pre-recorded and put into our system at least a couple of weeks before the event. The presentations should be uploaded by 15th of January and made available under Creative Commons licenses. In the Submission notes field, please indicate that you agree that your presentation will be licensed under the CC-By-SA-4.0 or CC-By-4.0 license and that you agree to have your presentation recorded. For example: "If my presentation is accepted for FOSDEM, I hereby agree to license all recordings, slides, and other associated materials under the Creative Commons Attribution Share-Alike 4.0 International License. Sincerely, ." In the Submission notes field, please also confirm that if your talk is accepted, you will be able to attend the virtual FOSDEM event for the Q We will not consider proposals from prospective speakers who are unsure whether they will be able to attend the FOSDEM virtual event. If you are experiencing problems with Pentabarf, the proposal submission interface, or have other questions, you can email our devroom mailing list[2] and we will try to help you. Code of Conduct --- Following the release of the updated code of conduct for FOSDEM, we'd like to remind all speakers and attendees that all of the presentations and discussions in our devroom are held under the guidelines set in the CoC and we expect attendees, speakers, and volunteers to follow the CoC at all times. If you submit a proposal and it is accepted, you will be required to confirm that you accept the FOSDEM CoC. If you have any questions about the CoC or wish to have one of the devroom organizers review your presentation slides or any other content for CoC compliance, please email us and we will do our best to assist you. Call for Volunteers --- We are also looking for volunteers to help run the devroom. We need assistance with helping speakers to record the presentation as well as helping with streaming and chat moderation for the devroom. Please contact devroom mailing list [2] for more information. Questions? -- If you have any questions about this devroom, please send your questions to our devroom mailing list. You can
[libvirt PATCH] rpm: convert mingw spec to meson
The meson build system is configured to only ever build shared
libraries, so we delete the -static sub-RPMs.
The few driver conditionals are deleted as there was never any
scenario in which their value changed.
Signed-off-by: Daniel P. Berrangé
---
mingw-libvirt.spec.in | 205 +-
1 file changed, 81 insertions(+), 124 deletions(-)
diff --git a/mingw-libvirt.spec.in b/mingw-libvirt.spec.in
index 06bb9dfe7f..0686cbaf78 100644
--- a/mingw-libvirt.spec.in
+++ b/mingw-libvirt.spec.in
@@ -11,26 +11,6 @@
%define supported_platform 0
%endif
-# Default to skipping autoreconf. Distros can change just this one line
-# (or provide a command-line override) if they backport any patches that
-# touch configure.ac or Makefile.am.
-%{!?enable_autotools:%define enable_autotools 0}
-
-# The mingw build is client only. Set up defaults for hypervisor drivers
-# that talk via a native remote protocol, and for which prereq mingw
-# libraries exist.
-%define with_esx 0%{!?_without_esx:1}
-# missing libwsman, so can't build hyper-v
-%define with_hyperv0%{!?_without_hyperv:0}
-%define with_xenapi0%{!?_without_xenapi:1}
-%define with_vz0%{!?_without_vz:0}
-
-# RHEL ships ESX but not PowerHypervisor, HyperV, or libxenserver (xenapi)
-%if 0%{?rhel}
-%define with_xenapi 0
-%define with_hyperv 0
-%endif
-
Name: mingw-libvirt
Version:@VERSION@
Release:1%{?dist}
@@ -74,20 +54,15 @@ BuildRequires: libxslt
BuildRequires: python3
BuildRequires: perl-interpreter
BuildRequires: perl(Getopt::Long)
-%if 0%{?enable_autotools}
-BuildRequires: autoconf
-BuildRequires: automake
-BuildRequires: gettext-devel
-BuildRequires: libtool
-%endif
+BuildRequires: make
+BuildRequires: meson
+BuildRequires: ninja-build
BuildRequires: python3-docutils
BuildRequires: mingw32-libssh2
BuildRequires: mingw64-libssh2
-%if %{with_esx}
BuildRequires: mingw32-curl
BuildRequires: mingw64-curl
-%endif
BuildRequires: cpp
%if 0%{?fedora} || 0%{?rhel} > 7
BuildRequires: rpcgen
@@ -101,31 +76,19 @@ MinGW Windows libvirt virtualization library.
# Mingw32
%package -n mingw32-libvirt
Summary: %{summary}
+Obsoletes: mingw32-libvirt < 7.0.0
%description -n mingw32-libvirt
MinGW Windows libvirt virtualization library.
-%package -n mingw32-libvirt-static
-Summary: %{summary}
-Requires: mingw32-libvirt = %{version}-%{release}
-
-%description -n mingw32-libvirt-static
-MinGW Windows libvirt virtualization library, static version.
-
# Mingw64
%package -n mingw64-libvirt
Summary: %{summary}
+Obsoletes: mingw64-libvirt < 7.0.0
%description -n mingw64-libvirt
MinGW Windows libvirt virtualization library.
-%package -n mingw64-libvirt-static
-Summary: %{summary}
-Requires: mingw64-libvirt = %{version}-%{release}
-
-%description -n mingw64-libvirt-static
-MinGW Windows libvirt virtualization library, static version.
-
%{?mingw_debug_package}
@@ -138,55 +101,82 @@ echo "This RPM requires Fedora >= %{min_fedora}"
exit 1
%endif
-%if ! %{with_esx}
-%define _without_esx --without-esx
-%endif
-
-%if ! %{with_hyperv}
-%define _without_hyperv --without-hyperv
-%endif
-
-%if ! %{with_xenapi}
-%define _without_xenapi --without-xenapi
-%endif
-
-%if ! %{with_vz}
-%define _without_vz --without-vz
-%endif
-
-%if 0%{?enable_autotools}
-autoreconf -if
-%endif
-
-# XXX enable SASL in future
-%mingw_configure \
- --enable-static \
- --without-xen \
- --without-qemu \
- --without-openvz \
- --without-lxc \
- --without-vbox \
- %{?_without_xenapi} \
- --without-sasl \
- --without-polkit \
- --without-libvirtd \
- %{?_without_esx} \
- %{?_without_hyperv} \
- --without-vmware \
- --without-parallels \
- --without-netcf \
- --without-audit \
- --without-dtrace \
- --enable-expensive-tests
-
-%mingw_make %{?_smp_mflags}
-
+%mingw_meson \
+ --auto-features=enabled \
+ -Ddriver_remote=enabled \
+ -Ddriver_esx=enabled \
+ -Dcurl=enabled \
+ -Ddocs=enabled \
+ -Dapparmor=disabled \
+ -Dattr=disabled \
+ -Daudit=disabled \
+ -Dbash_completion=disabled \
+ -Dblkid=disabled \
+ -Dcapng=disabled \
+ -Ddriver_bhyve=disabled \
+ -Ddriver_hyperv=disabled \
+ -Ddriver_interface=disabled \
+ -Ddriver_libvirtd=disabled \
+ -Ddriver_libxl=disabled \
+ -Ddriver_lxc=disabled \
+ -Ddriver_network=disabled \
+ -Ddriver_openvz=disabled \
+ -Ddriver_qemu=disabled \
+ -Ddriver_secrets=disabled \
+ -Ddriver_vbox=disabled \
+ -Ddriver_vmware=disabled \
+ -Ddriver_vz=disabled \
+ -Ddtrace=disabled \
+ -Dexpensive_tests=enabled \
+ -Dfirewalld=disabled \
+ -Dfirewalld_zone=disabled \
+ -Dfuse=disabled \
+ -Dglusterfs=disabled \
+ -Dhost_validate=disabled \
+ -Dlibiscsi=disabled \
+ -Dlibnl=disabled \
+ -Dlibpcap=disabled \
+ -Dlibssh2=disabled \
+ -Dlibssh=disabled \
+ -Dlogin_shell=disabled \
+ -Dnetcf=disabled \
+ -Dnls=disabled \
+ -Dnss=disabled \
+ -Dnumactl=disabled \
+
Re: [PATCH] accel/tcg: Remove deprecated '-tb-size' option
On 02/12/2020 12.27, Philippe Mathieu-Daudé wrote:
> The '-tb-size' option (replaced by '-accel tcg,tb-size') is
> deprecated since 5.0 (commit fe174132478). Remove it.
>
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> docs/system/deprecated.rst | 12 +---
> accel/tcg/translate-all.c | 2 +-
> softmmu/vl.c | 8
> qemu-options.hx| 8
> 4 files changed, 6 insertions(+), 24 deletions(-)
>
> diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
> index 565389697e8..70bdb62a6d6 100644
> --- a/docs/system/deprecated.rst
> +++ b/docs/system/deprecated.rst
> @@ -100,13 +100,6 @@ QEMU 5.1 has three options:
>to the user to load all the images they need.
> 3. ``-bios `` - Tells QEMU to load the specified file as the firmwrae.
>
> -``-tb-size`` option (since 5.0)
> -'''
> -
> -QEMU 5.0 introduced an alternative syntax to specify the size of the
> translation
> -block cache, ``-accel tcg,tb-size=``. The new syntax deprecates the
> -previously available ``-tb-size`` option.
> -
> ``-show-cursor`` option (since 5.0)
> '''
>
> @@ -523,6 +516,11 @@ for the ``id`` parameter, which should now be used
> instead.
>
> The ``-no-kvm`` argument was a synonym for setting ``-machine accel=tcg``.
>
> +``-tb-size`` option (removed in 6.0)
> +'''
> +
> +QEMU 5.0 introduced an alternative syntax to specify the size of the
> translation
> +block cache, ``-accel tcg,tb-size=``.
>
> QEMU Machine Protocol (QMP) commands
>
> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
> index 4572b4901fb..b7d50a73d44 100644
> --- a/accel/tcg/translate-all.c
> +++ b/accel/tcg/translate-all.c
> @@ -2379,7 +2379,7 @@ void dump_exec_info(void)
> qemu_printf("Translation buffer state:\n");
> /*
> * Report total code size including the padding and TB structs;
> - * otherwise users might think "-tb-size" is not honoured.
> + * otherwise users might think "-accel tcg,tb-size" is not honoured.
> * For avg host size we use the precise numbers from tb_tree_stats
> though.
> */
> qemu_printf("gen code size %zu/%zu\n",
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index e6e0ad5a925..3f052849d8c 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -3639,14 +3639,6 @@ void qemu_init(int argc, char **argv, char **envp)
> exit(1);
> }
> break;
> -case QEMU_OPTION_tb_size:
> -#ifndef CONFIG_TCG
> -error_report("TCG is disabled");
> -exit(1);
> -#endif
> -warn_report("The -tb-size option is deprecated, use -accel
> tcg,tb-size instead");
> -object_register_sugar_prop(ACCEL_CLASS_NAME("tcg"),
> "tb-size", optarg);
> -break;
> case QEMU_OPTION_icount:
> icount_opts =
> qemu_opts_parse_noisily(qemu_find_opts("icount"),
>optarg, true);
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 104632ea343..7ce06290b68 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -4080,14 +4080,6 @@ SRST
> Show cursor.
> ERST
>
> -DEF("tb-size", HAS_ARG, QEMU_OPTION_tb_size, \
> -"-tb-size n set TB size\n", QEMU_ARCH_ALL)
> -SRST
> -``-tb-size n``
> -Set TCG translation block cache size. Deprecated, use
> -'\ ``-accel tcg,tb-size=n``\ ' instead.
> -ERST
> -
> DEF("incoming", HAS_ARG, QEMU_OPTION_incoming, \
> "-incoming tcp:[host]:port[,to=maxport][,ipv4][,ipv6]\n" \
> "-incoming rdma:host:port[,ipv4][,ipv6]\n" \
>
Reviewed-by: Thomas Huth
[PATCH] accel/tcg: Remove deprecated '-tb-size' option
The '-tb-size' option (replaced by '-accel tcg,tb-size') is
deprecated since 5.0 (commit fe174132478). Remove it.
Signed-off-by: Philippe Mathieu-Daudé
---
docs/system/deprecated.rst | 12 +---
accel/tcg/translate-all.c | 2 +-
softmmu/vl.c | 8
qemu-options.hx| 8
4 files changed, 6 insertions(+), 24 deletions(-)
diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
index 565389697e8..70bdb62a6d6 100644
--- a/docs/system/deprecated.rst
+++ b/docs/system/deprecated.rst
@@ -100,13 +100,6 @@ QEMU 5.1 has three options:
to the user to load all the images they need.
3. ``-bios `` - Tells QEMU to load the specified file as the firmwrae.
-``-tb-size`` option (since 5.0)
-'''
-
-QEMU 5.0 introduced an alternative syntax to specify the size of the
translation
-block cache, ``-accel tcg,tb-size=``. The new syntax deprecates the
-previously available ``-tb-size`` option.
-
``-show-cursor`` option (since 5.0)
'''
@@ -523,6 +516,11 @@ for the ``id`` parameter, which should now be used instead.
The ``-no-kvm`` argument was a synonym for setting ``-machine accel=tcg``.
+``-tb-size`` option (removed in 6.0)
+'''
+
+QEMU 5.0 introduced an alternative syntax to specify the size of the
translation
+block cache, ``-accel tcg,tb-size=``.
QEMU Machine Protocol (QMP) commands
diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index 4572b4901fb..b7d50a73d44 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -2379,7 +2379,7 @@ void dump_exec_info(void)
qemu_printf("Translation buffer state:\n");
/*
* Report total code size including the padding and TB structs;
- * otherwise users might think "-tb-size" is not honoured.
+ * otherwise users might think "-accel tcg,tb-size" is not honoured.
* For avg host size we use the precise numbers from tb_tree_stats though.
*/
qemu_printf("gen code size %zu/%zu\n",
diff --git a/softmmu/vl.c b/softmmu/vl.c
index e6e0ad5a925..3f052849d8c 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -3639,14 +3639,6 @@ void qemu_init(int argc, char **argv, char **envp)
exit(1);
}
break;
-case QEMU_OPTION_tb_size:
-#ifndef CONFIG_TCG
-error_report("TCG is disabled");
-exit(1);
-#endif
-warn_report("The -tb-size option is deprecated, use -accel
tcg,tb-size instead");
-object_register_sugar_prop(ACCEL_CLASS_NAME("tcg"), "tb-size",
optarg);
-break;
case QEMU_OPTION_icount:
icount_opts = qemu_opts_parse_noisily(qemu_find_opts("icount"),
optarg, true);
diff --git a/qemu-options.hx b/qemu-options.hx
index 104632ea343..7ce06290b68 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -4080,14 +4080,6 @@ SRST
Show cursor.
ERST
-DEF("tb-size", HAS_ARG, QEMU_OPTION_tb_size, \
-"-tb-size n set TB size\n", QEMU_ARCH_ALL)
-SRST
-``-tb-size n``
-Set TCG translation block cache size. Deprecated, use
-'\ ``-accel tcg,tb-size=n``\ ' instead.
-ERST
-
DEF("incoming", HAS_ARG, QEMU_OPTION_incoming, \
"-incoming tcp:[host]:port[,to=maxport][,ipv4][,ipv6]\n" \
"-incoming rdma:host:port[,ipv4][,ipv6]\n" \
--
2.26.2
Re: [PATCH] coding-style: Document 80 chars limit for line length
On 02/12/2020 12.20, Michal Privoznik wrote:
> On 12/2/20 11:52 AM, Daniel P. Berrangé wrote:
>> On Mon, Nov 30, 2020 at 01:58:24PM +0100, Michal Privoznik wrote:
>>> The idea is to have it like a soft limit: if possible then break
>>> lines, if not then have a long line instead of some creative
>>> approach.
>>>
>>> Signed-off-by: Michal Privoznik
>>> ---
>>> docs/coding-style.rst | 14 +-
>>> 1 file changed, 13 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/docs/coding-style.rst b/docs/coding-style.rst
>>> index cfd7b16638..813128bfb6 100644
>>> --- a/docs/coding-style.rst
>>> +++ b/docs/coding-style.rst
>>> @@ -131,7 +131,7 @@ around operators and keywords:
>>> indent-libvirt()
>>> {
>>> - indent -bad -bap -bbb -bli4 -br -ce -brs -cs -i4 -l75 -lc75 \
>>> + indent -bad -bap -bbb -bli4 -br -ce -brs -cs -i4 -l80 -lc80 \
>>
>> The indent tool enforces line length no matter what
>
> Yeah, it's not perfect, but I am no friend with gnu indent so I don't know
> how to specify hard and soft limits and quick skim through manpage did not
> suggest it's possible.
>
>>
>>> -sbi4 -psl -saf -sai -saw -sbi4 -ss -sc -cdw -cli4 -npcs -nbc \
>>> --no-tabs "$@"
>>> }
>>> @@ -141,6 +141,18 @@ further, by piping it through ``expand -i``, since
>>> some leading
>>> TABs can get through. Usually they're in macro definitions or
>>> strings, and should be converted anyhow.
>>> +The recommended length for lines is 80 characters, but common sense
>>> +should prevail. It may get tricky around some names (because of how
>>> +Libvirt constructs names for functions/enums/etc.)
>>
>> but this is a mere recommendation.
>>
>> IMHO we should say
>>
>> "The maximum permitted line length is 100 characters, but lines
>> should aim to be approximately 80 characters."
>>
>> and then use -l100 for indent
>
> Works for me. Thomas, since you suggested we document this, does this
> wording sound reasonable to you? If so, I will post v2.
Yes, I think using -l100 for indent and saying that 80 is preferred is
better! Thanks for tackling this!
Thomas
Re: [PATCH] coding-style: Document 80 chars limit for line length
On 12/2/20 11:52 AM, Daniel P. Berrangé wrote:
On Mon, Nov 30, 2020 at 01:58:24PM +0100, Michal Privoznik wrote:
The idea is to have it like a soft limit: if possible then break
lines, if not then have a long line instead of some creative
approach.
Signed-off-by: Michal Privoznik
---
docs/coding-style.rst | 14 +-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/docs/coding-style.rst b/docs/coding-style.rst
index cfd7b16638..813128bfb6 100644
--- a/docs/coding-style.rst
+++ b/docs/coding-style.rst
@@ -131,7 +131,7 @@ around operators and keywords:
indent-libvirt()
{
-indent -bad -bap -bbb -bli4 -br -ce -brs -cs -i4 -l75 -lc75 \
+indent -bad -bap -bbb -bli4 -br -ce -brs -cs -i4 -l80 -lc80 \
The indent tool enforces line length no matter what
Yeah, it's not perfect, but I am no friend with gnu indent so I don't
know how to specify hard and soft limits and quick skim through manpage
did not suggest it's possible.
-sbi4 -psl -saf -sai -saw -sbi4 -ss -sc -cdw -cli4 -npcs -nbc \
--no-tabs "$@"
}
@@ -141,6 +141,18 @@ further, by piping it through ``expand -i``, since some
leading
TABs can get through. Usually they're in macro definitions or
strings, and should be converted anyhow.
+The recommended length for lines is 80 characters, but common sense
+should prevail. It may get tricky around some names (because of how
+Libvirt constructs names for functions/enums/etc.)
but this is a mere recommendation.
IMHO we should say
"The maximum permitted line length is 100 characters, but lines
should aim to be approximately 80 characters."
and then use -l100 for indent
Works for me. Thomas, since you suggested we document this, does this
wording sound reasonable to you? If so, I will post v2.
Michal
Re: regression in meson build, AC_PATH_PROG lost
Am Thu, 12 Nov 2020 22:40:02 +0100 schrieb Olaf Hering : > AC_PATH_PROG([PARTED], [parted], [], [$LIBVIRT_SBIN_PATH]) Is there a consensus now how to address this? >From what I understand, using just exec(BASENAME) to search for the binary in >PATH is acceptable. It may result in surprises on systems which have BASENAME >in /usr/local/*bin. Previously the absolute path at build time was used. I >think this can be tolerated. Should there be a list of each called binary in meson.build? This would at least give some hint about what might be called at runtime, even if the result of find_program() will not be stored in the resulting libvirt binaries. Olaf pgpsMIk9nfwr5.pgp Description: Digitale Signatur von OpenPGP
[libvirt PATCH] meson: add winsock2 library on windows builds
If building for windows with curl disabled we get build failures due to
missing ws2_32 library needed for winsock2.
Signed-off-by: Daniel P. Berrangé
---
meson.build | 2 ++
1 file changed, 2 insertions(+)
diff --git a/meson.build b/meson.build
index 01e7fbd409..b280809a65 100644
--- a/meson.build
+++ b/meson.build
@@ -1368,10 +1368,12 @@ libutil_dep = cc.find_library('util', required: false)
if host_machine.system() == 'windows'
ole32_dep = cc.find_library('ole32')
oleaut32_dep = cc.find_library('oleaut32')
+ winsock2_dep = cc.find_library('ws2_32')
win32_dep = declare_dependency(
dependencies: [
ole32_dep,
oleaut32_dep,
+ winsock2_dep,
],
)
if get_option('default_library') == 'static'
--
2.28.0
Re: [PATCH] coding-style: Document 80 chars limit for line length
On Mon, Nov 30, 2020 at 01:58:24PM +0100, Michal Privoznik wrote:
> The idea is to have it like a soft limit: if possible then break
> lines, if not then have a long line instead of some creative
> approach.
>
> Signed-off-by: Michal Privoznik
> ---
> docs/coding-style.rst | 14 +-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/docs/coding-style.rst b/docs/coding-style.rst
> index cfd7b16638..813128bfb6 100644
> --- a/docs/coding-style.rst
> +++ b/docs/coding-style.rst
> @@ -131,7 +131,7 @@ around operators and keywords:
>
>indent-libvirt()
>{
> -indent -bad -bap -bbb -bli4 -br -ce -brs -cs -i4 -l75 -lc75 \
> +indent -bad -bap -bbb -bli4 -br -ce -brs -cs -i4 -l80 -lc80 \
The indent tool enforces line length no matter what
> -sbi4 -psl -saf -sai -saw -sbi4 -ss -sc -cdw -cli4 -npcs -nbc \
> --no-tabs "$@"
>}
> @@ -141,6 +141,18 @@ further, by piping it through ``expand -i``, since some
> leading
> TABs can get through. Usually they're in macro definitions or
> strings, and should be converted anyhow.
>
> +The recommended length for lines is 80 characters, but common sense
> +should prevail. It may get tricky around some names (because of how
> +Libvirt constructs names for functions/enums/etc.)
but this is a mere recommendation.
IMHO we should say
"The maximum permitted line length is 100 characters, but lines
should aim to be approximately 80 characters."
and then use -l100 for indent
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 :|
[PATCH 4/4] libvirt_recover_xattrs: Allow fixing multiple PATHs
Loop for multiple PATH arguments to support shell pattern expansion.
Signed-off-by: Peter Krempa
---
tools/libvirt_recover_xattrs.sh | 51 +++--
1 file changed, 29 insertions(+), 22 deletions(-)
diff --git a/tools/libvirt_recover_xattrs.sh b/tools/libvirt_recover_xattrs.sh
index bb5f23cff0..59f1f3f476 100755
--- a/tools/libvirt_recover_xattrs.sh
+++ b/tools/libvirt_recover_xattrs.sh
@@ -7,7 +7,7 @@ function die {
function show_help {
cat << EOF
-Usage: ${0##*/} -[hqnu] [PATH]
+Usage: ${0##*/} -[hqnu] [PATH ...]
Clear out any XATTRs set by libvirt on all files that have them.
The idea is to reset refcounting, should it break.
@@ -25,7 +25,6 @@ EOF
QUIET=0
DRY_RUN=0
UNSAFE=0
-DIR="/"
# So far only qemu and lxc drivers use security driver.
URI=("qemu:///system"
@@ -57,15 +56,6 @@ while getopts hqnu opt; do
esac
done
-shift $((OPTIND - 1))
-if [ $# -gt 0 ]; then
-DIR=$1
-else
-if [ ${UNSAFE} -eq 1 ]; then
-die "Unsafe mode (-u) requires explicit 'PATH' argument"
-fi
-fi
-
case $(uname -s) in
Linux)
XATTR_PREFIX="trusted.libvirt.security"
@@ -95,17 +85,34 @@ for i in "dac" "selinux"; do
XATTRS+=("$XATTR_PREFIX.$i" "$XATTR_PREFIX.ref_$i"
"$XATTR_PREFIX.timestamp_$i")
done
+fix_xattrs() {
+local DIR="$1"
-for i in $(getfattr -R -d -m ${XATTR_PREFIX} --absolute-names ${DIR}
2>/dev/null | grep "^# file:" | cut -d':' -f 2); do
-if [ ${DRY_RUN} -ne 0 ]; then
-getfattr -d -m $p --absolute-names $i | grep -v "^# file:"
-continue
-fi
+for i in $(getfattr -R -d -m ${XATTR_PREFIX} --absolute-names ${DIR}
2>/dev/null | grep "^# file:" | cut -d':' -f 2); do
+if [ ${DRY_RUN} -ne 0 ]; then
+getfattr -d -m $p --absolute-names $i | grep -v "^# file:"
+continue
+fi
-if [ ${QUIET} -eq 0 ]; then
-echo "Fixing $i";
-fi
-for x in ${XATTRS[*]}; do
-setfattr -x $x $i
+if [ ${QUIET} -eq 0 ]; then
+echo "Fixing $i";
+fi
+for x in ${XATTRS[*]}; do
+setfattr -x $x $i
+done
done
-done
+}
+
+
+shift $((OPTIND - 1))
+if [ $# -gt 0 ]; then
+while [ $# -gt 0 ]; do
+fix_xattrs "$1"
+shift $((OPTIND - 1))
+done
+else
+if [ ${UNSAFE} -eq 1 ]; then
+die "Unsafe mode (-u) requires explicit 'PATH' argument"
+fi
+fix_xattrs "/"
+fi
--
2.28.0
Re: [PATCH 00/18] qapi/qom: QAPIfy object-add
Am 02.12.2020 um 10:30 hat Paolo Bonzini geschrieben:
> On 01/12/20 23:08, Eduardo Habkost wrote:
> > > 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).
>
> Yes.
Are internal uses mostly just right after object creation, or do we make
a lot of use of them during runtime?
> > > 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.
>
> Yeah, "all about exposing properties" includes internal usage. And you're
> right that some "phase 3" monitor commands do work at the property level
> (mostly "info qtree", but also "qom-get" because there are some cases of
> public run-time properties).
>
> > I'm liking the direction this is taking. However, I would still
> > like to have a clearer and feasible plan that would work for
> > -device, -machine, and -cpu.
>
> -cpu is not a problem since it's generally created with a static
> configuration (now done with global properties, in the future it could be a
> struct).
>
> -machine and -device in principle could be done the same way as -object,
> just through a different registry (_not_ a huge struct; that's an acceptable
> stopgap for -object but that's it). The static aka field properties would
> remain as read-only, with defaults moved to instance_init or realize. But
> there would be again "triplication" with a trivial conversion:
>
> 1) in the QAPI schema, e.g. 'num_queues': 'int16'
>
> 2) in the struct, "int16_t num_queues;"
This one is optional, you can use the QAPI type even in the run-time
state. I guess this goes back to how much separation you want between
the configuration and the internal state.
> 3) in the realize function,
>
> s->num_queues = cfg->has_num_queues ? cfg->num_queues : 8;
>
> So having a mechanism for defaults in the QAPI schema would be good. Maybe
> 'num_queues': { 'type': 'int16', 'default': '8' }?
Defaults have been on the QAPI wishlist for a long time, and everyone
agrees that it would be nice to have them. Maybe it's time to finally
implement them.
> I also need to review more the part of this code with respect to the
> application of global properties. I wonder if there are visitor tricks that
> we can do, so that global properties keep working but correspond to QAPI
> fields instead of QOM properties.
Kevin
[PATCH 3/4] libvirt_recover_xattrs: Add unsafe operation mode
In some cases you want to fix a certain directory while you don't really
care whether there are other VMs running. Add a option to disable the
check.
Signed-off-by: Peter Krempa
---
tools/libvirt_recover_xattrs.sh | 15 ---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/tools/libvirt_recover_xattrs.sh b/tools/libvirt_recover_xattrs.sh
index b7a8c05cf4..bb5f23cff0 100755
--- a/tools/libvirt_recover_xattrs.sh
+++ b/tools/libvirt_recover_xattrs.sh
@@ -7,7 +7,7 @@ function die {
function show_help {
cat << EOF
-Usage: ${0##*/} -[hqn] [PATH]
+Usage: ${0##*/} -[hqnu] [PATH]
Clear out any XATTRs set by libvirt on all files that have them.
The idea is to reset refcounting, should it break.
@@ -15,6 +15,7 @@ The idea is to reset refcounting, should it break.
-hdisplay this help and exit
-qquiet (don't print which files are being fixed)
-ndry run; don't remove any XATTR just report the file name
+ -uunsafe; don't check whether there are running VMs; PATH must be
specified
PATH can be specified to refine search to only to given path
instead of whole root ('/'), which is the default.
@@ -23,6 +24,7 @@ EOF
QUIET=0
DRY_RUN=0
+UNSAFE=0
DIR="/"
# So far only qemu and lxc drivers use security driver.
@@ -33,7 +35,7 @@ if [ $(whoami) != "root" ]; then
die "Must be run as root"
fi
-while getopts hqn opt; do
+while getopts hqnu opt; do
case $opt in
h)
show_help
@@ -45,6 +47,9 @@ while getopts hqn opt; do
n)
DRY_RUN=1
;;
+u)
+UNSAFE=1
+;;
*)
show_help >&2
exit 1
@@ -55,6 +60,10 @@ done
shift $((OPTIND - 1))
if [ $# -gt 0 ]; then
DIR=$1
+else
+if [ ${UNSAFE} -eq 1 ]; then
+die "Unsafe mode (-u) requires explicit 'PATH' argument"
+fi
fi
case $(uname -s) in
@@ -72,7 +81,7 @@ case $(uname -s) in
esac
-if [ ${DRY_RUN} -eq 0 ]; then
+if [ ${DRY_RUN} -eq 0 ] && [ ${UNSAFE} -eq 0 ]; then
for u in ${URI[*]} ; do
if [ -n "`virsh -q -c $u list 2>/dev/null`" ]; then
die "There are still some domains running for $u"
--
2.28.0
[PATCH 1/4] libvirt_recover_xattrs: Avoid backticks for subshell
Signed-off-by: Peter Krempa
---
tools/libvirt_recover_xattrs.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/libvirt_recover_xattrs.sh b/tools/libvirt_recover_xattrs.sh
index 3907413c63..cb98497732 100755
--- a/tools/libvirt_recover_xattrs.sh
+++ b/tools/libvirt_recover_xattrs.sh
@@ -34,7 +34,7 @@ URI=("qemu:///system"
LIBVIRT_XATTR_PREFIXES=("trusted.libvirt.security"
"system.libvirt.security")
-if [ `whoami` != "root" ]; then
+if [ $(whoami) != "root" ]; then
die "Must be run as root"
fi
--
2.28.0
[PATCH 2/4] libvirt_recover_xattrs: Use only the correct xattr prefix
Linux and FreeBSD have different prefix. In the current state we've
tried to reset the labels for both systems which resulted in errors like
this:
Fixing /tmp/bitmaps2.qcow2
setfattr: /tmp/bitmaps2.qcow2: Operation not supported
setfattr: /tmp/bitmaps2.qcow2: Operation not supported
setfattr: /tmp/bitmaps2.qcow2: Operation not supported
setfattr: /tmp/bitmaps2.qcow2: Operation not supported
setfattr: /tmp/bitmaps2.qcow2: Operation not supported
setfattr: /tmp/bitmaps2.qcow2: Operation not supported
The 6 failed 'setfattrs' correspond to the wrong prefix.
Select the correct prefix based on the kernel name and modify the code
appropriately.
Signed-off-by: Peter Krempa
---
tools/libvirt_recover_xattrs.sh | 48 ++---
1 file changed, 27 insertions(+), 21 deletions(-)
diff --git a/tools/libvirt_recover_xattrs.sh b/tools/libvirt_recover_xattrs.sh
index cb98497732..b7a8c05cf4 100755
--- a/tools/libvirt_recover_xattrs.sh
+++ b/tools/libvirt_recover_xattrs.sh
@@ -29,11 +29,6 @@ DIR="/"
URI=("qemu:///system"
"lxc:///system")
-# On Linux we use 'trusted' namespace, on FreeBSD we use 'system'
-# as there is no 'trusted'.
-LIBVIRT_XATTR_PREFIXES=("trusted.libvirt.security"
-"system.libvirt.security")
-
if [ $(whoami) != "root" ]; then
die "Must be run as root"
fi
@@ -62,6 +57,21 @@ if [ $# -gt 0 ]; then
DIR=$1
fi
+case $(uname -s) in
+Linux)
+XATTR_PREFIX="trusted.libvirt.security"
+;;
+
+FreeBSD)
+XATTR_PREFIX="system.libvirt.security"
+;;
+
+*)
+die "$0 is not supported on this platform"
+;;
+esac
+
+
if [ ${DRY_RUN} -eq 0 ]; then
for u in ${URI[*]} ; do
if [ -n "`virsh -q -c $u list 2>/dev/null`" ]; then
@@ -73,24 +83,20 @@ fi
declare -a XATTRS
for i in "dac" "selinux"; do
-for p in ${LIBVIRT_XATTR_PREFIXES[@]}; do
-XATTRS+=("$p.$i" "$p.ref_$i" "$p.timestamp_$i")
-done
+XATTRS+=("$XATTR_PREFIX.$i" "$XATTR_PREFIX.ref_$i"
"$XATTR_PREFIX.timestamp_$i")
done
-for p in ${LIBVIRT_XATTR_PREFIXES[*]}; do
-for i in $(getfattr -R -d -m ${p} --absolute-names ${DIR} 2>/dev/null |
grep "^# file:" | cut -d':' -f 2); do
-echo $i;
-if [ ${DRY_RUN} -ne 0 ]; then
-getfattr -d -m $p --absolute-names $i | grep -v "^# file:"
-continue
-fi
-if [ ${QUIET} -eq 0 ]; then
-echo "Fixing $i";
-fi
-for x in ${XATTRS[*]}; do
-setfattr -x $x $i
-done
+for i in $(getfattr -R -d -m ${XATTR_PREFIX} --absolute-names ${DIR}
2>/dev/null | grep "^# file:" | cut -d':' -f 2); do
+if [ ${DRY_RUN} -ne 0 ]; then
+getfattr -d -m $p --absolute-names $i | grep -v "^# file:"
+continue
+fi
+
+if [ ${QUIET} -eq 0 ]; then
+echo "Fixing $i";
+fi
+for x in ${XATTRS[*]}; do
+setfattr -x $x $i
done
done
--
2.28.0
[PATCH 0/4] libvirt_recover_xattrs: Improve usability for libvirt development
I run into scenarios where I forget to destroy a VM which has a new WIP qemu capability. Starting a libvirtd which doesn't support it yet makes the libvirt vanish, thus after manually killing it libvirtd doesn't clear the xattrs, making it impossible to start the VM. libvirt_recover_xattrs fixes this, but is unusable unless you want to turn off all VMs. Peter Krempa (4): libvirt_recover_xattrs: Avoid backticks for subshell libvirt_recover_xattrs: Use only the correct xattr prefix libvirt_recover_xattrs: Add unsafe operation mode libvirt_recover_xattrs: Allow fixing multiple PATHs tools/libvirt_recover_xattrs.sh | 64 ++--- 1 file changed, 43 insertions(+), 21 deletions(-) -- 2.28.0
[libvirt PATCH] cpu_map: Fix Icelake Server model number
See arch/x86/include/asm/intel-family.h in the Kernel: #define INTEL_FAM6_ICELAKE_X 0x6A Signed-off-by: Tim Wiederhake --- src/cpu_map/x86_Icelake-Server-noTSX.xml | 2 +- src/cpu_map/x86_Icelake-Server.xml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/cpu_map/x86_Icelake-Server-noTSX.xml b/src/cpu_map/x86_Icelake-Server-noTSX.xml index 2fd6906406..34a0f7c18c 100644 --- a/src/cpu_map/x86_Icelake-Server-noTSX.xml +++ b/src/cpu_map/x86_Icelake-Server-noTSX.xml @@ -1,7 +1,7 @@ - + diff --git a/src/cpu_map/x86_Icelake-Server.xml b/src/cpu_map/x86_Icelake-Server.xml index 367ade7240..1ee4ea9cd4 100644 --- a/src/cpu_map/x86_Icelake-Server.xml +++ b/src/cpu_map/x86_Icelake-Server.xml @@ -1,7 +1,7 @@ - + -- 2.26.2
Re: [libvirt PATCH] scripts: ignore whitespace in pdwtags output
On a Wednesday in 2020, Daniel P. Berrangé wrote:
The pdwtags program changed its whitespace formatting for enum
values in release 1.19:
@@ -145,22 +145,22 @@
u_int flags;
};
enum admin_procedure {
-ADMIN_PROC_CONNECT_OPEN = 1,
-ADMIN_PROC_CONNECT_CLOSE = 2,
-ADMIN_PROC_CONNECT_GET_LIB_VERSION = 3,
-ADMIN_PROC_CONNECT_LIST_SERVERS = 4,
-ADMIN_PROC_CONNECT_LOOKUP_SERVER = 5,
+ADMIN_PROC_CONNECT_OPEN = 1,
+ADMIN_PROC_CONNECT_CLOSE= 2,
+ADMIN_PROC_CONNECT_GET_LIB_VERSION = 3,
+ADMIN_PROC_CONNECT_LIST_SERVERS = 4,
+ADMIN_PROC_CONNECT_LOOKUP_SERVER= 5,
Workaround this by telling diff to ignore whitespace changes.
Signed-off-by: Daniel P. Berrangé
---
scripts/check-remote-protocol.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Ján Tomko
Jano
signature.asc
Description: PGP signature
