bhyve supports intel hda sound devices that could be specified
on the command like using "-1:0,hda,play=$play_dev,rec=$rec_dev",
where "1:0" is a PCI address, and "$play_dev" and "$rec_dev"
point to the playback and recording device on the host respectively.
Currently, schema of the 'sound' element doesn't allow specifying
neither playback nor recording devices, so for now hardcode
/dev/dsp0, which is the first audio device on the host.
Signed-off-by: Roman Bogorodskiy <bogorodskiy@gmail.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Add 'ich7' sound model. This is a preparation for sound support in
bhyve, as 'ich7' is the only model it supports.
Signed-off-by: Roman Bogorodskiy <bogorodskiy@gmail.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Back when macvtap support was added in commit 315baab9443 in Feb. 2010
(libvirt-0.7.7), it was setup to autogenerate a name for the device if
one wasn't supplied, in the pattern "macvtap%d" (or "macvlan%d"),
similar to the way an unspecified standard tap device name will lead
to an autogenerated "vnet%d".
As a matter of fact, in commit ca1b7cc8e45 added in May 2010, the code
was changed to *always* ignore a supplied device name for macvtap
interfaces by deleting *any* name immediately during the <interface>
parsing (this was intended to prevent one domain which had failed to
completely start from deleting the macvtap device of another domain
which had subsequently been provided the same device name (this will
seem mildly ironic later). This was later fixed to only clear the
device name when inactive XML was being parsed. HOWEVER - this was
only done if the xml was <interface type='direct'> - autogenerated
names were not cleared for <interface type='network'> (which could
also result in a macvtap device).
Although the names of "vnetX" tap devices had always been
automatically cleared when parsing <interface> (see commit d1304583d
from July 2008 (!)), at the time macvtap support was added, both vnetX
and macvtapX device names were always included when formatting the
XML.
Then in commit a8be259d0cc (July 2011, libvirt-0.9.4), <interface>
formatting was changed to also clear out "vnetX" device names during
XML formatting as well. However the same treatment wasn't given to
"macvtapX".
Now in 2020, there has been a report that a failed migration leads to
the macvtap device of some other unrelated guest on the destination
host losing its network connectivity. It was determined that this was
due to the domain XML in the migration containing a macvtap device
name, e.g. "macvtap0", that was already in use by the other guest on
the destination. Normally this wouldn't be a problem, because libvirt
would see that the device was already in use, and then find a
different unused name. But in this case, other external problems were
causing the migration to fail prior to selecting a macvtap device and
successfully opening it, and during error recovery, qemuProcessStop()
was called, which went through all def->nets objects and (if they were
macvtap) deleted the device specified in net->ifname; since libvirt
hadn't gotten to the point of replacing the incoming "macvtap0" with
the name of a device it actually created for this guest, that meant
that "macvtap0" was deleted, *even though it was currently in use by a
different guest*!
Whew!
So, it turns out that when formatting "migratable" XML, "vnetX"
devices are omitted, just as when formatting "inactive" XML. By making
the code in both interface parsing and formatting consistent for
"vnetX", "macvtapX", and "macvlanX", we can thus make sure that the
autogenerated (and unneeded / completely *not* wanted) macvtap device
name will not be sent with the migration XML. This way when a
migration fails, net->ifname will be NULL, and libvirt won't have any
device to try and (erroneously) delete.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Back when the original version of this chunk of code was added (commit
41b087198 in libvirt-0.8.1 in April 2010), we used virExecDaemonize()
to start the qemu process, and would continue on in the function
(which at that time was called qemudStartVMDaemon()) even if a -1 was
returned. So it was possible to get to this code with rv == -1 (it was
called "ret" in that version of the code).
In modern libvirt code, qemu is started with virCommandRun(); then we
call virPidFileReadPath(); those are the only two ways of setting "rv"
prior to this code being removed, and in either case if the new value
of rv < 0, then we immediately skip over the rest of the code to the
cleanup: label.
This means that the code being removed by this patch is
unreachable.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Even if namespaces are disabled, then due to a missing check at the
beginning of qemuDomainBuildNamespace(), the domain startup code
still tries to populate (nonexistent) domain's namespace.
Fixes: 8da362fe62766b4eee209cd3ce591ceb62299d13
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
After the recent changes, this function is now always returning
zero. Turn it to 'void' to relieve callers from checking it.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
We don't need the auto-alignment now that the user is handling
it by hand.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
The existing auto-align behavior for pSeries has the idea to
alleviate user configuration of the NVDIMM size, given that the
alignment calculation is not trivial to do (256MiB alignment
of mem->size - mem->label_size value, a.k.a guest area). We
align mem->size down to avoid end of file problems.
The end result is not ideal though. We do not touch the domain
XML, meaning that the XML can report a NVDIMM size 255MiB smaller
than the actual size the guest is seeing. It also adds one more
thing to consider in case the guest is reporting less memory
than declared, since the auto-align is transparent to the
user.
Following Andrea's suggestion in [1], let's instead do an
size alignment validation. If the NVDIMM is unaligned, error out
and suggest a rounded up value. This can be bothersome to users,
but will bring consistency of NVDIMM size between the domain XML
and the guest.
This approach will force existing non-running pSeries guests to
readjust the NVDIMM value in their XMLs, if necessary. No changes
were made for x86 NVDIMM support.
[1] https://www.redhat.com/archives/libvir-list/2020-July/msg01471.html
Suggested-by: Andrea Bolognani <abologna@redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Next patch will use it outside of qemu_domain.c.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
This function is not changing the domain definition, it's only
reading from it. The function is going to be used from another
function which already takes const virDomainDef. Make the @def
const to avoid typecasting it.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Commit 43620689794507308fbd3def6992a68ee2f8fa97 moved the function to
util/virqemu.c which is compiled also on win32 and geteuid()/getegid()
doesn't exist there.
Move it to qemu_domain.c which is compiled only when the qemu driver is
enabled. Originally I didn't want to put it here as qemu_domain.c is a
code dump for helper functions but this is the least invasive fix.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
We've dumped all the snapshot helpers and related code into
qemu_driver.c. It accounted for ~10% of overal size of qemu_driver.c.
Separate the code to qemu_snapshot.c/h.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
There's a lot of helper code related to the save image handling. Extract
it to qemu_saveimage.c/h.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Move the code to qemu_domain.c so that it can be reused in other parts
of the qemu driver. 'qemu_domain' was chosen as we check the domain
state after closing the wrapper.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Move the code to qemu_domain.c so that it can be reused in other parts
of the qemu driver. 'qemu_domain' was chosen as the permissions are
based on the domain configuration.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Use automatic memory allocation to simplify the code and remove the need
for a 'cleanup:' label.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Use automatic memory allocation and move variables into correct scope to
simplify the code and remove the need for a 'cleanup:' label.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Use automatic memory allocation and move variables into correct scope to
simplify the code and remove the need for a 'error:' label.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Use automatic memory allocation and move variables into correct scope to
simplify the code and remove the need for a 'error:' label.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
This function doesn't have an overly verbose cleanup section as there
isn't any error code path. Unify it with the rest of the functions which
will simplify adding a possible error path.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
This is similar to one of previous patches.
When receiving stream (on virStorageVolUpload() and subsequent
virStreamSparseSendAll()) we may receive a hole. If the volume we
are saving the incoming data into is a regular file we just
lseek() and ftruncate() to create the hole. But this won't work
if the file is a block device. If that is the case we must write
zeroes so that any subsequent reader reads nothing just zeroes
(just like they would from a hole in a regular file).
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1852528
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
When handling sparse stream, a thread is executed. This thread
runs a read() or write() loop (depending what API is called; in
this case it's virStorageVolDownload() and this the thread run
read() loop). The read() is handled in virFDStreamThreadDoRead()
which is then data/hole section aware, meaning it uses
virFileInData() to detect data and hole sections and sends
TYPE_DATA or TYPE_HOLE virStream messages accordingly.
However, virFileInData() does not work with block devices. Simply
because block devices don't have data and hole sections. What we
can do though, is to mimic being always in a DATA section.
Partially resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1852528
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
For libvirt, the volume is just a binary blob and it doesn't
interpret data on volume upload/download. But as it turns out,
this unspoken assumption is not clear to our users. Document it
explicitly.
Suggested in: https://bugzilla.redhat.com/show_bug.cgi?id=1851023#c17
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Right now we're unconditionally adding RPATH information to the
installed binaries and libraries, but that's not always desired.
autotools seem to be smart enough to only include that information
when targeting a non-standard prefix, so most distro packages
don't actually contain it; moreover, both Debian and Fedora have
wiki pages encouraging packagers to avoid setting RPATH:
https://wiki.debian.org/RpathIssuehttps://fedoraproject.org/wiki/RPath_Packaging_Draft
Implement RPATH logic that Does The Right Thing™ in the most
common cases, while still offering users the ability to override
the default behavior if they have specific needs.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
In a very distant past, we came around machines that has not
continuous node IDs. This made us error out when constructing
capabilities XML. We resolved it by utilizing strange behaviour
of numa_node_to_cpus() in which it returned a mask with all bits
set for a non-existent node. However, this is not the only case
when it returns all ones mask - if the node exists and has enough
CPUs to fill the mask up (e.g. 128 CPUs).
The fix consists of using nodemask_isset(&numa_all_nodes, ..)
prior to calling numa_node_to_cpus() to determine if the node
exists.
Fixes: 628c93574758abb59e71160042524d321a33543f
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1860231
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
There was a report on libvirt-users [1] about the domxml to/from
native converter in the Xen driver not handling PCI addresses
without a domain specification. This patch improves parsing of PCI
addresses in the converter and allows PCI addresses with only
bb:ss.f. xl.cfg(5) also allows either the dddd:bb:ss.f or bb:ss.f
format. A test has been added to check the conversion from xl.cfg
to domXML.
[1] https://www.redhat.com/archives/libvirt-users/2020-August/msg00040.html
Signed-off-by: Jim Fehlig <jfehlig@suse.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
After previous cleanups, some labels in some functions have
nothing but 'return' statement in them. Drop the labels and
replace 'goto'-s with respective return statements.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Again, instead of closing FDs explicitly, we can automatically
close them when they go out of their respective scopes.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
A cleanup function can be declared for virFDStreamMsg type so
that the structure doesn't have to be freed explicitly.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
All callers of virFDStreamMsgQueuePush() have the same pattern:
they explicitly set @msg passed to NULL to avoid freeing it later
on. Well, the function can take address of the pointer and clear
it for them.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
The buffer that allocated in the virFDStreamThreadDoRead() can be
automatically freed, or if saved into the message structure it
can be stolen.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
So far, only ENOENT is ignored (to deal with kernels without
devmapper). However, as reported on the list, under certain
scenarios a different error can occur. For instance, when libvirt
is running inside a container which doesn't have permissions to
talk to the devmapper. If this is the case, then open() returns
-1 and sets errno=EPERM.
Assuming that multipath devices are fairly narrow use case and
using them in a restricted container is even more narrow the best
fix seems to be to ignore all open errors BUT produce a warning
on failure. To avoid flooding logs with warnings on kernels
without devmapper the level is reduced to a plain debug message.
Reported-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
Reviewed-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
When adding support for HMAT, in f0611fe8830 I've introduced a
check which aims to validate /domain/cpu/numa/interconnects. As a
part of that, there is a loop which checks whether all <latency/>
with @cache attribute refer to an existing cache level. For
instance:
<cpu mode='host-model' check='partial'>
<numa>
<cell id='0' cpus='0-5' memory='512000' unit='KiB' discard='yes'>
<cache level='1' associativity='direct' policy='writeback'>
<size value='8' unit='KiB'/>
<line value='5' unit='B'/>
</cache>
</cell>
<interconnects>
<latency initiator='0' target='0' cache='1' type='access' value='5'/>
<bandwidth initiator='0' target='0' type='access' value='204800' unit='KiB'/>
</interconnects>
</numa>
</cpu>
This XML defines that accessing L1 cache of node #0 from node #0
has latency of 5ns.
However, the loop was not written properly. Well, the check in
it, as it was always checking for the first cache in the target
node and not the rest. Therefore, the following example errors
out:
<cpu mode='host-model' check='partial'>
<numa>
<cell id='0' cpus='0-5' memory='512000' unit='KiB' discard='yes'>
<cache level='3' associativity='direct' policy='writeback'>
<size value='10' unit='KiB'/>
<line value='8' unit='B'/>
</cache>
<cache level='1' associativity='direct' policy='writeback'>
<size value='8' unit='KiB'/>
<line value='5' unit='B'/>
</cache>
</cell>
<interconnects>
<latency initiator='0' target='0' cache='1' type='access' value='5'/>
<bandwidth initiator='0' target='0' type='access' value='204800' unit='KiB'/>
</interconnects>
</numa>
</cpu>
This errors out even though it is a valid configuration. The L1
cache under node #0 is still present.
Fixes: f0611fe8830
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Laine Stump <laine@redhat.com>
Both parsing and formatting of NBD migration jobs is QEMU specific and
since we're trying to create a hypervisor-agnostic module out of
qemu_domainjob.c, move the NBD XML handling bits to the qemu_domain
module instead. Additionally, move the respective NBD XML calls to
the 'parseJob'/'formatJob' callbacks of the
qemuDomainObjPrivateJobCallbacks structure.
Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Functions `qemuDomainRemoveInactiveJob` and
`qemuDomainRemoveInactiveJobLocked` had their declaration misplaced in
`qemu_domainjob` and were moved to `qemu_domain` where their definitions
reside.
Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Most of our augeas files are generated during meson setup into build
directory and we were running augeas tests only for these files.
However, we have some other augeas and config files that are not
modified during meson setup and they are only in source directories.
In order to run tests for these files we need to provide different path
to both source and build directories.
Reported-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
This will be used later to specify different include directories for
augparse binary to run augeas tests.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
In case qemuDumpToFd() returns zero followed by a VIR_CLOSE(fd) fail,
we'd jump to the "cleanup" label with "ret=0", potentially resulting in
an unexpected success return value.
Signed-off-by: Hao Wang <wanghao232@huawei.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
In one of my latest patch (v6.6.0~30) I was trying to remove
libdevmapper use in favor of our own implementation. However, the
code did not take into account that device mapper can be not
compiled into the kernel (e.g. be a separate module that's not
loaded) in which case /proc/devices won't have the device-mapper
major number and thus virDevMapperGetTargets() and/or
virIsDevMapperDevice() fails.
However, such failure is safe to ignore, because if device mapper
is missing then there can't be any multipath devices and thus we
don't need to allow the deps in CGroups, nor create them in the
domain private namespace, etc.
Fixes: 22494556542c676d1b9e7f1c1f2ea13ac17e1e3e
Reported-by: Andrea Bolognani <abologna@redhat.com>
Reported-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
Tested-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
The device mapper major is needed in virIsDevMapperDevice() which
determines whether given device is managed by device-mapper. This
number is obtained by parsing /proc/devices and then stored in a
global variable so that the file doesn't have to be parsed again.
However, as it turns out this logic is flawed - the major number
is not static and can change as it can be specified as a
parameter when loading the dm-mod module.
Unfortunately, I was not able to come up with a good solution and
thus the /proc/devices file is being parsed every time we need
the device mapper major.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
Tested-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
Change the 'make check' reference after the switch to meson/ninja.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
GCC 10 complains about variables may be used uninitialized.
Even though it might be false positives, we can easily avoid them.
Avoiding
../src/storage/storage_backend_iscsi_direct.c:634:11: error: ‘nb_block’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
634 | while (lba < nb_block) {
| ^
../src/storage/storage_backend_iscsi_direct.c:619:14: note: ‘nb_block’ was declared here
619 | uint64_t nb_block;
| ^~~~~~~~
../src/storage/storage_backend_iscsi_direct.c:637:16: error: ‘block_size’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
637 | task = iscsi_write16_sync(iscsi, lun, lba, data,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
638 | block_size * to_write,
| ~~~~~~~~~~~~~~~~~~~~~~
639 | block_size, 0, 0, 0, 0, 0);
| ~~~~~~~~~~~~~~~~~~~~~~~~~~
../src/storage/storage_backend_iscsi_direct.c:618:14: note: ‘block_size’ was declared here
618 | uint32_t block_size;
| ^~~~~~~~~~
../src/storage/storage_backend_iscsi_direct.c: In function ‘virStorageBackendISCSIDirectRefreshPool’:
../src/storage/storage_backend_iscsi_direct.c:320:39: error: ‘nb_block’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
320 | vol->target.capacity = block_size * nb_block;
| ~~~~~~~~~~~^~~~~~~~~~
../src/storage/storage_backend_iscsi_direct.c:306:14: note: ‘nb_block’ was declared here
306 | uint64_t nb_block;
| ^~~~~~~~
../src/storage/storage_backend_iscsi_direct.c:320:39: error: ‘block_size’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
320 | vol->target.capacity = block_size * nb_block;
| ~~~~~~~~~~~^~~~~~~~~~
../src/storage/storage_backend_iscsi_direct.c:305:14: note: ‘block_size’ was declared here
305 | uint32_t block_size;
| ^~~~~~~~~~
Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
Reviewed-by: Marc Hartmayer <mhartmay@linux.ibm.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
GCC 10 complains about "well_formed_uri" may be used uninitialzed.
Even though it is a false positive, we can easily avoid it.
Avoiding
../src/qemu/qemu_migration.c: In function ‘qemuMigrationDstPrepareDirect’:
../src/qemu/qemu_migration.c:2920:16: error: ‘well_formed_uri’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
2920 | if (well_formed_uri) {
| ^
Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
Reviewed-by: Marc Hartmayer <mhartmay@linux.ibm.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
sc_spacing-check FAIL reporting a case of "Curly brackets around
single-line body:" in a recent commit.
Fixes: d9c21f4b "apparmor: allow adding permanent per guest rules"
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
