It is only used for debug and error purposes which can be easily
replaced by @placement.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
With cgroups v2 working with controllers is a bit more complicated then
with cgroups v1 where the controller had to be mounted.
There are two files, cgroups.controllers and cgroup.subtree_control.
The file cgroup.controllers lists all controllers enabled in the current
cgroup and cgroups.subtree_control, as the name suggest, controls which
controllers are enabled for a subtree of cgroups.
Now the issue here is that the current code doesn't make any difference
if the @parent variable is NULL or not because ../cgroup.subtree_control
will list the same controllers as ./cgroup.controllers.
The whole point of the @parent variable is when we are building the
cgroup topology ourselves without systemd help we need to detect which
controllers are enabled in the parent cgroup in order to enable them for
the current cgroup as well and for that we need to check
cgroup.controllers of the parent group.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
When libvirtd starts a VM it internally stores a path to the main
cgroup. When we restart libvirtd we should get to the same state.
When we start a VM on host with systemd the cgroup is created for us and
the process is already placed into that cgroup and we detect the path
created by systemd using /proc/$PID/cgroup. After that we create
sub-cgroups and move all threads there.
Once libvirtd is restarted we again detect the cgroup path using
/proc/$PID/cgroup, but in this case we will get a different path because
the main thread was moved to a "emulator" cgroup.
Instead of ignoring the "emulator" directory when validating cgroups
remove it completely when detecting cgroup otherwise cgroups will not
work properly when libvirtd is restarted.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
With cgroups v2 the file cgroup.procs will never be empty if threading
is enabled as it will always have ID of all processes even if all
threads of the processes are moved to sub-cgroups. If that happens the
file cgroup.threads will be empty.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
After converting all DIR* to g_autoptr(DIR), many cleanup: labels
ended up just having "return ret", and every place that set ret would
just immediately goto cleanup. Remove the cleanup label and its
return, and just return the set value immediately, thus eliminating
the need for the return variable itself.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Once the DIR* in virPCIGetName() was made g_autoptr, the cleanup:
label just had a "return ret;", but the rest of the function was more
compilcated than it needed to be, doing funky things with the value of
ret inside multi-level conditionals and a while loop that might exit
early via a break with ret == 0 or exit early via a goto cleanup with
ret == -1.
It really didn't need to be nearly as complicated. After doing the
trivial replacements of "goto cleanup" with appropriate direct
returns, it became obvious that:
1) the outermost level of the nested conditional at the end of the
function ("if (ret < 0)") was now redundant, since ret is now
*always* < 0 by that point (otherwise the function has returned).
2) by switching the sense of the next level of the conditional (making
it "if (!physPortID)", the "else" (which is now just "return 0;"
becomes the "if", and the new "else" no longer needs to be inside
the conditional.
3) the value of firstEntryName can be moved into *netname with
g_steal_pointer()
Once that is all done, ret is no longer used and can be removed.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Since every single use of DIR* was converted to use g_autoptr, this
function is not currently needed. Even if someone comes up with a
usage for a non-g_autoptr DIR* in the future, they can just use
virDirClose(), since there is no longer a semantic difference between
the two (VIR_DIR_CLOSE() previously had an extra & on the pointer so
that it could be transparently passed as a DIR** to virDirClose(), but
that was removed several commits back.)
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
All of these conversions are trivial - VIR_DIR_CLOSE() (aka
virDirClose()) is called only once on the DIR*, and it happens just
before going out of scope.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
In order to make a usable g_autoptr(DIR), we need to have a close
function that is a NOP when the pointer is NULL, but takes a simple
DIR*. But virDirClose() (candidate to be the g_autoptr cleanup
function) currently takes a DIR**, not DIR*. It does this so that it
can clear the pointer, thus making it safe to call virDirClose on the
same DIR multiple times.
In the past the clearing of the DIR* was essential in a few places,
but those few places have now been changed, so we can modify
virDirClose() to take a DIR*, and remove the side effect of clearing
the DIR*. This will make it directly usable as the g_autoptr cleanup,
and will mean that this:
{
DIR *dirp = NULL;
blah blah ...
VIR_DIR_CLOSE(dirp)
}
is functionally identical to
{
g_autoptr(DIR) dirp = NULL;
blah blah ...
}
which will make conversion to using g_autoptr mechanical and simple to review.
(Note that virDirClose() will still check for NULL before attempting
to close, so that it can always be safely called, as long as the DIR*
was initialized to NULL (another prerequisite of becoming a g_autoptr
cleanup function)
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
DIR *dh is being re-used each time through the for loop of this
function, so it must be closed and then re-opened, which means we
can't convert it to g_autoptr. By moving the definition of dh inside
the for loop, we make it possible to trivially convert to g_autoptr
(which will happen in a subsequent patch)
NB: VIR_DIR_CLOSE() is already called at the bottom of the for loop,
so removing the VIR_DIR_CLOSE() at the end of the function is *not*
creating a leak of a DIR*!
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
This will make it easier to review upcoming patches that use g_autoptr
to auto-close all DIRs.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
The cpu mask was free()'d immediately on any error and at the end of the
function, where it was expected that it would either error out and return or
goto another allocation if the code was to fail. However since commit
9514e24984 the error path did not return in one new case which caused
double-free in such situation. In order to make the code more straightforward
just free the mask after it's been used even before checking the return code of
the call.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1819801
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Since we use virHashTable for string-keyed values only, we can remove
all the callbacks which allowed universal keys.
Code which wishes to use non-string keys should use glib's GHashTable.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
All users of virHashTable pass strings as the name/key of the entry.
Make this an official requirement by turning the variables to 'const
char *'.
For any other case it's better to use glib's GHashTable.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
The only place we call it is in virHashNew. Move the code to virHashNew
and remove virHashCreateFull.
Code wishing to use non-strings as hash table keys will be better off
using glib's GHashTable directly.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
It doesn't make much sense to configure the bucket count in the hash
table for each case specifically. Replace all calls of virHashCreate
with virHashNew which has a pre-set size and remove virHashCreate
completely.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
Remove 'cleanup' label and simplify remembering of the returned value
from the callback.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
Rewrite using GHashTable which already has interfaces for using a number
as hash key. Glib's implementation doesn't copy the key by default, so
we need to allocate it, but overal the interface is more suited for this
case.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
It's used only in one place in tests which isn't even automatically
evaluated.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
virCgroupKillRecursive sneakily initializes 'ret' to 0 rather than the
usual -1. 401030499b moved an error condition but didn't actually
modify 'ret' return the proper error code.
Fixes: 401030499b
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
virPCIDeviceAddressGetSysfsFile() is simpler to call.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
These were nops once enough cleanup was g_auto'd.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
thisPhysPortID is only used inside a conditional, so reduce its scope
to just the body of that conditional, which will eliminate the need
for the undesirable manual VIR_FREE().
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
This function had a loop that was only executed twice; it was
artificially constructed with a label, a goto, and a boolean to tell
that it had already been executed once. Aside from that, the body of
the loop contained only two lines that needed to be repeated (the
second time through, everything beyond those two lines would be
skipped).
One side effect of this strange loop was that a g_autofree string was
manually freed and re-initialized; I've been told that manually
freeing a g_auto_free object is highly discouraged.
This patch refactors the function to simply repeat the 2 lines that
might possibly be executed twice, thus eliminating the ugly use of
goto to construct a loop, and also takes advantage of the fact that
virPCIDriverDir() was previously returning *exactly* the same string
both times it was called to eliminate the manual VIR_FREE of drvpath.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
There is no need for a temporary variable in this function, and since
it can't return NULL, no need for callers to check for it.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
There is no need for a temporary variable in this function, and ever
since we switched to glib for memory allocation, there is no possibility
it can return NULL, so callers don't need to check for it.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
When this function was recently changed to add in parsing of
IFLA_VF_STATS, I noticed that the checks for existence of IFLA_VF_MAC
and IFLA_VF_VLAN were looking in the *wrong array*. The array that
contains the results of parsing each IFLA_VFINFO in
tb[IFLA_VFINFO_LIST] is tb_vf, but we were checking for these in tb
(which is the array containing the results of the toplevel parsing of
the netlink message, *not* the results of parsing one of the nested
IFLA_VFINFO's.
This incorrect code has been here since the function was originally
written in 2012. It has only worked all these years due to coincidence
- the items at those indexes in tb are IFLA_ADDRESS and IFLA_BROADCAST
(of the *PF*, not of any of its VFs), and those happen to always be
present in the toplevel netlink message; since we are only looking in
the incorrect place to check for mere existence of the attribute (but
are doing the actual retrieval of the attribute from the correct
place), this bug has no real consequences other than confusing anyone
trying to understand the code.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
virNetDevParseVfConfig has became a multifunctional helper function,
rename it to virNetDevParseVfInfo.
Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
Reviewed-by: Laine Stump <laine@redhat.com>
libvirt can retrieve traffic stats for emulated interfaces that are
backed by tap or macvtap devices, but this information wasn't
available for hostdev interfaces (those that are implemented by
assigning an SR-IOV VF device to a guest using vfio):
#virsh domifstat instance --interface=52:54:00:2d:b2:35
error: Failed to get interface stats instance 52:54:00:2d:b2:35
error: internal error: Interface name not provided
For some SR-IOV VF devices this information is available via the
netlink VFINFO_LIST request/response, and that is what this patch uses
to implement stats retrieval for VF. Not that this is dependent on
support in the PF driver - for example, the Mellanox ConnectX-4 Lx
(mlx5) driver reports usable stats, while Intel 82599 (ixgbe) and
82576 (igb) just report all stats as 0. (this is the same result as
"ip -s link show").
Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
Reviewed-by: Laine Stump <laine@redhat.com>
By default, pfifo_fast queueing discipline (qdisc) is set on
newly created interfaces (including TAPs). This qdisc has three
queues and packets that want to be sent through given NIC are
placed into one of the queues based on TOS field. Queues are then
emptied based on their priority allowing interactive sessions
stay interactive whilst something else is downloading a large
file.
Obviously, this means that kernel has to be involved and some
locking has to happen (when placing packets into queues). If
virtualization is taken into account then the above algorithm
happens twice - once in the guest and the second time in the
host.
This is arguably not optimal as it burns host CPU cycles
needlessly. Guest already made it choice and sent packets in the
order it wants.
To resolve this, Linux kernel offers 'noqueue' qdisc which can be
applied on virtual interfaces and in fact for 'lo' it is by
default:
lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue
Set it for other TAP devices we create for domains too. With this
change I was able to squeeze 1Mbps more from a macvtap attached
to a guest and to my 1Gbps LAN (as measured by iperf3).
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1329644
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
This helper changes the root qdisc on given interface.
Ideally, it would be written using netlink but my attempts to
write the code were not successful and thus I've fallen back to
virCommand() + tc.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Currently setting max_len=0 causes virtlogd to spin in a busy loop. It
is natural to allow this to disable log rollover which can be useful for
developers debugging things.
Note disabling rollover exposes the host to denial of service from a
malicious guest, so must be used with care.
Closes https://gitlab.com/libvirt/libvirt/-/issues/85
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The aim of virSocketAddrPrefixToNetmask() is to initialize passed
virSocketAddr structure based on prefix length and family.
However, it doesn't set all members in the struct which may lead
to reads of uninitialized values:
==15421== Use of uninitialised value of size 8
==15421== at 0x50F297A: _itoa_word (in /lib64/libc-2.31.so)
==15421== by 0x510C8FE: __vfprintf_internal (in /lib64/libc-2.31.so)
==15421== by 0x5120295: __vsnprintf_internal (in /lib64/libc-2.31.so)
==15421== by 0x50F8969: snprintf (in /lib64/libc-2.31.so)
==15421== by 0x51BB602: getnameinfo (in /lib64/libc-2.31.so)
==15421== by 0x496DEE0: virSocketAddrFormatFull (virsocketaddr.c:486)
==15421== by 0x496DD9F: virSocketAddrFormat (virsocketaddr.c:444)
==15421== by 0x11871F: networkDnsmasqConfContents (bridge_driver.c:1404)
==15421== by 0x1118F5: testCompareXMLToConfFiles (networkxml2conftest.c:48)
==15421== by 0x111BAF: testCompareXMLToConfHelper (networkxml2conftest.c:112)
==15421== by 0x112679: virTestRun (testutils.c:142)
==15421== by 0x111D09: mymain (networkxml2conftest.c:144)
==15421== Uninitialised value was created by a stack allocation
==15421== at 0x1175D2: networkDnsmasqConfContents (bridge_driver.c:1056)
All callers expect the function to initialize the structure
fully.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Laine Stump <laine@redhat.com>
As preparation for g_autoptr() we need to change the function to take
only virCgroupPtr.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>
We don't use the lib prefix for all libraries but in these cases it
makes sense to use the prefix.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Validation is usually performed on an entire document. If we are only
interested in validating a single nested node that can occur in
different contexts, this would require writing different schemas for
any of those different contexts.
By temporarily replacing the document's root node, we can validate the
relevant node only.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
There are only 3 places using the function. Two can use virBitmapNewCopy
directly. In case of the qemu capabilities code we need to free the old
bitmap first.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
virBitmapCopy has a failure condition, which is impossible to meet when
creating a new copy. Copy the contents directly to make it obvious that
virBitmapNewCopy can't fail.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
We no longer report any errors so all callers can be replaced by
virBitmapNew. Additionally virBitmapNew can't return NULL now so error
handling is not necessary.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
We now always return a valid pointer or crash so the return value
doesn't need to be checked.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Modify the condition which would make virBitmapNewQuiet fail to possibly
overallocate by 1 rather than failing.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
We now have APIs which automatically expand the bitmap and also API
which allocates a 0 size bitmap. Remove the condition from virBitmapNew.
Effectively reverts ce49cfb48a
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
virBitmapNewEmpty() can create a bitmap with 0 length. With such a
bitmap virBitmapToString will return NULL rather than an empty string.
Initialize the buffer to avoid that.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Clarify which bit is considered most significant in the bitmap and
resulting string. Also be explicit that it's a hex string.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
There's only one combination used so we can remove the rest.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
When VIR_EXEC_DAEMON is true and cmd->pidfile exists, the parent
will expect the pidfile to be written before exiting, sitting
tight in a saferead() call waiting.
The child then does process tuning (via virProcessSet* functions)
before writing the pidfile. Problem is that these tunings can
fail, and trigger a 'fork_error' jump, before cmd->pidfile is
written. The result is that the process was aborted in the
child, but the parent is still hang in the saferead() call.
This behavior can be reproduced by trying to create and execute
a QEMU guest in user mode (e.g. using qemu:///session as non-root).
virProcessSetMaxMemLock() will fail if the spawned libvirtd user
process does not have CAP_SYS_RESOURCE capability. setrlimit() will
fail, and a 'fork_error' jump is triggered before cmd->pidfile
is written. The parent will hung in saferead() indefinitely. From
the user perspective, 'virsh start <guest>' will hang up
indefinitely. CTRL+C can be used to retrieve the terminal, but
any subsequent 'virsh' call will also hang because the previous
libvirtd user process is still there.
We can fix this by moving all virProcessSet*() tuning functions
to be executed after cmd->pidfile is taken care of. In the case
mentioned above, this would be the result of 'virsh start'
after this patch:
error: Failed to start domain vm1
error: internal error: Process exited prior to exec: libvirt: error :
cannot limit locked memory to 79691776: Operation not permitted
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1882093
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Lack of this one function (which is called for each active tap device
every time libvirtd is started) is the one thing preventing a
"WITHOUT_LIBNL" build of libvirt from being useful. With this
alternate implementation, guests using standard tap devices will work
properly even when libvirt is built without libnl support.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
There was one stray bit of code in virnetdev.c that required libnl to
build, but wasn't qualified by defined(WITH_LIBNL). Adding that, plus
putting a similar check around a static function only used by that
aforementioned code, makes libvirt build properly without libnl3-devel
installed.
How useful it is in that state is a separate issue :-)
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
This flag was originally created to indicate that either 1) the build
platform wasn't linux, 2) the build platform was linux, but the kernel
was too old to have macvtap support. Since there was already a switch
there, the ability to also disable it when 3) the kernel supports
macvtap but the user doesn't want it, was added in. I don't think that
(3) was ever an intentional goal, just something that grew naturally
out of having the flag there in the first place (unless possibly the
original author wanted a way to quickly disable their new code in case
it caused regressions elsewhere).
Now that the check for (2) has been removed, WITH_MACVTAP is just
checking (1) and (3), but (3) is pointless (because the extra code in
libvirt itself is miniscule, and the only external library needed for
it is libnl, which is also required for other unrelated features (and
itself has no subordinate dependencies and takes up < 1MB on
disk)). We can therfore eliminate the WITH_MACVTAP flag, as it is
functionally equivalent to WITH_LIBNL (which implies __linux__).
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
macvlan support was added to the Linux kernel in 2.6.33, but
MACVLAN_MODE_PASSTHRU wasn't added until 2.6.38, so a workaround had
been put in place to define that constant on those few systems where
it was missing. It's useful like was probably 6 months at most, but
it's been there for over 10 years.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
WITH_VIRTUALPORT just checks that we are building on Linux and that
IFLA_PORT_MAX is defined in linux/if_link.h. Back when 802.11Qb[gh]
support was added, the IFLA_* stuff was new (introduced in kernel
2.6.35, backported to RHEL6 2.6.32 kernel at some point), and so this
extra check was necessary, because libvirt was being built on Linux
distros that didn't yet have IFLA_* (e.g. older RHEL6, all
RHEL5). It's been in the kernel for a *very* long time now, so all
supported versions of all Linux platforms libvirt builds on have it.
Note that the above paragraph implies that the conditional compilation
should be changed to #if defined(__linux__). However, the astute
reader will notice that the code in question is sending and receiving
netlink messages, so it really should be conditional on WITH_LIBNL
(which implies __linux__) instead, so that's what this patch does.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
WITH_LIBNL will only be defined on Linux platforms (because libnl is a
library written to encapsulate parts of netlink, which is a Linux-only
API), so it's redundant to write:
#if defined(__linux__) && defined(WITH_LIBNL)
We can just check for WITH_LIBNL.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
IFLA_VF_MAX was introduced to the Linux kernel in 2.6.35, and was even
backported to the RHEL*6* 2.6.32 kernel downstream, so it is present
in all supported versions of all Linux distros that libvirt builds
on. Additionally, it can't be conditionally compiled out of a
kernel. There is no reason to conditionalize any piece of code on
presence of IFLA_VF_MAX - if the platform is Linux, it is supported.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
The former has been present since
commit f43798c27684ab925adde7d8acc34c78c6e50df8
Author: Rusty Russell <rusty@rustcorp.com.au>
Date: Thu Jul 3 03:48:02 2008 -0700
tun: Allow GSO using virtio_net_hdr
and the latter since
commit bbb009941efaece3898910a862f6d23aa55d6ba8
Author: Jason Wang <jasowang@redhat.com>
Date: Wed Oct 31 19:45:59 2012 +0000
tuntap: introduce multiqueue flags
these are old enough that they can be assumed present in all Linux
platforms we support. The tap device creation code changed is specific
to Linux, with a separate impl for non-Linux platforms.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
This flag was added by Linux with:
commit f43798c27684ab925adde7d8acc34c78c6e50df8
Author: Rusty Russell <rusty@rustcorp.com.au>
Date: Thu Jul 3 03:48:02 2008 -0700
tun: Allow GSO using virtio_net_hdr
so we can assume all Linux distros we support have this flag available
and thus the compile time check is sufficient.
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Add an abort() on the class/object allocation failures so that
virStorageSourceNew() always returns a virStorageSource and remove
checks from all callers.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
g_thread_join() eats a reference.
==295055== Invalid read of size 4
==295055== at 0x4DA4AE4: g_thread_unref (in /usr/lib64/libglib-2.0.so.0.6400.5)
==295055== by 0x491D5FA: vir_event_thread_finalize (vireventthread.c:47)
==295055== by 0x4E6BCFF: g_object_unref (in /usr/lib64/libgobject-2.0.so.0.6400.5)
==295055== by 0x22F35CF4: qemuProcessQMPFree (qemu_process.c:8525)
==295055== by 0x22E71B58: glib_autoptr_clear_qemuProcessQMP (qemu_process.h:237)
...
==295055== by 0x22E98A29: qemuDomainPostParseDataAlloc (qemu_domain.c:5476)
==295055== by 0x49ABF83: virDomainDefPostParse (domain_conf.c:6023)
==295055== Address 0x2acb1c68 is 24 bytes inside a block of size 88 free'd
==295055== at 0x483B9F5: free (vg_replace_malloc.c:538)
==295055== by 0x4D80A4C: g_free (in /usr/lib64/libglib-2.0.so.0.6400.5)
...
==295055== by 0x491D5F1: vir_event_thread_finalize (vireventthread.c:46)
==295055== by 0x4E6BCFF: g_object_unref (in /usr/lib64/libgobject-2.0.so.0.6400.5)
==295055== by 0x22F35CF4: qemuProcessQMPFree (qemu_process.c:8525)
==295055== by 0x22E71B58: glib_autoptr_clear_qemuProcessQMP (qemu_process.h:237)
...
==295055== Block was alloc'd at
==295055== at 0x483A809: malloc (vg_replace_malloc.c:307)
==295055== by 0x4D80958: g_malloc (in /usr/lib64/libglib-2.0.so.0.6400.5)
...
==295055== by 0x4DA4C32: g_thread_try_new (in /usr/lib64/libglib-2.0.so.0.6400.5)
==295055== by 0x491D3BC: virEventThreadStart (vireventthread.c:159)
==295055== by 0x491D3BC: virEventThreadNew (vireventthread.c:185)
...
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Fixes: f4fc3db920
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
g_variant_iter_loop() handles freeing all arguments unless we break out
of the loop, in that case we have to free them manually.
Reported-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
We used to check the format of reply data with libdbus so we should do
the same with GLib DBus as well.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
We need to pass pointer to `array`.
Reported-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Tested-by: Ján Tomko <jtomko@redhat.com>
virFileComparePaths just return 0 or 1 after commit 7b48bb8
so break while after virFileComparePaths return 1
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Yi Li <yili@winhong.com>
An extra parameter was added to virQEMUBuildQemuImgKeySecretOpts in
commit ecfc4094d8
Author: Daniel P. Berrangé <berrange@redhat.com>
Date: Tue Sep 15 16:30:37 2020 +0100
storage: add support for qcow2 LUKS encryption
but the non-null pointer annotations were not adjusted to take account.
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The storage driver was wired up to support creating raw volumes in LUKS
format, but was never adapted to support LUKS-in-qcow2. This is trivial
as it merely requires the encryption properties to be prefixed with
the "encrypt." prefix, and "encrypt.format=luks" when creating the
volume.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Crypt method number 2 indicates LUKS format.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
With libdbus our wrappers had a special syntax to create the DBus
messages by defining the DBus message signature followed by list
of arguments providing data based on the signature.
There will be no similar helper with GLib implementation as they
provide same functionality via GVariant APIs. The syntax is slightly
different mostly for how arrays, variadic types and dictionaries are
created/parsed.
Additional difference is that with GLib DBus everything is wrapped in
extra tuple (struct). For more details refer to the documentation [1].
[1] <https://developer.gnome.org/glib/stable/gvariant-format-strings.html>
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
The original motivation for adding virNetDevIPCheckIPv6Forwarding
(commit 00d28a78b5) was that networking routes would disappear when
ipv6 forwarding was enabled for an interface.
This is a fairly undocumented side-effect of the "accept_ra" sysctl
for an interface. 1 means the interface will accept_ra's if not
forwarding, 2 means always accept_RAs; but it is not explained that
enabling forwarding when accept_ra==1 will also clear any kernel RA
assigned routes, very likely breaking your networking.
The check to warn about this currently uses netlink to go through all
the routes and then look at the accept_ra status of the interfaces.
However, it has been noticed that this problem does not affect systems
where IPv6 RA configuration is handled in userspace, e.g. via tools
such as NetworkManager. In this case, the error message from libvirt
is spurious, and modifying the forwarding state will not affect the RA
state or disable your networking.
If you refer to the function rt6_purge_dflt_routers() in the kernel,
we can see that the routes being purged are only those with the
kernel's RTF_ADDRCONF flag set; that is, routes added by the kernel's
RA handling. Why does it do this? I think this is a Linux
implementation decision; it has always been like that and there are
some comments suggesting that it is because a router should be
statically configured, rather than accepting external configurations.
The solution implemented here is to convert the existing check into a
walk of /proc/net/ipv6_route (because RTF_ADDRCONF is apparently not
exposed in netlink) and look for routes with this flag set. We then
check the accept_ra status for the interface, and if enabling
forwarding would break things raise an error.
This should hopefully avoid "interactive" users, who are likely to be
using NetworkManager and the like, having false warnings when enabling
IPv6, but retain the error check for users relying on kernel-based
IPv6 interface auto-configuration.
Signed-off-by: Ian Wienand <iwienand@redhat.com>
Reviewed-by: Laine Stump <laine@redhat.com>
Reviewed-by: Cedric Bosdonnat <CBosdonnat@suse.com>
In v6.7.0-rc1~86 I've tried to fix a problem where we were not
detecting NUMA nodes properly because we misused behaviour of a
libnuma API and as it turned out the behaviour was correct for
hosts with 64 CPUs in one NUMA node. So I changed the code to use
nodemask_isset(&numa_all_nodes, ..) instead and it fixed the
problem on such hosts. However, what I did not realize is that
numa_all_nodes does not reflect all NUMA nodes visible to
userspace, it contains only those nodes that the process
(libvirtd) an allocate memory from, which can be only a subset of
all NUMA nodes. The bitmask that contains all NUMA nodes visible
to userspace and which one I should have used is: numa_nodes_ptr.
For curious ones:
4a22f22382
And as I was fixing virNumaGetNodeCPUs() I came to realize that
we already have a function that wraps the correct bitmask:
virNumaNodeIsAvailable().
Fixes: 24d7d85208
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1876956
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
This function was introduced in the 2.0.6 release which happened
in December 2010. I think it is safe to assume that all libnuma
we deal with have the function.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Since we no longer need to wait for IPv6 DAD to complete, we never
call this function.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
We are currently adding -lutil and -lkvm to the linker using the
add_project_link_arguments method. On FreeBSD 11.4, this results in
build errors because the args appear too early in the command line.
We need to pass the libraries as dependencies so that they get placed
at the same point in the linker args as other dependencies.
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
When starting a domain with <numatune/> set libvirt translates
given NUMA nodes into a set of host CPUs which is then used to
QEMU process affinity. But, if the numatune contains a
non-existent NUMA node then the translation fails with no error
reported. This is because virNumaNodesetToCPUset() calls
virNumaGetNodeCPUs() and expects it to report an error on
failure. Well, it does except for non-existent NUMA nodes. While
this behaviour might look strange it is actually desired because
of how we construct host capabilities. The virNumaGetNodeCPUs()
is called from virCapabilitiesHostNUMAInitReal() where we do not
want any error reported for non-existent NUMA nodes.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1724866
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
'blockdev-create' allows us to create the image with a custom cluster
size if we wish to. Wire it up for 'qcow2'.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
It it useful to be sure no thread is running after we drop all references to
virEventThread. Otherwise in order to avoid crashes we need to synchronize some
other way or we make extra references in event handler callbacks to all the
object in use. And some of them are not prepared to be refcounted.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Stop just send signal for threads to exit when they finish with
current task. Drain waits when all threads will finish.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Even if we have no priority threads on pool creation we can add them thru
virThreadPoolSetParameters later.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
The conditional was removed in
commit ebbf8ebe4f
Author: Ján Tomko <jtomko@redhat.com>
Date: Tue Sep 1 22:56:37 2020 +0200
util: virnetdevtap: stats: fix txdrop on FreeBSD
That commit was correct about this no longer being required for FreeBSD,
but missed that the code is also built on macOS.
Rather than testing for this field in meson though, we can simply use
a platform conditional test in the code.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Local socket connections were outright disabled because there was no "server"
part in the URI. However, given how requirements and usage scenarios are
evolving, some management apps might need the source libvirt daemon to connect
to the destination daemon over a UNIX socket for peer2peer migration. Since we
cannot know where the socket leads (whether the same daemon or not) let's decide
that based on whether the socket path is non-standard, or rather explicitly
specified in the URI. Checking non-standard path would require to ask the
daemon for configuration and the only misuse that it would prevent would be a
pretty weird one. And that's not worth it. The assumption is that whenever
someone uses explicit UNIX socket paths in the URI for migration they better
know what they are doing.
Partially resolves: https://bugzilla.redhat.com/1638889
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
For older FreeBSD, we needed an ifdef guard to use
if_data.ifi_oqdrops, which was introduced by:
commit 61bbdbb94c
Implement interface stats for BSD
But when we dropped the check because we deprecated
building on FreeBSD-10 in:
commit 83131d9714
configure: drop check for unsupported FreeBSD
We started building the wrong side of the ifdef.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Fixes: 83131d9714
Reviewed-by: Roman Bogorodskiy <bogorodskiy@gmail.com>
Currently, we are mixing: #if HAVE_BLAH with #if WITH_BLAH.
Things got way better with Pavel's work on meson, but apparently,
mixing these two lead to confusing and easy to miss bugs (see
31fb929eca for instance). While we were forced to use HAVE_
prefix with autotools, we are free to chose our own prefix with
meson and since WITH_ prefix appears to be more popular let's use
it everywhere.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
There are couple of conditional #includes at the beginning of
virfile.c and they try to be nice and document #endifs. But they
are mostly wrong because either they have the condition in the
comment inverted or the comment refers to a different condition
than they belong to. Just remove the comments as these #includes
are single line mostly.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
There are two places where we try to check whether the host
system has net/if.h before including it. But the check is missing
'_H' suffix.
Fixes: 7f3eb533f4
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Use https: links for websites that support them.
The URIs which are used as namespace identifiers
are left alone.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: Neal Gompa <ngompa13@gmail.com>
When creating a standard tap device, if provided with an ifname that
contains "%d", rather than taking that literally as the name to use
for the new device, the kernel will instead use that string as a
template, and search for the lowest number that could be put in place
of %d and produce an otherwise unused and unique name for the new
device. For example, if there is no tap device name given in the XML,
libvirt will always send "vnet%d" as the device name, and the kernel
will create new devices named "vnet0", "vnet1", etc. If one of those
devices is deleted, creating a "hole" in the name list, the kernel
will always attempt to reuse the name in the hole first before using a
name with a higher number (i.e. it finds the lowest possible unused
number).
The problem with this, as described in the previous patch dealing with
macvtap device naming, is that it makes "immediate reuse" of a newly
freed tap device name *much* more common, and in the aftermath of
deleting a tap device, there is some other necessary cleanup of things
which are named based on the device name (nwfilter rules, bandwidth
rules, OVS switch ports, to name a few) that could end up stomping
over the top of the setup of a new device of the same name for a
different guest.
Since the kernel "create a name based on a template" functionality for
tap devices doesn't exist for macvtap, this patch for standard tap
devices is a bit different from the previous patch for macvtap - in
particular there was no previous "bitmap ID reservation system" or
overly-complex retry loop that needed to be removed. We simply find
and unused name, and pass that name on to the kernel instead of
"vnet%d".
This counter is also wrapped when either it gets to INT_MAX or if the
full name would overflow IFNAMSIZ-1 characters. In the case of
"vnet%d" and a 32 bit int, we would reach INT_MAX first, but possibly
someday someone will change the name from vnet to something else.
(NB: It is still possible for a user to provide their own
parameterized template name (e.g. "mytap%d") in the XML, and libvirt
will just pass that through to the kernel as it always has.)
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
There have been some reports that, due to libvirt always trying to
assign the lowest numbered macvtap / tap device name possible, a new
guest would sometimes be started using the same tap device name as
previously used by another guest that is in the process of being
destroyed *as the new guest is starting.
In some cases this has led to, for example, the old guest's
qemuProcessStop() code deleting a port from an OVS switch that had
just been re-added by the new guest (because the port name is based on
only the device name using the port). Similar problems can happen (and
I believe have) with nwfilter rules and bandwidth rules (which are
both instantiated based on the name of the tap device).
A couple patches have been previously proposed to change the ordering
of startup and shutdown processing, or to put a mutex around
everything related to the tap/macvtap device name usage, but in the
end no matter what you do there will still be possible holes, because
the device could be deleted outside libvirt's control (for example,
regular tap devices are automatically deleted when the qemu process
terminates, and that isn't always initiated by libvirt but could
instead happen completely asynchronously - libvirt then has no control
over the ordering of shutdown operations, and no opportunity to
protect it with a mutex.)
But this only happens if a new device is created at the same time as
one is being deleted. We can effectively eliminate the chance of this
happening if we end the practice of always looking for the lowest
numbered available device name, and instead just keep an integer that
is incremented each time we need a new device name. At some point it
will need to wrap back around to 0 (in order to avoid the IFNAMSIZ 15
character limit if nothing else), and we can't guarantee that the new
name really will be the *least* recently used name, but "math"
suggests that it will be *much* less common that we'll try to re-use
the *most* recently used name.
This patch implements such a counter for macvtap/macvlan, replacing
the existing, and much more complicated, "ID reservation" system. The
counter is set according to whatever macvtap/macvlan devices are
already in use by guests when libvirtd is started, incremented each
time a new device name is needed, and wraps back to 0 when either
INT_MAX is reached, or when the resulting device name would be longer
than IFNAMSIZ-1 characters (which actually is what happens when the
template for the device name is "maccvtap%d"). The result is that no
macvtap name will be re-used until the host has created (and possibly
destroyed) 99,999,999 devices.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
On some platforms libm (needed for the pow() function) isn't being
linked in somehow. This patch adds the necessary bits to assure that
it's linked in when necessary.
Suggested-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
(cherry picked from commit 20a62b42ec001310a6329d7ee2021f0737d534ef)
Driver module loaders current hardcode ".so" as the file
extension. On MacOS, meson uses ".dylib" as a module file extension.
This patch adds VIR_FILE_MODULE_EXT to virfile.h defined as the
hosts module extension, and updates driver module loaders to make
use of it.
Signed-off-by: Scott Shambarger <scott-libvirt@shambarger.net>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
Previous patch handled the runtime case where a non-x86 host is
fetching /proc/cpuinfo data for a microcode info that we know
it doesn't exist. This change alone speeded everything by a
bit for non-x86, but there is at least one major culprit left.
qemuxml2argvtest does several arch-specific tests, and a good
chunk of them are x86 exclusive. This means that 'hostArch'
will be seen as x86 for these tests, even when running in
non-x86 hosts. In a Power 9 server with 128 CPUs, qemuxml2argvtest
takes 298 seconds to complete in average, and 'perf record'
indicates that 95% of the time is spent in
virHostCPUGetMicrocodeVersion().
This patch mocks virHostCPUGetMicrocodeVersion() to always return
0 in the tests, avoiding /proc/cpuinfo reads. This will make all
tests behave arch-agnostic, and the microcode value being 0 has no
impact on any existing test.
This is a CI speed across the board for all archs, including x86,
given that we're not reading /proc/cpuinfo in the tests. For
a Thinkpad T480 laptop with 8 Intel i7 CPUs, qemuxml2argvtest
went from 15.50 sec to 12.50 seconds. The performance gain is even
more noticeable for huge servers with lots of CPUs. For the
Power 9 server mentioned above, this patch speeds qemuxml2argvtest
to 9 seconds, down from 298 sec.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Non-x86 archs does not have a 'microcode' version like x86. This is
covered already inside the function - just return 0 if no microcode
is found. Regardless of that, a read of /proc/cpuinfo is always made.
Each read will invoke the kernel to fill in the CPU details every time.
Now let's consider a non-x86 host, like a Power 9 server with 128 CPUs.
Each /proc/cpuinfo read will need to fetch data for each CPU and it
won't even matter because we know beforehand that PowerPC chips don't
have microcode information.
We can do better for non-x86 hosts by skipping this process entirely.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Use g_autofree and remove the cleanup label.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Since the macro no longer includes the 'ignore_value'
statement, stop putting another empty statement after it.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
The VIR_XPATH_NODE_AUTORESTORE contains an ignore_value
statement to silence an unused variable warning on clang.
Use a pragma instead, which is not a statement.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
VIR_CGROUP_BACKEND_CALL is exclusively used at the end
of a function, but it declares a variable.
Wrap it in a do..while block.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Declare the variables at the beginning of the function,
then fill them up.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Many of our functions start with a DEBUG statement.
Move the statements after declarations to appease
our coding style.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Split those initializations that depend on a statement
above them.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Use g_autofree and move the declarations to the beginning
of the block.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Repeat the whole function header instead of mixing #ifdefs
in the code.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Commit 4362068979 moved the function to
util/virqemu.c which is compiled also on win32 and geteuid()/getegid()
doesn't exist there.
Move it to qemu_domain.c which is compiled only when the qemu driver is
enabled. Originally I didn't want to put it here as qemu_domain.c is a
code dump for helper functions but this is the least invasive fix.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
This is similar to one of previous patches.
When receiving stream (on virStorageVolUpload() and subsequent
virStreamSparseSendAll()) we may receive a hole. If the volume we
are saving the incoming data into is a regular file we just
lseek() and ftruncate() to create the hole. But this won't work
if the file is a block device. If that is the case we must write
zeroes so that any subsequent reader reads nothing just zeroes
(just like they would from a hole in a regular file).
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1852528
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
When handling sparse stream, a thread is executed. This thread
runs a read() or write() loop (depending what API is called; in
this case it's virStorageVolDownload() and this the thread run
read() loop). The read() is handled in virFDStreamThreadDoRead()
which is then data/hole section aware, meaning it uses
virFileInData() to detect data and hole sections and sends
TYPE_DATA or TYPE_HOLE virStream messages accordingly.
However, virFileInData() does not work with block devices. Simply
because block devices don't have data and hole sections. What we
can do though, is to mimic being always in a DATA section.
Partially resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1852528
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
In a very distant past, we came around machines that has not
continuous node IDs. This made us error out when constructing
capabilities XML. We resolved it by utilizing strange behaviour
of numa_node_to_cpus() in which it returned a mask with all bits
set for a non-existent node. However, this is not the only case
when it returns all ones mask - if the node exists and has enough
CPUs to fill the mask up (e.g. 128 CPUs).
The fix consists of using nodemask_isset(&numa_all_nodes, ..)
prior to calling numa_node_to_cpus() to determine if the node
exists.
Fixes: 628c935747
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1860231
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
After previous cleanups, some labels in some functions have
nothing but 'return' statement in them. Drop the labels and
replace 'goto'-s with respective return statements.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Again, instead of closing FDs explicitly, we can automatically
close them when they go out of their respective scopes.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
A cleanup function can be declared for virFDStreamMsg type so
that the structure doesn't have to be freed explicitly.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
All callers of virFDStreamMsgQueuePush() have the same pattern:
they explicitly set @msg passed to NULL to avoid freeing it later
on. Well, the function can take address of the pointer and clear
it for them.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
The buffer that allocated in the virFDStreamThreadDoRead() can be
automatically freed, or if saved into the message structure it
can be stolen.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
So far, only ENOENT is ignored (to deal with kernels without
devmapper). However, as reported on the list, under certain
scenarios a different error can occur. For instance, when libvirt
is running inside a container which doesn't have permissions to
talk to the devmapper. If this is the case, then open() returns
-1 and sets errno=EPERM.
Assuming that multipath devices are fairly narrow use case and
using them in a restricted container is even more narrow the best
fix seems to be to ignore all open errors BUT produce a warning
on failure. To avoid flooding logs with warnings on kernels
without devmapper the level is reduced to a plain debug message.
Reported-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
Reviewed-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
In one of my latest patch (v6.6.0~30) I was trying to remove
libdevmapper use in favor of our own implementation. However, the
code did not take into account that device mapper can be not
compiled into the kernel (e.g. be a separate module that's not
loaded) in which case /proc/devices won't have the device-mapper
major number and thus virDevMapperGetTargets() and/or
virIsDevMapperDevice() fails.
However, such failure is safe to ignore, because if device mapper
is missing then there can't be any multipath devices and thus we
don't need to allow the deps in CGroups, nor create them in the
domain private namespace, etc.
Fixes: 2249455654
Reported-by: Andrea Bolognani <abologna@redhat.com>
Reported-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
Tested-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
The device mapper major is needed in virIsDevMapperDevice() which
determines whether given device is managed by device-mapper. This
number is obtained by parsing /proc/devices and then stored in a
global variable so that the file doesn't have to be parsed again.
However, as it turns out this logic is flawed - the major number
is not static and can change as it can be specified as a
parameter when loading the dm-mod module.
Unfortunately, I was not able to come up with a good solution and
thus the /proc/devices file is being parsed every time we need
the device mapper major.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
Tested-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
BPF syscall BPF_MAP_GET_NEXT_KEY returns -1 if something fails but it
will also return -1 if trying to get next key using the last key in the
map with errno set to ENOENT.
If there are VMs running and libvirtd is restarted and user tries to
call some cgroup devices operation on a VM we need to get the count of
entries in BPF map and it fails which will result in error when trying
to attach/detech devices.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1833321
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
There is a race between vir_event_thread_finalize and
virEventThreadWorker in releasing the last reference on
the GMainContext. If virEventThreadDataFree() runs after
vir_event_thread_finalize releases its reference, then
it will release the last reference on the GMainContext.
As a result g_autoptr cleanup on the GSource will access
free'd memory.
The race can be seen in non-deterministic crashes of the
virt-run-qemu program during its shutdown, but could
also likely affect the main libvirtd QEMU driver:
Thread 2 (Thread 0x7f508ffff700 (LWP 222813)):
#0 0x00007f509c8e26b0 in malloc_consolidate (av=av@entry=0x7f5088000020) at malloc.c:4488
#1 0x00007f509c8e4b08 in _int_malloc (av=av@entry=0x7f5088000020, bytes=bytes@entry=2048) at malloc.c:3711
#2 0x00007f509c8e6412 in __GI___libc_malloc (bytes=2048) at malloc.c:3073
#3 0x00007f509d6e925e in g_realloc (mem=0x0, n_bytes=2048) at gmem.c:164
#4 0x00007f509d705a57 in g_string_maybe_expand (string=string@entry=0x7f5088001f20, len=len@entry=1024) at gstring.c:102
#5 0x00007f509d705ab6 in g_string_sized_new (dfl_size=dfl_size@entry=1024) at gstring.c:127
#6 0x00007f509d708c5e in g_test_log_dump (len=<synthetic pointer>, msg=<synthetic pointer>) at gtestutils.c:3330
#7 0x00007f509d708c5e in g_test_log
(lbit=G_TEST_LOG_ERROR, string1=0x7f508800fcb0 "GLib:ERROR:ghash.c:377:g_hash_table_lookup_node: assertion failed: (hash_table->ref_count > 0)", string2=<optimized out>, n_args=0, largs=0x0) at gtestutils.c:975
#8 0x00007f509d70af2a in g_assertion_message
(domain=<optimized out>, file=0x7f509d7324a2 "ghash.c", line=<optimized out>, func=0x7f509d732750 <__func__.11348> "g_hash_table_lookup_node", message=<optimized out>)
at gtestutils.c:2504
#9 0x00007f509d70af8e in g_assertion_message_expr
(domain=domain@entry=0x7f509d72d76e "GLib", file=file@entry=0x7f509d7324a2 "ghash.c", line=line@entry=377, func=func@entry=0x7f509d732750 <__func__.11348> "g_hash_table_lookup_node", expr=expr@entry=0x7f509d732488 "hash_table->ref_count > 0") at gtestutils.c:2555
#10 0x00007f509d6d197e in g_hash_table_lookup_node (hash_table=0x55b70ace1760, key=<optimized out>, hash_return=<synthetic pointer>) at ghash.c:377
#11 0x00007f509d6d197e in g_hash_table_lookup_node (hash_return=<synthetic pointer>, key=<optimized out>, hash_table=0x55b70ace1760) at ghash.c:361
#12 0x00007f509d6d197e in g_hash_table_remove_internal (hash_table=0x55b70ace1760, key=<optimized out>, notify=1) at ghash.c:1371
#13 0x00007f509d6e0664 in g_source_unref_internal (source=0x7f5088000b60, context=0x55b70ad87e00, have_lock=0) at gmain.c:2103
#14 0x00007f509d6e1f64 in g_source_unref (source=<optimized out>) at gmain.c:2176
#15 0x00007f50a08ff84c in glib_autoptr_cleanup_GSource (_ptr=<synthetic pointer>) at /usr/include/glib-2.0/glib/glib-autocleanups.h:58
#16 0x00007f50a08ff84c in virEventThreadWorker (opaque=0x55b70ad87f80) at ../../src/util/vireventthread.c:114
#17 0x00007f509d70bd4a in g_thread_proxy (data=0x55b70acf3850) at gthread.c:784
#18 0x00007f509d04714a in start_thread (arg=<optimized out>) at pthread_create.c:479
#19 0x00007f509c95cf23 in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95
Thread 1 (Thread 0x7f50a1380c00 (LWP 222802)):
#0 0x00007f509c8977ff in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
#1 0x00007f509c881c35 in __GI_abort () at abort.c:79
#2 0x00007f509d72a823 in g_mutex_clear (mutex=0x55b70ad87e00) at gthread-posix.c:1307
#3 0x00007f509d72a823 in g_mutex_clear (mutex=mutex@entry=0x55b70ad87e00) at gthread-posix.c:1302
#4 0x00007f509d6e1a84 in g_main_context_unref (context=0x55b70ad87e00) at gmain.c:582
#5 0x00007f509d6e1a84 in g_main_context_unref (context=0x55b70ad87e00) at gmain.c:541
#6 0x00007f50a08ffabb in vir_event_thread_finalize (object=0x55b70ad83180 [virEventThread]) at ../../src/util/vireventthread.c:50
#7 0x00007f509d9c48a9 in g_object_unref (_object=<optimized out>) at gobject.c:3340
#8 0x00007f509d9c48a9 in g_object_unref (_object=0x55b70ad83180) at gobject.c:3232
#9 0x00007f509583d311 in qemuProcessQMPFree (proc=proc@entry=0x55b70ad87b90) at ../../src/qemu/qemu_process.c:8355
#10 0x00007f5095790f58 in virQEMUCapsInitQMPSingle
(qemuCaps=qemuCaps@entry=0x55b70ad88010, libDir=libDir@entry=0x55b70ad049e0 "/tmp/virt-qemu-run-VZC9N0/lib/qemu", runUid=runUid@entry=107, runGid=runGid@entry=107, onlyTCG=onlyTCG@entry=false) at ../../src/qemu/qemu_capabilities.c:5409
#11 0x00007f509579108f in virQEMUCapsInitQMP (runGid=107, runUid=107, libDir=0x55b70ad049e0 "/tmp/virt-qemu-run-VZC9N0/lib/qemu", qemuCaps=0x55b70ad88010)
at ../../src/qemu/qemu_capabilities.c:5420
#12 0x00007f509579108f in virQEMUCapsNewForBinaryInternal
(hostArch=VIR_ARCH_X86_64, binary=binary@entry=0x55b70ad7dc40 "/usr/libexec/qemu-kvm", libDir=0x55b70ad049e0 "/tmp/virt-qemu-run-VZC9N0/lib/qemu", runUid=107, runGid=107, hostCPUSignature=0x55b70ad01320 "GenuineIntel, Intel(R) Xeon(R) Silver 4210 CPU @ 2.20GHz, family: 6, model: 85, stepping: 7", microcodeVersion=83898113, kernelVersion=0x55b70ad00d60 "4.18.0-211.el8.x86_64 #1 SMP Thu Jun 4 08:08:16 UTC 2020") at ../../src/qemu/qemu_capabilities.c:5472
#13 0x00007f5095791373 in virQEMUCapsNewData (binary=0x55b70ad7dc40 "/usr/libexec/qemu-kvm", privData=0x55b70ad5b8f0) at ../../src/qemu/qemu_capabilities.c:5505
#14 0x00007f50a09a32b1 in virFileCacheNewData (name=0x55b70ad7dc40 "/usr/libexec/qemu-kvm", cache=<optimized out>) at ../../src/util/virfilecache.c:208
#15 0x00007f50a09a32b1 in virFileCacheValidate (cache=cache@entry=0x55b70ad5c030, name=name@entry=0x55b70ad7dc40 "/usr/libexec/qemu-kvm", data=data@entry=0x7ffca39ffd90)
at ../../src/util/virfilecache.c:277
#16 0x00007f50a09a37ea in virFileCacheLookup (cache=cache@entry=0x55b70ad5c030, name=name@entry=0x55b70ad7dc40 "/usr/libexec/qemu-kvm") at ../../src/util/virfilecache.c:310
#17 0x00007f5095791627 in virQEMUCapsCacheLookup (cache=0x55b70ad5c030, binary=0x55b70ad7dc40 "/usr/libexec/qemu-kvm") at ../../src/qemu/qemu_capabilities.c:5647
#18 0x00007f50957c34c3 in qemuDomainPostParseDataAlloc (def=<optimized out>, parseFlags=<optimized out>, opaque=<optimized out>, parseOpaque=0x7ffca39ffe18)
at ../../src/qemu/qemu_domain.c:5470
#19 0x00007f50a0a34051 in virDomainDefPostParse
(def=def@entry=0x55b70ad7d200, parseFlags=parseFlags@entry=258, xmlopt=xmlopt@entry=0x55b70ad5d010, parseOpaque=parseOpaque@entry=0x0)
at ../../src/conf/domain_conf.c:5970
#20 0x00007f50a0a464bb in virDomainDefParseNode
(xml=xml@entry=0x55b70aced140, root=root@entry=0x55b70ad5f020, xmlopt=xmlopt@entry=0x55b70ad5d010, parseOpaque=parseOpaque@entry=0x0, flags=flags@entry=258)
at ../../src/conf/domain_conf.c:22520
#21 0x00007f50a0a4669b in virDomainDefParse
(xmlStr=xmlStr@entry=0x55b70ad5f9e0 "<domain type='kvm'>\n <name>83</name>\n <uuid>9350639d-1c8a-4f51-a4a6-4eaf8eabe83e</uuid>\n <metadata>\n <libosinfo:libosinfo xmlns:libosinfo=\"http://libosinfo.org/xmlns/libvirt/domain/1.0\">\n <"..., filename=filename@entry=0x0, xmlopt=0x55b70ad5d010, parseOpaque=parseOpaque@entry=0x0, flags=flags@entry=258) at ../../src/conf/domain_conf.c:22474
#22 0x00007f50a0a467ae in virDomainDefParseString
(xmlStr=xmlStr@entry=0x55b70ad5f9e0 "<domain type='kvm'>\n <name>83</name>\n <uuid>9350639d-1c8a-4f51-a4a6-4eaf8eabe83e</uuid>\n <metadata>\n <libosinfo:libosinfo xmlns:libosinfo=\"http://libosinfo.org/xmlns/libvirt/domain/1.0\">\n <"..., xmlopt=<optimized out>, parseOpaque=parseOpaque@entry=0x0, flags=flags@entry=258)
at ../../src/conf/domain_conf.c:22488
#23 0x00007f50958ce112 in qemuDomainCreateXML
(conn=0x55b70acf9090, xml=0x55b70ad5f9e0 "<domain type='kvm'>\n <name>83</name>\n <uuid>9350639d-1c8a-4f51-a4a6-4eaf8eabe83e</uuid>\n <metadata>\n <libosinfo:libosinfo xmlns:libosinfo=\"http://libosinfo.org/xmlns/libvirt/domain/1.0\">\n <"..., flags=0) at ../../src/qemu/qemu_driver.c:1744
#24 0x00007f50a0c268ac in virDomainCreateXML
(conn=0x55b70acf9090, xmlDesc=0x55b70ad5f9e0 "<domain type='kvm'>\n <name>83</name>\n <uuid>9350639d-1c8a-4f51-a4a6-4eaf8eabe83e</uuid>\n <metadata>\n <libosinfo:libosinfo xmlns:libosinfo=\"http://libosinfo.org/xmlns/libvirt/domain/1.0\">\n <"..., flags=0) at ../../src/libvirt-domain.c:176
#25 0x000055b709547e7b in main (argc=<optimized out>, argv=<optimized out>) at ../../src/qemu/qemu_shim.c:289
The solution is to explicitly unref the GSource at a safe time instead
of letting g_autoptr unref it when leaving scope.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
There is a fairly long standing race condition bug in glib which can hit
if you call g_source_destroy or g_source_unref from a non-main thread:
https://gitlab.gnome.org/GNOME/glib/-/merge_requests/1358
Unfortunately it is really common for libvirt to call g_source_destroy
from a non-main thread. This glib bug is the cause of non-determinstic
crashes in eventtest, and probably in libvirtd too.
To work around the problem we need to ensure that we never release
the last reference on a GSource from a non-main thread. The previous
patch replaced our use of g_source_destroy with a pair of
g_source_remove and g_source_unref. We can now delay the g_source_unref
call by using a idle callback to invoke it from the main thread which
avoids the race condition.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The source ID number is an alternative way to identify a source that has
been added to a GMainContext. Internally when a source ID is given, glib
will lookup the corresponding GSource and use that. The use of a source
ID is racy in some cases though, because it is invalid to continue to
use an ID number after the GSource has been removed. It is thus safer
to use the GSource object directly and have full control over the ref
counting and thus cleanup.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
When COW is not explicitly requested to be disabled or enabled, the
function is supposed to do nothing on non-BTRFS file systems.
Fixes commit 7230bc95aa.
https://bugzilla.redhat.com/show_bug.cgi?id=1866157
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Many of our calls to xmlNodeGetContent() (which are now all via
virXMLNodeContentString() are failing to check for a NULL return. We
need to remedy that, but in order to make the remedy simpler, let's
log an error in virXMLNodeContentString(), so that the callers don't
all individually need to (since it would be the same error message for
all of them anyway).
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
If there's a list of mdevs to be assigned to a domain, but one of them
(NOT the first) is already assigned to a different domain we're going
to crash in the qemuProcessStop phase in
virMediatedDeviceListFindIndex, because some of the pointers in
mgr->activeMediatedHostdevs are dangling. This is due to
virMediatedDeviceListMarkDevices using cleanup instead of rollback when
we find out that a device is already taken.
Reproducer steps:
1. start vm1 with mdev1
2. start vm2 with mdev2, mdev1 (the order is important!)
Backtrace:
#0 0x0000ffffb8c36250 in strcmp
#1 0x0000ffffb9b80754 in virMediatedDeviceListFindIndex
#2 0x0000ffffb9b80870 in virMediatedDeviceListFind
#3 0x0000ffffb9c9e168 in virHostdevReAttachMediatedDevices
#4 0x0000ffff9949f724 in qemuHostdevReAttachMediatedDevices
#5 0x0000ffff9949f7f8 in qemuHostdevReAttachDomainDevices
#6 0x0000ffff994bcd70 in qemuProcessStop
#7 0x0000ffff994bf4e0 in qemuProcessStart
Signed-off-by: Binfeng Wu <wubinfeng@huawei.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
These variables are only used for assignment and have
no other effect.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
In virCgroupV2BindMount there is an unused variable containing
what seem to be tmpfs mount options.
Delete it. Unlike with cgroups v1, we do not create a tmpfs
here.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
Now that everything uses g_strfreev, this function is no longer
needed.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
Both accept a NULL value gracefully and virStringFreeList
does not zero the pointer afterwards, so a straight replace
is safe.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
The g_strdupv function from GLib provides
the same functionality.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
Last usage out of virlog.c was removed by
commit 91268c715c
node_device_udev: remove deprecated logging function
Also drop the virbuffer.h include - it seems it was never used
for anything else than the transitive stdarg.h include.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
This function calls virLogVMessage. Move it below the definition
of virLogVMessage so it can call it even without a prototype.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
The XML function is needed in the C file,
not in the header.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
It was needed for virAsprintf, which is now dropped.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Fixes: 33ed622106
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
We use an array of size VIR_NODE_MEMORY_STATS_FIELD_LENGTH
to store the string read from sysfs, but pass unbound "%s"
to sscanf.
Make the array larger by one and simply stringify that
constant as the field width specifier.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
There is no distinction between Read/Write locks for resctrl from libvirt's
point of view any more.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
It was created to get rid of conditional compilation in the resctrl code and
make it usable anywhere else. However this is not something that is going to be
used in other places because it is not portable and resctrl is just very
specific in this regard. And there is no reason why there could not be a
preprocessor conditional in the resctrl code. Also the interface of
virFileFlock() was very ambiguous which lead to some issues.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
That's the way it should've been all the time. It was originally the case, but
then the rework to virFileFlock() made the function ambiguous when it was
created in commit 5a0a5f7fb5, and due to that it was misused in commit
657ddeff23 and since then the lock being taken was shared rather than
exclusive.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Meson doesn't use .libs directory, everything is placed directly into
directories where meson.build file is used.
In order to have working tests and running libvirt directly from GIT we
need to fix all the paths pointing '.libs' directory.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Neal Gompa <ngompa13@gmail.com>
With meson we no longer have .libs directory with the actual binary so
we have to take a different approach to detect if running from build
directory.
This is not as robust as for autotools because if you select --prefix
in the build directory it will incorrectly enable the override as well
but nobody should do that.
We have to modify some of the tests to not add current build path into
PATH variable and use the full path for virsh instead. Otherwise it
would be impossible to figure out that we are running virsh from build
directory.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Neal Gompa <ngompa13@gmail.com>
There is no point of having this option in libvirt because the debug
logs can be configured using log filters.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Neal Gompa <ngompa13@gmail.com>
EXTRA_DIST is not relevant because meson makes a git copy when creating
dist archive so everything tracked by git is part of dist tarball.
The remaining ones are not converted to meson files as they are
automatically tracked by meson.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Neal Gompa <ngompa13@gmail.com>
After the switch to libnl these are no longer used.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Fixes: 77e7c13b2e
Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>
CVE-2020-14339
When building domain's private /dev in a namespace, libdevmapper
is consulted for getting full dependency tree of domain's disks.
The reason is that for a multipath devices all dependent devices
must be created in the namespace and allowed in CGroups.
However, this approach is very fragile as building of namespace
happens in the forked off child process, after mass close of FDs
and just before dropping privileges and execing QEMU. And it so
happens that when calling libdevmapper APIs, one of them opens
/dev/mapper/control and saves the FD into a global variable. The
FD is kept open until the lib is unlinked or dm_lib_release() is
called explicitly. We are doing neither.
However, the virDevMapperGetTargets() function is called also
from libvirtd (when setting up CGroups) and thus has to be thread
safe. Unfortunately, libdevmapper APIs are not thread safe (nor
async signal safe) and thus we can't use them. Reimplement what
libdevmapper would do using plain C (ioctl()-s, /proc/devices
parsing, /dev/mapper dirwalking, and so on).
Fixes: a30078cb83
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1858260
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Since we have VIR_AUTOSTRINGLIST we can use it to free string
lists used in the function automatically.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
There are two distinct WITH_DEVMAPPER sections in the file, for
different functions each. Rearrange the code to make some of
future commits smaller.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
btrfs defaults to performing copy-on-write for files. This is often
undesirable for VM images, so we need to be able to control whether this
behaviour is used.
The virFileSetCOW() will allow for this. We use a tristate, since out of
the box, we want the default behaviour attempt to disable cow, but only
on btrfs, silently do nothing on non-btrfs. If someone explicitly asks
to disable/enable cow, then we want to raise a hard error on non-btrfs.
Reviewed-by: Neal Gompa <ngompa13@gmail.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
gcc 10.1.0 on Debian sid has a bug where the bounds checking gets
confused beteen two branches:
In file included from /usr/include/string.h:495,
from ../../src/internal.h:28,
from ../../src/util/virsocket.h:21,
from ../../src/util/virsocketaddr.h:21,
from ../../src/util/virnetdevip.h:21,
from ../../src/util/virnetdevip.c:21:
In function 'memcpy',
inlined from 'virNetDevGetifaddrsAddress' at ../../src/util/virnetdevip.c:914:13,
inlined from 'virNetDevIPAddrGet' at ../../src/util/virnetdevip.c:962:16:
/usr/include/arm-linux-gnueabihf/bits/string_fortified.h:34:10: error: '__builtin_memcpy' offset [16, 27] from the object at 'addr' is out of the bounds of referenced subobject 'inet4' with type 'struct sockaddr_in' at offset 0 [-Werror=array-bounds]
34 | return __builtin___memcpy_chk (__dest, __src, __len, __bos0 (__dest));
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from ../../src/util/virnetdevip.h:21,
from ../../src/util/virnetdevip.c:21:
../../src/util/virnetdevip.c: In function 'virNetDevIPAddrGet':
../../src/util/virsocketaddr.h:29:28: note: subobject 'inet4' declared here
29 | struct sockaddr_in inet4;
| ^~~~~
cc1: all warnings being treated as errors
Note the source location is pointing to the "inet6" / AF_INET6 branch of
the "if", but is complaining about bounds of the "inet4" field. Changing
the code into a switch() is sufficient to avoid triggering the bug and
is arguably better code too.
Reviewed-by: Laine Stump <laine@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
g_new() is used in only 3 places. Switching them to g_new0() will do
no harm, reduces confusion, and helps me sleep better at night knowing
that all allocated memory is initialized to 0 :-) (Yes, I *know* that
in all three cases the associated memory is immediately assigned some
other value. Today.)
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Historically, we've used security_context_t for variables passed
to libselinux APIs. But almost 7 years ago, libselinux developers
admitted in their API that in fact, it's just a 'char *' type
[1]. Ever since then the APIs accept 'char *' instead, but they
kept the old alias just for API stability. Well, not anymore [2].
1: 9eb9c93275
2: 7a124ca275
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Commit <f650e86703847af544762d02f79c70131ff7fbab> added check for
openpty function from util library using AC_CHECK_LIB(). However, that
macro doesn't define OPENPTY_LIBS, it only defines WITH_LIBUTIL and
prepends -lutil into LIBS for the whole project.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
It was introduced by commit <c606671aaad10a9bc87f226bc473a091e00a9629>
as a gnulib ldexp module and later removed by commit
<09fe607b4de8eb883c966e90aaf5563299a22738>.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Fixes inconsistency with macro names for external programs.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
This was introduced together with clock-time gnulib module by commit
<d74e5a4dfc434d3a1d01856d013a7f50d910fa95> and removed from libvirt
by commit <86d223a762990c9d529065a2d3b30b6a00ea63dd>.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
virFileIsAccessible does not return true on accessible
directories. Check whether it set EISDIR and only
then assume the directory is inaccessible.
Return 0 (not found) instead of 1 (found),
since the bridge driver taints the network based on
this return value, not whether the hook actually ran.
Remove the bogus check from virHookCall, since it already
checks the virHooksFound bitmap that was filled before
by virHookCheck.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Fixes: 7fa7f7eeb6
Closes: https://gitlab.com/libvirt/libvirt/-/issues/47
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
When preparing for the removal of GNULIB commit 18dca21a32 removed the
unneeded O_DIRECTORY, but unfortunately started opening the directory for
writing which fails every time for a directory. There is also no need for that
as flock() works on O_RDONLY file descriptor as well, even for LOCK_EX.
https://bugzilla.redhat.com/1852741
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
There are several calls to virBufferFreeAndReset() when functions
encounter an error, but the caller never uses the virBuffer once an
error has been encountered (all callers detect error by looking at the
function return value, not the contents of the virBuffer being
operated on), and now that all virBuffers are auto-freed there is no
reason for the lower level functions like these to spend time freeing
a buffer that is guaranteed to be freed momentarily anyway.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The idea is to have a function that calls virHostCPUGetOnlineBitmap()
but, instead of returning NULL if the host does not have CPU
offlining capabilities, fall back to a bitmap containing all
present CPUs.
Next patch will use this helper in two other places.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
This function reads the string in sysfspath/cpu/present and
parses it manually to retrieve the number of present CPUs.
virHostCPUGetPresentBitmap() reads and parses the same file,
using a more robust parser via virBitmapParseUnlimited(),
but returns a bitmap. Let's drop all the manual parsing done
here and simply return the size of the resulting bitmap
from virHostCPUGetPresentBitmap().
Given that no more parsing is being done manually in the function,
rename it to virHostCPUCountLinux().
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
There is nothing domain specific about the function, thus it
should not have virDomain prefix. Also, the fact that it is a
static function makes it impossible to use from other files.
Move the function to virxml.c and drop the 'Domain' infix.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
The strings allocated in virGetHostnameImpl() are all allocated via
g_strdup(), which will exit on OOM anyway, so the call to
virReportOOMError() is redundant, and removing it allows slight
modification to the code, in particular the cleanup label can be
eliminated.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
AUTOPTR_CLEANUP_FUNC is set to xmlBufferFree() in util/virxml.h (This
is actually new - added accidentally (but fortunately harmlessly!) in
commit 257aba2daf. I had added it along with the hunks in this patch,
then decided to remove it and submit separately, but missed taking out
the hunk in virxml.h)
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Since feb83c1e71 libvirtd will abort on
startup if run as non-root
2020-07-01 16:30:30.738+0000: 1647444: error : virDirOpenInternal:2869 : cannot open directory '/etc/libvirt/hooks/daemon.d': Permission denied
The root cause flaw is that non-root libvirtd is using /etc/libvirt for
its hooks. Traditionally that has been harmless though since we checked
whether we could access the hook file and degraded gracefully. We need
the same access check for iterating over the hook directory.
Long term we should make it possible to have an unprivileged hook dir
under $HOME.
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The ZPCI device validation is specific to qemu. So, let us move the
ZPCI uid validation out of domain xml parsing into qemu domain device
validation.
Signed-off-by: Shalini Chellathurai Saroja <shalini@linux.ibm.com>
Reviewed-by: Bjoern Walk <bwalk@linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Let us fix the issues with zPCI address validation and auto-generation
on s390.
Currently, there are two issues with handling the ZPCI address
extension. Firstly, when the uid is to be auto-generated with a
specified fid, .i.e.:
...
<address type='pci'>
<zpci fid='0x0000001f'/>
</address>
...
we expect uid='0x0001' (or the next available uid for the domain).
However, we get a parsing error:
$ virsh define zpci.xml
error: XML error: Invalid PCI address uid='0x0000', must be > 0x0000
and <= 0xffff
Secondly, when the uid is specified explicitly with the invalid
numerical value '0x0000', we actually expect the parsing error above.
However, the domain is being defined and the uid value is silently
changed to a valid value.
The first issue is a bug and the second one is undesired behaviour, and
both issues are related to how we (in-band) signal invalid values for
uid and fid. So let's fix the XML parsing to do validation based on what
is actually specified in the XML.
The first issue is also related to the current code behaviour, which
is, if either uid or fid is specified by the user, it is incorrectly
assumed that both uid and fid are specified. This bug is fixed by
identifying when the user specified ZPCI address is incomplete and
auto-generating the missing ZPCI address.
Signed-off-by: Bjoern Walk <bwalk@linux.ibm.com>
Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
Signed-off-by: Shalini Chellathurai Saroja <shalini@linux.ibm.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Refer to the notion of mount propagation instead which describes
the actual behaviour more clearly.
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The two sides of a PTY can be referred to as primary and secondary
TTYs.
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
This new naming matches the terminology used in the error
messages that the callers report.
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The term "access control list" better describes the concept involved.
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The XML format used for QEMU capabilities is not required to be
stable across releases, as we invalidate the cache whenever the
libvirt binary changes.
We none the less always try to parse te entire XML file before
we do any validity checks. Thus if we change the format of any
part of the data, or change permitted values for enums, then
libvirtd logs will be spammed with errors.
These are not in fact errors, but an expected scenario.
This change makes the loading code validate the cache timestamp
against the libvirtd timestamp immediately. If they don't match
then we stop loading the rest of the XML file.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
It is easier for management software (and subsequently
distributions) to install hook script under
/etc/libvirt/hooks/$driver.d/ and have libvirt execute them in
alphabetical order. To maintain backwards compatibility,
/etc/libvirt/hooks/$driver hook script is executed the first
followed by scripts from the $driver.d directory.
The stdio is chained between the scripts. The output of the first
script is input of the second and so on.
Signed-off-by: Dmitry Nesterenko <dmitry.nesterenko@virtuozzo.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
This refactor is needed to support support hooks placed in
several files.
Signed-off-by: Dmitry Nesterenko <dmitry.nesterenko@virtuozzo.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Using virKModConfig would not simplify any existing code.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
All callers except for the test suite pass the same value
for the second arg, so it can be removed, simplifying the
code.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Mediated devices support arbitrary vendor-specific attributes that can
be attached to a mediated device. These attributes are ordered, and are
written to sysfs in order after a device is created. This patch adds
support for these attributes to the mdev data types and XML schema.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Introduced by commit 72ab0b6dc8 which
added some code depending on libvirt's log format string into
qemuProcessReadLogOutput. This function was deleted by commit
932534e85f later.
Drop the comment.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Closes: https://gitlab.com/libvirt/libvirt/-/issues/35
In v6.4.0-72-g3dda889a44 I've introduced parsing and formatting
of new sysinfo type 'fwcfg'. However, I've forgot to introduce
code that would free parsed data.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Since 9ea90206, @drvpath could be overwritten if we jumped to recheck
Found by Coverity.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Since 5084091a, @authcred is filled by a g_key_file_get_string which is
now an allocated string as opposed to some hash table lookup value, so
we need to treat it as so.
Found by Coverity
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Since 5084091a, @tmp is filled by a g_key_file_get_string which is
now an allocated string as opposed to some hash table lookup value,
so we need to treat it as so.
Found by Coverity
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Introduce two utility functions to parse a kernel command
line string according to the kernel code parsing rules in
order to enable the caller to perform operations such as
verifying whether certain argument=value combinations are
present or retrieving an argument's value.
Signed-off-by: Paulo de Rezende Pinatti <ppinatti@linux.ibm.com>
Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
This was mostly boilerplate conversion, but in one case I needed to
define several differently named char* to take the place of a single
char *tmp that was re-used multiple times, and in another place there
was a single char* that was used at the toplevel of the function, and
then later used repeatedly inside a for loop, so I defined a new
separate char* inside the loop.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
IPv6 does support masquerade since Linux 3.9.0 / ip6tables 1.4.18,
which is Fedora 18 / RHEL-7 vintage, which covers all our supported
Linux versions.
Reviewed-by: Laine Stump <laine@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
In v6.4.0-rc1~143 I've introduced a check that is supposed to
return from the function early, if given path is not a dm target.
While the idea is still valid, the implementation had a flaw.
It calls stat() over given path and the uses major(sb.st_dev) to
learn the major of the device. This is then passed to
dm_is_dm_major() which returns true or false depending whether
the device is under devmapper's control or not.
The problem with this approach is in how the major of the device
is obtained - paths managed by devmapper are special files and
thus we want to be using st_rdev instead of st_dev to obtain the
major number. Well, that's what virIsDevMapperDevice() does
already so might as well us that.
Fixes: 01626c668e
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1839992
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
When introducing virdevmapper.c (in v4.3.0-rc1~427) I didn't
realize there is a function that calls in devmapper. The function
is called virIsDevMapperDevice() and lives in virutil.c. Now that
we have a special file for handling devmapper move it there.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
Compilers are not very good at detecting this problem. Fixed by manual
inspection of compilation warnings after replacing 'VIR_FREE' with an
empty macro.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com
QEMU has -fw_cfg which allows users to tweak how firmware
configures itself and/or provide new configuration blobs.
Introduce new <sysinfo/> type "fwcfg" that will hold these
new blobs.
It's possible to either specify new value as a string or
provide a filename which contents then serve as the value.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Setting OEM strings for a domain was introduced in
v4.1.0-rc1~315. However, any application that wanted to use them
(e.g. to point to an URL where a config file is stored) had to
'dmidecode -u --oem-string N' (where N is index of the string).
Well, we can expose them under our <sysinfo/> XML and if the
domain is running Libvirt inside it can be obtained using
virConnectGetSysinfo() API.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Since nobody sets custom dmidecode path anymore, we can drop all
code that exists only because of that.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Problem with custom dmidecode scripts is that they are hard to
modify, especially if we will want them to act differently based
on passed arguments. So far, we have two scripts which do no more
than 'cat $sysinfo' where $sysinfo is saved dmidecode output.
The virCommandSetDryRun() can be used to trick
virSysinfoReadDMI() thinking it executed real dmidecode.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
When trying to decode DMI table, just before constructing
virCommand() the decoder is looked for in PATH using
virFindFileInPath(). Well, this is not necessary because
virCommandRun() will do this too (in virExec()).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Virtually every variable defined in the function can be freed
automatically when going out of scope.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
The virStateInitialize() function has ATTRIBUTE_NONNULL()
referring to @root argument (incorrectly anyway) but in
daemonRunStateInit() NULL is passed in anyway.
Then there is virCommandAddArgPair() which also has
ATTRIBUTE_NONNULL() for one of its arguments and then checks the
argument for being NULL anyways.
Signed-off-by:Bihong Yu <yubihong@huawei.com>
Reviewed-by:Chuan Zheng <zhengchuan@huawei.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
This is convenience macro, use it more. This commit was generated
using the following spatch:
@@
symbol node;
identifier old;
identifier ctxt;
type xmlNodePtr;
@@
- xmlNodePtr old;
+ VIR_XPATH_NODE_AUTORESTORE(ctxt);
...
- old = ctxt->node;
... when != old
- ctxt->node = old;
@@
symbol node;
identifier old;
identifier ctxt;
type xmlNodePtr;
@@
- xmlNodePtr old = ctxt->node;
+ VIR_XPATH_NODE_AUTORESTORE(ctxt);
... when != old
- ctxt->node = old;
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
This reverts b897973f2e
Even though it may have been the case in the past, relative
XPaths don't overwrite the ctxt->node. Thus, there's no need to
save it.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
To avoid bugs with mixing of g_object_(ref|unref) vs
virObject(Ref|Unref), we want every virObject to be
a GObject.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
To prepare for a conversion to GObject, we need virObjectUnref
to have the same API design as g_object_unref, which means it
needs to be void.
A few places do actually care about the return value though,
and in these cases a thread local flag is used to determine
if the dispose method was invoked.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The purpose of this function is to give a short description that would
be change when a host CPU is replaced with a different model. This is
currently implemented by reading /proc/cpuinfo.
It should be implemented for all architectures for which the QEMU driver
stores host CPU data in the capabilities cache. In other words for archs
that support host-model CPUs.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
qemuxml2argv test suite is way more comprehensive than the hotplug
suite. Since we share the code paths for monitor and command line
hotplug we can easily test the properties of devices against the QAPI
schema.
To achieve this we'll need to skip the JSON->commandline conversion for
the test run so that we can analyze the pure properties. This patch adds
flags for the comand line generator and hook them into the
JSON->commandline convertor for -netdev. An upcoming patch will make use
of this new infrastructure.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
QEMU models guestfwd as:
'guestfwd': [
{ "str": "tcp:10.0.2.1:4600-chardev:charchannel0" },
{ "str": "...."},
]
but the command line as:
guestfwd=tcp:10.0.2.1:4600-chardev:charchannel0,guestfwd=...
I guess the original idea was to make it extensible while not worrying
about adding another object for it. Either way it requires us to add yet
another JSON->cmdline convertor for arrays.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
In preparation for converting the generator of -netdev to generate JSON
which will be used to do the command line rather than the other way
around we need to introduce a convertor which properly configures
virQEMUBuildCommandLineJSON for the quirks of -netdev.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Add a variant similar to virJSONValueObjectAppendString which also
formats more complex value strings with printf syntax.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
The helper returns a list of arguments of a virCommand. This will be
useful in tests where we'll inspect certain already formatted arguments.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
In some cases we use 'on/off' for command line arguments. Add a switch
which will select the preferred spelling for a specific usage.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Allow reusing this for formatting of netdev_add arguments into -netdev.
We need to be able to skip the 'type' property as it's used without the
prefix by our generator.
Add infrastructure which allows skipping property with a specific name.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
The @tmpIfname is a pointer into a const string. To avoid
mistakenly changing the const string via the pointer, make the
pointer const too.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
It was never used since commit 57b5e27d3d introduced it.
Signed-off-by: Yan Wang <wangyan122@huawei.com>
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Modern way to store <auth> and <encryption> of a <disk> is under
<source>. This was added to mirror how <backingStore> handles these and
in fact they are relevant to the source rather than to any other part of
the disk. Historically we allowed them to be directly under <disk> and
we need to keep compatibility.
This wasn't a problem until introduction of -blockdev in qemu using of
<auth> or <encryption> plainly wouldn't work with backing chains.
Now that it works in backing chains and can be moved back and forth
using snapshots/block-commit we need to ensure that the original
placement is properly kept even if the source changes.
To achieve the above semantics we need to store the preferred placement
with the disk definition rather than the storage source definitions and
also ensure that the modern way is chosen when the VM started with
<source/encryption> only in the backing store.
https://bugzilla.redhat.com/show_bug.cgi?id=1822878
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
As suggested in the linked bug, libvirt should firstly check
whether the major number of the device is device mapper major.
Because if it isn't subsequent DM_DEVICE_DEPS task may not only
fail, but also yield different results. In the bugzilla this is
demonstrated by creating a devmapper target named 'loop0' and
then creating loop target /dev/loop0. When the latter is then
passed to a domain, our virDevMapperGetTargetsImpl() function
blindly asks devmapper to provide target dependencies for
/dev/loop0 and because of the way devmapper APIs work, it will
'sanitize' the input by using the last component only which is
'loop0' and thus return different results than expected.
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1823976
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
We need this for all tests that use virHostdevManager, because
during creation of this object for unprivileged connections
like those used in the test suite we would end up writing inside
the user's home directory.
That's bad manners in general, but when running the test suite
inside a purposefully constrained environment such as the one
exposed by pbuilder, it turns into an outright test failure:
Could not initialize HostdevManager - operation failed: Failed
to create state dir '/nonexistent/.cache/libvirt/hostdevmgr'
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
In a few places we use 0 and false, or 1 and true interchangeably
even though the variable or return type in question is boolean.
Fix those places.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Instead of the following pattern:
type ret;
...
ret = func();
return ret;
we can use:
return func()
directly.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
The implementation was never finished in libvirt. Remove it.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Our implementation wasn't quite able to parse everything that qemu does.
This patch rewrites the parser to a code that semantically resembles the
combination of 'nbd_parse_filename' and 'inet_parse' methods in qemu to
be able to parse the strings in an equivalent manner.
The only thing that libvirt doesn't do is to check the lengths of
various components in the nbd string in places where qemu uses constant
size buffers.
The test cases validate that some of the corner cases involving colons
are parsed properly.
https://bugzilla.redhat.com/show_bug.cgi?id=1826652
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
virCommand is now used everywhere.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Suggested-by: Sebastian Mitterle <smitterl@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Catch the individual usage not removed in previous commits.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
If an user is trying to configure a dhcp neetwork settings, it is not
possible to change the leasetime of a range or a host entry. This is
available using dnsmasq extra options, but they are associated with
dhcp-range or dhcp-hosts fields. This patch implements a leasetime for
range and hosts tags. They can be defined under that settings:
<dhcp>
<range ...>
<lease/>
</range>
<host ...>
<lease/>
</host>
</dhcp>
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=913446
Signed-off-by: Julio Faracco <jcfaracco@gmail.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
The udev monitor thread "udevEventHandleThread()" will lag the
actual/real view of devices in sysfs as it serially processes udev
monitor events. So for instance if you were to run the following cmd
to create a new veth pair and rename one of the veth endpoints
you might see the following monitor events and real world that looks like
time
| create v0 sysfs entry
wake udevEventHandleThread | create v1 sysfs entry
udev_monitor_receive_device(v1-add) | move v0 sysfs to v2
udevHandleOneDevice(v1) |
udev_monitor_receive_device(v0-add) |
udevHandleOneDevice(v0) | <--- error msgs in virNetDevGetLinkInfo()
udev_monitor_receive_device(v2-move) | as v0 no longer exists
udevHandleOneDevice(v2) |
\/
As you can see the changes in sysfs can take place well before we get
to act on the events in the udevEventHandleThread(), so by the time we
get around to processing the v0 add event, the sysfs entry has been
moved to v2.
To work around this we check if the sysfs entry is valid before
attempting to read it and don't bother trying to read link info if
not. This is safe since we will never read sysfs entries earlier than
it existing, ie. if the entry is not there it has either been removed
in the time since we enumerated the device or something bigger is
busted, in either case, no sysfs entry, no link info. In the case
described above we will eventually get the link info as we work
through the queue of monitor events and get to the 'move' event.
https://bugzilla.redhat.com/show_bug.cgi?id=1557902
Signed-off-by: Mark Asselstine <mark.asselstine@windriver.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
While I'm at it, use more g_autofree and g_autoptr() in this
file. This also fixes a possible mem-leak in
virNetDevGetVirtualFunctions().
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
I've just got a new machine and I'm still converging on the
kernel config. Anyway, since I don't have enabled any of SRIO-V
drivers, my kernel doesn't have NET_DEVLINK enabled (i.e.
virNetDevGetFamilyId() returns 0). But this makes nodedev driver
ignore all interfaces, because when enumerating all devices via
udev, the control reaches virNetDevSwitchdevFeature() eventually
and subsequently virNetDevGetFamilyId() which 'fails'. Well, it's
not really a failure - the virNetDevSwitchdevFeature() stub
simply returns 0.
Also, move the call a few lines below, just around the place
where it's needed.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Introduced in v3.8.0-rc1~96, the virNetDevGetFamilyId() gets
netlink family ID for passed family name (even though it's used
only for getting "devlink" ID). Nevertheless, the function
returns 0 on an error or if no family ID was found. This makes it
harder for a caller to distinguish these two. Change the retval
so that a negative value is returned upon error, zero is no ID
found (but no error encountered) and a positive value is returned
on successful translation.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
This commit partially reverts
commit c360ea28dc
Refs: v6.2.0-rc1-1-gc360ea28dc
Author: Rafael Fonseca <r4f4rfs@gmail.com>
AuthorDate: Fri Mar 27 18:40:47 2020 +0100
Commit: Michal Prívozník <mprivozn@redhat.com>
CommitDate: Mon Mar 30 09:48:22 2020 +0200
util: virdaemon: fix compilation on mingw
The daemons are not supported on Win32 and therefore were not compiled
in that platform. However, with the daemon code sharing, all the code in
utils *is* compiled and it failed because `waitpid`, `fork`, and
`setsid` are not available. So, as before, let's not build them on
Win32 and make the code more portable by using existing vir* wrappers.
Not compiling virDaemonForkIntoBackground on Win32 is good, but the
second part of the original patch incorrectly replaced waitpid and fork
with our virProcessWait and virFork APIs. These APIs are more than just
simple wrappers and we don't want any of the extra functionality.
Especially virFork would reset any setup made before
virDaemonForkIntoBackground is called, such as logging, signal handling,
etc.
As a result of the change the additional fix in v6.2.0-67-ga87e4788d2
(util: virdaemon: fix waiting for child processes) is no longer
needed and it is effectively reverted by this commit.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Unlike `waitpid`, `virProcessWait` only returns -1 (error) or 0
(success), so comparing that to `pid` will always be false and the
parent will report failure with:
error : main:851 : Failed to fork as daemon: No such file or directory
even though the grandchild process is succesfully running. Note that the
errno message is misleading: it was last set when trying to find a
restart state file.
Signed-off-by: Rafael Fonseca <r4f4rfs@gmail.com>
Reported-by: Marcin Krol <hawk@tld-linux.org>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Ján Tomko <jtomko@redhat.com>
For http/https URIs we need to preserve the query part as it may be
important to refer to the image.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Add a new attribute for holding the query part for http(s) disks.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
While it is impossible for VIR_ALLOC() to return an error, we
should be consistent with the rest of the code and not continue
initializing the virSecurityDeviceLabelDef structure.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Unfortunately, yajl_free() is not NOP on NULL. It really does
expect a valid pointer. Therefore, check whether the pointer we
want to pass to it is NULL or not.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
The daemons are not supported on Win32 and therefore were not compiled
in that platform. However, with the daemon code sharing, all the code in
utils *is* compiled and it failed because `waitpid`, `fork`, and
`setsid` are not available. So, as before, let's not build them on
Win32 and make the code more portable by using existing vir* wrappers.
Signed-off-by: Rafael Fonseca <r4f4rfs@gmail.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Several daemons have similar code around general daemon startup code.
Let's move it into a file and share it among them.
Signed-off-by: Rafael Fonseca <r4f4rfs@gmail.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
The quotes are forbidden only inside the value, but the value itself may
be enclosed in quotes. Fix the RNG schema and validator and add a test
case.
https://bugzilla.redhat.com/show_bug.cgi?id=1804750
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Unfortunately, advisory record locking lose the lock if any fd refering
to the file is closed. There doesn't seem to be a way to preserve the
lock atomically. We could eventually retake the lock if low pidfilefd
is required.
This fixes processes being leaked, as they are not killed in
virPidFileForceCleanupPath() if the lock can be taken. Here also, we may
consider this is not good enough, as a process may leak by simply
closing the pidfilefd.
Fixes commit d146105f1e ("virCommand:
Actually acquire pidfile instead of just writing it")
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Our virCommand module allows us to set a pidfile for commands we
want to spawn. The caller constructs the string of pidfile path
and then uses virCommandSetPidFile() to tell the module to write
the pidfile once the command is ran. This usually works, but has
two flaws:
1) the child process does not hold the pidfile open & locked.
Therefore, the caller (or anybody else) can't use our fancy
virPidFileForceCleanupPath() function to kill the command
afterwards. Also, for everybody else on the system it's
needlessly harder to check if the pid from the pidfile is still
alive or not.
2) if the caller ever makes a mistake and passes the same pidfile
path for two different commands, the start of the second command
will overwrite the pidfile even though the first command might
still be running.
NOTE that this temporarily renders some command spawning
unusable, specifically those code patterns where both
virCommandSetPidFile() is used together with instructing spawned
command to acquire pidfile itself. Fortunately, there is only one
occurrence of such pattern and it is in
qemuProcessStartManagedPRDaemon(). This is fixed in next commit.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Our code allows snapshots of NVMe based disks which means we create
overlay file with a 'json:{}' pseudo-uri refering to the NVME device.
Our parser code doesn't handle them though. Add the parser and test it
via the XML->json->XML round-trip and reference data.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
qemublocktest showed that we don't add the "fat:" prefix for directory
storage when formatting the backing store string. While it's unlikely to
be used it's simple enough to actually implement the support rather than
trying to forbid it.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
While 'namespace' is not a reserved word in C, it is in C++. Our
compilers are happy with it but syntax-hilighting in some editors
hilights is as a keyword. Rename it to prevent confusion.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
virHostCPUGetStatsLinux walks through every cpu in /proc/stat until it
finds cpu%cpuNum that matches with the requested cpu.
If none is found it logs the error but it should return -1, instead of 0.
Otherwise virsh nodecpustats --cpu <invalid cpu number> and API bindings
don't fail properly, printing a blank line instead of an error message.
This patch also includes an additional test for virhostcputest to avoid
this regression to happen again in the future.
Fixes: 93af79fba3
Reported-by: Satheesh Rajendran <satheera@in.ibm.com>
Signed-off-by: Mauro S. M. Rodrigues <maurosr@linux.vnet.ibm.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
Tested-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
The functionality is now provided by glib's GKeyFile.
Signed-off-by: Rafael Fonseca <r4f4rfs@gmail.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Replace libvirt's virKeyFile by glib's GKeyFile.
Signed-off-by: Rafael Fonseca <r4f4rfs@gmail.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
When running a function in a forked child, so far the only thing
we could report is exit status of the child and the error
message. However, it may be beneficial to the caller to know the
actual error that happened in the child.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Pavel Mores <pmores@redhat.com>
The @src is not always a file. It may also be a directory (for
instance qemuDomainCreateDeviceRecursive() assumes that) - even
though it doesn't happen usually. Anyway, mount() can mount only
a dir onto a dir and a file onto a file.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Pavel Mores <pmores@redhat.com>
For the few instances where we'd generate an array in dotted syntax we
should be able to parse it back. Add another step in deflattening of the
dotted syntax which reconstructs the arrays so that the backing store
parser can parse it.
https://bugzilla.redhat.com/show_bug.cgi?id=1466177
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Extract the code so that there's a clean separation once we'll want do
do other steps.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Use automatic memory handling to remove the cleanup section.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
virBitmapNewEmpty can't fail now so we can make it obvious and fix all
callers.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
virStorageEncryptionSecretPtr may have a string inside it, thus we must
copy the string too. Use virSecretLookupDefCopy to do that.
Caused by non-obvious code introduced in 756b46ddd2 and later 47e88b33b
which added a string that needed to be copied.
https://bugzilla.redhat.com/show_bug.cgi?id=1814923
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
The function always returns succes so there's no need for a return
value.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Commit 7b79ee2f78 makes assumptions about die_id parsing in
the sysfs that aren't true for Power hosts. In both Power8
and Power9, running 5.6 and 4.18 kernel respectively,
'die_id' is set to -1:
$ cat /sys/devices/system/cpu/cpu0/topology/die_id
-1
This breaks virHostCPUGetDie() parsing because it is trying to
retrieve an unsigned integer, causing problems during VM start:
virFileReadValueUint:4128 : internal error: Invalid unsigned integer
value '-1' in file '/sys/devices/system/cpu/cpu0/topology/die_id'
This isn't necessarily a PowerPC only behavior. Linux kernel commit
0e344d8c70 added in the former Documentation/cputopology.txt, now
Documentation/admin-guide/cputopology.rst, that:
To be consistent on all architectures, include/linux/topology.h
provides default definitions for any of the above macros that are
not defined by include/asm-XXX/topology.h:
1) topology_physical_package_id: -1
2) topology_die_id: -1
(...)
This means that it might be expected that an architecture that
does not implement the die_id element will mark it as -1 in
sysfs.
It is not required to change die_id implementation from uInt to
Int because of that. Instead, let's change the parsing of the
die_id in virHostCPUGetDie() to read an integer value and, in
case it's -1, default it to zero like in case of file not found.
This is enough to solve the issue Power hosts are experiencing.
Fixes: 7b79ee2f78
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
We currently don't model the 'ssh' protocol properties properly and
since it seems impossible for now (agent path passed via environment
variable). To allow libguestfs to work as it used in pre-blockdev era we
must carry the properties over to the command line. For this instance we
just store it internally and format it back.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
libguestfs abuses a quirk of qemu's parser to accept also other variants
of the 'sslverify' field which would be valid on the command line but
are not documented in the QMP schema.
If we encounter the 'off' string instead of an boolean handle it rather
than erroring out to continue support of pre-blockdev configurations.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Add support for parsing the recently added fields from backing file
pseudo-protocol strings.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Some disk backends support configuring the readahead buffer or timeout
for requests. Add the knobs to the XML.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Add possibility to specify one or more cookies for http based disks.
This patch adds the config parser, storage and validation of the
cookies.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
To allow turning off verification of SSL cerificates add a new element
<ssl> to the disk source XML which will allow configuring the validation
process using the 'verify' attribute.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Now that we use g_strerror exclusively, remove this unused
function.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
The stub impl of virGetDeviceID just returns ENOSYS and does not
initialize the min/maj output parameters. This lead to a false
positive warning on mingw about possible use of uninitialized
variables.
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
If a disk has persistent reservations enabled, qemu-pr-helper
might open not only /dev/mapper/control but also individual
targets of the multipath device. We are already querying for them
in CGroups, but now we have to create them in the namespace too.
This was brought up in [1].
1: https://bugzilla.redhat.com/show_bug.cgi?id=1711045#c61
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Tested-by: Lin Ma <LMa@suse.com>
Reviewed-by: Jim Fehlig <jfehlig@suse.com>
We want a way to easily run a private GMainContext in a
thread, with correct synchronization between startup
and shutdown of the thread.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The virbpf module wraps syscalls to BPF. However, if the kernel
headers used at the compile time don't have support for BPF the
module offers stubs which return a negative one to signal error
to the caller. But there is a slight discrepancy between real
functions and these stubs. While the former set errno and return
-1 the latter report an error (without setting the errno) and
return -1. This is not optimal because the caller might see stale
errno and overwrite the error message with a less accurate one.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
In the virCgroupV2DevicesAvailable() function we try to determine
whether CGroups version 2 are available. We do this by opening
what we believe is the CGroup mount point and issuing a BPF call.
When the call fails, a debug message is printed. However, the BPF
call sets errno too. Include it in the debug message to help us
with debugging.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Virtualization event types were added in 2.0.5:
https://github.com/linux-audit/audit-userspace/commit/3755e9ff
Even Ubuntu 14.04 (which we don't support) has 2.3.2.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
When spawning a thread via our virThread APIs we let pthread
spawn this helper thread which sets couple of thread local
variables (e.g. thread job name or thread worker name) and as of
v6.1.0-40-gc85256b31b it also sets pthread name (which is then
visible in `ps' output for instance). Only after these steps the
intended function is called. However, just before calling it we
free the buffer that holds the thread name which results in
invalid memory reads:
==47027== Invalid read of size 1
==47027== at 0x48389C2: strlen (vg_replace_strmem.c:459)
==47027== by 0x58BB3D6: __vfprintf_internal (vfprintf-internal.c:1645)
==47027== by 0x58CE6E0: __vasprintf_internal (vasprintf.c:57)
==47027== by 0x574BA28: g_vasprintf (in /usr/lib64/libglib-2.0.so.0.6000.7)
==47027== by 0x57240CC: g_strdup_vprintf (in /usr/lib64/libglib-2.0.so.0.6000.7)
==47027== by 0x48E0EFA: vir_g_strdup_vprintf (glibcompat.c:209)
==47027== by 0x493AA05: virLogVMessage (virlog.c:573)
==47027== by 0x493A8FE: virLogMessage (virlog.c:513)
==47027== by 0x4992FC7: virThreadJobClear (virthreadjob.c:121)
==47027== by 0x4992844: virThreadHelper (virthread.c:237)
==47027== by 0x5817496: start_thread (pthread_create.c:486)
==47027== by 0x59563CE: clone (clone.S:95)
The problem is that neither virThreadJobSetWorker() nor
virThreadJobSet() create a copy of passed name. They just set a
thread local variable to point to the buffer which is then
freed. Moving the free towards the end of the wrapper function
solves the issue.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Our implementation masks GCC warnings of uninitialized use of the passed
argument. After changing this I got a load of following warnings:
src/conf/virnetworkportdef.c: In function 'virNetworkPortDefSaveStatus':
/usr/include/glib-2.0/glib/gmem.h:136:8: error: 'path' may be used uninitialized in this function [-Werror=maybe-uninitialized]
136 | if (_p) \
| ^
src/conf/virnetworkportdef.c:447:11: note: 'path' was declared here
447 | char *path;
| ^~~~
For the curious, g_clear_pointer is still safe for arguments with
side-effect. Here's the pre-processed output of trying to do a
VIR_FREE(*(test2++)):
do {
typedef char _GStaticAssertCompileTimeAssertion_1[(sizeof *(&(*(test2++))) == sizeof (gpointer)) ? 1 : -1] __attribute__((__unused__));
__typeof__((&(*(test2++)))) _pp = (&(*(test2++)));
__typeof__(*(&(*(test2++)))) _ptr = *_pp;
*_pp = ((void *)0);
if (_ptr)
(g_free) (_ptr);
} while (0) ;
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Historically threads are given a name based on the C function,
and this name is just used inside libvirt. With OS level thread
naming this name is now visible to debuggers, but also has to
fit in 15 characters on Linux, so function names are too long
in some cases.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Setting the thread name makes it easier to debug libvirtd
when many threads are running.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Make it obvious that the function always returns a valid pointer and fix
all callers.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
