While the callers should make sure that they don't call
qemuBlockJobEmitEvents for any internal state or job, let's add checks
that prevents us from emitting wrong events altogether.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
We have this very handy macro called VIR_STEAL_PTR() which steals
one pointer into the other and sets the other to NULL. The
following coccinelle patch was used to create this commit:
@ rule1 @
identifier a, b;
@@
- b = a;
...
- a = NULL;
+ VIR_STEAL_PTR(b, a);
Some places were clean up afterwards to make syntax-check happy
(e.g. some curly braces were removed where the body become a one
liner).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Use qemuBuildControllersCommandLine since it builds the command line
for (nearly) all controllers, not just one.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
Now that the inner loop does not require any other variables,
it can be easily separated. Apart from reducing the indentation
level this will allow it to be called from different code paths.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
Now that it's no longer needed, remove the argument.
This removes the last helper variable in
qemuBuildControllerDevCommandLine.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
qemuBuildLegacyUSBControllerCommandLine is the only place where
we need to count the USB controllers.
Count them again instead of keeping track in a variable passed to
qemuBuildControllerDevStr.
This removes the need for another variable in the loop in
qemuBuildControllerDevCommandLine.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
Count them in qemuBuildLegacyUSBControllerCommandLine to remove
yet another variable accessed from the loop in
qemuBuildControllerDevCommandLine.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
This removes the need to mark it in the 'usbcontroller' variable.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
Move out the code formatting "-usb" on the QEMU command line.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
Switch qemuBuildVirtioDevStr to use virDomainDeviceSetData: callers
pass in the virDomainDeviceType and the void * DefPtr. This will
save us from having to repeatedly extend the function argument
list in subsequent patches.
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
Current code essentially duplicates the same logic, but misses
some cases (like vhost-vsock-device).
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
The vhost-scsi device string should depend on the requested
address type, not strictly on the emulated arch. This is the
same logic used by qemuBuildVirtioDevStr, and this particular
path is already tested in the hostdev-scsi-vhost-scsi-ccw tests
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
Move the rng->model == VIRTIO check to parse time. This also
allows us to remove similar checks throughout the qemu driver
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
If we validate that memballoon is NONE|VIRTIO at parse time,
we can drop similar checks elsewhere in the qemu driver
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
This will be extended in the future, so let's simplify things by
centralizing the checks.
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
Asserting the value we set four lines earlier in qemuBlockjobState
doesn't buy us any safety (if the public header adds a value, we end
up skipping that value without the compiler warning us of our gap);
what we really want is to assert that the value auto-assigned by the
compiler matches the actual last value in the public headers (as was
done below for qemuBlockJobType). Add useful comments while at it.
Signed-off-by: Eric Blake <eblake@redhat.com>
ACKed-by: Peter Krempa <pkrempa@redhat.com>
Hanlde all the possible failure codes as per ACPI standard documented in
the function header.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1660410
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
We forgot to document the specific fields for the 0x103 and 0x200
sources which are tied to device removal and device hotplug
respectively.
The value description is based on the ACPI 6.2A standard Table 6-207 and
Table 6-208. At the time of writing of this patch the standard can be
accessed e.g. at:
https://www.uefi.org/sites/default/files/resources/ACPI%206_2_A_Sept29.pdf
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The device xml parser code does not set "model" while parsing the
following XML:
<interface type='hostdev'>
<source>
<address type='pci' domain='0x0002' bus='0x01' slot='0x00' function='0x2'/>
</source>
</interface>
The net->model can be NULL and therefore must be compared using
STREQ_NULLABLE instead of plain STREQ.
Fixes: ac47e4a622 (qemu: replace "def->nets[i]" with "net" and "def->sounds[i]" with "sound")
Fixes: c7fc151eec (qemu: assign virtio devices to PCIe slot when appropriate)
Signed-off-by: Radoslaw Biernacki <radoslaw.biernacki@linaro.org>
Signed-off-by: dann frazier <dann.frazier@canonical.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Add a capability check to qemuDomainDefValidate and refuse to start
a domain with VNC graphics if the TLS secret was set in qemu.conf
and it's not supported.
Note that qemuDomainSecretGraphicsPrepare does not generate any
secret data if the capability is not present and qemuBuildTLSx509BackendProps
is not called at all.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Use the password stored in the secret driver under
the uuid specified by the vnc_tls_x509_secret_uuid
option in qemu.conf.
https://bugzilla.redhat.com/show_bug.cgi?id=1602418
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Add an option that lets the user specify the secret
that unlocks the server TLS key.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Be generic instead of trying to enumerate all the involved
device types.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Instead of hardcoding the TLS creds alias in
qemuBuildGraphicsVNCCommandLine, store it
in the domain private data.
Given that we only support one VNC graphics
and thus have only one alias per-domain,
this is overengineered, but it will allow us
to prepare the secret upfront when we start
supporting encrypted server TLS keys.
Note that the alias is not formatted anywhere
since we won't need to access it after domain
startup.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: John Ferlan <jferlan@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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
In many files there are header comments that contain an Author:
statement, supposedly reflecting who originally wrote the code.
In a large collaborative project like libvirt, any non-trivial
file will have been modified by a large number of different
contributors. IOW, the Author: comments are quickly out of date,
omitting people who have made significant contribitions.
In some places Author: lines have been added despite the person
merely being responsible for creating the file by moving existing
code out of another file. IOW, the Author: lines give an incorrect
record of authorship.
With this all in mind, the comments are useless as a means to identify
who to talk to about code in a particular file. Contributors will always
be better off using 'git log' and 'git blame' if they need to find the
author of a particular bit of code.
This commit thus deletes all Author: comments from the source and adds
a rule to prevent them reappearing.
The Copyright headers are similarly misleading and inaccurate, however,
we cannot delete these as they have legal meaning, despite being largely
inaccurate. In addition only the copyright holder is permitted to change
their respective copyright statement.
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Support for nested KVM is handled via a kernel module configuration
parameters values for kvm_intel, kvm_amd, kvm_hv (PPC), or kvm (s390).
While it's possible to fetch the kmod config values via virKModConfig,
unfortunately that is the static value and we need to get the
current/dynamic value from the kernel file system.
So this patch adds a new API virHostKVMSupportsNesting that will
search the 3 kernel modules to get the nesting value and check if
it is 'Y' (or 'y' just in case) to return a true/false whether
the KVM kernel supports nesting.
We need to do this in order to handle cases where adjustments to
the value are made after libvirtd is started to force a refetch of
the latest QEMU capabilities since the correct CPU settings need
to be made for a guest to add the "vmx=on" to/for the guest config.
Signed-off-by: John Ferlan <jferlan@redhat.com>
ACKed-by: Michal Privoznik <mprivozn@redhat.com>
https://bugzilla.redhat.com/show_bug.cgi?id=1624223
There are two ways to request memory preallocation on cmd line:
-mem-prealloc and .prealloc attribute for a memory-backend-file.
However, as it turns out it's not safe to use both at the same
time. If -mem-prealloc is used then qemu will fully allocate the
memory (this is done by actually touching every page that has
been allocated). Then, if .prealloc=yes is specified,
mbind(flags = MPOL_MF_STRICT | MPOL_MF_MOVE) is called which:
a) has to (possibly) move the memory to a different NUMA node,
b) can have no effect when hugepages are in play (thus ignoring user
request to place memory on desired NUMA nodes).
Prefer -mem-prealloc as it is more backward compatible
compared to switching to "-numa node,memdev= + -object
memory-backend-file".
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
So far we have two arguments that we are passing to
qemuBuildMemoryBackendProps() and that are taken from domain
private data: @qemuCaps and @autoNodeset. In the next commit I
will use one more item from there. Therefore, instead of having
it as yet another argument to the function, pass pointer to the
private data object.
There is one change in qemuDomainAttachMemory() where previously
@autoNodeset was NULL but now is priv->autoNodeset (which may be
set). This is safe to do as @autoNodeset is advisory only.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Since the code was never run, it would have been very hard to spot this
mistake, especially since the compiler can't really warn about it.
Signed-off-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Disable external snapshot of a readonly disk for domains as
this operation is not very useful. Such a snapshot is not
possible for active domains but the error message from QEMU
is more cryptic:
error: internal error: unable to execute QEMU command 'transaction':
Could not create file: Permission denied
This error at least makes the error more understandable for
active domains and disallows for inactive domains as well.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Commit 212dc9286 made a generic qemuDomainGetIOThreadsMon which
would fail if the QEMU_CAPS_OBJECT_IOTHREAD didn't exist. Then
commit d1eac927 used that helper for the collection of all domain
stats. However, if the capability doesn't exist, then the entire
stats collection fails. Since the IOThread stats were meant to be
if available only, thus rather than failing if the capability
doesn't exist, let's just not collect the stats. Restore the caps
failure logic for qemuDomainGetIOThreadsLive.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
During qemuConnectGetAllDomainStats if qemuDomainGetStats causes
a failure, then when collecting more than one domain's worth of
statistics the loop in virDomainStatsRecordListFree would call
virDomainFree which would call virResetLastError effectively wiping
out the reason we failed leaving the caller with no idea why the
collection failed.
To fix this, let's Preserve the error and Restore it prior to return
so that a caller such as 'virsh domstats' doesn't get the generic
"error: An error occurred, but the cause is unknown".
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
We are preparing a certain disk source passed in as '@src' so the
individual functions should use that rather than disk->src which
corresponds to the top level element of the chain only.
Without this change TLS and persistent reservations would not work for
backing images of a chain when using -blockdev.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The function clears and frees the passed buffers on success, but not in
one case of failure. Modify the control flow that the args are always
consumed, record it in the docs and remove few pointless cleanup paths
in callers.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
https://bugzilla.redhat.com/show_bug.cgi?id=1656014
An RNG device can consists of more devices than RND device
itself. For instance, in case of EGD there is a chardev that
connects to EGD daemon and feeds the qemu with random data. When
doing RNG device removal we have to remove the associated chardev
as well.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The way that the code is currently written makes my eyes hurt.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
There are two functions called from syncNicRxFilterMultiMode:
virNetDevSetRcvAllMulti() and virNetDevSetRcvMulti(). Both of
them return 0 on success and -1 on error. However, currently
their return value is checked for != 0 which conflicts with our
assumptions on retvals: a positive value is still considered
success but with current check it would lead to failure.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Depending on whether QEMU actually supports the option, we can put the
'rendernode' on the '-display egl-headless' cmdline.
https://bugzilla.redhat.com/show_bug.cgi?id=1628892
Signed-off-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Just like for SPICE, we need to put the render node DRI device into the
device cgroup list so that users don't need to add it manually via
qemu.conf file.
Signed-off-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Just like for SPICE, we need to put the DRI device into the namespace,
otherwise it will be left out from the DAC relabeling process.
Signed-off-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Unlike with SPICE and SDL which use the <gl> subelement to enable OpenGL
acceleration, specifying egl-headless graphics in the XML has
essentially the same meaning, thus in case of egl-headless we don't have
a need for the 'enable' element attribute and we'll only be interested
in the 'rendernode' one further down the road.
Signed-off-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Now that we have QAPI introspection of display types in QEMU upstream,
we can check whether the 'rendernode' option is supported with
egl-headless display type.
Signed-off-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
We're going to need a bit more logic for egl-headless down the road so
prepare a helper just like for the other display types.
Signed-off-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Up until now, we formatted 'rendernode=' onto QEMU cmdline only if the
user specified it in the XML, otherwise we let QEMU do it for us. This
causes permission issues because by default the /dev/dri/renderDX
permissions are as follows:
crw-rw----. 1 root video
There's literally no reason why it shouldn't be libvirt picking the DRM
render node instead of QEMU, that way (and because we're using
namespaces by default), we can safely relabel the device within the
namespace.
Signed-off-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Guest network devices can set 'overflow' when there are a number of multicast
ips configured. For virtio_net, the limit is only 64. In this case, the list
of mac addresses is empty and the 'overflow' condition is set. Thus, the guest
will currently receive no multicast traffic in this state.
When 'overflow' is set in the guest, let's turn this into ALLMULTI on the host.
Signed-off-by: Jason Baron <jbaron@akamai.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
Acked-by: Michal Privoznik <mprivozn@redhat.com>
Support for armv6l qemu guests has been added.
Tested with arm1176 CPU on x86.
Signed-off-by: Stefan Schallenberg <infos@nafets.de>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Post-copy migration has been broken on the source since commit
v3.8.0-245-g32c29f10db which implemented support for
pause-before-switchover QEMU migration capability.
Even though the migration itself went well, the source did not really
know when it switched to the post-copy mode despite the messages logged
by MIGRATION event handler. As a result of this, the events emitted by
source libvirtd were not accurate and statistics of the completed
migration would cover only the pre-copy part of migration. Moreover, if
migration failed during the post-copy phase for some reason, the source
libvirtd would just happily resume the domain, which could lead to disk
corruption.
With the pause-before-switchover capability enabled, the order of events
emitted by QEMU changed:
pause-before-switchover
disabled enabled
MIGRATION, postcopy-active STOP
STOP MIGRATION, pre-switchover
MIGRATION, postcopy-active
The STOP even handler checks the migration status (postcopy-active) and
sets the domain state accordingly. Which is sufficient when
pause-before-switchover is disabled, but once we enable it, the
migration status is still active when we get STOP from QEMU. Thus the
domain state set in the STOP handler has to be corrected once we are
notified that migration changed to postcopy-active.
This results in two SUSPENDED events to be emitted by the source
libvirtd during post-copy migration. The first one with
VIR_DOMAIN_EVENT_SUSPENDED_MIGRATED detail, while the second one reports
the corrected VIR_DOMAIN_EVENT_SUSPENDED_POSTCOPY detail. This is
inevitable because we don't know whether migration will eventually
switch to post-copy at the time we emit the first event.
https://bugzilla.redhat.com/show_bug.cgi?id=1647365
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Both VIR_DOMAIN_FEATURE_HPT and VIR_DOMAIN_FEATURE_HTM are
handled in the exact same way, so we can remove some duplicated
code without losing any functionality.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
If migration is cancelled or confirm phase fails the domain
should be kept on the source even if VIR_MIGRATE_UNDEFINE_SOURCE
was requested.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
There are some checks done when parsing a migration cookie. For
instance, one of the checks ensures that the domain is not being
migrated onto the same host. If that is the case, then we are in
big trouble because the @vm is the same domain object used by
source and it has some jobs sets and everything so recovering
from failed cookie parsing would be needlessly hard.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
