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>
