Commit 15a71e60 introduced the virNetlinkEventServiceStopAll function, and
the code in virNetlinkEventServiceStop is copied to this function, so just
call virNetlinkEventServiceStop instead.
Namely, this patch is about virMediatedDeviceGetIOMMUGroup{Dev,Num}
functions. There's no compelling reason why these functions should take
an object, on the contrary, having to create an object every time one
needs to query the IOMMU group number, discarding the object afterwards,
seems odd.
Signed-off-by: Erik Skultety <eskultet@redhat.com>
Commit 8e09663 "pci: recognize/report GEN4 (PCIe 4.0) card 16GT/s Link
speed" introduced another speed into enum, but mistakenly also altered
field width, so one bit of link width was included there.
Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
Basically, what is needed here is to introduce new message type
for the messages passed between the event loop callbacks and the
worker thread that does all the I/O. The idea is that instead of
a queue of read buffers we will have a queue where "hole of size
X" messages appear. That way the event loop callbacks can just
check the head of the queue and see if the worker thread is in
data or a hole section and how long the section is.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
This function takes a FD and determines whether the current
position is in data section or in a hole. In addition to that,
it also determines how much bytes are there remaining till the
current section ends.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
One big downside of using the pipe to transfer the data is that
we can really transfer just bare data. No metadata can be carried
through unless some formatted messages are introduced. That would
be quite painful to achieve so let's use a message queue. It's
fairly easy to exchange info between threads now that iohelper is
no longer used.
The reason why we cannot use the FD for plain files directly is
that despite us setting noblock flag on the FD, any
read()/write() blocks regardless (which is a show stopper since
those parts of the code are run from the event loop) and poll()
reports such FD as always readable/writable - even though the
subsequent operation might block.
The pipe is still not gone though. It is used to signal the event
loop that an event occurred (e.g. data is available for reading
in the queue, or vice versa).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
The QEMU default is GICv2, and some of the code in libvirt
relies on the exact value. Stop pretending that's not the
case and use GICv2 explicitly where needed.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
The metadata libvirt cares about is identical for version 3
as for previous versions, so we merely need list the new
version number.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Simply tries to match the provided regex on a string and returns
the result. Useful if caller don't care about the matched substring
and want to just test if some pattern patches a string.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
GCC complains that inlining virStringTrimOptionalNewline is not
likely on some platforms:
cc1: warnings being treated as errors
../../src/util/virfile.c: In function 'virFileReadValueBitmap':
../../src/util/virstring.h:292: error: inlining failed in call to 'virStringTrimOptionalNewline': call is unlikely and code size would grow [-Winline]
../../src/util/virfile.c:3987: error: called from here [-Winline]
Inlining this function is not going to be a measurable performance
benefit either, since the time required to execute it is going to
be dominated by running of strlen() over the string, not by the
function call overhead.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
It is no longer needed thanks to the great virfilewrapper.c. And this
way we don't have to add a new set of functions for each prefixed
path.
While on that, add two functions that weren't there before, string and
scaled integer reading ones. Also increase the length of the string
being read by one to accompany for the optional newline at the
end (i.e. change INT_STRLEN_BOUND to INT_BUFSIZE_BOUND).
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
So, because mingw is somehow OK with dereferencing a pointer within a
VIR_DEBUG macro, compared to outside of it to which it complained with a
"potential NULL pointer dereference" error (still a false positive), we
can make the code a tiny bit cleaner.
Sighed-by: Erik Skultety <eskultet@redhat.com>
Signed-off-by: Erik Skultety <eskultet@redhat.com>
After bdcf6e481 there is a crasher in libvirt. The commit assumes
that priv->perf is always set. That is not true. For inactive
domains, the priv->perf is not allocated as it is set in
qemuProcessLaunch(). Now, usually we differentiate between
accesses to inactive and active definition and it works just
fine. Except for 'domstats'. There priv->perf is accessed without
prior check for domain inactivity. While we could check for that,
more robust solution is to make virPerfEventIsEnabled() accept
NULL.
How to reproduce:
1) ensure you have at least one inactive domain
2) virsh domstats
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrange <berrange@redhat.com>
This patch fixes the following MinGW error (although actually being a
false positive):
../../src/util/virmdev.c: In function 'virMediatedDeviceListMarkDevices':
../../src/util/virmdev.c:453:21: error: potential null pointer
dereference [-Werror=null-dereference]
const char *mdev_path = mdev->path;
^~~~~~~~~
Signed-off-by: Erik Skultety <eskultet@redhat.com>
The problem resides in virHostdevUpdateActiveMediatedDevices which gets
called during qemuProcessReconnect. The issue here is that
virMediatedDeviceListAdd takes a pointer to the item to be added to the
list to which VIR_APPEND_ELEMENT is used, which also clears the pointer.
However, in this case only the local copy of the pointer got cleared,
leaving the original pointing to valid memory. To sum it up, during
cleanup phase, the original pointer is freed and the daemon crashes
basically any time it would access it.
Backtrace:
0x00007ffff3ccdeba in __strcmp_sse2_unaligned
0x00007ffff72a444a in virMediatedDeviceListFindIndex
0x00007ffff7241446 in virHostdevReAttachMediatedDevices
0x00007fffc60215d9 in qemuHostdevReAttachMediatedDevices
0x00007fffc60216dc in qemuHostdevReAttachDomainDevices
0x00007fffc6046e6f in qemuProcessStop
0x00007fffc6091596 in processMonitorEOFEvent
0x00007fffc6091793 in qemuProcessEventHandler
0x00007ffff7294bf5 in virThreadPoolWorker
0x00007ffff7294184 in virThreadHelper
0x00007ffff3fdc3c4 in start_thread () from /lib64/libpthread.so.0
0x00007ffff3d269cf in clone () from /lib64/libc.so.6
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1446455
Signed-off-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: Laine Stump <laine@laine.org>
Use a local variable to hold data, rather than accessing the pointer
after calling virMediatedDeviceListAdd (therefore VIR_APPEND_ELEMENT).
Although not causing an issue at the moment, this change is a necessary
prerequisite for tweaking virMediatedDeviceListAdd in a separate patch,
which will take a reference for the source pointer (instead of pointer
value) and will clear it along the way.
Signed-off-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: Laine Stump <laine@laine.org>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
If we are encoding a block of data that is 16 bytes in length,
we cannot leave it as 16 bytes, we must pad it out to the next
block boundary, 32 bytes. Without this padding, the decoder will
incorrectly treat the last byte of plain text as the padding
length, as it can't distinguish padded from non-padded data.
The problem exhibited itself when using a 16 byte passphrase
for a LUKS volume
$ virsh secret-set-value 55806c7d-8e93-456f-829b-607d8c198367 \
$(echo -n 1234567812345678 | base64)
Secret value set
$ virsh start demo
error: Failed to start domain demo
error: internal error: process exited while connecting to monitor: >>>>>>>>>>Len 16
2017-05-02T10:35:40.016390Z qemu-system-x86_64: -object \
secret,id=virtio-disk1-luks-secret0,data=SEtNi5vDUeyseMKHwc1c1Q==,\
keyid=masterKey0,iv=zm7apUB1A6dPcH53VW960Q==,format=base64: \
Incorrect number of padding bytes (56) found on decrypted data
Notice how the padding '56' corresponds to the ordinal value of
the character '8'.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
... with VIR_NET_GENERATED_MACV???_PREFIX, which is defined in
util/virnetdevmacvlan.h.
Since VIR_NET_GENERATED_PREFIX is used for plain tap devices, it is
renamed to VIR_NET_GENERATED_TAP_PREFIX and moved to virnetdev.h
MACVTAP_NAME_PREFIX and MACVLAN_NAME_PREFIX could be useful to other
files if they were defined in virnetdevmacvlan.h instead of
virnetdevmacvlan.c, so do that (while slightly renaming them and also
adding yet another #define that chooses between macvlan/macvtap based
on flags).
This is a prerequisite to fix: https://bugzilla.redhat.com/1335798
After 1eb6647979 nobody calls the iohelper with 6 arguments.
Everybody uses the other mode. Well, the only user of iohelper
after the previous commit is virFileWrapperFd really.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Currently we use iohelper for virFDStream implementation. This is
because UNIX I/O can lie sometimes: even though a FD for a
file/block device is set as unblocking, actual read()/write() can
block. To avoid this, a pipe is created and one end is kept for
read/write while the other is handed over to iohelper to
write/read the data for us. Thus it's iohelper which gets blocked
and not our event loop.
This approach has two problems:
1) we are spawning a new process.
2) any exchange of information between daemon and iohelper can be
done only through the pipe.
Therefore, iohelper is replaced with an implementation in thread
which is created just for the stream lifetime. The data are still
transferred through pipe (for now), but both problems described
above are solved.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
While this is no functional change, it makes the code look a bit
nicer. Moreover, it prepares ground for future work.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
There is really no reason why we should have to have 'struct'
everywhere.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
A long time ago we imported the keymaps.csv file from GTK-VNC so we
can do conversions between keycode sets. Meanwhile lots of bug fixes
have gone into this CSV file and libvirt hasn't kept in sync. The
keymaps.csv file and associated generator script has been pulled out
of GTK-VNC into a dedicated GIT repo for use as a submodule. This
allows GTK-VNC, SPICE-GTK, QEMU and libvirt to share the same master
database and tools and pushing updates merely requires a submodule
commit update as with gnulib.
The test suite is updated to cover some extra boundary conditions.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Currently, virNetDevSetCoalesce() stub is always returning error. As
it's used by virNetDevTapCreateInBridgePort(), it essentially breaks
bridged networking if coalesce is not supported.
To make it work, relax the stub to trigger error only when its
coalesce argument is not NULL, otherwise report success.
Commit f4ef3a71 made a variation of virNetDevSetMAC that would return
without logging an error message if errno was set to
EADDRNOTAVAIL. This errno is set by some SRIOV VF drivers (in
particular igbvf) when they fail to set the device's MAC address due
to the PF driver refusing the request. This is useful if we want to
try a different method of setting the VF MAC address before giving up
(Commit 86556e16 actually does this, setting the desired MAC address
to the "admin MAC in the PF, then detaching and reattaching the VF
netdev driver to force a reinit of the MAC address).
During testing of Bug 1442040 t was discovered that the ixgbe driver
returns EPERM in this situation, so this patch changes the exception
case for silent+non-terminal failure to account for this difference.
Completes resolution to: https://bugzilla.redhat.com/1415609 (RHEL 7.4)
https://bugzilla.redhat.com/1442040 (RHEL 7.3.z)
The current fallback stub for virNetDevSetCoalesce is inside an
earlier conditional block. This deals with the feature being
missing on older Linux platforms. We need a second fallback stub
though, outside the top level conditional, to ensure builds work
on Win32/FreeBSD platforms too.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
This patch makes use of the virNetDevSetCoalesce() function to make
appropriate settings effective for devices that support them.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1414627
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
That function is able to configure coalesce settings for an interface,
similarly to 'ethtool -C'. This function also updates back the
structure so that it contains actual data on the device (if the device
doesn't support some settings kernel might just return 0 and not set
whatever is not supported), so this way we'll have up-to-date
information in the live domain XML.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Reported by Rafał Wojciechowski <it@rafalwojciechowski.pl>.
Thread 1 (Thread 0x7f194b99d700 (LWP 5631)):
0 virNetDevGetifaddrsAddress (addr=0x7f194b99c7c0, ifname=0x7f193400e2b0 "ovirtmgmt") at util/virnetdevip.c:738
1 virNetDevIPAddrGet (ifname=0x7f193400e2b0 "ovirtmgmt", addr=addr@entry=0x7f194b99c7c0) at util/virnetdevip.c:795
2 0x00007f19467800d6 in networkGetNetworkAddress (netname=<optimized out>, netaddr=netaddr@entry=0x7f1924013f18) at network/bridge_driver.c:4780
3 0x00007f193e43a33c in qemuProcessGraphicsSetupNetworkAddress (listenAddr=0x7f19340f7650 "127.0.0.1", glisten=0x7f1924013f10) at qemu/qemu_process.c:4062
4 qemuProcessGraphicsSetupListen (vm=<optimized out>, graphics=0x7f1924014f10, cfg=0x7f1934119f00) at qemu/qemu_process.c:4133
5 qemuProcessSetupGraphics (flags=17, vm=0x7f19240155d0, driver=0x7f193411f1d0) at qemu/qemu_process.c:4196
6 qemuProcessPrepareDomain (conn=conn@entry=0x7f192c00ab50, driver=driver@entry=0x7f193411f1d0, vm=vm@entry=0x7f19240155d0, flags=flags@entry=17) at qemu/qemu_process.c:4969
7 0x00007f193e4417c0 in qemuProcessStart (conn=conn@entry=0x7f192c00ab50, driver=driver@entry=0x7f193411f1d0, vm=0x7f19240155d0,asyncJob=asyncJob@entry=QEMU_ASYNC_JOB_START, migrateFrom=migrateFrom@entry=0x0, migrateFd=migrateFd@entry=-1, migratePath=migratePath@entry=0x0,snapshot=snapshot@entry=0x0, vmop=vmop@entry=VIR_NETDEV_VPORT_PROFILE_OP_CREATE, flags=17, flags@entry=1) at qemu/qemu_process.c:5553
Man page for getifaddrs also states that the "ifa_addr" may contain
a null pointer which happens if there is an existing network interface
on the host without IP address.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
CLang's optimizer is more aggressive at inlining functions than
gcc and so will often inline functions that our tests want to
mock-override. This causes the test to fail in bizarre ways.
We don't want to disable inlining completely, but we must at
least prevent inlining of mocked functions. Fortunately there
is a 'noinline' attribute that lets us control this per function.
A syntax check rule is added that parses tests/*mock.c to extract
the list of functions that are mocked (restricted to names starting
with 'vir' prefix). It then checks that src/*.h header file to
ensure it has a 'ATTRIBUTE_NOINLINE' annotation. This should prevent
use from bit-rotting in future.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Without this added enum value, nodedev-dumpxml of a GEN4 (PCIe 4.0)
card will fail (due to the unrecognized link speed), and since
nodedev-detach and nodedev-reattach internally do a dumpxml+parse,
they will also fail. With this patch, all those operations succeed.
Signed-off-by: Shivaprasad G Bhat <sbhat@linux.vnet.ibm.com>
Fix xlconfigtest runs build for --enable-test-oom on
Xen XL-2-XML Parse channel-pty
Program received signal SIGSEGV, Segmentation fault.
#0 0x00007ffff3c2b373 in __strchr_sse2 () from /lib64/libc.so.6
==> #1 0x00007ffff7875701 in virConfSaveValue (buf=buf@entry=0x7fffffffd8a0, val=val@entry=0x674750) at util/virconf.c:290
#2 0x00007ffff7875668 in virConfSaveValue (buf=buf@entry=0x7fffffffd8a0, val=<optimized out>) at util/virconf.c:306
#3 0x00007ffff78757ef in virConfSaveEntry (buf=buf@entry=0x7fffffffd8a0, cur=cur@entry=0x674780) at util/virconf.c:338
#4 0x00007ffff78783eb in virConfWriteMem (memory=0x665570 "", len=len@entry=0x7fffffffd910, conf=conf@entry=0x65b940)
at util/virconf.c:1543
#5 0x000000000040eccb in testCompareParseXML (replaceVars=<optimized out>, xml=<optimized out>,
xlcfg=0x662c00 "/home/wtenhave/WORK/libvirt/OOMtesting/libvirt-devel/tests/xlconfigdata/test-channel-pty.cfg")
at xlconfigtest.c:108
#6 testCompareHelper (data=<optimized out>) at xlconfigtest.c:205
#7 0x0000000000410b3a in virTestRun (title=title@entry=0x432cc0 "Xen XL-2-XML Parse channel-pty",
body=body@entry=0x40e9b0 <testCompareHelper>, data=data@entry=0x7fffffffd9f0) at testutils.c:247
#8 0x000000000040f322 in mymain () at xlconfigtest.c:278
#9 0x0000000000411410 in virTestMain (argc=1, argv=0x7fffffffdba8, func=0x40f660 <mymain>) at testutils.c:992
#10 0x00007ffff3bc0401 in __libc_start_main () from /lib64/libc.so.6
#11 0x000000000040e86a in _start ()
(gdb) frame 1
#1 0x00007ffff7875701 in virConfSaveValue (buf=buf@entry=0x7fffffffd8a0, val=val@entry=0x674750) at util/virconf.c:290
290 if (strchr(val->str, '\n') != NULL) {
(gdb) print *val
$1 = {type = VIR_CONF_STRING, next = 0x0, l = 0, str = 0x0, list = 0x0}
Signed-off-by: Wim ten Have <wim.ten.have@oracle.com>
Fix the build with clang:
util/virperf.c:86:27: error: use of GNU 'missing =' extension
in designator [-Werror,-Wgnu-designator]
[VIR_PERF_EVENT_MBML] {
^
=
The virPerfGetEvent method pointlessly checks for a NULL
parameter and the range of an enum value. The whole point
of using an enum is that we can avoid such checks. Just
replace calls to virPerfGetEvent, with perf->events[type]
array access.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
The virPerfGetEventAttr method contains a totally pointless
loop. Remove it, verify the array size statically, and then
just use an array index to access the perf event.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
This function runs an iscsi command and parses its output.
However, due to the nature of things, virISCSIExtractSession()
callback can be called multiple times. In each run it would
allocate new memory and overwrite the variable where we keep
pointer to it and thus leaking old allocations.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Imagine that this function is called twice over the same disk
source. While in the first run all allocated memory is freed, not
all pointers are set to NULL (e.g. def->srcpool). So when called
again, these poitners are freed again resulting in double free.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Similar to commit b202c39 ignore the warning that breaks the build
with clang:
util/virnetlink.c:365:52: error: cast from 'char *' to 'struct nlmsghdr *'
increases required alignment from 1 to 4 [-Werror,-Wcast-align]
for (msg = resp; NLMSG_OK(msg, len); msg = NLMSG_NEXT(msg, len)) {
^~~~~~~~~~~~~~~~~~~~
/usr/include/linux/netlink.h:87:7: note: expanded from macro 'NLMSG_NEXT'
(struct nlmsghdr*)(((char*)(nlh)) + NLMSG_ALIGN((nlh)->nlmsg_len)))
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
https://bugzilla.redhat.com/show_bug.cgi?id=1371892
The 'capacity' value (e.g. guest logical size) for a LUKS volume is
smaller than the 'physical' value of the file in the file system, so
we need to account for that.
When peeking at the encryption information about the volume add a fetch
of the payload_offset which is described as the offset to the start of
the volume data (in 512 byte sectors) in QEMU's QCryptoBlockLUKSHeader.
Then adjust the ->capacity appropriately when we determine that the
volume target encryption has a payload_offset value.
Add check for more than one RTA_OIF, even though this is rather
unlikely.
Get rid of the buggy switch / break as this code won't need to
handle more attributes.
Use VIR_WARNINGS_NO_CAST_ALIGN to fix impossible to fix
util/virnetdevip.c:560:17: error: cast increases required alignment of target type [-Werror=cast-align]
There was an unhandled 'open' call which resulted in:
"error: Library function returned error but did not set virError"
Even if this happens during the daemon's start when we still don't have
any set of outputs defined yet, we can safely report an error, since we
automatically fallback to stderr which is fine even for both
running as a daemonized process, since this happens before the daemon
forks into the background, and running as a systemd service, since
systemd re-directs std outputs to journald by default.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1436060
Signed-off-by: Erik Skultety <eskultet@redhat.com>
The value we use internally to represent the lack of a memory
locking limit, VIR_DOMAIN_MEMORY_PARAM_UNLIMITED, doesn't
match the value setrlimit() and prlimit() use for the same
purpose, RLIM_INFINITY, so we have to handle the translation
ourselves.
Partially-resolves: https://bugzilla.redhat.com/1431793
If if_indextoname is not defined, the whole function using it should
not be defined either. Add stub to fix build on mingw.
Caused by 5dd607059d
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
- Make virMediatedDeviceNew() stub args match its prototype
- Fix typo: virRerportError -> virReportError
- Move MDEV_SYSFS_DEVICES definition out of the #ifdef __linux__ block
so we don't have to stub virMediatedDeviceGetSysfsPath()
Previously, this function must've been called only on Linux in order
to fail gracefully. That lead to #ifdef mess in callers, so the
function was redesigned so it failed gracefully on non-existing
files. However that commit forgot to define the function outside the
__linux__ ifdef, it broke non-Linux builds.
Caused by c67e04e25f.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Having this information available will make it easier to determine the
culprit when MAC or vlan tag appear to not be set, eg.:
https://bugzilla.redhat.com/1364073
(This patch doesn't fix that bug, just makes it easier to diagnose)
If an SRIOV VF has previously been used for VFIO device assignment,
the "admin MAC" that is stored in the PF driver's table of VF info
will have been set to the MAC address that the virtual machine wanted
the device to have. Setting the admin MAC for a VF also sets a flag in
the PF that is loosely called the "administratively set" flag. Once
that flag is set, it is no longer possible for the net driver of the
VF (either on the host or in a virtual machine) to directly set the
VF's MAC again; this flag isn't reset until the *PF* driver is
restarted, and that requires taking *all* VFs offline, so it's not
really feasible to do.
If the same SRIOV VF is later used for macvtap passthrough mode, the
VF's MAC address must be set, but normally we don't unbind the VF from
its host net driver (since we actually need the host net driver in
this case). Since setting the VF MAC directly will fail, in the past
"we" ("I") had tried to fix the problem by simply setting the admin MAC
(via the PF) instead. This *appeared* to work (and might have at one
time, due to promiscuous mode being turned on somewhere or something),
but it currently creates a non-working interface because only the
value for admin MAC is set to the desired value, *not* the actual MAC
that the VF is using.
Earlier patches in this series reverted that behavior, so that we once
again set the MAC of the VF itself for macvtap passthrough operation,
not the admin MAC. But that brings back the original bug - if the
interface has been used for VFIO device assignment, you can no longer
use it for macvtap passthrough.
This patch solves that problem by noticing when virNetDevSetMAC()
fails for a VF, and in that case it sets the desired MAC to the admin
MAC via the PF, then "bounces" the VF driver (by unbinding and the
immediately rebinding it to the VF). This causes the VF's MAC to be
reinitialized from the admin MAC, and everybody is happy (until the
*next* time someone wants to set the VF's MAC address, since the
"administratively set" bit is still turned on).
Some PF drivers allow setting the admin MAC (that is the MAC address
that the VF will be initialized to the next time the VF's driver is
loaded) to 00:00:00:00:00:00, and some don't. Multiple drivers
initialize the admin MACs to all 0, but don't allow setting it to that
very same value. It has been an uphill battle convincing the driver
people that it's reasonable to expect The argument that's used is
that an all 0 device MAC address on a device is invalid; however, from
an outsider's point of view, when the admin MAC is set to 0 at the
time the VF driver is loaded, the VF's MAC is *not* set to 0, but to a
random non-0 value. But that's beside the point - even if I could
convince one or two SRIOV driver maintainers to permit setting the
admin MAC to 0, there are still several other drivers.
So rather than fighting that losing battle, this patch checks for a
failure to set the admin MAC due to an all 0 value, and retries it
with 02:00:00:00:00:00. That won't result in a random value being set
in the VF MAC at next VF driver init, but that's okay, because we
always want to set a specific value anyway. Rather, the "almost 0"
setting makes it easy to visually detect from the output of "ip link
show" which VFs are currently in use and which are free.
The global functions virNetDevReplaceMacAddress(),
virNetDevReplaceNetConfig(), virNetDevRestoreMacAddress(), and
virNetDevRestoreNetConfig() are no longer used, as their functionality
has been replaced by virNetDev(Save|Read|Set)NetConfig().
The static functions virNetDevReplaceVfConfig() and
virNetDevRestoreVfConfig() were only used by the above-named global
functions that were removed.
It takes longer to explain this than to fix it...
In the past we weren't able to save the VF's own MAC address *at all*
when using it for hostdev assignment, because we had already unbound
the VF from the host net driver prior to saving its config. With the
previous patch, that problem has been solved, so we now have the VF's
MAC address saved and can move on to the *next* problem, which is twofold:
1) during teardown we restore the config before we've re-bound, so the
VF doesn't have a net driver, and thus we can't set its MAC address
directly.
2) even if we delay restoring the config until the VF is bound to a
net driver, the request to set its MAC address would fail, since
(during device setup) we had set the "admin MAC" for the VF via an
RTM_SETLINK to the PF - once you've set the admin MAC for a VF, the
VF driver (either on host or on guest) is not allowed to change the
VF's MAC address "forever" (well, until you reload the PF driver,
but that requires destroying and recreating every single VF, which
isn't something you can require).
The solution is to keep the restoration of config at the same place,
but to set the *admin MAC* to the address you want the VF to have -
when the VF net driver is later initialized (as a part of re-binding
to the VF net driver) its MAC will be initialized to the current value
of the admin MAC.
In order to properly restore the original state of an SRIOV VF when
we're finished with it, we need to save the MAC address of the VF
itself (not just the admin MAC address for the VF that is stored in
the PF). But that can only be done when the VF is still bound to the
host's netdev driver, and we have always done the saving of device
config after the VF is already bound to vfio-pci. This patch prepares
us for adding a save of the VF's MAC by calling the function that
saves netconfig earlier in the device preparation, before we've
unbound it from the host netdev driver.
These two operations will need to be separated so that saving of the
original config is done before detaching the host net driver, and
setting the new config is done after attaching vfio-pci. This patch
splits the single function into two, but for now calls them together
(to make bisecting easier if there is a regression).
virHostdevNetConfigReplace() and virHostdevNetConfigRestore() are
modified to use the new virNetDev*NetConfig() functions.
Note that due to the VF's original MAC addresses being saved after it
has already been un-bound from the host net driver, the actual current
VF MAC address won't be saved (because it no longer exists) - only the
"admin MAC" will be saved. This reflects existing behavior that will
be fixed in an upcoming patch.
This patch modifies the macvtap passthrough setup to use
virNetDevSaveNetConfig()+virNetDevSetConfig() instead of
virNetDevReplaceNetConfig() or virNetDevReplaceMacAddress(), and the
teardown to use virNetDevReadNetConfig()+virNetDevSetConfig() instead
of virNetDevRestoreNetConfig() or virNetDevRestoreMacAddress().
Since the older functions only saved/restored the admin MAC and vlan
tag (which is incorrect) and the new functions save/restore the VF's
own MAC address and vlan tag (correct), this actually fixes a bug
(which was introduced by commit cb3fe38c7, which was itself supposed
to be a fix for https://bugzilla.redhat.com/1113474 ).
The downside to this patch is that it causes an *apparent* regression
in that bug (because there will once again be an error reported if the
interface had previously been used for VFIO device assignment), but in
reality, the code hasn't been working for *any* case before this
current patch (at least not with any recent kernel). Anyway, that
"regression" will be fixed with an upcoming patch that fixes it the
*right* way.
These three functions are destined to replace
virNetDev(Replace|Restore)NetConfig() and
virNetDev(Replace|Restore)MacAddress(), which both do the save and set
together as a single step. We need to separate the save, read, and set
steps because there will be situations where we need to do something
else in between (in particular, we will need to rebind a VF's driver
after save but before set).
This patch creates the new functions, but doesn't call them - that
will come in a subsequent patch. Note that the new functions to
read/write the file that stores the original network config now uses
JSON rather than plaintext (it still recognizes the old format as well
though, so it won't get confused during an upgrade).
Keep track of the assigned mediated devices the same way we do it for
the rest of hostdevs. Methods like 'Prepare', 'Update', and 'ReAttach'
are introduced by this patch.
Signed-off-by: Erik Skultety <eskultet@redhat.com>
Beside creation, disposal, getter, and setter methods the module exports
methods to work with lists of mediated devices.
Signed-off-by: Erik Skultety <eskultet@redhat.com>
We keep forgetting that older setups don't like 'index':
CC util/libvirt_util_la-virsysinfo.lo
cc1: warnings being treated as errors
util/virstoragefile.c: In function 'virStorageSourceFindByNodeName':
util/virstoragefile.c:3804: error: declaration of 'index' shadows a global declaration [-Wshadow]
/usr/include/string.h:489: error: shadowed declaration is here [-Wshadow]
Signed-off-by: Eric Blake <eblake@redhat.com>
That file has only two exported files and each one of them has
different naming. virNode is what all the other files use, so let's
use it. It wasn't used before because the clash with public API
naming, so let's fix that by shortening the name (there is no other
private variant of it anyway).
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
There is no reason for it not to be in the utils, all global symbols
under that file already have prefix vir* and there is no reason for it
to be part of DRIVER_SOURCES because that is just a leftover from
older days (pre-driver modules era, I believe).
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
While on that, drop support for kernels from RHEL-5 era (missing
cpu/present file). Also add some useful functions and export them.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
By using this we are able to easily switch the sysfs path being
used (fake it). This will not only help tests in the future but can
be also used from files where the code is duplicated currently.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
These helpers are doing just a read and covert the value, but they
properly size the read limit, handle additional whitespace characters,
and unify error reporting.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
STREQ_NULLABLE returns true if both parameters are NULL. And that's
not what we want here. We just want to skop comparing source nodes
that don't have that info set. The function wouldn't make much sense
with nodeName == NULL, so we don't need to check that. Moreover, the
function's declaration uses ATTRIBUDE_NONNULL for nodeName, which not
only means that function expects the parameter not to be NULL, but
actually tells the compiler that it can optimize out the NULL checks.
That way it could end up calling strcmp on NULL (either nodeformat or
nodebacking). GCC figures this out if libvirt is compiled with
lv_cv_static_analysis=yes, unfortunately not everyone uses that.
Caused by cbc6d53513.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
The function has very specific semantics. Split out the part that parses
the backing store specification string into a separate helper so that it
can be reused later while keeping the wrapper with existing semantics.
Note that virStorageFileParseChainIndex is pretty well covered by the
test suite.
Fix typo in virNetDevPFGetVF() stub:
ATTRUBUTE_UNUSED -> ATTRIBUTE_UNUSED.
While here, use common indent style for arguments in
virNetDevGetVirtualFunctionIndex() stub.
Given an SRIOV PF netdev name (e.g. "enp2s0f0") and VF#, this new
function returns the netdev name of the referenced VF device
(e.g. "enp2s11f6"), or NULL if the device isn't bound to a net driver.
We will want to allow silent failure of virNetDevSetMAC() in the case
that the SIOSIFHWADDR ioctl fails with errno == EADDRNOTAVAIL. (Yes,
that is very specific, but we really *do* want a logged failure in all
other circumstances, and don't want to duplicate code in the caller
for the other possibilities).
This patch renames the 3 different virNetDevSetMAC() functions to
virNetDevSetMACInternal(), adding a 3rd arg called "quiet" and making
them static (because this extra control will only be needed within
virnetdev.c). A new global virNetDevSetMAC() is defined that calls
whichever of the three *Internal() functions gets compiled with quiet
= false. Callers in virnetdev.c that want to notice a failure with
errno == EADDRNOTAVAIL and retry with a different strategy rather than
immediately failing, can call virNetDevSetMACInternal(..., true).
This function unbinds a device from its driver, then immediately
rebinds it to its driver again. The code for this new function is just
the 2nd half of virPCIDeviceBindWithDriverOverride(), so that
function's 2nd half is replaced with a call to virPCIDeviceRebind().
...and cleanup the callers to report it when it *is* an error.
In many cases It's useful for virPCIGetNetName() to not log an error
and simply return a NULL pointer when the given device isn't bound to
a net driver (e.g. we're looking at a VF that is permanently bound to
vfio-pci). The existing code would silently return an error in this
case, which could eventually lead to the dreaded "An error occurred
but the cause is unknown" log message.
This patch changes virPCIGetNetName() to still return success if the
device simply isn't bound to a net driver, and adjusts all the callers
that require a non-null netname to check for that condition and log an
error when it happens.
vf in virNetDevMacVLanDeleteWithVPortProfile() is initialized to -1
and never set. It's not set for a good reason - because it doesn't
make sense during macvtap device setup to refer to a VF device as
"PF:VF#". This patch replaces the two uses of "vf" with "-1", and
removes the local variable, so that it's more clear we are always
calling the utility functions with vf set to -1.
This function is only called in two places, and the ifindex,
nltarget_kernel, and getPidFunc args are never used (and never will
be).
ifindex - we always know the name of the device, and never know the
ifindex - if we really did need the ifindex we would have to get it
from the name using virNetDevGetIndex(). In practice, we just send -1
to virNetDevSetVfConfig(), which doesn't bother to learn the real
ifindex (you only need a name *or* an ifindex for the netlink command
to succeed, not both).
nltarget_kernel - messages to set the config of an SRIOV VF will
always go to netlink in the kernel, not to another user process, so
this arg is always true (there are other uses of netlink messages
where the message might need to go to another user process, but never
in the case of RTM_SETLINK for SRIOV).
getPidFunc - this arg is only used if nltarget_kernel is false, and it
never is.
None of this has any functional effect, it just makes it easier to
follow what's happening when virNetDevSetVfConfig() is called.
virNetDevParseVfConfig() assumed that both the MAC address and VLAN
tag pointers were valid, so even if you only wanted one or the other,
you would need a variable to hold the returned value for both. This
patch checks each for a NULL pointer before filling it in.
The source code will check for NULL arguments for 'macvtap_macaddr' and
'vmuuid', so no need for the NONNULL in the prototypes. Following the stack
for both arguments to virNetDevVPortProfileOpSetLink also shows called
functions would handle a NULL value.
Additionally, modified the prototype to use the same 'macvtap_macaddr'
name as the source code for consistency.
Since the code checks 'mgr == NULL' anyway, no need for the prototype
to have the NONNULL arg check. Also add an error message to indicate what
the failure is so that there isn't a failed for some reason error.
Signed-off-by: John Ferlan <jferlan@redhat.com>
The comparison code used STREQ_NULLABLE anyway for both 'drv_name' and
'dom_name', so no need. Add a NULLSTR on the 'dom_name' too.
Signed-off-by: John Ferlan <jferlan@redhat.com>
The comparison code used STREQ_NULLABLE anyway for both 'drv_name' and
'dom_name', so no need. Add a NULLSTR on the 'dom_name' too.
Signed-off-by: John Ferlan <jferlan@redhat.com>
The code checks and handles a NULL 'str', so just remove the NONNULL.
Update the error message to add the NULLSTR() around 'str' also.
Signed-off-by: John Ferlan <jferlan@redhat.com>
This patch splits out the part of virNetDevTapCreateInBridgePort()
that would need to be re-done if an existing tap device had to be
re-attached to a bridge, and puts it into a separate function. This
can be used both when an existing domain interface config is updated
to change its connection, and also to re-attach to the "same" bridge
when a network has been stopped and restarted. So far it is used for
nothing.
This function provides the bridge/bond device that the given network
device is attached to. The return value is 0 or -1, and the master
device is a char** argument to the function - this is needed in order
to allow for a "success" return from a device that has no master.
The only reason that the ethtool features weren't being retrieved in
an unprivileged libvirtd was because they required ioctl(), and the
ioctl was using an AF_PACKET socket, which requires root. Now that we
are using AF_UNIX for ioctl(), this restriction can be removed.
The exact family of the socket created for the fd used by ioctl(7)
doesn't matter, it just needs to be a socket and not a file. But for
some reason when macvtap support was added, it used
AF_PACKET/SOCK_DGRAM sockets for its ioctls; we later used the same
AF_PACKET/SOCK_DGRAM socket for new ioctls we added, and eventually
modified the other pre-existing ioctl sockets (for creating/deleting
bridges) to also use AF_PACKET/SOCK_DGRAM (that code originally used
AF_UNIX/SOCK_STREAM).
The problem with using AF_PACKET (intended for sending/receiving "raw"
packets, i.e. packets that can be some protocol other than TCP or UDP)
is that it requires root privileges. This meant that none of the
ioctls in virnetdev.c or virnetdevip.c would work when running
libvirtd unprivileged.
This packet solves that problem by changing the family to AF_UNIX when
creating the socket used for any ioctl().
When enabling IPv6 on all interfaces, we may get the host Router
Advertisement routes discarded. To avoid this, the user needs to set
accept_ra to 2 for the interfaces with such routes.
See https://www.kernel.org/doc/Documentation/networking/ip-sysctl.txt
on this topic.
To avoid user mistakenly losing routes on their hosts, check
accept_ra values before enabling IPv6 forwarding. If a RA route is
detected, but neither the corresponding device nor global accept_ra
is set to 2, the network will fail to start.
virNetlinkCommand() processes only one response message, while some
netlink commands, like route dumping, need to process several.
Add virNetlinkDumpCommand() as a virNetlinkCommand() sister.
Allow to reuse as much as possible from virNetlinkCommand(). This
comment prepares for the introduction of virNetlinkDumpCommand()
only differing by how it handles the responses.
While connecting to qemu monitor, the first thing we do is wait
for it to show up. However, we are doing it with some timeout to
avoid indefinite waits (e.g. when qemu doesn't create the monitor
socket at all). After beaa447a29 we are using exponential back
off timeout meaning, after the first connection attempt we wait
1ms, then 2ms, then 4 and so on. This allows us to bring down
wait time for small domains where qemu initializes quickly.
However, on the other end of this scale are some domains with
huge amounts of guest memory. Now imagine that we've gotten up to
wait time of 15 seconds. The next one is going to be 30 seconds,
and the one after that whole minute. Well, okay - with current
code we are not going to wait longer than 30 seconds in total,
but this is going to change in the next commit.
The exponential back off is usable only for first few iterations.
Then it needs to be caped (one second was chosen as the limit)
and switch to constant wait time.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
The function is actually in virutil.c, but prototyped in virfile.h.
This patch fixes that by renaming the function to virWaitForDevices,
adding the prototype in virutil.h and libvirt_private.syms, and then
changing the callers to use the new name.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Found by Coverity. Because there's an "if ((cur = strstr(base, "revision"))
!= NULL) {" followed by a "base = cur" coverity notes that 'base' could
then be NULL causing the return to the top of the "while ((tmp_base =
strstr(base, "processor")) != NULL) {" to have strstr deref a NULL 'base'
pointer because the setting of base at the bottom of the loop is unconditional.
Alter the code to set "base = cur" after processing each key. That will
"ensure" that base doesn't get set to NULL if both "cpu" and "revision"
do no follow a "processor".
While a /proc/cpuinfo file that has a "processor" key but with neither
a "cpu" nor a "revision" doesn't seem feasible, the code is written as if
it could happen, so we have to account for it.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Calls to virFileReadAll after a VIR_ALLOC that return NULL all show
a memory leak since 'ret' isn't virSysinfoDefFree'd and normal path
"return ret" doesn't free outbuf.
Reported by Coverity
Signed-off-by: John Ferlan <jferlan@redhat.com>
Whole implementations along with helper totalling screens of code were
conditionally compiled. That made the code totally unreadable and
untestable. Rename functions to have the architecture in the name so
that all can be compiled at the same time and introduce header to allow
testing them all.
Proposed formal coding conventions encourage defining typedefs for
vir[Blah] and vir[Blah]Ptr separately from the associated struct named
_vir[Blah]:
typedef struct _virBlah virBlah;
typedef virBlah *virBlahPtr;
struct _virBlah {
...
};
At some point in the past, I had submitted several patches using a
more compact style that I prefer, and they were accepted:
typedef struct _virBlah {
...
} virBlah, *virBlahPtr;
Since these are by far a minority among all struct definitions, this
patch changes all those definitions to reflect the style prefered by
the proposal so that there is 100% consistency.
After the system has been booted, it should not change.
Cache the return value of virSystemdHasMachined.
Allow starting and terminating machines with just one
DBus call, instead of three, reducing the chance of
the call timing out.
Also introduce a small function for resetting the cache
to be used in tests.
Both virSystemdTerminateMachine and virSystemdCreateMachine
propagate the error to tell between a non-systemd system
and a hard error.
In virSystemdGetMachineNameByPID both are treated the same,
but an error is ignored by the callers.
Split out the checks into a separate function.
Arguably though, function returning only on success is a very
interesting, although quite impractical concept. Also, the errno isn't
and shouldn't be preserved in this case, since the errno can be directly
fed to the virReportSystemError.
Signed-off-by: Erik Skultety <eskultet@redhat.com>
This will eventually replace virQEMUBuildBufferEscapeComma, however
it's not possible right now. Some parts of the code that uses the
old function needs to be refactored.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
GCC 7 gets upset by
if (!tmp && (size * count))
warning
util/viralloc.c: In function 'virReallocN':
util/viralloc.c:246:23: error: '*' in boolean context, suggest '&&' instead [-Werror=int-in-bool-context]
if (!tmp && (size * count)) {
~~~~~~^~~~~~~~
Keep it happy by adding != 0 to the right hand expression
so it realizes we really are wanting to treat the result
of the arithmetic expression as a boolean
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
The 'raw' block driver in Qemu is not directly interesting from
libvirt's perspective, but it can be layered above some other block
drivers and this may be interesting for the user.
The patch adds support for the 'raw' block driver. The driver is treated
simply as a pass-through and child driver in JSON is queried to get the
necessary information.
Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
Split virStorageSourceParseBackingJSON into two functions so that the
core can be reused by other functions. The new function called
virStorageSourceParseBackingJSONInternal accepts virJSONValuePtr.
Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
The utils code should stay separated from other code (except for very
well justified cases). Unfortunately commit 272769becc
made it trivial to break the separation (and not get slapped by the
syntax-check rule) by adding -I src/conf to the CFLAGS for utils.
Remove this shortcut and except the two offenders from the syntax check
so that the codebase can be kept separated.
Create a virscsihost.c and place the functions there. That removes the
last #ifdef __linux__ from virutil.c.
Take the opporunity to also change the function names and in one case
the parameters slightly
Rather than have them mixed in with the virutil apis, create a separate
virvhba.c module and move the vHBA related calls into there. Soon there
will be more added.
Also modify the names of the functions and some arguments to be more
indicative of what is really happening. Adjust the callers respectively.
While I was changing fchosttest, rather than the non-descriptive names
test1...test6, rename them to match what the test is doing.
Before 9c17d665fd (v1.3.2 - I know, right?) it was possible to
have the following interface configuration:
<interface type='ethernet'/>
<script path=''/>
</interface>
This resulted in -netdev tap,script=,.. Fortunately, qemu helped
us to get away with this as it just ignored the empty script
path. However, after the commit mentioned above it's libvirtd
who is executing the script. Unfortunately without special
case-ing empty script path.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Currently disk names do not follow the
(regex) /^[fhv]d[a-z]+[0-9]*$/ completely
and hence one can assign disk names like
vd2 etc. This patch ensures that the
disk names follow the regex mentioned.
This patch also adds a testcase.
Signed-off-by: Nitesh Konkar <nitkon12@linux.vnet.ibm.com>
