Switch the function to use VIR_AUTOFREE and VIR_AUTOPTR macros
to get rid of the cleanup section.
Requested-by: John Ferlan <jferlan@redhat.com>
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Switch the function to use VIR_AUTOFREE and VIR_AUTOPTR macros
to get rid of the cleanup section.
Requested-by: John Ferlan <jferlan@redhat.com>
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Switch the function to use VIR_AUTOFREE and VIR_AUTOPTR macros
to get rid of the cleanup section.
Requested-by: John Ferlan <jferlan@redhat.com>
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Switch the function to use VIR_AUTOFREE and VIR_AUTOPTR macros
to get rid of the cleanup section.
Requested-by: John Ferlan <jferlan@redhat.com>
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
If a -drive has no image, using image properties makes qemu whine that
they should not be used.
This patch stops formating cache/readonly/... for empty drives
for the pre-blockdev syntax. Unfortunately those parameters can't be
added later when inserting media, but on the other hand qemu will start
with an empty drive.
Since we already were able to start a VM with such config previously due
to qemu ignoring them I've opted just to skip formatting them.
Additionally with -blockdev support it will work as expected as the
image properties will be formatted when adding the image itself which is
not possible without it.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1651457
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
When commit 361c8dc17 added support for hotplugging the i6300esb
watchdog device (first in libvirt-3.9.0), it accidentally contstructed
the commandline for the device_add command before allocating a PCI
address for the device. With no PCI address specified in the command,
the watchdog would simply be placed at the lowest unused PCI slot.
On a 440fx guest, this doesn't cause a problem, because libvirt's PCI
address allocation algorithm would most likely give the same address
anyway (usually a slot on pci-root), so nobody noticed the omission of
address from the command.
But on a Q35 guest, the lowest unused PCI slot is on pcie-root, which
doesn't support hotplug; libvirt knows enough to assign a PCI address
that is on a pcie-to-pci-bridge (because its slots *do* support
hotplug), but qemu doesn't, so if there is no PCI address in the
command, qemu just tries to plug the new device into pcie-root, and
fails because it doesn't support hotplug, e.g.:
error: Failed to attach device from watchdog.xml
error: internal error: unable to execute QEMU command 'device_add':
Bus 'pcie.0' does not support hotplugging
The solution is simply to build the command string after assigning a
PCI address, not before.
Resolves: https://bugzilla.redhat.com/1666559
Signed-off-by: Laine Stump <laine@laine.org>
Reviewed-by: John Ferlan <jferlan@redhat.com>
If code in the @actualType switch needs to have/know which PCI
Address is being used, then we must assign it earlier. In particular
a vhost-user device needs to call qemuDomainSupportsNicdev which
requires an address to be defined.
Signed-off-by: Wang Yechao <wang.yechao255@zte.com.cn>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Split out parts of the config parsing code to make
the parent function easier to read.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Split out parts of the config parsing code to make
the parent function easier to read.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Split out parts of the config parsing code to make
the parent function easier to read.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Split out parts of the config parsing code to make
the parent function easier to read.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Split out parts of the config parsing code to make
the parent function easier to read.
This is the only patch that mixes various augeas entry
groups in one function.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Split out parts of the config parsing code to make
the parent function easier to read.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Split out parts of the config parsing code to make
the parent function easier to read.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Split out parts of the config parsing code to make
the parent function easier to read.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Split out parts of the config parsing code to make
the parent function easier to read.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Split out parts of the config parsing code to make
the parent function easier to read.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Split out parts of the config parsing code to make
the parent function easier to read.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Split out parts of the config parsing code to make
the parent function easier to read.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Split out parts of the config parsing code to make
the parent function easier to read.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Split out parts of the config parsing code to make
the parent function easier to read.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Split out parts of the config parsing code to make
the parent function easier to read.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Split out parts of the config parsing code to make
the parent function easier to read.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Split out parts of the config parsing code to make
the parent function easier to read.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
if virNetClientNew finishes with error before sock is set
to client object then sock does not get unrefed. This is
unexpected by function clients like virNetClientNewUNIX.
Let's make sure sock gets unrefed on any error path.
Next some clients like virNetClientNewLibSSH2 try to unref
sock on virNetClientNew errors. This is not correct even
before this patch because in some cases virNetClientNew
unrefed sock on error path by itself. Let's give up
sock managment to virNetClientNew entirely.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Currently the job name corresponds to the disk the job belongs to. For
jobs which will not correspond to disks we'll need to track the name
separately.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Now that the data is per-job, we don't really need to bother with
finishing the synchronous job handling if the job is already terminated.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Rather than storing the presence of the blockjob in a flag we can bind
together the lifecycle of the job with the lifecycle of the object which
is tracking the data for it.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Instead of passing in the disk information, pass in the job and name the
function accordingly.
Few callers needed to be modified to have the job pointer handy.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The processing function modifies the job state so it should make sure
that the variable holding the new state is cleared properly and not the
caller. The caller should only deal with the job state and not the
transition that happened.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The job error can be safely accessed in the job structure, so we don't
need to propagate it through qemuBlockJobUpdateDisk.
Drop the propagation and refactor any caller that pased non-NULL error.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The same message is reported in 3 distinct places. Move it out into a
single function.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Add a field tracking the current state of job so that it can be queried
later. Until now the job state e.g. that the job is _READY for
finalizing was tracked only for mirror jobs. Add tracking of state for
all jobs.
Similarly to 'qemuBlockJobType' this maps the existing states of the
blockjob from virConnectDomainEventBlockJobStatus to
'qemuBlockJobState' so that we can track some internal states as well.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Modify qemuBlockJobSyncBeginDisk to operate on qemuBlockt sJobDataPtr and
rename it accordingly.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
We can properly track the job type when starting the job so that we
don't have to infer it later.
This patch also adds an enum of block job types specific to qemu
(qemuBlockjobType) which mirrors the public block job types
(virDomainBlockJobType) but allows for other types to be added later
which will not be public.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Block jobs can also happen on objects which are not a disk at a given
point (e.g. the frontend was not hotplugged yet) and thus will be
eventually kept separately. Add a reference back to the disk for
blockjobs which do correspond to a disk.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
If the job wasn't started, we don't need to end the synchronous job. Add
a note and drop the unnecessary calls.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Rather than directly modifying fields in the qemuBlockJobDataPtr
structure add a bunch of fields which allow to do the transitions.
This will help later when adding more complexity to the job handling.
APIs introduced in this patch are:
qemuBlockJobDiskNew - prepare for starting a new blockjob on a disk
qemuBlockJobDiskGetJob - get the block job data structure for a disk
For individual job state manipulation the following APIs are added:
qemuBlockJobStarted - Sets the job as started with qemu. Until that
the job can be cancelled without asking qemu.
qemuBlockJobStartupFinalize - finalize job startup. If the job was
started in qemu already, just releases
reference to the job object. Otherwise
clears everything as if the job was never
started.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Extract the disk mirroring startup code from the loop into a separate
function to allow cleaner cleanup paths.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The field is used to note the state the job has transitioned to while
handling the blockjob state change event. Rename the field so that it's
obvious that this is the new state and not the general state of the
blockjob.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Reference counting will simplify semantics of the lifecycle of the
object.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
When cancelling job after a reconnect we can now use the disk block job
state rather than having to re-detect it in the migration code.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Now that we reprobe the status of blockjobs when reconnecting in
addition to handling job status events, the status reprobing can be
removed as we always track the correct status internally.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Block job state was widely untracked by libvirt across restarts which
was allowed by a stateless block job finishing handler which discarded
disk state and redetected it. This is undesirable since we'll need to
track more information for individual blockjobs due to -blockdev
integration requirements.
In case of legacy blockjobs we can recover whether the job is present at
reconnect time by querying qemu. Adding tracking whether a job is
present will allow simplification of the non-shared-storage cancellation
code.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Internally we do a 'block-copy' to accomodate non-shared storage
migration but the code did not fill in that the block job was active on
the disk when starting the copy job. Since we handle block jobs finishes
regardless of having it registered it's not a problem but soon will
become one.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
qemuBlockJobEventProcessLegacy was getting too big. Remove handling of
completed jobs in a separate function.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
This will handle blockjob finalizing for the old approach so rename it
accordingly.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
'cleanup' label was accessed only from a jump to 'error'. Consolidate
everyting into 'cleanup'.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Struct qemuDomainDiskPrivate was holding multiple variables connected to
a disk block job. Consolidate them into a new struct qemuBlockJobData.
This will also allow simpler extensions to the block job mechanisms.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The blockjob module uses 'qemuDomainAsyncJob' in it's public headers.
As I plan adding a new structure containing job data which will need to
be included in "qemu_domain.h" it's necessary to break the circular
dependency.
Convert 'qemuDomainAsyncJob' type to 'int' as it's an enum.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
All the public APIs of the qemu_blockjob module operate on a 'disk'.
Since I'll be adding APIs which operate on a job later let's rename the
existing ones.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The function is now only called locally. Some code movement was
necessary to avoid forward declarations.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Replace use of qemuBlockJobEventProcess with the general helper. A small
tweak is required to pass in the 'type' and 'status' of the job via the
appropriate private data variables.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The event reports the disk path to identify the disk which makes sense
only for local disks. Additionally network backed disks like NBD don't
need to have a path so the callback would return NULL.
Report VIR_DOMAIN_EVENT_ID_BLOCK_JOB only for non-empty local disks.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Put the emitting of VIR_DOMAIN_EVENT_ID_BLOCK_JOB and
VIR_DOMAIN_EVENT_ID_BLOCK_JOB_2 into a separate function.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Instead of copying the default default values upfront
and then wondering whether the user has given us a new default,
leave the per-usage TLS certdirs and secrets empty during
parsing and only fill them afterwards if they weren't provided
by the user.
This means that instead of looking whether the specific certdir
paths match the default default, the Validate function (which
is called in between parsing and setting the defaults) can error
out for missing directories if the value is present, because
it must've come from the user.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Introduce a set of bool variables with the 'present' suffix
to track whether the value was actually specified.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
According to the GNU Make manual, "double-colon rules are
somewhat obscure and not often very useful". Looking at
the few instances we have in libvirt, that certainly seems
to be the case, so just drop them.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Commit 7282f455a got rid of the VIR_WARNINGS_NO_CAST_ALIGN macro
when refactoring the code and broke the build with clang.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Turns out, that there are few bugs that are not that trivial to
fix (e.g. around block jobs). Instead of rushing in not
thoroughly tested fixes disable the feature temporarily for the
release.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
ACKed-by: Peter Krempa <pkrempa@redhat.com>
I had intended to make these changes to commit d40b820c before
pushing, but forgot about it during the day between the initial review
and ACK.
Neither change is significant - just returning immediately when
virNetDevGetName() fails (instead of logging a debug message first)
and eliminating a comment that adds to confusion rather than
eliminating it. Still, the changes should be made to be more
consistent with nearly identical code just a few lines up (added in
commit 7282f455)
Signed-off-by: Laine Stump <laine@laine.org>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
When checking the setting of accept_ra, we have assumed that all
routes have a single nexthop, so the interface of the route would be
in the RTA_OIF attribute of the netlink RTM_NEWROUTE message. But
multipath routes don't have an RTA_OIF; instead, they have an
RTA_MULTIPATH attribute, which is an array of rtnexthop, with each
rtnexthop having an interface. This patch adds a loop to look at the
setting of accept_ra of the interface for every rtnexthop in the
array.
Signed-off-by: Laine Stump <laine@laine.org>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
When commit 1d94b3e7 added code to walk the [n]hostdevs list looking
to add shared hostdevs, it should've filtered any hostdevs that were
not SCSI hostdev's.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
This is about the same number of code lines, but is simpler, and more
consistent with what will be added to check another attribute in a
coming patch.
As a side effect, it
Resolves: https://bugzilla.redhat.com/1583131
Signed-off-by: Laine Stump <laine@laine.org>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
This same operation needs to be done in multiple places, so move the
inline code into a separate function.
Signed-off-by: Laine Stump <laine@laine.org>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
This is problematic if a callback function wants to send the nlmsghdr
to a library function that has no "const" in its prototype
(e.g. nlmsg_find_attr())
Signed-off-by: Laine Stump <laine@laine.org>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
These files need to be installed on the system for apparmor
support to work, so they don't belong with examples.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Instead of defining targets conditionally and depending on
them unconditionally, define a couple of variables and
conditionally add targets to them.
In addition to removing a bunch of useless code, this has
the nice effect of no longer requiring the main Makefile.am
to have any knowledge about the contents of the various
snippets it includes.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
This is consistent with the way we already handle
configuration for other init systems such as upstart and
systemd.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
The feature was added to QEMU in 3.1.0 and it is currently blocking
migration, which is expected to change in the future. Luckily 3.1.0 is
new enough to give us migratability hints on each feature via
query-cpu-model-expension, which means we don't need to use the
"migratable" attribute on the CPU map XML.
The kernel calls this feature arch_capabilities and RHEL/CentOS 7.* use
arch-facilities. Apparently some CPU test files were gathered with the
RHEL version of QEMU. Let's update the test files to avoid possible
confusion about the correct naming.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The session daemon is unable to set XATTRs in 'trusted'
namespace because it doesn't run as privileged process.
Therefore, when creating the default qemu config enable
rememberOwner only when running as privileged process.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Since its introduction in commit 0977b8aa07 (released in v1.2.14)
qemuAgentGetInterfaces calls qemuAgentCommand with needReply=false,
which allows qemuAgentCommand to return 0 even when it did not get
any reply from the agent.
Set needReply to true, since we dereference it right after.
This can be hit if libvirt is waiting for an event from the agent
(e.g. shutdown) and the agent cannot reply in time (e.g. due to
the guest being shut down), as reported in:
https://bugzilla.redhat.com/show_bug.cgi?id=1663051
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
The use of 'lxc://' was mistakenly broken in:
commit 4c8574c85c
Author: Daniel P. Berrangé <berrange@redhat.com>
Date: Wed Mar 28 12:49:29 2018 +0100
driver: ensure NULL URI isn't passed to drivers with whitelisted URIs
Allow it again for historical compatibility.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
In the previous commit we are using uint64_t for storing subnet
prefix and interface id that qemu reports in
RDMA_GID_STATUS_CHANGED event. We also report them in some debug
messages. This poses a problem because uint64_t can be UL or ULL
depending on the host architecture and hence we wouldn't know
which format to use. Switch to ULL which is big enough and
doesn't suffer from the issue.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
This event is emitted on the monitor when a GID table in pvrdma device
is modified and the change needs to be propagate to the backend RDMA
device's GID table.
The control over the RDMA device's GID table is done by updating the
device's Ethernet function addresses.
Usually the first GID entry is determine by the MAC address, the second
by the first IPv6 address and the third by the IPv4 address. Other
entries can be added by adding more IP addresses. The opposite is the
same, i.e. whenever an address is removed, the corresponding GID entry
is removed.
The process is done by the network and RDMA stacks. Whenever an address
is added the ib_core driver is notified and calls the device driver's
add_gid function which in turn update the device.
To support this in pvrdma device we need to hook into the create_bind
and destroy_bind HW commands triggered by pvrdma driver in guest.
Whenever a changed is made to the pvrdma device's GID table a special
QMP messages is sent to be processed by libvirt to update the address of
the backend Ethernet device.
Signed-off-by: Yuval Shaia <yuval.shaia@oracle.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
These were not caught by our current regular expressions
but will be caught by the improved ones we're about to
introduce, so fix them ahead of time.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Essentially, bring back the old behaviour as of commit eba36a38 which
was later changed by commit ae06048bf5. Even though all the stderr
messages will eventually end up in the journal, we're not making use of
the fields journald provides.
https://bugzilla.redhat.com/show_bug.cgi?id=1592644
Signed-off-by: Erik Skultety <eskultet@redhat.com>
Acked-by: Michal Privoznik <mprivozn@redhat.com>
Our use of INCLUDES in Makefile.am hearkens back to when we had to
cater to automake 1.9.6 (thanks, RHEL 5) which lacked AM_CPPFLAGS.
Modern Automake flags a warning that INCLUDES is deprecated, and
now that we mandate RHEL 7 or better (see commit c1bc9c66), we no
longer have to cater to the old spelling. This change will also
make it easier to do per-binary CPPFLAGS.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Commit c0a8ea45 removed the use of gettextize, and the setting of
GETTEXT_CPPFLAGS, but did not scrub the now-unused variable from
Makefile.am snippets.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
In 600462834f we've tried to remove Author(s): lines
from comments at the beginning of our source files. Well, in some
files while we removed the "Author" line we did not remove the
actual list of authors.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
According to the result parsing from xml, add the unarmed property
into QEMU command line:
-device nvdimm,...[,unarmed=on]
Signed-off-by: Luyao Zhong <luyao.zhong@intel.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
According to the result parsing from xml, add pmem property
into QEMU command line:
-object memory-backend-file,...[,pmem=on]
Signed-off-by: Luyao Zhong <luyao.zhong@intel.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
According to the result parsing from xml, add align property
into QEMU command line:
-object memory-backend-file,...[,align=xxx]
Signed-off-by: Luyao Zhong <luyao.zhong@intel.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
This capability tracks if nvdimm has the unarmed attribute or not
for the nvdimm readonly xml attribute.
Signed-off-by: Luyao Zhong <luyao.zhong@intel.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
This capability tracks if memory-backend-file has the pmem
attribute or not.
Signed-off-by: Luyao Zhong <luyao.zhong@intel.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
This capability tracks if memory-backend-file has the align
attribute or not.
Signed-off-by: Luyao Zhong <luyao.zhong@intel.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
NVDIMM emulation will mmap the backend file, it uses host pagesize
as the alignment of mapping address before, but some backends may
require alignments different from the pagesize. So the 'alignsize'
option is introduced to allow specification of the proper alignment:
<devices>
...
<memory model='nvdimm' access='shared'>
<source>
<path>/dev/dax0.0</path>
<alignsize unit='MiB'>2</alignsize>
</source>
<target>
<size unit='MiB'>4094</size>
<node>0</node>
<label>
<size unit='MiB'>2</size>
</label>
</target>
</memory>
...
</devices>
Signed-off-by: Luyao Zhong <luyao.zhong@intel.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Before launching a SEV guest we take the base64-encoded guest owner's
data specified in launchSecurity and create files with the same content
under /var/lib/libvirt/qemu/<domain>. The reason for this is that we
need to pass these files on to QEMU which then uses them to communicate
with the SEV firmware, except when it doesn't have permissions to open
those files since we don't relabel them.
https://bugzilla.redhat.com/show_bug.cgi?id=1658112
Signed-off-by: Erik Skultety <eskultet@redhat.com>
Acked-by: Michal Privoznik <mprivozn@redhat.com>
Since SEV operates on a per domain basis, it's very likely that all
SEV launch-related data will be created under
/var/lib/libvirt/qemu/<domain_name>. Therefore, when calling into
qemuProcessSEVCreateFile we can assume @libDir as the directory prefix
rather than passing it explicitly.
Signed-off-by: Erik Skultety <eskultet@redhat.com>
Acked-by: Michal Privoznik <mprivozn@redhat.com>
The @con type security_context_t is actually a "char *", so the
correct check should be to dereference one more level; otherwise,
we could return/use the NULL pointer later in a subsequent
virSecuritySELinuxSetFileconImpl call (using @fcon).
Suggested-by: Michal Prívozník <mprivozn@redhat.com>
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
If virSecuritySELinuxRestoreFileLabel returns 0 or -1 too soon, then
the @newpath will be leaked.
Suggested-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Because missing optional storage source is not error. The patch
address only local files. Fixing other cases is a bit ugly.
Below is example of error notice in log now:
error: virStorageFileReportBrokenChain:427 :
Cannot access storage file '/path/to/missing/optional/disk':
No such file or directory
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
Every time we call all domain stats for inactive domain with
unavailable storage source we get error message in logs [1]. It's a bit noisy.
While it's arguable whether we need such message or not for mandatory
disks we would like not to see messages for optional disks. Let's
filter at least for cases of local files. Fixing other cases would
require passing flag down the stack to .backendInit of storage
which is ugly.
Stats for active domain are fine because we either drop disks
with unavailable sources or clean source which is handled
by virStorageSourceIsEmpty in qemuDomainGetStatsOneBlockFallback.
We have these logs for successful stats since 25aa7035d (version 1.2.15)
which in turn fixes 596a13713 (version 1.2.12 )which added substantial
stats for offline disks.
[1] error message example:
qemuOpenFileAs:3324 : Failed to open file '/path/to/optional/disk': No such file or directory
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
Introduce caching whether /dev/kvm is usable as the QEMU user:QEMU
group. This reduces the overhead of the QEMU capabilities cache
lookup. Before this patch there were many fork() calls used for
checking whether /dev/kvm is accessible. Now we store the result
whether /dev/kvm is accessible or not and we only need to re-run the
virFileAccessibleAs check if the ctime of /dev/kvm has changed.
Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
This test checks if security label remembering works correctly.
It uses qemuSecurity* APIs to do that. And some mocking (even
though it's not real mocking as we are used to from other tests
like virpcitest). So far, only DAC driver is tested.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
We are setting label on kernel, initrd, dtb and slic_table files.
But we never restored it.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
It helps whe trying to match calls with virSecuritySELinuxSetAllLabel
if the order in which devices are set/restored is the same in
both functions.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
When iterating over list of paths/disk sources to relabel it may
happen that the process fails at some point. In that case, for
the sake of keeping seclabel refcount (stored in XATTRs) in sync
with reality we have to perform rollback. However, if that fails
too the only thing we can do is warn user.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
It's important to keep XATTRs untouched (well, in the same state
they were in when entering the function). Otherwise our
refcounting would be messed up.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Similarly to what I did in DAC driver, this also requires the
same SELinux label to be used for shared paths. If a path is
already in use by a domain (or domains) then and the domain we
are starting now wants to access the path it has to have the same
SELinux label. This might look too restrictive as the new label
can still guarantee access to already running domains but in
reality it is very unlikely and usually an admin mistake.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
It is going to be important to know if the current transaction we
are running is a restore operation or set label operation so that
we know whether to call virSecurityGetRememberedLabel() or
virSecuritySetRememberedLabel(). That is, whether we are in a
restore and therefore have to fetch the remembered label, or we
are in set operation and therefore have to store the original
label.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Now that we have seclabel remembering we can safely restore
labels for shared and RO disks. In fact we need to do that to
keep seclabel refcount stored in XATTRs in sync with reality.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
This also requires the same DAC label to be used for shared
paths. If a path is already in use by a domain (or domains) then
and the domain we are starting now wants to access the path it
has to have the same DAC label. This might look too restrictive
as the new label can still guarantee access to already running
domains but in reality it is very unlikely and usually an admin
mistake.
This requirement also simplifies seclabel remembering, because we
can store only one seclabel and have a refcounter for how many
times the path is in use. If we were to allow different labels
and store them in some sort of array the algorithm to match
labels to domains would be needlessly complicated.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Because the implementation that will be used for label
remembering/recall is not atomic we have to give callers a chance
to enable or disable it. That is, enable it if and only if
metadata locking is enabled. Otherwise the feature MUST be turned
off.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
We are setting label on kernel, initrd, dtb and slic_table files.
But we never restored it.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
It helps whe trying to match calls with virSecurityDACSetAllLabel
if the order in which devices are set/restored is the same in
both functions.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
When iterating over list of paths/disk sources to relabel it may
happen that the process fails at some point. In that case, for
the sake of keeping seclabel refcount (stored in XATTRs) in sync
with reality we have to perform rollback. However, if that fails
too the only thing we can do is warn user.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
It's important to keep XATTRs untouched (well, in the same state
they were in when entering the function). Otherwise our
refcounting would be messed up.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
This file implements wrappers over XATTR getter/setter. It
ensures the proper XATTR namespace is used.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
For consistency, handle the @data "char **" (or remote_string)
assignments and processing similarly between various APIs
Signed-off-by: John Ferlan <jferlan@redhat.com>
ACKed-by: Michal Privoznik <mprivozn@redhat.com>
Using a combination of VIR_ALLOC and VIR_STRDUP into a local
variable and then jumping to error on the VIR_STRDUP before
assiging it into the @data would cause a memory leak. Let's
just avoid that by assiging directly into @data.
Signed-off-by: John Ferlan <jferlan@redhat.com>
ACKed-by: Michal Privoznik <mprivozn@redhat.com>
The virtualization driver has two connections to the virtlogd daemon,
one pipe fd for writing to the log file, and one socket fd for making
RPC calls. The typical sequence is to write some data to the pipe fd and
then make an RPC call to determine the current log file offset.
Unfortunately these two operations are not guaranteed to be handling in
order by virtlogd. The event loop for virtlogd may identify an incoming
event on both the pipe fd and socket fd in the same iteration of the
event loop. It is then entirely possible that it will process the socket
fd RPC call before reading the pending log data from the pipe fd.
As a result the virtualization driver will get an outdated log file
offset reported back.
This can be seen with the QEMU driver where, when a guest fails to
start, it will randomly include too much data in the error message it
has fetched from the log file.
The solution is to ensure we have drained all pending data from the pipe
fd before reporting the log file offset. The pipe fd is always in
blocking mode, so cares needs to be taken to avoid blocking. When
draining this is taken care of by using poll(). The extra complication
is that they might already be an event loop dispatch pending on the pipe
fd. If we have just drained the pipe this pending event will be invalid
so must be discarded.
See also https://bugzilla.redhat.com/show_bug.cgi?id=1356108
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The arguments to the N_() macro must only ever be a literal string. It
is not possible to use macro arguments, or use macro string
concatenation in this context. The N_() macro is a no-op whose only
purpose is to act as a marker for xgettext when it extracts translatable
strings from the source code. Anything other than a literal string will
be silently ignored by xgettext.
Unfortunately this means that the clever MSG, MSG2 & MSG_EXISTS macros
used for building up error message strings have prevented any of the
error messages getting marked for translation. We must sadly, revert to
a more explicit listing of strings for now.
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The autostart under session daemon might not behave as you'd
expect it to behave. This patch is inspired by latest
libvirt-users discussion:
https://www.redhat.com/archives/libvirt-users/2018-December/msg00047.html
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
The driver is unmaintained, untested and severely broken for
quite some time now. Since nobody even reported any issue with it
let us drop it.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
QEMU can report how many times during post-copy migration the domain
running on the destination host tried to access a page which has not
been migrated yet.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The QEMU command line arguments are very long and currently all written
on a single line to /var/log/libvirt/qemu/$GUEST.log. This introduces
logic to add line breaks after every env variable and "-" optional
argument, and every positional argument. This will create a clearer log
file, which will in turn present better in bug reports when people cut +
paste from the log into a bug comment.
An example log file entry now looks like this:
2018-12-14 12:57:03.677+0000: starting up libvirt version: 5.0.0, qemu version: 3.0.0qemu-3.0.0-1.fc29, kernel: 4.19.5-300.fc29.x86_64, hostname: localhost.localdomain
LC_ALL=C \
PATH=/usr/local/bin:/usr/local/sbin:/usr/bin:/usr/sbin \
HOME=/home/berrange \
USER=berrange \
LOGNAME=berrange \
QEMU_AUDIO_DRV=none \
/usr/bin/qemu-system-ppc64 \
-name guest=guest,debug-threads=on \
-S \
-object secret,id=masterKey0,format=raw,file=/home/berrange/.config/libvirt/qemu/lib/domain-33-guest/master-key.aes \
-machine pseries-2.10,accel=tcg,usb=off,dump-guest-core=off \
-m 1024 \
-realtime mlock=off \
-smp 1,sockets=1,cores=1,threads=1 \
-uuid c8a74977-ab18-41d0-ae3b-4041c7fffbcd \
-display none \
-no-user-config \
-nodefaults \
-chardev socket,id=charmonitor,fd=23,server,nowait \
-mon chardev=charmonitor,id=monitor,mode=control \
-rtc base=utc \
-no-shutdown \
-boot strict=on \
-device qemu-xhci,id=usb,bus=pci.0,addr=0x1 \
-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x2 \
-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \
-msg timestamp=on
2018-12-14 12:57:03.730+0000: shutting down, reason=failed
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The virCommand APIs do not expect to be given a NULL value for an arg
name or value. Such a mistake can lead to execution of the wrong
command, as the NULL may prematurely terminate the list of args.
Detect this and report suitable error messages.
This identified a flaw in the storage test which was passing a NULL
instead of the volume path. This flaw was then validated by an incorrect
set of qemu-img args as expected data.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Simplify adding of new errors by just adding them to the array of
messages rather than having to add conversion code.
Additionally most of the messages add the format string part as a suffix
so we can avoid some of the duplication by using a macro which adds the
suffix to the original string. This way most messages fit into the 80
column limit and only 3 exceed 100 colums.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Erik Skultety <eskultet@redhat.com>
Clarify how @info is used and what the returned values look like.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Simplify wording of the error string for VIR_ERR_OPEN_FAILED and
VIR_ERR_CALL_FAILED. The error codes itself are currently unused so it
will not impact any client.
This will simplify upcomming patch which refactors how we convert these.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Few error codes were missing the version of the message with additional
info. In case of the modified messages it's not very likely they'll ever
report any additional data, but for the sake of consistency we should
provide them.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Additional information for an error message is either in form of a
string or empty. Fix two offenders. One used %d as the format modifier
and the second one always expected a string.
Thankfully, neither of the offenders are currently in effect.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
We do have one for the error domain but not for the error number itself.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Require that all headers are guarded by a symbol named
LIBVIRT_$FILENAME
where $FILENAME is the uppercased filename, with all characters
outside a-z changed into '_'.
Note we do not use a leading __ because that is technically a
namespace reserved for the toolchain.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
This introduces a syntax-check script that validates header files use a
common layout:
/*
...copyright header...
*/
<one blank line>
#ifndef SYMBOL
# define SYMBOL
....content....
#endif /* SYMBOL */
For any file ending priv.h, before the #ifndef, we will require a
guard to prevent bogus imports:
#ifndef SYMBOL_ALLOW
# error ....
#endif /* SYMBOL_ALLOW */
<one blank line>
The many mistakes this script identifies are then fixed.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
For some reason, xdr_free uses char * instead of void * for its 2nd
argument which is passed to a custom free routine. Commit
dc54b3ec missed this detail which made the build fail on a number of
platforms. Fix it by explicitly casting the object pointer to char *
just like we do in other places throughout the code base.
Signed-off-by: Erik Skultety <eskultet@redhat.com>
When libvirt is reconnecting to running domain that uses cgroup v2
the QEMU process reports cgroup for the emulator directory because the
main thread is in that cgroup. We need to remove the "/emulator" part
in order to match with the root cgroup directory name for that domain.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
The rewrite to support cgroup v2 missed this function. In cgroup v2
we have different files to track tasks.
We would fail to remove cgroup on non-systemd OSes if there is any
extra process assigned to guest cgroup because we would not kill any
process form the guest cgroup.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Turns out there some build platforms that must not define MOUNT
or VGCHANGE in config.h... So moving the commands from the storage
backend specific module into a common storage_util module causes
issues for those platforms.
So instead of assuming they are there, let's just pass the command
string to the storage util API's from the storage backend specific
code (as would have been successful before). Also modify the test
to determine whether the MOUNT and/or VGCHANGE doesn't exist and
just define it to (for example) what Fedora has for the path. Could
have just used "mount" and "vgchange" in the call, but that defeats
the purpose of adding the call to virTestClearCommandPath.
Signed-off-by: John Ferlan <jferlan@redhat.com>
