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 overridden 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 <pkrempa@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Fix the type for a variable holding flags to the usual 'unsigned int'
and change the name to be more appropriate to its use.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
We can extract './domain' directly and let the parser deal with the
type.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
See arch/x86/include/asm/intel-family.h in the Kernel:
#define INTEL_FAM6_ICELAKE_X 0x6A
Signed-off-by: Tim Wiederhake <twiederh@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
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 <jfehlig@suse.com>
Reviewed-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
qemuBuildCommandLine() is calling qemuDomainAlignMemorySizes(),
which is an operation that changes live XML and domain and has
little to do with the command line build process.
Move it to qemuProcessPrepareDomain() where we're supposed to
make live XML and domain changes before launch. qemuProcessStart()
is setting VIR_QEMU_PROCESS_START_NEW if !migrate && !snapshot,
same conditions used in qemuBuildCommandLine() to call
qemuDomainAlignMemorySizes(), making this change seamless.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
qemuProcessCreatePretendCmdPrepare() is setting the
VIR_QEMU_PROCESS_START_NEW regardless of whether this is
a migration case or not. This behavior differs from what we're
doing in qemuProcessStart(), where the flag is set only
if !migrate && !snapshot.
Fix it by making the flag setting consistent with what we're
doing in qemuProcessStart().
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
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 <jferlan@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
In this previous commit:
commit 65491a2dfe
Author: Martin Kletzander <mkletzan@redhat.com>
Date: Thu Nov 12 13:58:53 2020 +0100
Do not disable incompatible-pointer-types-discards-qualifiers
We selectively rewrite G_DEFINE_TYPE to avoid warnings about
mismatched volatile/non-volatile pointers that appeared with
CLang when using GLib2 >= 2.67
We have now just hit the reverse problem, GCC >= 11 has started
warning about mismatched volatile/non-volatile pointers but only
with GLib2 < 2.67. The new GLib2 avoids the warning, as does
older GCC.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
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 05a75ca2ce
Author: Marc-André Lureau <marcandre.lureau@redhat.com>
Date: Fri Nov 20 22:09:46 2020 +0400
domain: add disk informations to virDomainGetGuestInfo
commit 0cb2d9f05d
Author: Marc-André Lureau <marcandre.lureau@redhat.com>
Date: Fri Nov 20 22:09:47 2020 +0400
qemu_driver: report guest disk informations
commit 172b830435
Author: Marc-André Lureau <marcandre.lureau@redhat.com>
Date: Fri Nov 20 22:09:48 2020 +0400
virsh: add --disk informations to guestinfo command
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
We don't need the index that virDomainDiskIndexByName returns.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
We don't need the index that virDomainDiskIndexByName returns.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Similarly to d3c029bb10 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 <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Add a local variable holding the pointer instead of indexing the array
multiple times.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Clarify that the variable refers to the definition of the disk from the
checkpoint definition.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
In most cases 'def' is used for the domain definition. Rename it to
chkdef to prevent confusion.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Extract the pointer and use a local variable throughout the function.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Use g_autoptr for virBitmap and get rid of the 'cleanup:' label and ret
variable.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
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/pci0000:00/0000:00:02.0", mdev_types=0x7fff801c9b40, nmdev_types=0x7fff801c9b48) at ../src/conf/node_device_conf.c:2676
#1 0x00007ffff7caf53d in virNodeDeviceGetPCIDynamicCaps (sysfsPath=0x7fff801661e0 "/sys/devices/pci0000:00/0000:00:02.0", pci_dev=0x7fff801c9ac8) at ../src/conf/node_device_conf.c:2705
#2 0x00007ffff7cae38f in virNodeDeviceUpdateCaps (def=0x7fff80168a10) at ../src/conf/node_device_conf.c:2342
#3 0x00007ffff7cb11c0 in virNodeDeviceObjMatch (obj=0x7fff84002e50, flags=0) at ../src/conf/virnodedeviceobj.c:850
#4 0x00007ffff7cb153d in virNodeDeviceObjListExportCallback (payload=0x7fff84002e50, name=0x7fff801cbc20 "pci_0000_00_02_0", opaque=0x7fffe2ffc6a0) at ../src/conf/virnodedeviceobj.c:909
#5 0x00007ffff7b69146 in virHashForEach (table=0x7fff9814b700 = {...}, iter=0x7ffff7cb149e <virNodeDeviceObjListExportCallback>, opaque=0x7fffe2ffc6a0) at ../src/util/virhash.c:394
#6 0x00007ffff7cb1694 in virNodeDeviceObjListExport (conn=0x7fff98013170, devs=0x7fff98154430, devices=0x7fffe2ffc798, filter=0x7ffff7cf47a1 <virConnectListAllNodeDevicesCheckACL>, flags=0)
at ../src/conf/virnodedeviceobj.c:943
#7 0x00007fffe00694b2 in nodeConnectListAllNodeDevices (conn=0x7fff98013170, devices=0x7fffe2ffc798, flags=0) at ../src/node_device/node_device_driver.c:228
#8 0x00007ffff7e703aa in virConnectListAllNodeDevices (conn=0x7fff98013170, devices=0x7fffe2ffc798, flags=0) at ../src/libvirt-nodedev.c:130
#9 0x000055555557f796 in remoteDispatchConnectListAllNodeDevices (server=0x555555627080, client=0x5555556bf050, msg=0x5555556c0000, rerr=0x7fffe2ffc8a0, args=0x7fffd4008470, ret=0x7fffd40084e0)
at src/remote/remote_daemon_dispatch_stubs.h:1613
#10 0x000055555557f6f9 in remoteDispatchConnectListAllNodeDevicesHelper (server=0x555555627080, client=0x5555556bf050, msg=0x5555556c0000, rerr=0x7fffe2ffc8a0, args=0x7fffd4008470, ret=0x7fffd40084e0)
at src/remote/remote_daemon_dispatch_stubs.h:1591
#11 0x00007ffff7ce9542 in virNetServerProgramDispatchCall (prog=0x555555690c10, server=0x555555627080, client=0x5555556bf050, msg=0x5555556c0000) at ../src/rpc/virnetserverprogram.c:428
#12 0x00007ffff7ce90bd in virNetServerProgramDispatch (prog=0x555555690c10, server=0x555555627080, client=0x5555556bf050, msg=0x5555556c0000) at ../src/rpc/virnetserverprogram.c:302
#13 0x00007ffff7cf042b in virNetServerProcessMsg (srv=0x555555627080, client=0x5555556bf050, prog=0x555555690c10, msg=0x5555556c0000) at ../src/rpc/virnetserver.c:137
#14 0x00007ffff7cf04eb in virNetServerHandleJob (jobOpaque=0x5555556b66b0, opaque=0x555555627080) at ../src/rpc/virnetserver.c:154
#15 0x00007ffff7bd912f in virThreadPoolWorker (opaque=0x55555562bc70) at ../src/util/virthreadpool.c:163
#16 0x00007ffff7bd8645 in virThreadHelper (data=0x55555562bc90) at ../src/util/virthread.c:233
#17 0x00007ffff6d90432 in start_thread () at /lib64/libpthread.so.0
#18 0x00007ffff75c5913 in clone () at /lib64/libc.so.6
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Currently it is simply ignored.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Currently it is simply ignored.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
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
disastrous. So let's reinitialize 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 <jferlan@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Initialize and free @magic since virJSONValueObjectAppendString
does not free it for us eventually.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Initialize and free @magic since virJSONValueObjectAppendString
does not free it for us eventually.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Ján Tomko <jtomko@redhat.com>
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 <jferlan@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Now that no one uses VIR_AUTOSTRINGLIST it can be dropped.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Glib provides g_auto(GStrv) which is in-place replacement of our
VIR_AUTOSTRINGLIST.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
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 <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Remove the function along with helpers for caching the reply and tests.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Use the new handler to fetch the required data and do the extraction
locally without conversion to string list.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Add a new set hander for getting the data for
'query-command-line-options' which returns everything at once and lets
the caller extract the data. This way we don't need to cache the output
of the monitor command for repeated calls.
Note that we will have enough testing of this code path via
qemucapabilitiestest.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
The 'error' label is just returning -1, so let's 'return -1'
directly.
Use g_autoptr() with virDomainControllerDefPtr to remove the
need to call virDomainControllerDefFree() in the error path.
There is no need to VIR_FREE(nodes) explictly since 'nodes'
is using g_autofree.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Let's register AUTOPTR_CLEANUP_FUNC for virDomainControllerDefPtr
and modernize this function, removing the 'error' label using
g_autoptr().
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
The 'error' label is just doing a 'return -1'.
There's also a couple of 'VIR_FREE(nodes)' calls that are happening
right before exiting on error, but 'nodes' is already set for
autocleanup. These calls can also be removed.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Register a AUTOPTR_CLEANUP_FUNC for virDomainSmartcardDef and use
g_autoptr() to eliminate the 'error' label.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Register an AUTOPTR_CLEANUP_FUNC for virDomainDiskDefPtr, then
use g_autoptr() in virDomainDiskDef and virStorageEncryption
pointers to get rid of the 'cleanup' and 'error' labels.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
This will open an opportunity to modernize virDomainDiskDefParseXML()
in the next patch.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
This will modernize virDomainVideoDefParseXML() and
virDomainDefAddImplicitVideo() by removing unneeded
cleanup labels.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
The 'video' pointer is only being freed on error path, meaning
that we're leaking it after each loop restart.
There are more opportunities for auto cleanups of virDomainVideoDef
pointers, so let's register AUTOPTR_CLEANUP_FUNC for it to use
g_autoptr() later on.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Use g_autoptr() with the hash and remove the 'cleanup' label.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
This spares us of 2 explicit VIR_FREE() calls.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Do not look up the index of the passed FD in places where
we already have it.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
An alternative to qemuVirCommandGetFDSet that takes the index
into the passed FD set as an argument and does not try to look it up.
Use it as well ass virCommandPassFDIndex in qemuBuildChrChardevFileStr
and qemuBuildInterfaceCommandLine.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Just like virCommandPassFD, but it also returns an index of
the passed FD in the FD set.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
We only jump to cleanup before allocating any lists.
Drop the dead code.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Currently, ret is only used in the 'cleanup' section
and initialized right before the jump.
Switch to the customary initialization to -1 and only
leave in the 'ret = 0' statement on an empty list.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
In a few commit back (v6.10.0-5-gb3dad96972) a new helper for
obtaining string arrays from a virJSONObject was introduced:
virJSONValueObjectGetStringArray(). I've identified three places
where it can be used instead of open coding it:
qemuAgentSSHGetAuthorizedKeys(),
qemuMonitorJSONGetStringListProperty() and
qemuMonitorJSONGetCPUDefinitions().
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The virJSONValueObjectGetStringArray() function is given a @key
which is supposed to be an array inside given @object. Well, if
it's not then an error state is returned (NULL), but no error
message is set.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
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: Guoyi Tu<tu.guoyi@h3c.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The original logic is incorrect. We would delete the device entry
from eBPF map only if the newval would be same as current val in the
map. In case that the device was allowed only as read-only but later
we remove all permissions for that device it would remain in the table
with empty values.
The old code would still deny the device but it's not working as
intended. Instead we will update the value in advance. If the updated
value is 0 it means that we are removing all permissions so it should
be removed from the map, otherwise we will update the value in map.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1810356
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Kernel commit <d505b8af58912ae1e1a211fabc9995b19bd40828> added proper
check for cpu quota maximum limit to prevent internal overflow.
Even though this change is not present in all kernels it makes sense
to enforce the same limit in libvirt.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1750315
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Tested-by: Han Han <hhan@redhat.com>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Tested-by: Han Han <hhan@redhat.com>
guest-get-disks is available since QEMU 5.2:
https://wiki.qemu.org/ChangeLog/5.2#Guest_agent
Note that the test response was manually edited based on a reply on my
bare-metal computer. It shows partial results due to pcieport driver not
being currently supported by QGA.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Tested-by: Han Han <hhan@redhat.com>
There might be more potential users around, I haven't looked thoroughly.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Tested-by: Han Han <hhan@redhat.com>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Tested-by: Han Han <hhan@redhat.com>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Tested-by: Han Han <hhan@redhat.com>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Tested-by: Han Han <hhan@redhat.com>
To match the QGA schema name (we are introducing a qemuAgentDiskInfo
struct again for different purpose).
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Tested-by: Han Han <hhan@redhat.com>
I previously did a workaround for a glib event loop race
that causes crashes:
commit 0db4743645
Author: Daniel P. Berrangé <berrange@redhat.com>
Date: Tue Jul 28 16:52:47 2020 +0100
util: avoid crash due to race in glib event loop code
it turns out that the workaround has a significant performance
penalty on I/O intensive workloads. We thus need to avoid the
workaround if we know we have a new enough glib to avoid the
race condition.
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Tested-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
It was reported that the performance of tunnelled migration and
volume upload/download regressed in 6.9.0, when the virt-ssh-helper
is used for remote SSH tunnelling instead of netcat.
When seeing data available to read from stdin, or the socket,
the current code will allocate at most 1k of extra space in
the buffer it has.
After writing data to the socket, or stdout, if more than 1k
of extra space is in the buffer, it will reallocate to free
up that space.
This results in a huge number of mallocs when doing I/O, as
well as a huge number of syscalls since at most 1k of data
will be read/written at a time.
Also if writing blocks for some reason, it will continue to
read data with no memory bound which is bad.
This changes the code to use a 1 MB fixed size buffer in each
direction. If that buffer becomes full, it will update the
watches to stop reading more data. It will never reallocate
the buffer at runtime.
This increases the performance by orders of magnitude.
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Tested-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Even though it is technically possible, when running the migrations QEMU's
nbd-server-start errors out with:
"TLS is only supported with IPv4/IPv6"
We can always enable it when QEMU adds this feature, but for now it is safer to
show our error message rather than rely on QEMU to error out properly.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
When executing the hypervisor-cpu-baseline command and if there is
only a single CPU definition present in the XML file, then the
baseline handler will exit early and libvirt will print an unhelpful
message:
"error: An error occurred, but the cause is unknown"
This is due to no CPU definition ever being "baselined", since the
handler expects at least two CPU models.
Let's fix this by performing a CPU model expansion on the single CPU
definition and returning the result to the caller. This will also
ensure the CPU model's feature set is sane if any were provided in
the file.
Signed-off-by: Collin Walling <walling@linux.ibm.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
Check the provided CPU models against the CPU models
known by the hypervisor before baselining and print
an error if an unrecognized model is found.
Signed-off-by: Collin Walling <walling@linux.ibm.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
When executing the hypervisor-cpu-baseline command and the
XML file contains a CPU definition without a model name, or
an invalid CPU definition, then the commands will fail and
return an error message from the QMP response.
Let's clean this up by checking for a valid definition and
presence of a model name.
This code is copied from virCPUBaseline.
Signed-off-by: Collin Walling <walling@linux.ibm.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
Hypervisor-cpu-baseline requires the cpu-model-expansion
capability when expanding CPU model features if the
--features flag is provided.
Signed-off-by: Collin Walling <walling@linux.ibm.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
When libvirt added support for firewalld, we were unable to use
firewalld's higher level rules, because they weren't detailed enough
and could not be applied to the iptables FORWARD or OUTPUT chains
(only to the INPUT chain). Instead we changed our code so that rather
than running the iptables/ip6tables/ebtables binaries ourselves, we
would send these commands to firewalld as "passthrough commands", and
firewalld would run the appropriate program on our behalf.
This was done under the assumption that firewalld was somehow tracking
all these rules, and that this tracking was benefitting proper
operation of firewalld and the system in general.
Several years later this came up in a discussion on IRC, and we
learned from the firewalld developers that, in fact, adding iptables
and ebtables rules with firewalld's passthrough commands actually has
*no* advantage; firewalld doesn't keep track of these rules in any
way, and doesn't use them to tailor the construction of its own rules.
Meanwhile, users have been complaining for some time that whenever
firewalld is restarted on a system with libvirt virtual networks
and/or nwfilter rules active, the system logs would be flooded with
warning messages whining that [lots of different rules] could not be
deleted because they didn't exist. For example:
firewalld[3536040]: WARNING: COMMAND_FAILED:
'/usr/sbin/iptables -w10 -w --table filter --delete LIBVIRT_OUT
--out-interface virbr4 --protocol udp --destination-port 68
--jump ACCEPT' failed: iptables: Bad rule
(does a matching rule exist in that chain?).
(See https://bugzilla.redhat.com/1790837 for many more examples and a
discussion)
Note that these messages are created by iptables, but are logged by
firewalld - when an iptables/ebtables command fails, firewalld grabs
whatever is in stderr of the program, and spits it out to the system
log as a warning. We've requested that firewalld not do this (and
instead leave it up to the calling application to do the appropriate
logging), but this request has been respectfully denied.
But combining the two problems above ( 1) firewalld doesn't do
anything useful when you use it as a proxy to add/remove iptables
rules, 2) firewalld often insists on logging lots of
annoying/misleading/useless "error" messages when you use it as a
proxy to remove iptables rules that don't already exist), leads to a
solution - simply stop using firewalld to add and remove iptables
rules. Instead, exec iptables/ip6tables/ebtables directly in the same
way we do when firewalld isn't active.
We still need to keep track of whether or not firewalld is active, as
there are some things that must be done, e.g. we need to add some
actual firewalld rules in the firewalld "libvirt" zone, and we need to
take notice when firewalld restarts, so that we can reload all our
rules.
This patch doesn't remove the infrastructure that allows having
different firewall backends that perform their functions in different
ways, as that will very possibly come in handy in the future when we
want to have an nftables direct backend, and possibly a "pure"
firewalld backend (now that firewalld supports more complex rules, and
can add those rules to the FORWARD and OUTPUT chains). Instead, it
just changes the action when the selected backend is "firewalld" so
that it adds rules directly rather than through firewalld, while
leaving as much of the existing code intact as possible.
In order for tests to still pass, virfirewalltest also had to be
modified to behave in a different way (i.e. by capturing the generated
commandline as it does for the DIRECT backend, rather than capturing
dbus messages using a mocked dbus API).
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
When it is starting up, firewalld will delete all existing iptables
rules and chains before adding its own rules. If libvirtd were to try
to directly add iptables rules during the time before firewalld has
finished initializing, firewalld would end up deleting the rules that
libvirtd has just added.
Currently this isn't a problem, since libvirtd only adds iptables
rules via the firewalld "passthrough command" API, and so firewalld is
able to properly serialize everything. However, we will soon be
changing libvirtd to add its iptables and ebtables rules by directly
calling iptables/ebtables rather than via firewalld, thus removing the
serialization of libvirtd adding rules vs. firewalld deleting rules.
This will especially apparent (if we don't fix it in advance, as this
patch does) when libvirtd is responding to the dbus NameOwnerChanged
event, which is used to learn when firewalld has been restarted. In
that case, dbus sends the event before firewalld has been able to
complete its initialization, so when libvirt responds to the event by
adding back its iptables rules (with direct calls to
/usr/bin/iptables), some of those rules are added before firewalld has
a chance to do its "remove everything" startup protocol. The usual
result of this is that libvirt will successfully add its private
chains (e.g. LIBVIRT_INP, etc), but then fail when it tries to add a
rule jumping to one of those chains (because in the interim, firewalld
has deleted the new chains).
The solution is for libvirt to preface it's direct calling to iptables
with a iptables command sent via firewalld's passthrough command
API. Since commands sent to firewalld are completed synchronously, and
since firewalld won't service them until it has completed its own
initialization, this will assure that by the time libvirt starts
calling iptables to add rules, that firewalld will not be following up
by deleting any of those rules.
To minimize the amount of extra overhead, we request the simplest
iptables command possible: "iptables -V" (and aside from logging a
debug message, we ignore the result, for good measure).
(This patch is being done *before* the patch that switches to calling
iptables directly, so that everything will function properly with any
fractional part of the series applied).
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Even though *we* don't call ebtables/iptables/ip6tables (yet) when the
firewalld backend is selected, firewalld does, so these binaries need
to be there; let's check for them. (Also, the patch after this one is
going to start execing those binaries directly rather than via
firewalld).
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
iptables and ip6tables have had a "-w" commandline option to grab a
systemwide lock that prevents two iptables invocations from modifying
the iptables chains since 2013 (upstream commit 93587a04 in
iptables-1.4.20). Similarly, ebtables has had a "--concurrent"
commandline option for the same purpose since 2011 (in the upstream
ebtables commit f9b4bcb93, which was present in ebtables-2.0.10.4).
Libvirt added code to conditionally use the commandline option for
iptables/ip6tables in upstream commit ba95426d6f (libvirt-1.2.0,
November 2013), and for ebtables in upstream commit dc33e6e4a5
(libvirt-1.2.11, November 2014) (the latter actually *re*-added the
locking for iptables/ip6tables, as it had accidentally been removed
during a refactor of firewall code in the interim).
I say "conditionally" because a check was made during firewall module
initialization that tried executing a test command with the
-w/--concurrent option, and only continued using it for actual
commands if that test command completed successfully. At the time the
code was added this was a reasonable thing to do, as it had been less
than a year since introduction of -w to iptables, so many distros
supported by libvirt were still using iptables (and possibly even
ebtables) versions too old to have the new commandline options.
It is now 2020, and as far as I can discern from repology.org (and
manually examining a RHEL7.9 system), every version of every distro
that is supported by libvirt now uses new enough versions of both
iptables and ebtables that they all have support for -w/--concurrent.
That means we can finally remove the conditional code and simply
always use them.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
All the unit tests that use iptables/ip6tables/ebtables have been
written to omit the locking/exclusive use primitive on the generated
commandlines. Even though none of the tests actually execute those
commands (and so it doesn't matter for purposes of the test whether or
not the commands support these options), it still made sense when some
systems had these locking options and some didn't.
We are now at a point where every supported Linux distro has supported
the locking options on these commands for quite a long time, and are
going to make their use non-optional. As a first step, this patch uses
the virFirewallSetLockOverride() function, which is called at the
beginning of all firewall-related tests, to set all the bools
controlling whether or not the locking options are used to true. This
means that all the test cases must be updated to include the proper
locking option in their commandlines.
The change to make actual execs of the commands unconditionally use
the locking option will be in an upcoming patch - this one affects
only the unit tests.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
The feature is never enabled by default on KVM and QEMU dropped it from
the models long ago.
https://bugzilla.redhat.com/show_bug.cgi?id=1798004
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Tim Wiederhake <twiederh@redhat.com>
For backward compatibility with older versions of libvirt CPU models in
our CPU map are mostly immutable. We only changed them in a few specific
cases after showing it was safe. Sometimes QEMU developers realize a
specific feature should not be part of a particular (or any) CPU model
because it can never be enabled automatically without further
configuration. But we couldn't follow them because doing so would break
migration to older libvirt.
If QEMU drops feature F from CPU model M because F could not be enabled
automatically anyway, asking for M would never enable F. Even with older
QEMU versions. Naively removing F from libvirt's definition of M would
seem to work nicely on a single host. Libvirt would consider M to be
compatible with hosts CPU that do not support F. However, trying to
migrate domains using M without explicitly enabling or disabling F could
fail, because older libvirt would think F was enabled (it is part of M
there), but QEMU reports it as disabled once started.
Thus we can remove such feature from a libvirt's CPU model, but we have
to make sure any CPU definition using the affected model will always
explicitly mention the state of the removed feature.
https://bugzilla.redhat.com/show_bug.cgi?id=1798004
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Tim Wiederhake <twiederh@redhat.com>
The patch adds a new attribute for the 'feature' element in CPU model
specification to indicate that a given feature was removed from a CPU
model. In other words, older versions of libvirt would consider such
feature to be included in the CPU model.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Tim Wiederhake <twiederh@redhat.com>
This is just a preparation for adding new functionality to
virCPUx86Update.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Tim Wiederhake <twiederh@redhat.com>
Until now, the function returned immediately when the guest CPU
definition did not use optional features or minimum match. Clearly,
there's nothing to be updated according to the host CPU in this case,
but the arch specific code may still want to do some compatibility
updates based on the model and features used in the guest CPU
definition.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Tim Wiederhake <twiederh@redhat.com>
This new function adds a feature to a CPU definition only if it is not
present there yet.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Tim Wiederhake <twiederh@redhat.com>
Replace the 'update' bool parameter with an enum so that we can have
more than two possible values.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Tim Wiederhake <twiederh@redhat.com>
The function is supposed to add a feature to a CPU definition, let's
name it virCPUDefAddFeatureInternal. The behavior in case the feature is
already present in the CPU def is configurable and we will soon add a
new option to not do anything in that case, which wouldn't really work
well with the current *Update* name.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Tim Wiederhake <twiederh@redhat.com>
dumpxml can now serialize:
* floppy drives
* file-backed and device-backed disk drives
* images mounted to virtual CD/DVD drives
* IDE and SCSI controllers
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Co-authored-by: Sri Ramanujam <sramanujam@datto.com>
Signed-off-by: Matt Coleman <matt@datto.com>
Forgetting to use the VIR_MIGRATE_TLS flag with migration can lead to
leak of sensitive information. Add an administrative knob to force use
of the flag.
Note that without VIR_MIGRATE_PEER2PEER, the migration is driven by an
instance of the client library which doesn't necessarily run on either
of the hosts so the flag can't be used to assume VIR_MIGRATE_TLS even
if it wasn't provided by the user instead of rejecting if it's not.
Resolves: https://gitlab.com/libvirt/libvirt/-/issues/67
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
qemu's internals were not prepared for switching to -blockdev for the
legacy storage migration. Add a proper error message since qemu is
unlikely to attempt fixing the old protocol.
Resolves: https://gitlab.com/libvirt/libvirt/-/issues/65
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Move and aggregate all the logic which is switched based on whether the
migration is tunnelled or not before other checks. Further checks will
be added later.
While the code is being moved the error message is put on a single line
per new coding style.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Our streams are not the best transport for migration data and we support
TLS for security now. It's unlikely that there will be enough motivation
to add a new migration protocol to tunnel NBD too.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Similarly to previous commit dealing with snapshots we must rewrite the
metadata of the previously-'current' checkpoint when changing which
checkpoint is considered 'current'.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Whether a snapshot definition is considered 'current' or active is
stored in the metadata XML libvirt writes when we create metadata.
This means that if we are changing the 'current' snapshot we must
re-write the metadata of the previously 'current' snapshot to update the
field to prevent having multiple active snapshots.
Unfortunately the snapshot creation code didn't do this properly, which
resulted in the following error:
error : qemuDomainSnapshotLoad:430 : internal error: Too many snapshots claiming to be current for domain snapshot-test
being printed if libvirtd was terminated and restarted.
Introduce qemuSnapshotSetCurrent which writes out the old snapshot's
metadata when updating the current snapshot.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
In some cases such as when creating an internal inactive snapshot we
know that the domain definition in the snapshot is equivalent to the
current definition. Additionally we set up the current definition for
the snapshotting but not the one contained in the snapshot. Thus in some
cases the caller knows better which def to use.
Make qemuDomainSnapshotForEachQcow2 take the definition by the caller
and copy the logic for selecting the definition to callers where we
don't know for sure that the above claim applies.
This fixes internal inactive snapshots when <disk type='volume'> is used
as we translate the pool/vol combo only in the current def.
Resolves: https://gitlab.com/libvirt/libvirt/-/issues/97
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Don't try to manipulate snapshots on network or unresolved volume backed
storage.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
'continue' the loop if the device is not a disk. Saving the level makes
one of the error messages fit on a single line.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Commit 912c6b22fc added abort() when the
'val' parameter is NULL along with setting the error variable for the
command. We don't want to abort in this case, just set the error.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
When the host is shutting down then we get PrepareForShutdown
signal on DBus to which we react by creating a thread which
runs virStateStop() and thus qemuStateStop(). But if scheduling
the thread is delayed just a but it may happen that we receive
SIGTERM (sent by systemd) to which we respond by quitting our
event loop and cleaning up everything (including drivers). And
only after that the thread gets to run only to find qemu_driver
being NULL.
What we can do is to delay exiting event loop and join the thread
that's executing virStateStop(). If the join doesn't happen in
given timeout (currently 30 seconds) then libvirtd shuts down
forcefully anyways (see virNetDaemonRun()).
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1895359
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1739564
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The ESP SCSI controllers (NCR53C90, DC390, AM53C974) have the same
requirement as the LSI Logic controller for each disk to be set via
the scsi-id=NNN property, not the lun=NNN property.
Switching the code to use an enum will force authors to pay attention
to this difference when adding future SCSI controllers.
Reviewed-by: Laine Stump <laine@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
When introducing the API I've mistakenly used 'int' type for
@nkeys argument which does nothing more than tells the API how
many items there are in @keys array. Obviously, negative values
are not expected and therefore 'unsigned int' should have been
used.
Reported-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
The NCR53C90 is the built-in SCSI controller on all sparc machine types,
but not sparc64. Note that it has the fixed alias "scsi", which differs
from our normal naming convention of "scsi0".
The DC390 and AM53C974 are PCI SCSI controllers that can be added to any
PCI machine.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Probing for the NCR53C90 controller is a little unusual. The
qom-list-types QMP command returns a list of all types known to
the QEMU binary. It does not distinguish devices which are user
creatable from those which are built-in.
Any QEMU target that supports PCI will have the DC390 / AM53C974
devices because they are PCI based. Due to code dependencies
in QEMU though, existence of these two devices will also pull in
the NCR53C90 device (called just 'esp' in QEMU). The NCR53C90 is
not user-creatable and can only be used when built-in to the
machine type.
This is only the case on sparc machines, and certain mips64 and
m68k machines. IOW, we don't rely on qom-list-types as a guide
for existence of NCR53C90, as it shouldn't really exist in most
QEMU binaries.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The NCR53C90 is the built-in SCSI controller on all sparc machine types,
and some mips and m68k machine types.
The DC390 and AM53C974 are PCI SCSI controllers that can be added to any
PCI machine.
These are only interesting for emulating obsolete hardware platforms.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The NCR53C90 ESP SCSI controller is only usable when built-in to the
machine type. This method will facilitate checking that restriction
across many places.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The sparc machines have little in common with sparc64 machines.
No sparc machine type includes a PCI bus, so we should not be adding one
to the XML. This further means that we should not be adding a memory
balloon device, nor USB controller as these are both PCI based.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
We are generating a fresh UUID and storing it in the XML for the
default network, but this is unnecessary because the network
driver will automatically generate one if it's missing from the
XML; the fact that we only do this if the uuidgen command happens
to be available on the build machine is further proof that we can
safely skip this step.
This patch is best viewed with 'git show -w'.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Laine Stump <laine@redhat.com>
While we generally expect libvirt objects to be defined using the
appropriate APIs, there are cases where it's reasonable for an
external entity, usually a package manager, to drop a valid
configuration file under /etc/libvirt and have libvirt take over
from there: notably, this is exactly how the default network is
handled.
For the most part, whether the configuration is saved back to disk
after being parsed by libvirt doesn't matter, because we'll end up
with the same values anyway, but an obvious exception to this is
data that gets randomly generated when not present, namely MAC
address and UUID.
Historically, both were handled by our build system, but commit
a47ae7c004 moved handling of the former inside libvirt proper;
this commit extends such behavior to the latter as well.
Proper error handling for the virNetworkSaveConfig() call, which
was missing until now, is introduced in the process.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Laine Stump <laine@redhat.com>
We jump to the error label if the 'if' condition is true.
Remove the explicit else to make it more obvious that 'hostname'
is filled on both branches of 'if (!uri_in)'.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
In case no uri_in was supplied, we forgot to set the hostname
to the current hostname and formatted a useless uri_out.
src/util/glibcompat.h:57:26: error: ‘%s’ directive argument is null [-Werror=format-overflow=]
57 | # define g_strdup_printf vir_g_strdup_printf
src/openvz/openvz_driver.c:2136:16: note: in expansion of macro ‘g_strdup_printf’
2136 | *uri_out = g_strdup_printf("ssh://%s", hostname);
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reported-by: Jaroslav Suchanek <jsuchane@redhat.com>
Fixes: e3c626a61d
Reviewed-by: Erik Skultety <eskultet@redhat.com>
The way our domain capabilities work currently, is that we have
virDomainCapsEnum struct which contains 'unsigned int values'
member which serves as a bitmask. More complicated structs are
composed from this struct, giving us whole virDomainCaps
eventually.
Whenever we want to report that a certain value is supported, the
'1 << value' bit is set in the corresponding unsigned int member.
This works as long as the resulting value after bitshift does not
overflow unsigned int. There is a check inside
virDomainCapsEnumSet() which ensures exactly this, but no caller
really checks whether virDomainCapsEnumSet() succeeded. Also,
checking at runtime is a bit too late.
Fortunately, we know the largest value we want to store in each
member, because each enum of ours ends with _LAST member.
Therefore, we can check at build time whether an overflow can
occur.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Using virtCgroupNewSelf() is not correct with cgroups v2 because the
the virt-host-validate process is executed from from the same cgroup
context as the terminal and usually not all controllers are enabled
by default.
To do a proper check we need to use the root cgroup to see what
controllers are actually available. Libvirt or systemd ensures that
all controllers are available for VMs as well.
This still doesn't solve the devices controller with cgroups v2 where
there is no controller as it was replaced by eBPF. Currently libvirt
tries to query eBPF programs which usually works only for root as
regular users will get permission denied for that operation.
Fixes: https://gitlab.com/libvirt/libvirt/-/issues/94
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
In QEMU 5.2, the guest agent learned to manipulate a user
~/.ssh/authorized_keys. Bind the JSON API to libvirt.
https://wiki.qemu.org/ChangeLog/5.2#Guest_agent
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Since both APIs accept/return an array of strings we can't have
client/server dispatch code generated. But implementation is
fairly trivial, although verbose.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
When setting up a new guest or when a management software wants
to allow access to an existing guest the
virDomainSetUserPassword() API can be used, but that might be not
good enough if user want to ssh into the guest. Not only sshd has
to be configured to accept password authentication (which is
usually not the case for root), user have to type in their
password. Using SSH keys is more convenient. Therefore, two new
APIs are introduced:
virDomainAuthorizedSSHKeysGet() which lists authorized keys for
given user, and
virDomainAuthorizedSSHKeysSet() which modifies the authorized
keys file for given user (append, set or remove keys from the
file).
It's worth nothing that while authorized_keys file entries have
some structure (as defined by sshd(8)), expressing that structure
goes beyond libvirt's focus and thus "keys" are nothing but an
opaque string to libvirt.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
This reverts commit b3710e9a2a.
That check is very valuable for our code, but it causes issue with glib >=
2.67.0 when building with clang.
The reason is a combination of two commits in glib, firstly fdda405b6b1b which
adds a g_atomic_pointer_{set,get} variants that enforce stricter type
checking (by removing an extra cast) for compilers that support __typeof__, and
commit dce24dc4492d which effectively enabled the new variant of glib's atomic
code for clang. This will not be necessary when glib's issue #600 [0] (8 years
old) is fixed. Thankfully, MR #1719 [1], which is supposed to deal with this
issue was opened 3 weeks ago, so there is a slight sliver of hope.
[0] https://gitlab.gnome.org/GNOME/glib/-/issues/600
[1] https://gitlab.gnome.org/GNOME/glib/-/merge_requests/1719
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Similarly to previous commits, we can utilize domCaps to check if
graphics type is supported.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
QEMU supports egl-headless if QEMU_CAPS_EGL_HEADLESS capability
is present. There are some additional requirements but those are
checked for in qemuValidateDomainDeviceDefGraphics() and depend
on domain configuration and thus are not representable in domain
capabilities. Let's stick with plain qemuCaps check then.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
In my recent commit of 5216304bfe I've moved RNG model check
from domain capabilities validator into qemu validator. During
that I had to basically duplicate RNG model to qemuCaps checks.
Problem with this approach is that after my commit qemu validator
and domCaps are disconnected and thus domCaps might report (in
general) different set of supported RNG models.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
In my recent commit of a33279daa8 I've moved video model check
from domain capabilities validator into qemu validator. During
that I had to basically duplicate video model to qemuCaps checks.
Problem with this approach is that after my commit qemu validator
and domCaps are disconnected and thus domCaps might report (in
general) different set of supported video models.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
This is a convenient macro for querying whether particular domain
caps enum value is set or not.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
The helper methods for getting integer properties ignore a missing
property setting its value to zero. This lack of error reporting
resulted in missing the regression handling hotplug of USB devices
with the vendor and model IDs getting set to zero silently.
The few callers which relied on this silent defaulting have been fixed,
so now we can report fatal errors immediately.
Reviewed-by: Laine Stump <laine@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The access of DKD_MEDIA_AVAILABLE for floppy disks, is mistakenly
protected by a check for ID_CDROM_MEDIA, introduced in:
commit 10427db779
Author: Ján Tomko <jtomko@redhat.com>
Date: Fri Jun 3 16:10:21 2016 +0200
Only return two values in udevGetUintProperty
Thus the check of DKD_MEDIA_AVAILABLE never run. In practice this didn't
matter since this property is set by the DeviceKit-Disks daemon which
was only around for 3 Fedora releases before being killed off around
F13. Thus we can just remove this legacy property.
Reviewed-by: Laine Stump <laine@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The udevProcessStorage method relies on udevGetIntProperty ignoring
errors about non-existant properties and instead setting the value to
zero. In theory when seeing ID_CDROM=1, you might expect that devices
which are not CDs will get ID_CDROM=0, but that's not what happens in
practice. Instead the property simply won't get set at all.
IOW, the code does not need to care about the value of the property,
merely whether it exists or not.
Reviewed-by: Laine Stump <laine@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
'kvm-spice' is a binary name used to call 'kvm' which actually is a wrapper
around qemu-system-x86_64 enabling kvm acceleration. This isn't in use
for quite a while anymore, but required to work for compatibility e.g.
when migrating in old guests.
For years this was a symlink kvm-spice->kvm and therefore covered
apparmor-wise by the existing entry:
/usr/bin/kvm rmix,
But due to a recent change [1] in qemu packaging this now is no symlink,
but a wrapper on its own and therefore needs an own entry that allows it
to be executed.
[1]: https://salsa.debian.org/qemu-team/qemu/-/commit/9944836d3
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
Reviewed-by: Michal Privoznik <mprivozn redhat com>
QEMU version 4.2 introduced a performance feature under commit
d645e13287 ("kvm: i386: halt poll control MSR support").
This patch adds a new KVM feature 'poll-control' to set this performance
hint for KVM guests. The feature is off by default.
To enable this hint and have libvirt add "-cpu host,kvm-poll-control=on"
to the QEMU command line, the following XML code needs to be added to the
guest's domain description:
<features>
<kvm>
<poll-control state='on'/>
</kvm>
</features>
Signed-off-by: Tim Wiederhake <twiederh@redhat.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
In v6.3.0-rc1~67 I've made a switch: instead of listening on udev
events the nodedev driver started listening for kernel events.
This was because when a device changes its name (e.g. NICs) we
will get "move" event with DEVPATH_OLD property set, which we can
then use to remove the old device and thus keep our internal list
up to date. The switch to "kernel" source was made because if the
old NICs naming (eth0, eth1, ...) is enabled (e.g. via
net.ifnames=0 on the kernel cmd line) then udev overwrites the
property with the new name making our internal list go out of
sync. Interestingly, when the od NICs naming is not enabled then
the DEVPATH_OLD contains the correct value.
But as it turns out, "kernel" source might be missing some other
important properties, e.g. USB vendor/product IDs. Therefore,
switch back to "udev" source and wish the best of luck to users
using the old NICs naming.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1897625
Fixes: 9a13704818
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Now that the domCaps cache is history, this code is no longer
used and thus can be removed.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Currently, whenever a domain capabilities is needed (fortunately,
after cleanup done by previous commits it is now only in
virConnectGetDomainCapabilities()), the object is stored in a
cache. But there is no invalidation mechanism for the cache
(except the implicit one - the cache is part of qemuCaps and thus
share its lifetime, but that is not enough). Therefore, if
something changes - for instance new firmware files are
installed, or old are removed these changes are not reflected in
the virConnectGetDomainCapabilities() output.
Originally, the caching was there because domCaps were used
during device XML validation and they were used a lot from our
test suite. But this is no longer the case. And therefore, we
don't need the cache and can construct fresh domCaps on each
virConnectGetDomainCapabilities() call.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1807198
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Now that nothing uses virDomainCapsDeviceDefValidate() it can be
removed.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
The aim is to eliminate virDomainCapsDeviceDefValidate(). And in
order to do so, the domain video model has to be validated in
qemuValidateDomainDeviceDefVideo().
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
The aim is to eliminate virDomainCapsDeviceDefValidate(). And in
order to do so, the domain RNG model has to be validated in
qemuValidateDomainRNGDef().
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
This is a Coverity fix pointed out by John in IRC. This code
was introduced in 19d74fdf0e, when the TPM Proxy device for
for ppc64 was introduced.
This will leak in case we have 2 TPMs in the same domain, a
possible scenario with the protected Ultravisor execution in
PowerPC guests.
Fixes: 19d74fdf0e
Reported-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Introduced by commit <22494556542c676d1b9e7f1c1f2ea13ac17e1e3e> which
fixed a CVE.
If the @path passed to virDMSanitizepath() is not a DM name or not a
path to DM name this function could return incorrect sanitized path as
it would always be the first device under /dev/mapper/.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Commit <2f3b7a5555c4cf4127ff3f8e00746eafcc91432c> replaced VIR_STRDUP
by g_strdup which made the error: path mostly useless.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
The issue was introduced together with the function itself by commit
<da1eba6bc8f58bfce34136710d1979a3a44adb17>. Calling
`virDomainObjGetPersistentDef` may return NULL which is later passed
to `virDomainDefFormat` where the `def` attribute is marked as NONNULL
and later in `virDomainDefFormatInternalSetRootName` it is actually
defererenced without any other check.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Leftover after commit <479a8c1fa1e0f58d3165c0446cd1abd72160256e>.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Libvirt's backup code has two modes:
1) push - where qemu actively writes the difference since the checkpoint
into the output file
2) pull - where we instruct qemu to expose a frozen disk state along
with a bitmap of blocks which changed since the checkpoint
For push mode qemu needs the temporary bitmap we use where we calculate
the actual changes to be present on the block node backing the disk.
For pull mode where we expose the bitmap via NBD qemu actually wants the
bitmap to be present for the exported block node which is the scratch
file.
Until now we've calculated the bitmap twice and installed it both to the
scratch file and to the disk node, but we don't need to since we know
when it's needed.
Pass in the 'pull' flag and decide where to install the bitmap according
to it and also when to register the bitmap name with the blockjob.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
The NBD server used to export pull-mode backups doesn't have any other
form of client authentication on top of the TLS transport, so the only
way to authenticate clients is to verify their certificate.
Enable this option by defauilt when both 'backup_tls_x509_verify' and
'default_tls_x509_verify' were not configured.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1879477
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
The migration stream connection and also the NBD server for non-shared
storage migration don't have any other form of client authentication on
top of the TLS transport, so the only way to authenticate clients is to
verify their certificate.
Enable this option by defauilt when both 'migrate_tls_x509_verify' and
'default_tls_x509_verify' were not configured.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1879477
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Chardevs don't have any other form of client authentication on top of
the TLS transport, so the only way to authenticate clients is to verify
their certificate.
Enable this option by defauilt when both 'chardev_tls_x509_verify' and
'default_tls_x509_verify' were not configured.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1879477
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
If both "vnc_tls_x509_verify" and "default_tls_x509_verify" are missing
from the config file the client certificate validation is disabled. VNC
provides a layer of authentication so client certificate validation is
not strictly required.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Store whether "default_tls_x509_verify" was provided and enhance the
SET_TLS_VERIFY_DEFAULT macro so that indiviual users can provide their
own default if "default_tls_x509_verify" config option was not provided.
For now we keep setting it to 'false'.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Typecast the controller type variable to the appropriate type and add
the missing controller types for future extension.
Note that we currently allow only unplug of
VIR_DOMAIN_CONTROLLER_TYPE_SCSI thus the other controller types which
are not implemented return false now.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Iterate through hostdevs only when the controller type is
VIR_DOMAIN_CONTROLLER_TYPE_SCSI.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The tests which match the disk bus to the controller type were backwards
in this function. This meant that any disk bus type (such as
VIR_DOMAIN_DISK_BUS_SATA) would not skip the controller index comparison
even if the removed controller was of a different type.
Switch the internals to a switch statement with selects the controller
type in the first place and a proper type so that new controller types
are added in the future.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1870072
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
While it's certainly good to log events like "failed to close fd"
and "tried to close invalid fd", which are likely to be the
consequence of some bug in libvirt, logging a message every single
time a file descriptor is closed successfully is perhaps excessive
and can lead to useful information being missed among the noise.
Log filters don't help in this situation, because filtering out all
of util.file is too big a hammer and would cause important messages
to be left out as well.
To give an idea of just how much noise this single debug statement
can cause, here's a real life example from a quite large libvirtd
log I had to look at recently:
$ grep virFile libvirt.log | wc -l
1307
$ grep virFile libvirt.log | grep -v 'Closed fd' | wc -l
343
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Before commit 24d8968c, virDirClose took a DIR**, and that was never
NULL, so its declaration included ATTRIBUTE_NONNULL(1). Since that
commit, virDirClose takes a DIR*, and it may be NULL (e.g. if the DIR*
is initialized to NULL and was never closed).
Even though virDirClose() is currently only called implicitly (as the
cleanup for a g_autoptr(DIR)), and (as I've just newly learned) the
autocleanup function g_autoptr will only be called if the pointer in
question is non-null (see the definition of
_GLIB_AUTOPTR_CLEAR_FUNC_NAME in
/usr/include/glib-2.0/glib/gmacros.h), it does still cause Coverity to
complain that it *could* be called with a NULL, and it's also possible
that in the future someone might add code that explicitly calls
virDirClose.
To eliminate the Coverity complaints, and protect against the
hypothetical future where someone both explicitly calls virDirClose()
with a potentially NULL value, *and* re-enables the nonnull directive
when not building with Coverity (disabled by commit eefb881) this
patch removes the ATTRIBUTE_NONNULL(1) from the declaration of
virDirClose().
Fixes: 24d8968cd0
Reported-by: John Ferlan <jferlan@redhat.com>
Details-Research-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Laine Stump <laine@redhat.com>
A bad merge while rebasing 74b2834333 caused the @event variable
to be defined twice, inside the 'cleanup' label, causing coverity
errors.
This code was originally moved outside of the label by commit
773c7c4361. Delete the unintended code in the 'cleanup'
label.
Fixes: 74b2834333
Reported-by: John Ferlan <jferlan@redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
After e4c29e2904 the function has one argument more and the
argument that can't be NULL moved from second to third position.
Reported-by: John Ferlan <jferlan@redhat.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Use g_autoptr() and remove the 'cleanup' label.
Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Use VIR_AUTOCLOSE with 'fd' and delete the 'cleanup' label.
Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Use g_autoptr() to deprecate the 'cleanup' label.
Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Remove obsolete 'cleanup' labels after the changes from the
previous patch.
Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Some labels became deprecated after the previous patches.
Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Due to failures to unlink on previous rename/undefine we can already have
autolink etc files for the domain to be defined. Remove them.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Let's move objlist restoring to cleanup section so that we can handle failure
of actions between virDomainObjListAdd and virDomainDefSave. We are going
to add such actions in next patch.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
If domain name is changed since snapshot we need to update it to current in
config taken from snapshot.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
This is basically just saves checkpoints metadata on disk after name is changed
in memory as path to domain checkpoints directory depends on name. After that
old checkpoint directory is deleted with checkpoint metadata files.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
This is basically just saves snapshots metadata on disk after name is changed
in memory as path to domain snapshot directory depends on name. After that
old snapshot directory is deleted with snapshot metadata files.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
This patch also changes functionality a bit.
First if unlinking of old config file is failed we rollback and return error
previously and now we return success. I don't think this makes much difference.
I guess in both cases on libvirtd restart we have to deal with both new and old
config existing on disk with different names but same uuid.
Second if unlinking of old autolink is failed we rollback previously which
was not right as at this point we already unlink old config file. So this
is fixed now.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Going to cleanup label is mere return -1 thus let's just return
instead of goto to this label.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
