The function can't fail at this point. Remove the return value.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The function can't fail at this point. Remove the return value.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The function can't fail at this point. Remove the return value.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
There's nothing that can fail in the function. Remove the return value
and adjust callers.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The function isn't used besides tests. Since the separator parsing
capability is trivial we can keep it in place and just unexport it for
now.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Use automatic memory freeing for the temporary bitmap and remove the
pointless 'cleanup' section.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Use automatic memory freeing for the temporary bitmap and remove the
pointless 'cleanup' section.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Use automatic memory freeing for the temporary bitmap and remove the
pointless 'cleanup' section.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Whenever virPCIGetNetName() is called, it is either called with
physPortID = NULL, or with it set by the caller calling
virNetDevGetPhysPortID() soon before virPCIGetNetName(). The
physPortID is then used *only* in virPCIGetNetName().
Rather than replicating that same call to virNetDevGetPhysPortID() in
all the callers of virPCIGetNetName(), lets just have all those
callers send the NetDevName whose physPortID they want down to
virPCIGetNetName(), and let virPCIGetNetName() call
virNetDevGetPhysPortID().
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Commit 795e9e05c3 (libvirt-7.7.0) refactored the code in virpci.c and
virnetdev.c that gathered lists of the Virtual Functions (VF) of an
SRIOV Physical Function (PF) to simplify the code.
Unfortunately the simplification made the assumption, in the new
function virPCIGetVirtualFunctionsFull(), that a VF's netdev
interface name should only be retrieved if the PF had a valid
phys_port_id. That is an incorrect assumption - only a small handful
of (now previous-generation) Mellanox SRIOV cards actually use
phys_port_id (this is for an odd design where there are multiple
physical network ports on a single PCI address); all other SRIOV cards
(including new Mellanox cards) have a file in sysfs called
phys_port_id, but it can't be read, and so the pfPhysPortID string is
NULL.
The result of this logic error is that virtual networks that are a
pool of VFs to be used for macvtap connections will be unable to
start, giving an errror like this:
VF 0 of SRIOV PF enp130s0f0 couldn't be added to the interface pool because it isn't bound to a network driver - possibly in use elsewhere
This error message is misinformed - the caller of
virNetDevGetVirtualFunctionsFull() only *thinks* that the VF isn't
bound to a network driver because it doesn't see a netdev name for the
VF in the list. But that's only because
virNetDevGetVirtualFunctionsFull() didn't even try to get the names!
We do need a way for virPCIGetVirtualFunctionsFull() to sometimes
retrieve the netdev names and sometimes not. One way of doing that
would be to send down the netdev name of the PF whenever we also want
to know the netdev names of the VFs, but send a NULL when we
don't. This can conveniently be done by just *replacing* pfPhysPortID
in the arglist with pfNetDevName - pfPhysPortID is determined by
simply calling virNetDevGetPhysPortID(pfNetDevName) so we can just
make that call down in virPCIGetVirtualFunctionsFull() (when needed).
This solves the regression introduced by commit 795e9e05c3, and also
nicely sets us up to (in a subsequent commit) move the call to
virNetDevGetPhysPortID() down one layer further to virPCIGetNetName(),
where it really belongs!
Resolves: https://bugzilla.redhat.com/2025432
Fixes: 795e9e05c3
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
For some weird reason we are ignoring errors when creating veth
pair that netlink reports. This affects the LXC driver which
creates interfaces for container in
virLXCProcessSetupInterfaces(). If creating a veth pair fails, no
error is reported and the control jumps onto cleanup label where
some cryptic error message is reported instead (something about
inability to remove veth pair).
Let's report error that netlink returned - it's probably the most
accurate reason anyways.
Resolves: https://gitlab.com/libvirt/libvirt/-/issues/225
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
Currently virThreadPoolNewFull relies on the caller to ensure the job
name outlives the thread pool. Which basically enforces static strings.
Let's drop this implicit requirement by making a copy of the job name.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
We pass through to glib's hash table functions so we can also use glibs
function prototype definition.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
The code was converted to stop using this function.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
Use 'g_clear_pointer(&ptr, g_hash_table_unref)' instead.
In few instances it allows us to also remove explicit clearing of
pointers.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
The helpers will update the passed boolean if the tristate's value is
not _ABSENT.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The QEMU domain capabilities code wants to quietly know whether swtpm is
available on the host.
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Many methods merely want to know that the swtpm binaries have been
found, and don't care about probing for capabilities. Even when
starting a guest, the QEMU driver may not need the capabilities.
Skipping probing ensures the VM startup path is as fast as possible
when capabilities are not required. It also removes various error
scenarios from the main init method.
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The virTPMEmulatorInit method updates various global variables
and holds a lock while doing so. Other methods which access
these variables, however, don't reliably hold locks over all
of their accesses.
Since virTPMEmulatorInit is no longer exported, we can push
the locking up into all the callers and achieve proper safety
for concurrent usage.
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Every other exported API from virtpm.h will internally call
virTPMEmulatorInit, so there is no reason for this initializer
to be exported on its own.
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The virTPMEmulatorInit function defines a struct that gets filled with
pointers to global variables. It will be simpler to just use the struct
for the global variables directly.
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The TPM helper methods for querying the binary path and capabilities
have the same patterns across all swtpm binaries. This code duplication
can be reduced by introducing helper methods.
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Best practice is to have all types use a naming convention based on the
filename.
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
macOS on Apple silicon reports 'arm64' as the architecture from uname,
which we need to canonicalize to VIR_ARCH_AARCH64 / 'aarch64'.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The check for whether the swtpm binary was modified is checking pointers
to the mtime field in two distinct structs, so will always compare
different. This resulted in re-probing swtpm capabilities every time,
as many as 20 times for a single VM launch.
Fixes:
commit 01cf7a1bb9
Author: Stefan Berger <stefanb@us.ibm.com>
Date: Thu Jul 25 14:22:04 2019 -0400
tpm: Check whether previously found executables were updated
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
When VIR_EXEC_DAEMON is set, if virPidFileAcquirePath/virSetInherit failed,
then pipesync[0] can not be closed when granchild process exit, because
pipesync[1] still opened in child process. and then saferead in child
process may blocked forever, and left grandchild process in defunct state.
Signed-off-by: Xu Chao <xu.chao6@zte.com.cn>
Signed-off-by: Yi Wang <wang.yi59@zte.com.cn>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
The functions have very difficult semantics where callers are not able
to tell whether the property is missing or failed the length check. Only
the latter produces errors.
Since usage of the functions was phased out, remove them completely to
avoid further broken code.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Use the appropriate enum type instead of an int and fix the XML parser
and one missing fully populated switch.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
This eliminates one incorrect parsing implementation which relied on the
command field not having a closing bracket. This possibility is already
tested against in the virProcessGetStat() tests.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
This reads and separates all fields from /proc/<pid>/stat or
/proc/<pid>/task/<tid>/stat as there are easy mistakes to be done in the
implementation. Some tests are added to show it works correctly. No number
parsing is done as it would be unused for most of the fields most, if not all,
of the time. No struct is used for the result as the length can vary (new
fields can be added in the future).
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
It does not need a tty to work, it opens its controlling terminal for user
interaction and with this patch even crazy things like this work:
echo 'list --name' | virsh -q >/dev/null
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
With this function we can decide whether to try running the polkit text agent
only if it is available, removing a potential needless error saying that the
agent binary does not exist, which is useful especially when running the agent
before knowing whether it is going to be needed.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Automatic "Ptr " -> " *" also wreaked havoc in comments. Fix it and while at it
reword the sentence so it is clear that the object is newly allocated.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Recently, FreeBSD has got sched_get/setaffinity(3) implementations and
the sched.h header as well [1]. To make these routines visible,
users have to define _WITH_CPU_SET_T.
This breaks current detection. Specifically, meson sees the
sched_getaffinity() symbol and defines WITH_SCHED_GETAFFINITY. This
define unlocks Linux implementation of virProcessSetAffinity() and other
functions, which fails to build on FreeBSD because cpu_set_t is not
visible as _WITH_CPU_SET_T is not defined.
For now, change detection to the following:
- Instead of checking sched_getaffinity(), check if 'cpu_set_t' is
available through sched.h
- Explicitly check the sched.h header instead of assuming its presence
if WITH_SCHED_SETSCHEDULER is defined
1:
https://cgit.freebsd.org/src/commit/?id=43736b71dd051212d5c55be9fa21c45993017fbbhttps://cgit.freebsd.org/src/commit/?id=160b4b922b6021848b6b48afc894d16b879b7af2https://cgit.freebsd.org/src/commit/?id=90fa9705d5cd29cf11c5dc7319299788dec2546a
Signed-off-by: Roman Bogorodskiy <bogorodskiy@gmail.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
Git bisect took me to commit where incorrect usage of ATTRIBUTE_NONNULL
was introduced and caused coverity scan to fail. This patch fixes the
issue where the index starts from 1 and not 0 and two other different
cases.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
In some cases we have a label that contains nothing but a return
statement. The amount of such labels rises as we use automagic
cleanup. Anyway, such labels are pointless and can be dropped.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
The functions were obsoleted by virJSONValueObjectAdd(VArgs)
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Until now the code would crash if virJSONValueObjectAdd is used without
a valid object. Adding the functionality of allocating it if it's NULL
will allow us to replace all uses of virJSONValueObjectCreate with this
single function.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Pass in the double pointer from the wrappers directly to
virJSONValueObjectAddVArgs, which will allow us to directly allocate the
new objects inside the function.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Change the first argument to a double pointer so that later the function
can be unified with virJSONValueObjectCreate and fix all callers.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
There are a few cases where a string list is freed by an explicit
call of g_strfreev(), but the same result can be achieved by
g_atuo(GStrv).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Tim Wiederhake <twiederh@redhat.com>
Remove unnecessary label, goto, and closing of not-open file descriptor.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Extend the TPM backend XML with a node 'active_pcr_banks' that allows a
user to specify the PCR banks to activate before starting a VM. Valid
choices for PCR banks are sha1, sha256, sha384 and sha512. When the XML
node is provided, the set of active PCR banks is 'enforced' by running
swtpm_setup before every start of the VM. The activation requires that
swtpm_setup v0.7 or later is installed and may not have any effect
otherwise.
<tpm model='tpm-tis'>
<backend type='emulator' version='2.0'>
<active_pcr_banks>
<sha256/>
<sha384/>
</active_pcr_banks>
</backend>
</tpm>
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2016599
Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
I've noticed one function inside virpcivpd.c, namely
virPCIVPDParseVPDLargeResourceFields() that declares some
variables at the top level even though they are used only inside
a loop in which they have to be freed explicitly.
Bringing variable declarations into the loop allows us to make
the code nicer.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Tim Wiederhake <twiederh@redhat.com>
While invalid values need to be ignored when presenting VPD data to the
user, it would be good to attempt to parse a valid portion of the VPD
instead of marking it invalid as a whole.
Based on a mailing list discussion, the set of accepted characters is
extended to the set of printable ASCII characters.
https://listman.redhat.com/archives/libvir-list/2021-October/msg01043.html
The particular example encountered on real hardware was multi-faceted:
* "N/A" strings present in read-only fields. This would not be a useful
valid value for a field (especially if a unique serial number is
expected), however, it was decided to delegate handling of those kinds
of values to higher-level software;
* "4W/1W PCIeG2x4" - looks like some vendors use even more printable
characters in the ASCII range than we currently allow. Since the
PCI/PCIe VPD specs mention alphanumeric characters without specifying
the full character set, it looks like this is ambiguous for vendors
and they tend to use printable ASCII characters;
* 0xFF bytes present in VPD-W field values. Those bytes do not map to
printable ASCII code points and were probably used by the vendor as
placeholders. Ignoring the whole VPD because of that would be too
strict.
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Dmitrii Shcherbakov <dmitrii.shcherbakov@canonical.com>
* RV and RW fields must be at the last position in their respective
section (per the conditions in the spec). Therefore, the parser now
stops iterating over fields as soon as it encounters one of those
fields and checks whether the end of the resource has been reached;
* The lack of the RW field is not treated as a parsing error since we
can still extract valid data even though this is a PCI/PCIe VPD spec
violation;
* Individual fields must have a valid length - the parser needs to check
for invalid length values that violate boundary conditions of the
resource.
* A zero-length field may be the last one in the resource, however, the
boundary check is currently too strict to allow that.
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Dmitrii Shcherbakov <dmitrii.shcherbakov@canonical.com>
For other interface type, values in tc rules are calculated by
multiply by 8*1000 instead of 8*1024.
Signed-off-by: zhangjl02 <zhangjl02@inspur.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
When 'swtpm_setup --print-capabilities' shows the 'tpm12-not-need-root'
flag, then it is possible to create certificates for the TPM 1.2 also
in non-privileged mode since swtpm_setup doesn't need tcsd anymore.
Check for this flag and create the certificates if this flag is found.
Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
In some cases the worker func running inside the pool may rely on
virIdentity. While worker func could check for identity and set
one it is not optimal - it may not have access to the identity of
the thread creating the pool and thus would have to call
virIdentityGetSystem(). Allow passing identity when creating the
pool.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
It's only used once and open coding it is at least as clear as using the
macro.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
In a few places we declare a variable (which is optionally
followed by a code not touching it) then set the variable to a
value and return the variable immediately. It's obvious that the
variable is needless and the value can be returned directly
instead.
This patch was generated using this semantic patch:
@@
type T;
identifier ret;
expression E;
@@
- T ret;
... when != ret
when strict
- ret = E;
- return ret;
+ return E;
After that I fixed couple of formatting issues because coccinelle
formatted some lines differently than our coding style.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Add helper functions to virpci to provide means of checking for a VPD
file presence and for VPD resource retrieval using the PCI VPD parser.
The added test assesses the basic functionality of VPD retrieval while
the full parser is tested by virpcivpdtest.
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Dmitrii Shcherbakov <dmitrii.shcherbakov@canonical.com>
Add support for deserializing the binary PCI/PCIe VPD format and storing
results in memory.
The VPD format is specified in "I.3. VPD Definitions" in PCI specs
(2.2+) and "6.28.1 VPD Format" PCIe 4.0. As section 6.28 in PCIe 4.0
notes, the PCI Local Bus and PCIe VPD formats are binary compatible
and PCIe 4.0 merely started incorporating what was already present in
PCI specs.
Linux kernel exposes a binary blob in the VPD format via sysfs since
v2.6.26 (commit 94e6108803469a37ee1e3c92dafdd1d59298602f) which requires
a parser to interpret.
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Dmitrii Shcherbakov <dmitrii.shcherbakov@canonical.com>
Using swtpm v0.7.0 we can run swtpm_setup to create default config files
for swtpm_setup and swtpm-localca in session mode. Now a user can start
a VM with an attached TPM without having to run this program on the
command line before. This program needs to run once.
This patch addresses the issue raised in
https://bugzilla.redhat.com/show_bug.cgi?id=2010649
Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
For conversion of '-device' we'll try to avoid usage of arrays if
possible, so for now if the array coversion function is not provided the
convertor will error out.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
With automatic memory freeing we can simplify the function to avoid two
almost-identical calls to virQEMUBuildCommandLineJSONRecurse.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The returned argument list is a NULL-terminated string list and the only
caller doesn't use the count. Remove the argument.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Optimize the number of string copies by using the virBuffers in the
callers directly. Simplest way to achieve this is to just open code the
one function call 'virQEMUBuildDriveCommandlineFromJSON' was wrapping
in the two callers.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Now that everything was replaced by the new code we can remove this
function.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Base the JSON output on a regular capability flag rather than purely
internal flag. This will prepare for the time when QEMU will accept JSON
argumets for -netdev.
For now the capability is not set (thus we for now don't have QMP
schema validation) but that will be addressed later.
To achieve this 'qemuBuildNetdevCommandlineFromJSON' is introduced
and all callers of 'virQEMUBuildNetdevCommandlineFromJSON' are
refactored to use the new helper.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Enforce that the ':' separator between the key and value is always
present.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
In many cases we use a signed value, but use the sign to note that it
was not assigned. For converting to JSON objects it will be handy to
have possibility to do this automatically.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
When a server decides to close a client, the
virNetServerClientCloseLocked() is called. In here various
cleanup steps are taken, but the most important part (from this
commit's POV at least) is the way that the socket is closed.
Firstly, removal of the socket associated with the client from
the event loop is signalized and then the socket is unrefed. The
socket is not closed just yet though, because the event loop
holds a reference to it. This reference will be freed as soon as
the event loop wakes up and starts issuing callbacks (in this
case virNetSocketEventFree()).
So far, this is how things usually work. But if the daemon
reaches the number of opened files limit, things start to work
differently.
If the RLIMIT_NOFILE limit is reached and there's a client that
wants to connect then the event loop wakes up, sees POLLIN on the
socket and calls virNetServerServiceAccept() which in turn calls
virNetSocketAccept(). But because of the limit, accept() fails
with EMFILE leaving the POLLIN event unhandled. The dispatch then
continues to next FDs with events on them. BUT, it will NOT call
the socket removal callback (virNetSocketEventFree()) because it
has low priority (G_PRIORITY_DEFAULT_IDLE). Per glib's
documentation:
* Each event source is assigned a priority. The default priority,
* %G_PRIORITY_DEFAULT, is 0. Values less than 0 denote higher priorities.
* Values greater than 0 denote lower priorities. Events from high priority
* sources are always processed before events from lower priority sources.
and per g_idle_add() documentation:
* Adds a function to be called whenever there are no higher priority
* events pending to the default main loop. The function is given the
* default idle priority, %G_PRIORITY_DEFAULT_IDLE.
Now, because we did not accept() the client we are constantly
seeing POLLIN on the main socket and thus the removal of the
client socket won't ever happen.
The fix is to set at least the same priority as other sources,
but since we want to just close an FD, let's give it the highest
priority and call it before handling other events.
This issue can be easily reproduced, for instance:
# ulimit -S -n 40 (tweak this number if needed)
# ./src/libvirtd
from another terminal:
# for ((i=0; i<100; i++)); do virsh list & done; virsh list
The last `virsh list` must not get stuck.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2007168
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
New virHostMemGetTHPSize() is introduced which allows caller to
obtain THP PMD (Page Middle Directory) size, which is equal to
the minimal size that THP can use, taken from kernel doc
(Documentation/admin-guide/mm/transhuge.rst):
Some userspace (such as a test program, or an optimized memory allocation
library) may want to know the size (in bytes) of a transparent hugepage::
cat /sys/kernel/mm/transparent_hugepage/hpage_pmd_size
Since this size depends on the host architecture and the kernel
it won't change whilst libvirtd is running. Therefore, we can use
virOnce() and cache the value. Of course, we can be running under
kernel that has THP disabled or has no notion of THP at all. In
that case a negative value is returned to signal error.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
It always returns true. Make the logic a bit simpler to see through.
This completely removes 'virCryptoHaveCipher' as it's pointless in the
current form.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Due to the way we detect programs at runtime there's no
difference between $PROG and $PROG_PATH macros that come from
meson-config.h. Either both are set to the path found during
configure or both are set to just "$prog", e.g.:
#define EBTABLES "/sbin/ebtables"
#define EBTABLES_PATH "/sbin/ebtables"
#define FLAKE8 "flake8"
#define FLAKE8_PATH "flake8"
Change those few places which use _PATH.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Of the two callers one simply iterates over the returned paths and the
second one appends the returned paths to another linked list. Simplify
all of this by directly returning a linked list.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
There are two distinct uses of an arbitrary buffers size when querying
the device mapper. One is related to loading the /proc/devices file,
while the other is used as buffer for ioctls to the devmapper.
Split up the macros used here so that it's clear that they are not meant
for the same thing.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
In the unlikely case that we were unable to set the new
identity, we would unref the old one even though it still
could be in the thread-local storage.
Fixes: c6825d8813
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Clang on Rawhide started to complain that @tmp variable in
virSCSIDeviceListDel() is set but not used. This is obviously a
false positive because the variable is used to free device stolen
from the list. Anyway, we can do without the variable so in this
specific case let's fix our code to appease Clang.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
The virPCIDeviceIsBehindSwitchLackingACS() function checks
whether given PCI device is not behind a switch that lacks ACS.
It does so by starting at given device and traversing up, one
parent at time towards the root. The parent device is obtained
via virPCIDeviceGetParent() which allocates new virPCIDevice
structure. For freeing the structure we use g_autoptr() and a
temporary variable @tmp. However, Clang fails to understand our
clever algorithm and complains that the variable is set but never
used. This is obviously a false positive, but using a small trick
we can shut Clang up.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Simon Rowe <simon.rowe@nutanix.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
When using 'virsh freepages' or 'virsh allocpages' then
virHostMemGetFreePages() or virHostMemAllocPages() is called,
respectively. But the following may happen: libvirt was built
without numactl support and thus a fake NUMA node was constructed
for capabilities, which means that startCell is going to be 0.
But we can't blindly pass startCell = 0 to virNumaGetPageInfo()
nor virNumaSetPagePoolSize() because they would operate over node
specific path (/sys/devices/system/node/nodeX) rather than NUMA
agnostic path (/sys/kernel/mm/hugepages/) and we are not
guaranteed that the former exists (kernel might have been built
without NUMA support).
Resolves:https://bugzilla.redhat.com/show_bug.cgi?id=1978574
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
In all three cases (LXC, QEMU and VBox drivers) the caller has
access to host capabilities and thus know the maximum NUMA node.
This means, that virHostMemAllocPages() doesn't have to query
it. Querying may fail if libvirt was compiled without numactl
support.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
In all three cases (LXC, QEMU and VBox drivers) the caller has
access to host capabilities and thus know the maximum NUMA node.
This means, that virHostMemGetFreePages() doesn't have to query
it. Querying may fail if libvirt was compiled without numactl
support.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
When setting O_CLOEXEC flag on received FD fails the FD is closed
using VIR_FORCE_CLOSE(). But the call is wrapped in errno save
which is not necessary because VIR_FORCE_CLOSE() preserves errno
value.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Warn these error instead of return when removing qos or queues. This will
avoid residual qos clearance on multiple interfaces.
Signed-off-by: zhangjl02 <zhangjl02@inspur.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Separate virNetDevOpenvswitchInterfaceClearQos into two steps. When setting
qos, we can set only rx or tx and the other one should be cleared.
Signed-off-by: zhangjl02 <zhangjl02@inspur.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Add vmuuid notes on virNetDevOpenvswitchInterfaceSetQos,
and change vmid to vmuuid.
Signed-off-by: Jinsheng Zhang <zhangjl02@inspur.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
For new feature Fibre Channel VMID we will need to get inode of the
VM root cgroup as it is used in the new kernel API together with VMID.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
We need this in order to validate XML against schema at one
place, rather than have the same code for validation in different
functions.
I will add '--validate' option to more virsh commands soon and
this makes it easier as virXMLParse() is called in every one I
plan to change.
Signed-off-by: Kristina Hanicova <khanicov@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Remove the conversion from virPCIVirtualFunctionList which encapsulates
the list of virtual functions to two disjunct arrays.
This greatly simplifies the fetching of the parameters as well as
cleanup in the caller.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
'virNetDevGetVirtualFunctions' calls 'virPCIGetVirtualFunctions' and
then re-iterates the returned list to fetch the interface names for the
returned virtual functions.
If we move the fetching of the interface name into
virPCIGetVirtualFunctions we can simplify the code and remove a bunch of
impossible error states.
To accomplish this the function is renamed to
'virPCIGetVirtualFunctionsFull' while keeping a wrapper with original
name and if the physical port ID is passed the interface name is fetched
too without the need to re-convert the address into a sysfs link.
For now 'virNetDevGetVirtualFunctions' still converts the returned data
into two lists.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Introduce a struct for holding the list of VFs returned by
virPCIGetVirtualFunctions so that we can employ automatic memory
clearing and also allow querying more information at once.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
We currently query the host MSRs to determine if TSC scaling is
supported. This works OK when running privileged and can open
the /dev/cpu/0/msr. When unprivileged we fallback to querying
MSRs from /dev/kvm. This is incorrect because /dev/kvm only
reports accurate info for MSRs that are valid to use from inside
a guest. The TSC scaling support MSR is not, thus we always end
up reporting lack of TSC scaling when unprivileged.
The solution to this is easy, because KVM can directly report
whether TSC scaling is available, which matches what QEMU will
do at startup.
Closes: https://gitlab.com/libvirt/libvirt/-/issues/188
Reported-by: Roman Mohr <rmohr@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
VIR_APPEND_ELEMENT doesn't report any errors now so we can remove
VIR_APPEND_ELEMENT_QUIET and replace all uses by VIR_APPEND_ELEMENT
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Use virAppendElement instead of virInsertElementsN to implement
VIR_APPEND_ELEMENT which allows us to remove error handling as the
only relevant errors were removed when switching to aborting memory
allocation functions.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
For now it was an alias to VIR_APPEND_ELEMENT. Use virAppendElement
directly until VIR_APPEND_ELEMENT is refactored too and we'll be able to
get rid of VIR_APPEND_ELEMENT_QUIET completely.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Use virAppendElement instead of virInsertElementsN to implement
VIR_APPEND_ELEMENT_COPY which allows us to remove error handling as the
only relevant errors were removed when switching to aborting memory
allocation functions.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
VIR_APPEND_ELEMENT_INPLACE and VIR_APPEND_ELEMENT_COPY_INPLACE already
ignore the return value from 'virInsertElementsN' which allows a trivial
conversion to virAppendElement without the need for 'ignore_value'.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
The new wrapper calls virInsertElementInternal with the appropriate
arguments without any checks which are unnecessary for appension. This
allows to have no return value.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Split out the code doing the movement of the elements and insertion from
the range checks. This will help in adding an error-free version for
appension.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
The idea of @add was that the insersion/appension macros would allow
adding more than one element but this feature was never implemented.
'add' is nowadays used as a dummy variable consuming the result of the
VIR_TYPEMATCH compile time check.
Make it obvious that we don't use 'add' by renaming it to
'typematchDummy', marking it as unused and replacing all occurences
where the value was used by literal '1'.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Add a version of virPidFileForceCleanupPath that takes
a 'group' bool argument and propagate it all the way
down to virProcessKillPainfullyDelay.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
When loop in function virNVMeDeviceListCreateReAttachList() there may be
reused index @i, this patch fix this by using a new @j.
Signed-off-by: Jia Zhou <zhou.jia2@zte.com.cn>
Signed-off-by: Yi Wang <wang.yi59@zte.com.cn>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Add helper function virHostCPUGetHaltPollTime to obtain halt polling
time. If the kernel support halt polling time statistic, and mount
debugfs. This function will take effect on KVM VMs.
Signed-off-by: Yang Fei <yangfei85@huawei.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Use function virFileReadValueUllongQuiet to read unsigned long
long value without error report.
Signed-off-by: Yang Fei <yangfei85@huawei.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Declare the function with G_GNUC_WARN_UNUSED_RESULT as we always want to
use the returned value.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Tim Wiederhake <twiederh@redhat.com>
In one of my previous patches I've tried to postpone dropping
CAP_SETPCAP until the very end because it's needed for
capng_apply(). What I did not realize back then was that we might
not have the capability to begin with. Because of unknown reasons
capng_apply() pollutes logs only for CAPNG_SELECT_BOUNDS and not
for CAPNG_SELECT_CAPS.
Reproducer is really simple: run libvirtd as a regular user.
During its initialization, libvirtd will spawn some binaries
(dnsmasq, qemu-*, etc.) and while doing so it will try to drop
capabilities.
Anyway, let's call capng_apply(CAPNG_SELECT_BOUNDS) only if we
have the CAP_SETPCAP (which is tracked in need_setpcap variable).
Fixes: 438b50dda8
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1924218
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
After all capabilities were set (except for CAP_SETGID,
CAP_SETUID and CAP_SETPCAP) and after UID:GID was changed we drop
the last aforementioned capabilities (we couldn't drop them
before because we needed UID:GID and capabilities change).
Therefore, there's final capng_apply() call. However, it is
wrapped in one layer of parenthesis more than needed.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
If the function is called with maxlen equal to `INT_MAX`, adding
one will trigger a signed integer overflow.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
This appears to be a copy-paste mistake from the check directly above.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
On libvirtd startup, the list of priority worker threads is uninitialized
(`pool->prioWorkers` is NULL), and then "expanded" to zero (`prioWorkers`)
entries.
This causes `virThreadPoolExpand` to call `VIR_EXPAND_N` on a null pointer
and an increment of zero. The zero increment triggers `virReallocN` to not
actually allocate any memory and leave the pointer NULL, which, eventually,
causes `memset(NULL, 0, 0)` to be called in `virExpandN`.
`memset` is declared `__attribute__ ((__nonnull__ 1))`, which triggers the
following warning when libvirt is compiled with address sanitizing enabled:
$ meson -Dbuildtype=debug -Db_lundef=false -Db_sanitize=address,undefined
build && ninja -C build
$ ./build/run build/src/libvirtd
src/util/viralloc.c:82:5: runtime error: null pointer passed as
argument 1, which is declared to never be null
Signed-off-by: Tim Wiederhake <twiederh@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
This was bothering someone as the debug message looked like there was an issue
despite it being just a debug message. Change it to what is actually happening
and why the name is being skipped.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
We have an example in virDirRead() documentation on how to use
the function. In there, the directory structure is plain DIR, but
that won't work anymore. Switch over to g_autoptr(DIR) which is
what we use now.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Introduce qos setting and cleaning method. Use ovs command to set qos
parameters on specific interface of qemu virtual machine.
When an ovs port is created, we add 'ifname' to external-ids. When setting
qos on an ovs port, query its qos and queue. If found, change qos on queried
queue and qos, otherwise create new queue and qos. When cleaning qos, query
and clean queues and qos in ovs table record by 'ifname' and 'vmid'.
Signed-off-by: Jinsheng Zhang <zhangjl02@inspur.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
It all started as a simple bug: trying to move domain memory
between NUMA nodes (e.g. via virsh numatune) did not work. I've
traced the problem to qemuProcessHook() because that's where we
decide whether to rely on CGroups or use numactl APIs to satisfy
<numatune/>. The problem was that virCgroupControllerAvailable()
was telling us that cpuset controller is unavailable. This is
CGroupsV2, and pretty weird because CGroupsV2 definitely do
support cpuset controller and I had them mounted in a standard
way. What I found out (with Pavel's help) was that
virCgroupNewSelf() was looking into the following path to detect
supported controllers:
/sys/fs/cgroup/system.slice/cgroup.controllers
However, if there's no other VM running then the system.slice
only has 'memory' and 'pids' controllers. Therefore, we saw
'cpuset' as not available. The fix is to look at the top most
path, which has the full set of controllers:
/sys/fs/cgroup/cgroup.controllers
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1976690
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
In 'virResctrlAllocUpdateMask', mask is updated only if 'previous mask' is NULL.
By default, the bitmask for a cache resource for a VM is initialized with
'default-resctrl-group' bitmask. So the 'previous mask' would not be NULL and
mask won't get updated if cachetune is configured for a VM. This causes libvirt
to use same bitmask as 'default-resctrl-group' bitmask for a cache resource for
a VM. This patch fixes the issue.
Fixes: d8a354954a
Signed-off-by: Vinayak Kale <vkale@nvidia.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Bounding set capabilities were introduced in kernel commit of
v2.6.25-rc1~912. I guess it is safe to assume that all Linux
hosts we ran on have at least that version or newer.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
There are few cases where we execute a virCommand with all caps
cleared (virCommandClearCaps()). For instance
dnsmasqCapsRefreshInternal() does just that. This means, that
after fork() and before exec() the virSetUIDGIDWithCaps() is
called. But since the caller did not want to change anything,
just drop capabilities, these are the values of arguments:
virSetUIDGIDWithCaps (uid=-1, gid=-1, groups=0x0, ngroups=0,
capBits=0, clearExistingCaps=true)
This means that indeed all capabilities will be dropped,
including CAP_SETPCAP. But this capability controls whether
capabilities can be set, IOW whether capng_apply() succeeds.
There are two calls of capng_apply() in the function. The
CAP_SETPCAP is dropped after the first call and thus the other
call (capng_apply(CAPNG_SELECT_BOUNDS);) fails.
The solution is to keep the capability for as long as needed
(just like CAP_SETGID and CAP_SETUID) and drop it only at the
very end (just like CAP_SETGID and CAP_SETUID).
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1949388
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
When removing check for return value of VIR_EXPAND_N this place was
incorrectly modified causing failure to start a VM with cputune
memorytune configured with useless error message:
error: Failed to start domain 'vm1'
error: An error occurred, but the cause is unknown
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1973094
Fixes: 7d2fd6ef01
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
If given file is not found in $PATH then g_find_program_in_path()
returns NULL. However, g_canonicalize_filename() does not accept
NULL as input.
Fixes: 65c2901906
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
The new version allows passing a virBuffer to format the string into.
This will be helpful in solving a memory lean in wrong usage of
virCommandToString and also in tests where we need to add a newline
after the command in certain cases.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Connecting a tap device to an Open vSwitch is done by adding a "port"
to the switch with the ovs-vsctl "add-port" command. The port will
have the same name as the tap device, but it is a separate entity, and
can survive beyond the destruction of the tap device (although under
normal circumstances the port will be deleted around the same time the
tap device is deleted).
This makes it possible for a port of a particular name to already
exist at the time libvirt calls ovs-vsctl to add that port. The
original commit of Open vSwitch support (commit df81004632, libvirt
0.9.10, Feb. 2012) used the "--may-exist" option to the add-port
command to indicate that a port of the desired name might already
exist, and that it was okay to simply re-use this port (rather than
failing with an error message).
Then in commit 33445ce844 (libvirt 1.2.7, April 2014) the command
was changed to use "--if-exists del-port blah" instead of
"--may-exist". The reason given was that there was a bug in OVS where
a stale port would be unusable even though it still existed; the
workaround was to forcibly delete any existing port prior to adding
the new port (of the same name). This is the ovs-vsctl command still
in use by libvirt today.
It recently came up in the discussion of a bug concerning guest packet
loss during OpenStack upgrades (https://bugzilla.redhat.com/1963164)
that the bug in OVS that necessitated the del-port workaround was
fixed quite a long time ago (August 2015):
e21c6643a0
thus rendering the workaround in libvirt unnecessary. The assertion in
that discussion is that this workaround is now the cause of the packet
loss being experienced during OpenStack upgrades. I'm not convinced
this is the case, but it does appear that there is no reason to carry
this workaround in libvirt any longer, so this patch reverts the code
back to the original behavior (using "--may-exist" instead of
"--if-exists del-port").
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
The g_build_filename() would decide which separator
to use instead of hardcoding in g_strdup_printf().
Related issue: https://gitlab.com/libvirt/libvirt/-/issues/12
Signed-off-by: Luke Yue <lukedyue@gmail.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
Cloud-Hypervisor is a KVM virtualization using hypervisor. It
functions similarly to qemu and the libvirt Cloud-Hypervisor driver
uses a very similar structure to the libvirt driver.
The biggest difference from the libvirt perspective is that the
"monitor" socket is seperated into two sockets one that commands are
issued to and one that events are notified from. The current
implementation only uses the command socket (running over a REST API
with json encoded data) with future changes to add support for the
event socket (to better handle shutdowns from inside the VM).
This patch adds support for the following initial VM actions using the
Cloud-Hypervsior API:
* vm.create
* vm.delete
* vm.boot
* vm.shutdown
* vm.reboot
* vm.pause
* vm.resume
To use the Cloud-Hypervisor driver, the v15.0 release of
Cloud-Hypervisor is required to be installed.
Some additional notes:
* The curl handle is persistent but not useful to detect ch process
shutdown/crash (a future patch will address this shortcoming)
* On a 64-bit host Cloud-Hypervisor needs to support PVH and so can
emulate 32-bit mode but it isn't fully tested (a 64-bit kernel and
32-bit userspace is fine, a 32-bit kernel isn't validated)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: William Douglas <william.douglas@intel.com>
libacl is Linux-only, so we don't need to explicitly check for
either the target platform or header availability, and we can
simply rely on cc.find_library() instead. The corresponding
preprocessor define is renamed to more accurately reflect the
nature of the check.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
There was a recent change in libxml2 that caused a trouble for
us. To us, <metadata/> in domain or network XMLs are just opaque
value where management application can store whatever data it
finds fit. At XML parser/formatter level, we just make a copy of
the element during parsing and then format it back. For
formatting we use xmlNodeDump() which allows caller to specify
level of indentation. Previously, the indentation was not
applied onto the very first line, but as of v2.9.12-2-g85b1792e
libxml2 is applying indentation also on the first line.
This does not work well with out virBuffer because as soon as we
call virBufferAsprintf() to append <metadata/> element,
virBufferAsprintf() will apply another level of indentation.
Instead of version checking, let's skip any indentation added by
libxml2 before virBufferAsprintf() is called.
Note, the problem is only when telling xmlNodeDump() to use
indentation, i.e. level argument is not zero. Therefore,
virXMLNodeToString() which also calls xmlNodeDump() is safe as it
passes zero.
Tested-by: Bjoern Walk <bwalk@linux.ibm.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
I guess this is more of an academic problem, because if
<metadata/> content was problematic we would have caught the
error during parsing. Anyway, as is this function returns -1
without any error reported. Fix it by reporting one.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
So far, we have to places where we format <metadata/> into XMLs:
domain and network. Bot places share the same code. Move it into
a helper function and just call it from those places.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
None of them are currently needed to pass our upstream CI, most were
either for ancient clang versions or coverity for silencing false
positives.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
They were added mostly randomly and we don't really want to keep working
around of false positives.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
virNetDevOpenvswitchInterfaceGetMaster is declared twice in
src/util/virnetdevopenvswitch.h. Remove the last one.
Signed-off-by: Peng Liang <liangpeng10@huawei.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
The basic use case of VIR_IDENTITY_AUTORESTORE() is in
conjunction with virIdentityElevateCurrent(). What happens is
that virIdentityElevateCurrent() gets current identity (which
increases the refcounter of thread local virIdentity object) and
returns a pointer to it. Later, when the variable goes out of
scope the virIdentityRestoreHelper() is called which calls
virIdentitySetCurrent() over the old identity. But this means
that the refcounter is increased again.
Therefore, we have to explicitly decrease the refcounter by
calling g_object_unref().
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
A secret can be marked with the "private" attribute. The intent was that
it is not possible for any libvirt client to be able to read the secret
value, it would only be accesible from within libvirtd. eg the QEMU
driver can read the value to launch a guest.
With the modular daemons, the QEMU, storage and secret drivers are all
running in separate daemons. The QEMU and storage drivers thus appear to
be normal libvirt client's from the POV of the secret driver, and thus
they are not able to read a private secret. This is unhelpful.
With the previous patches that introduced a "system token" to the
identity object, we can now distinguish APIs invoked by libvirt daemons
from those invoked by client applications.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
This is essentially a way to determine if the current identity
is that of another libvirt daemon.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
When talking to the secret driver, the callers inside libvirt daemons
need to be able to run with an elevated privileges that prove the API
calls are made by a libvirt daemon, not an end user application.
The virIdentityElevateCurrent method will take the current identity
and, if not already present, add the system token. The old current
identity is returned to the caller. With the VIR_IDENTITY_AUTORESTORE
annotation, the old current identity will be restored upon leaving
the codeblock scope.
... early work with regular privileges ...
if (something needing elevated privs) {
VIR_IDENTITY_AUTORESTORE virIdentity *oldident =
virIdentityElevateCurrent();
if (!oldident)
return -1;
... do something with elevated privileges ...
}
... later work with regular privileges ...
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
When creating the system identity set the system token. The system
token is currently stored in a local path
/var/run/libvirt/common/system.token
Obviously with only traditional UNIX DAC in effect, this is largely
security through obscurity, if the client is running at the same
privilege level as the daemon. It does, however, reliably distinguish
an unprivileged client from the system daemons.
With a MAC system like SELinux though, or possible use of containers,
access can be further restricted.
A possible future improvement for Linux would be to populate the
kernel keyring with a secret for libvirt daemons to share.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
We want a way to distinguish between calls from a libvirt daemon, and a
regular client application when both are running as the same user
account. This is not possible with the current set of attributes
recorded against an identity, as there is nothing that is common to all
of the modular libvirt daemons, while distinct to all other processes.
We thus introduce the idea of a system token, which is simply a random
hex string that is only known by the libvirt daemons, to be recorded
against the system identity.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
A random token is simply a string of random bytes formatted in
hexidecimal.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Since its introduction in v0.9.1~65 the virOnce() was expected to
follow the usual retval logic (0 for success, a negative number
for failure). However, that was never the case.
On the other hand, looking into glibc and musl the pthread_once()
never returns anything other than zero (uclibc-ng seems to not
implement pthread_once()), therefore we never really hit any
problem. But for code cleanliness (and to match POSIX
documentation), let's change to code so that our retval logic is
honoured.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Compilers aren't able to see whether @result is set or not and thus
don't warn of a potential use of uninitialized value. Always set @result
to prevent uninitialized use.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
The helper is almost identical to virXMLPropEnum but it allows to pass a
default value to initialize the result to.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Compilers aren't able to see whether @result is set or not and thus
don't warn of a potential use of uninitialized value. Always set @result
to prevent uninitialized use.
In two cases the code needed to be adjusted to preserve functionality.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Compilers aren't able to see whether @result is set or not and thus
don't warn of a potential use of uninitialized value. Always set @result
to prevent uninitialized use.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Compilers aren't able to see whether @result is set or not and thus
don't warn of a potential use of uninitialized value. Always set @result
to prevent uninitialized use.
This is done by adding a @defaultResult argument to virXMLPropInt since
many places have a non-0 default.
In certain cases such as in virDomainControllerDefParseXML we pass the
value from the original value, which will still trigger compiler checks
if unused while preserving the existing functionality of keeping the
previous value.
This commit fixes 3 uses of uninitialized value parsed by this function:
in virDomainDiskSourceNetworkParse introduced by 38dc25989c
in virDomainChrSourceDefParseTCP introduced by fa48004af5
in virDomainGraphicsListenDefParseXML introduced by 0b20fd3754
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Compilers aren't able to see whether @result is set or not and thus
don't warn of a potential use of uninitialized value. Always set @result
to prevent uninitialized use.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Compilers aren't able to see whether @result is set or not and thus
don't warn of a potential use of uninitialized value. Always set @result
to prevent uninitialized use.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
virXMLPropTristateBool/virXMLPropTristateSwitch/virXMLPropEnum can be
implemented using the same internal code. Extract it into a new function
called virXMLPropEnumInternal, which will also simplify adding versions
of these functions with a custom default value.
This way we'll be able to always initialize @result so that unused value
bugs can be prevented.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
When updating entries in a bridge forwarding database (i.e., when
macTableManager='libvirt' is configured for the bridge), we may end up
in a situation when the entry we want to add is already present. Let's
just ignore the error in such a case.
This fixes an error to resume a domain when fdb entries were not
properly removed when the domain was paused:
virsh # resume test
error: Failed to resume domain test
error: error adding fdb entry for vnet2: File exists
For some reason, fdb entries are only removed when libvirt explicitly
stops CPUs, but nothing happens when we just get STOP event from QEMU.
An alternative approach would be to make sure we always remove the
entries regardless on why a domain was paused (e.g., during migration),
but that would be a significantly more disruptive change with possible
side effects.
https://bugzilla.redhat.com/show_bug.cgi?id=1603155
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Laine Stump <laine@redhat.com>
Add a helper which will format an XML element with attributes and
children, but compared to virXMLFormatElement it also formats an empty
element if both buffers are empty.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Tested-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Function incorrectly returns 0 when property was successfully read.
Fixes: ab5d2776c9
Signed-off-by: Tim Wiederhake <twiederh@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Convenience function to return the value of an unsigned long long XML
attribute.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
The function in question uses "tc" binary so virnetdevbandwidth feels
like better place for it.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
This will allow us to run tests using firewall on hosts where the mocked
binaries are not available/installed instead of skipping these tests.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Following patches will make this change necessary as we will stop
detecting the full path during compile time.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
We always pass DNSMASQ so there is no need for the argument at all.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
We always pass DNSMASQ so there is no need for the argument at all.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Instead of removing binaryPath let's drop the function completely as
it is not used anywhere.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Instead of removing binaryPath let's drop the function completely as
it is not used anywhere.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
The new enum helpers use a set of flags to modify their behaviour, but
the declared set of flags is semantically confusing:
typedef enum {
VIR_XML_PROP_OPTIONAL = 0, /* Attribute may be absent */
VIR_XML_PROP_REQUIRED = 1 << 0, /* Attribute may not be absent */
Since VIR_XML_PROP_OPTIONAL is declared as 0 any other flag shadows it
and makes it impossible to detect. The functions are not able to detect
a semantic nonsense of VIR_XML_PROP_OPTIONAL | VIR_XML_PROP_REQUIRED and
it's a perfectly valid statement for the compilers.
In general having two flags to do the same boolean don't make sense and
the implementation doesn't fix any shortcomings either.
To prevent mistakes, rename VIR_XML_PROP_OPTIONAL to VIR_XML_PROP_NONE,
so that there's always an enum value used with the calls but it doesn't
imply that the flag makes the property optional when the actual value is
0.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
As I've pointed out in my review, the negative number wrapping for
unsigned variables is an anti-feature which should not be promoted in
any way.
Remove VIR_XML_PROP_WRAPNEGATIVE which would make it more accessible.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
xmlDocSetRootElement removes the node from its previous document tree,
effectively removing the "<cpu>" node from "<domain>" in virCPUDefParseXML.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
The g_path_is_absolute() considers more situations
than just a simply "path[0] == '/'".
Related issue: https://gitlab.com/libvirt/libvirt/-/issues/12
Signed-off-by: Luke Yue <lukedyue@gmail.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Use the new macro instead of virXMLParseStringCtxt in places where the
root node is being validated.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
