This new helper avoids more code repetition inside
lxcDomainSetBlkioParameters() and qemuDomainSetBlkioParameters().
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
After the introduction of virDomainDriverMergeBlkioDevice() in a
previous patch, it is now clear that lxcDomainSetBlkioParameters() and
qemuDomainSetBlkioParameters() uses the same loop to set cgroup
blkio parameter of a domain.
Avoid the repetition by adding a new helper called
virDomainCgroupSetupDomainBlkioParameters().
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
lxcDomainParseBlkioDeviceStr() and qemuDomainParseBlkioDeviceStr()
are the same function. Avoid code repetition by putting the code
in a new helper.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
lxcDomainMergeBlkioDevice() and qemuDomainMergeBlkioDevice()
are the same functions. This duplicated code can't be put in
the existing domain_cgroup.c since it's not cgroup related.
This patch introduces a new src/hypervisor/domain_driver.c to
host this more generic code that can be shared between virt
drivers. This new file is then used to create a new helper
called virDomainDeivceMergeBlkioDevice() to eliminate the code
repetition mentioned above. Callers in LXC and QEMU files
were updated.
This change is a preliminary step for more code reduction of
cgroup related code inside lxcDomainSetBlkioParameters() and
qemuDomainSetBlkioParameters().
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
qemuSetupCgroupVcpuBW() and lxcSetVcpuBWLive() shares the
same code to set CPU CFS period and quota. This code can be
moved to a new virCgroupSetupCpuPeriodQuota() helper to
avoid code repetition.
A similar code is also executed in virLXCCgroupSetupCpuTune(),
but without the rollback on error. Use the new helper in this
function as well since the 'period' rollback, if not a
straight improvement for virLXCCgroupSetupCpuTune(), is
benign. And we end up cutting more code repetition.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The code that calls virCgroupSetCpuShares() and virCgroupGetCpuShares()
is repeated in 4 different places. Let's put it in a new
virCgroupSetupCpuShares() to avoid code repetition.
There's a reason of why we execute a Get in the same value we
just executed Set, explained in detail by commit 97814d8ab3.
Let's add a gist of the reasoning behind it as a comment in
this new function as well.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The code from qemuSetupCgroupCpusetCpus() and virLXCCgroupSetupCpusetTune()
can be centralized in a new helper called virCgroupSetupCpusetCpus().
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
virLXCCgroupSetupMemTune() and qemuSetupMemoryCgroup() shares
duplicated code that can be put in a new helper to avoid
code repetition.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
There is duplicated code between virt drivers that needs to
be moved to avoid code repetition. In the case of duplicated
code between lxc_cgroup.c and qemu_cgroup.c a common place
would be utils/vircgroup.c. The problem is that this would
introduce /conf related definitions that shouldn't be imported
to vircgroup.c, which is supposed to be a place for utilitary
cgroups functions only. And syntax-check would forbid it anyway
due to cross-directory includes being used.
An alternative would be to overload domain_conf.c, which already
contains all the definitions required. But that file is already
crowded with XML handling code and we wouldn't do any favors to
it by putting more utilitary, non-XML parsing/formatting code
there.
In [1], Cole suggested a 'domain_cgroup' file to host common code
between lxc_cgroup and qemu_cgroup, and Daniel suggested a
'src/hypervisor' dir to host these type of files. This patch
introduces src/hypervisor/domain_cgroup.c and, to get started,
introduces a new virDomainCgroupSetupBlkio() function to host shared
code between virLXCCgroupSetupBlkioTune() and qemuSetupBlkioCgroup().
[1] https://www.redhat.com/archives/libvir-list/2019-December/msg00817.html
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Previous patch moved all duplicated code that were setting
and getting BlkioDevice parameters to vircgroup.c. We can
turn them into static and spare a few symbols in
libvirt_private.syms.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
There are code repetition of set() and get() blkio device
parameters across lxc and qemu files. Use the new vircgroup
helpers to trim the repetition a bit.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The current use of the functions that set and get
BlkioDevice attributes is doing a set(), followed by
a get() of the same parameter right after. This is done
because there is no guarantee that the kernel will accept
the desired value given by the set() call, thus we need to
execute a get() right after to get the actual value.
This patch adds helpers inside vircgroup.c to execute these
operations. Next patch will use these helpers to reduce
code repetition in LXC and QEMU files.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The inc.am Makfiles are included by src/Makefile.am.
Adjust the paths added to OPENRC_INIT_FILES_IN
accordingly.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Fixes: f4b1c020a2
These are missing files for OpenRC.
Signed-off-by: Ryan Moeller <ryan@iXsystems.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
The persistent alias name @persistent is allocated in
virDomainNetDefParseXML() but never freed.
==119642== 22 bytes in 2 blocks are definitely lost in loss record 178 of 671
==119642== at 0x483579F: malloc (vg_replace_malloc.c:309)
==119642== by 0x58F89F1: xmlStrndup (in /usr/lib64/libxml2.so.2.9.9)
==119642== by 0x4BA3B74: virXMLPropString (virxml.c:520)
==119642== by 0x4BDB0C5: virDomainNetDefParseXML (domain_conf.c:11876)
==119642== by 0x4BF9EF4: virDomainDefParseXML (domain_conf.c:21196)
==119642== by 0x4BFCD5B: virDomainDefParseNode (domain_conf.c:21943)
==119642== by 0x4BFCC36: virDomainDefParse (domain_conf.c:21901)
==119642== by 0x4BFCCCB: virDomainDefParseFile (domain_conf.c:21924)
==119642== by 0x114A9D: testCompareXMLToArgv (qemuxml2argvtest.c:452)
==119642== by 0x13894F: virTestRun (testutils.c:143)
==119642== by 0x11F46E: mymain (qemuxml2argvtest.c:1316)
==119642== by 0x13A60E: virTestMain (testutils.c:839
Fixes: fb0509d06a
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The privateData object is allocated in virDomainFSDefNew() but
never unref'd.
==119642== 480 bytes in 20 blocks are definitely lost in loss record 656 of 671
==119642== at 0x4837B86: calloc (vg_replace_malloc.c:762)
==119642== by 0x57806A0: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.6000.7)
==119642== by 0x4AE7392: virAllocVar (viralloc.c:331)
==119642== by 0x4B64395: virObjectNew (virobject.c:241)
==119642== by 0x48F1464: qemuDomainFSPrivateNew (qemu_domain.c:1427)
==119642== by 0x4BBF004: virDomainFSDefNew (domain_conf.c:2307)
==119642== by 0x4BD859A: virDomainFSDefParseXML (domain_conf.c:11217)
==119642== by 0x4BF9DD1: virDomainDefParseXML (domain_conf.c:21179)
==119642== by 0x4BFCD5B: virDomainDefParseNode (domain_conf.c:21943)
==119642== by 0x4BFCC36: virDomainDefParse (domain_conf.c:21901)
==119642== by 0x4BFCCCB: virDomainDefParseFile (domain_conf.c:21924)
==119642== by 0x114A9D: testCompareXMLToArgv (qemuxml2argvtest.c:452)
Fixes: 5120577ed7
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
During the hypervisor-agnostic validation of network devices, verify
that the interface type is either "network" or "bridge", and that if
there is any <virtualport>, that it doesn't have any type associated
with it.
This needs to be done both for the parse-time validation and for
runtime validation (after a port has been acquired from any associated
network), because an interface with type='network' could have an
actual type at runtime of "hostdev" or "direct", neither of which
support isolated='true' (yet). Likewise, if an interface is
type='network', then at runtime a <virtualport> with a type that
doesn't support isolated='yes' (e.g. "openvswitch", "802.1Qbh" -
currently *none* of the available virtualport types support it)
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
This setting can be updating very easily on an already active
interface by just changing it in sysfs. If the bridge used for
connection is also changed, there is no need to separately update it,
because the new setting isf done as a part of connecting to the bridge
anyway.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
This patch pushes the isolatedPort setting from the <interface> down
all the way to the callers of virNetDevBridgeAddPort(), and sets
BR_ISOLATED on the port (using virNetDevBridgePortSetIsolated()) after
the port has been successfully added to the bridge.
Signed-off-by: Laine Stump <laine@redhat.com>
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Similar to the way that the <vlan>, <bandwidth>, and <virtualport>
elements and the trustGuestRxFilters attribute in a <network> (or in
the appropriate <portgroup> element of a <network> can be applied to a
port when it is allocated for a domain's network interface, this patch
checks for a configured value of <port isolated="yes|no"/> in
either the domain <interface> or in the network, setting isolatedPort
in the <networkport> to the first one it finds (the setting from the
domain's <interface> is preferred). This, in turn, is passed back to
the domain when a port is allocated, so that the domain will use that
setting.
(One difference from <vlan>, <bandwidth>, <virtualport>, and
trustGuestRxFilters, is that all of those can be set in a <portgroup>
so that they can be applied only to a subset of interfaces connected
to the network. This didn't really make sense for the isolated setting
due to the way that it's implemented in Linux - the BR_ISOLATED flag
will prevent traffic from passing between two ports that both have
BR_ISOLATED set, but traffic can still go between those ports and
other ports that *don't* have BR_ISOLATED. (It would be nice if all
traffic from a BR_ISOLATED port could be blocked except traffic going
to/from a designated egress port or ports, but instead the entire
feature is implemented as a single flag. Because of this, it's really
only useful if all the ports on a network are isolated, so setting it
for a subset has no practical utility.)
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
This is a very simple thing to parse and format, but needs to be done
in 4 places, so two trivial utility functions have been made that can
be called from all the higher level parser/formatters:
<domain><interface>
<domain><interface><actual> (only in domain status)
<network>
<networkport>
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
When this flag is set for an interface attached to a bridge, traffic
to/from the specified interface can only enter/exit the bridge via
another attached interface that *doesn't* have the BR_ISOLATED flag
set. This can be used to permit guests to communicate with the rest of
the network, but not with each other.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Not only was the original error code destroyed in the case of
encountering an error during recovery from a failed attach to the
bridge (and then *that* error was destroyed by logging a *second*
error about the failure to recover - virNetDevBridgeAddPort() already
logs an error, so the one about failing to recover was redundant), but
if the recovery was successful, the function would then return success
to the caller even though it had failed.
Fixes: 2711ac8716
(overwritten errors were introduced along with this functionality)
Fixes: 6bde0a1a37
(the wrong return value was introduced by a refactor)
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Firstly, the check for disk I/O error can be moved into 'if
(!offline)' section a few lines below.
Secondly, checks for vmstate and slirp should be moved under the
same section because they reflect live state of a domain. For
offline migration no QEMU is involved and thus these restrictions
are not valid.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
In two places where virPidFileForceCleanupPath() is called, we
try to unlink() the pidfile again. This is needless because
virPidFileForceCleanupPath() has done just that.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The virPidFileReadPath() function is supposed to return 0 on
success or a negative value on failure. But the negative value
has a special meaning - it's negated errno. Therefore, when
converting string to int we shouldn't return -1 which translates
to EPERM. Returning EINVAL looks closer to the truth.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
There's nothing to clean up. Make it obvious what is returned.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Extract the code that directly deals with storage. This allows further
simplification and clarification of virStorageFileGetMetadataRecurse.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Replacing virHashLookup by virHashHasEntry allows us to use NULL as the
payload of the hash table rather than putting a fake '1' pointer into
the table.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
The path can be NULL e.g. for NBD disks. Use NULLSTR to prevent use of
NULL in %s.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Move the assignment to a place where we know that the backing store is
present rather than having to check in the cleanup section.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
We call virStorageFileSupportsBackingChainTraversal which already checks
that the 'storageFileRead' callback is non-NULL, which in turn means
that virStorageFileRead will not return -2.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Probing by file suffix was meant to be a last resort if probing by
contents fails or is not supported. For most formats we never specified
any suffix. There's a few formats implementing both magic bytes and
suffix and finally DMG which had only suffix probing. Since suffix
probing is nowhere reliable and only one format depends on in which has a
comment that qemu doesn't do the probing either drop the whole
infrastructure.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
In commit v5.9.0-400-gaf8e39921a I removed printing model's fallback and
vendor_id attributes when no model is specified. However, vendor_id
makes sense even without a specific CPU model (for host-model CPUs).
https://bugzilla.redhat.com/show_bug.cgi?id=1804549
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
qemuMonitorGetIOThreads returns a NULL-terminated list even when 0
iothreads are present. The caller didn't perform cleanup if there were 0
iothreads leaking the array.
https://bugzilla.redhat.com/show_bug.cgi?id=1804548
Fixes: d1eac92784
Reported-by: Jing Yan <jiyan@redhat.com>
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
To fix the actual bug, it was necessary to make networkPlugBandwidth() be
called also for 'bridge'-type networks implemented using macvtap's 'bridge'
mode (previously it was only called for those implemented on top of an
existing bridge).
However, it seems beneficial to call it for other network types as well, at
least because it removes an inconsistency in types of bandwidth configuration
changes permissible in inactive and active domain configs. It should also be
safe as the function pretty much amounts to NOP if no QoS is requested and the
new behaviour should not be any worse than before if it is.
Signed-off-by: Pavel Mores <pmores@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Even if an interface of type 'network', setting 'floor' is only supported
if the network's forward type is nat, route, open or none.
Signed-off-by: Pavel Mores <pmores@redhat.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
QoS 'floor' setting is documented to be only supported for interfaces of
type 'network'. Fail with an error message on attempt to set 'floor' on
an interface of any other type.
Signed-off-by: Pavel Mores <pmores@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
This compound condition will be useful in several places so it
makes sense to give it a name for better readability.
Signed-off-by: Pavel Mores <pmores@redhat.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
If the parsed 'raw' format JSON string has 'offset' or 'size' attributes
parse them as the format slice.
https://bugzilla.redhat.com/show_bug.cgi?id=1791788
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Implement support for the slice of type 'storage' which allows to set
the offset and size which modifies where qemu should look for the start
of the format container inside the image.
Since slicing is done using the 'raw' driver we need to add another
layer into the blockdev tree if there's any non-raw image format driver
used to access the data.
This patch adds the blockdev integration and setup of the image data so
that we can use the slices for any backing image.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
When creating overlay images e.g. for snapshots or when merging
snapshots we often specify the backing store string to use. Make the
formatter aware of backing chain entries which have a <slice>
configured so that we record it properly. Otherwise such images
would not work without the XML (when detecting the backing chain).
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The storage slice will require a specific node name in cases when the
image format is not raw. Store and format them in the status XML.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Specifically creating such images via libvirt during blockjobs would
be much more hassle than it's worth. Just forbid them for now.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
We support explicit storage slices only when using blockdev. Storage
slices expressed via the backing store string are left to qemu to
open correctly.
Reject storage slices configured via the XML for non-blockdev usage.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
If we have a 'format' type slice for a raw driver we can directly format
the values.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Introduce virStorageSourceSlice which will store the 'offset' and 'size'
of a virStorageSource and declare it as 'sliceStorage' and 'sliceFormat'
attributes of a virStorageSource.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
If a domain has a NVMe disk it already has the access configured.
Trying to configure it again on a commit or some other operation
is wrong and condemned to failure.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
