26127 Commits

Author SHA1 Message Date
Jiri Denemark
d802d9ab83 qemu: Fix migration without parameters
The virTypedParamsFilter function doesn't mind params == NULL if nparams
is zero. And there's no need to check for params == NULL && nparams > 0
because this is checked higher in the stack.

In fact all the virCheckNonNull* checks in virTypedParamsFilter are
useless.

https://bugzilla.redhat.com/show_bug.cgi?id=1777094

Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
2019-11-28 14:27:11 +01:00
Peter Krempa
d310b08697 qemu: blockjob: Finish handling job with broken data
Now that we have a separate job type which will not trigger normal code
paths for terminating job we can remove the ad-hoc handling.

This possibly fixes the issue of a broken job inheriting the disk and
then finishing in which case we'd not detach the backing chain.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
2019-11-27 15:59:33 +01:00
Peter Krempa
e67e8c545a qemu: blockjob: Introduce "broken" block job type
To better track jobs we couldn't parse let's introduce a new job type
which will clarify semantics internally in few places.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
2019-11-27 15:59:33 +01:00
Peter Krempa
2624c6d2d2 qemu: blockjob: Separate clearing of per-job data
We will need to clear per-job type data when we will be marking a
blockjob as broken in the new way. Extract the code for future reuse.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
2019-11-27 15:59:33 +01:00
Peter Krempa
0cb3061943 qemu: blockjob: Don't stop processing the finished job early
Both failure to refresh and to dismiss the job are very unlikely but if
they happen there's not much we can do about the blockjob.

The concluded job handlers treat it as if the job failed if we don't
update the state to 'QEMU_BLOCKJOB_STATE_COMPLETED' which is probably
the safest thing to do here.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
2019-11-27 15:59:33 +01:00
Peter Krempa
5b29c7dc27 qemu: blockjob: Mark job with broken data but tracked by qemu as reconnected
Otherwise it would get dropped later on as untracked despite us knowing
about it. Additionally since we cancelled it we must wait to dismiss it
which would not be possible if we unregister it. This also opened a
window for a race condition since the job state change event of the
just-cancelled job might be delivered prior to us unregistering the job
in which case everything would work properly.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
2019-11-27 15:59:33 +01:00
Peter Krempa
f15d4cb1c8 qemu: blockjob: Log blockjobs which are dropped when untracked by qemu
Since we don't know what happened to the job we can't do much about it
but we can at least log that this happened.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
2019-11-27 15:59:33 +01:00
Peter Krempa
8622498f6e qemu: blockjob: Fix deadlock when terminating job with invalid data
We must exit the monitor prior to refusing other work, otherwise the VM
object will become unusable.

This bug was introduced in commit v5.5.0-244-gc412383796 but thankfully
the code path was not excercised without QEMU_CAPS_BLOCKDEV.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
2019-11-27 15:59:33 +01:00
Peter Krempa
ccde9ca1f4 qemu: process: Move block job refresh after async job recovery
Block jobs may be members of async jobs so it makes more sense to
refresh block job state after we do steps for async job recovery.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
2019-11-27 15:59:33 +01:00
Peter Krempa
a887da529c qemu: blockjob: Properly propagate cancellation of blockjobs
qemu returns an error message in the job statistics even if the job was
cancelled to emphasize it was not successful. Libvirt didn't properly
transform it into QEMU_BLOCKJOB_STATE_CANCELLED though.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
2019-11-27 15:59:33 +01:00
Peter Krempa
01270a9a5f qemu: monitor: Finish implementation of infrastructure for 'query-jobs'
Commit ed56851f1bc6f5 didn't wire up fetching of the statistics for the
job which are reported by 'query-jobs'.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
2019-11-27 15:59:33 +01:00
Daniel P. Berrangé
27660f7990 util: whitelist Oracle ACFS as a shared filesystem
The magic number is taken from the coreutils stat.c file since
there is no constant for it in normal system headers.

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
2019-11-27 14:19:57 +00:00
Daniel P. Berrangé
6881ef2e4e libxl: fix typo in error message
Reviewed-by: Fabiano Fidêncio <fidencio@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
2019-11-27 14:19:41 +00:00
Peter Krempa
a10eb61340 Revert "qemu: Don't repeat virDomainObjEndAPI in qemuDomainBlockPull"
This reverts commit 421c9550f5446729b513ee50f5c44e6f6969b5a2

qemuDomainBlockPullCommon calls virDomainObjEndAPI internally so the
original commit made us shed two references of @vm instead of one
getting us into a premature free of @vm.

This is not a straight revert as qemuDomainBlockPull was modified
meanwhile. I've also added a warning comment that @vm is consumed.

https://bugzilla.redhat.com/show_bug.cgi?id=1777230

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
2019-11-27 14:19:50 +01:00
Michal Privoznik
c8579871a9 all: don't wait for driver lock during startup
There are two daemons that wait for acquiring their pid files:
virtnetworkd and virtstoraged. This is undesirable as the idea
is to quit early if unable to acquire the pid file.

Fixes: v5.6.0-rc1~207.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
2019-11-26 15:46:47 +01:00
Laine Stump
9d6920bd7d net/qemu: move vlan/bandwidth validation out of network driver
In the past the network driver was (mistakenly) being called for all
interfaces, not just those of type='network', and so it had a chance
to validate all interface configs after the actual type of the
interface was known.

But since the network driver has been more completely/properly
separated from qemu, the network driver isn't called during the
startup of any interfaces except those with type='network', so this
validation no longer takes place for, e.g. <interface type='bridge'>
(or direct, etc). This in turn meant that a config could erroneously
specify a vlan tag, or bandwidth settings, for a type of interface
that didn't support it, and the domain would start without complaint,
just silently ignoring those settings.

This patch moves those validation checks out of the network driver,
and into virDomainActualNetDefValidate() so they will be done for all
interfaces, not just type='network'.

https://bugzilla.redhat.com/1741121
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
2019-11-25 15:30:10 -05:00
Laine Stump
b03d9e9593 conf: add hypervisor agnostic, domain start-time, validation function for NetDef
<interface> devices (virDomainNetDef) are a bit different from other
types of devices in that their actual type may come from a network (in
the form of a port connection), and that doesn't happen until the
domain is started. This means that any validation of an <interface> at
parse time needs to be a bit liberal in what it accepts - when
type='network', you could think that something is/isn't allowed, but
once the domain is started and a port is created by the configured
network, the opposite might be true.

To solve this problem hypervisor drivers need to do an extra
validation step when the domain is being started. I recently (commit
3cff23f7, libvirt 5.7.0) added a function to peform such validation
for all interfaces to the QEMU driver -
qemuDomainValidateActualNetDef() - but while that function is a good
single point to call for the multiple places that need to "start" an
interface (domain startup, device hotplug, device update), it can't be
called by the other hypervisor drivers, since 1) it's in the QEMU
driver, and 2) it contains some checks specific to QEMU. For
validation that applies to network devices on *all* hypervisors, we
need yet another interface validation function that can be called by
any hypervisor driver (not just QEMU) right after its network port has
been created during domain startup or hotplug. This patch adds that
function - virDomainActualNetDefValidate(), in the conf directory,
and calls it in appropriate places in the QEMU, lxc, and libxl
drivers.

This new function is the place to put all network device validation
that 1) is hypervisor agnostic, and 2) can't be done until we know the
"actual type" of an interface.

There is no framework for validation at domain startup as there is for
post-parse validation, but I don't want to create a whole elaborate
system that will only be used by one type of device. For that reason,
I just made a single function that should be called directly from the
hypervisors, when they are initializing interfaces to start a domain,
right after conditionally allocating the network port (and regardless
of whether or not that was actually needed). In the case of the QEMU
driver, qemuDomainValidateActualNetDef() is already called in all the
appropriate places, so we can just call the new function from
there. In the case of the other hypervisors, we search for
virDomainNetAllocateActualDevice() (which is the hypervisor-agnostic
function that calls virNetworkPortCreateXML()), and add the call to our
new function right after that.

The new function itself could be plunked down into many places in the
code, but we already have 3 validation functions for network devices
in 2 different places (not counting any basic validation done in
virDomainNetDefParseXML() itself):

1) post-parse hypervisor-agnostic
   (virDomainNetDefValidate() - domain_conf.c:6145)
2) post-parse hypervisor-specific
   (qemuDomainDeviceDefValidateNetwork() - qemu_domain.c:5498)
3) domain-start hypervisor-specific
   (qemuDomainValidateActualNetDef() - qemu_domain.c:5390)

I placed (3) right next to (2) when I added it, specifically to avoid
spreading validation all over the code. For the same reason, I decided
to put this new function right next to (1) - this way if someone needs
to add validation specific to qemu, they go to one location, and if
they need to add validation applying to everyone, they go to the
other. It looks a bit strange to have a public function in between a
bunch of statics, but I think it's better than the alternative of
further fragmentation. (I'm open to other ideas though, of course.)

Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
2019-11-25 15:30:05 -05:00
Laine Stump
012624217e conf: change args/return values of remaining virDomainNetGetActual*() to const
These all just return a scalar value, so there's no daisy-chained
fallout from changing them, and they can easily be combined in a
single patch.

Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
2019-11-25 15:30:00 -05:00
Laine Stump
fdcd273be2 conf: return a const from virDomainNetGetActualVirtPortProfile
This also isn't required (due to the vportprofile being stored in the
NetDef as a pointer rather than being directly contained), but it
seemed dishonest to not mark it as const (and thus permit users to
modify its contents)

Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
2019-11-25 15:29:56 -05:00
Laine Stump
583ac17f5d conf: make virDomainNetGetActualBandwidth arg/return value const
In this case, the virNetDevBandwidthPtr that is returned is not to a
region within the virDomainNetDef arg, but points elsewhere (the
NetDef has the pointer, not the entire object), so technically it's
not necessary to make the return value a const, but it's a bit
disingenuous to *not* do it.

Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
2019-11-25 15:29:51 -05:00
Laine Stump
12207fcfcf conf: make virDomainNetGetActualVlan arg/return val const
This is needed if we want to call the function when the
virDomainNetDef* we have is a const.

Since virDomainNetGetActualVlan returns a pointer to memory that is
within the virDomainNetDefPtr arg, the returned pointer must also be
made const. This leads to a cascade of other virNetDevVlanPtr's that
must be changed to "const virNetDevVlan *".

Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
2019-11-25 15:29:46 -05:00
Laine Stump
1b029a929d qemu: add mac address to error messages in qemuDomainValidateActualNetDef
This makes it easier to understand which interface's config caused the
error.

Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
2019-11-25 15:29:41 -05:00
Michal Privoznik
9b1d53d417 qemu_capabilities: Use proper free function for caps->cpuModels
The cpuModels member of _virQEMUCapsAccel struct is not a
virObject but regular struct with a free function defined:
qemuMonitorCPUDefsFree(). Use that when clearing parent structure
instead of virObjectUnref() to avoid a memleak:

==212322== 57,275 (48 direct, 57,227 indirect) bytes in 3 blocks are definitely lost in loss record 623 of 627
==212322==    at 0x4838B86: calloc (vg_replace_malloc.c:762)
==212322==    by 0x554A158: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.6000.6)
==212322==    by 0x17B14BF5: qemuMonitorCPUDefsNew (qemu_monitor.c:3587)
==212322==    by 0x17B27BA7: qemuMonitorJSONGetCPUDefinitions (qemu_monitor_json.c:5616)
==212322==    by 0x17B14B0B: qemuMonitorGetCPUDefinitions (qemu_monitor.c:3559)
==212322==    by 0x17A6AFBB: virQEMUCapsFetchCPUDefinitions (qemu_capabilities.c:2571)
==212322==    by 0x17A6B2CC: virQEMUCapsProbeQMPCPUDefinitions (qemu_capabilities.c:2629)
==212322==    by 0x17A70C00: virQEMUCapsInitQMPMonitorTCG (qemu_capabilities.c:4769)
==212322==    by 0x17A70DDF: virQEMUCapsInitQMPSingle (qemu_capabilities.c:4820)
==212322==    by 0x17A70E99: virQEMUCapsInitQMP (qemu_capabilities.c:4848)
==212322==    by 0x17A71044: virQEMUCapsNewForBinaryInternal (qemu_capabilities.c:4891)
==212322==    by 0x17A7119C: virQEMUCapsNewData (qemu_capabilities.c:4923)

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
2019-11-25 17:13:05 +01:00
Jiri Denemark
24d8202294 qemu: Use host-model CPU on s390 by default
On s390 machines host-passthrough and host-model CPUs result in the same
guest ABI (with QEMU new enough to be able to tell us what "host" CPU is
expanded to, which was implemented around 2.9.0). So instead of using
host-passthrough CPU when there's no CPU specified in a domain XML we
can safely use host-model and benefit from CPU compatibility checks
during migration, snapshot restore and similar operations.

Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
2019-11-25 15:29:19 +01:00
Jiri Denemark
0a24331c6e cpu_s390: Don't check match attribute for host-model CPUs
The match attribute is only relevant for custom mode CPUs. Reporting
failure when match == 'minimum' regardless on CPU mode can cause
unexpected failures. We should only report the error for custom CPUs. In
fact, calling virCPUs390Update on a custom mode CPU should always report
an error as optional features are not supported on s390 either.

Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
2019-11-25 15:29:19 +01:00
Jiri Denemark
af8e39921a cpu_conf: Don't format empty model for host-model CPUs
Most likely for historical reasons our CPU def formatting code is
happily adding useless <model fallback='allow'/> for host-model CPUs. We
can just drop it.

Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
2019-11-25 15:29:19 +01:00
Jiri Denemark
65fa7bba1a cpu_conf: Fix default value for CPU match attribute
Commit v0.8.4-66-g95ff6b18ec (9 years ago) changed the default value for
the cpu/@match attribute to 'exact' in a rather complicated way. It did
so only if <model> subelement was present and set -1 otherwise (which is
not expected to ever happen). Thus the following two equivalent XML
elements:

    <cpu mode='host-model'/>

and

    <cpu mode='host-model'>
      <model/>
    </cpu>

would be parsed differently. The former would end up with match == -1
while the latter would have match == 1 ('exact'). This is not a big deal
since the match attribute is ignored for host-model CPUs, but we can
simplify the code and make it a little bit saner anyway.

Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
2019-11-25 15:29:19 +01:00
Pavel Mores
33a9757852 qemu: the actual change of default video devide type selection algorithm
If a graphics device was added to XML that had no video device, libvirt
automatically added a video device which was always of type 'cirrus' on
x86_64, even if the underlying qemu didn't support cirrus.

This patch refines a bit the decision about the type of the video device.
Based on QEMU capabilities, cirrus is still preferred but only added if
QEMU supports it, otherwise VGA is used if supported by QEMU.  There is now
no fallback as libvirt only aspires to generate a basic working config and
leaves anything more specific up to higher-level management tools.

Reviewed-by: Cole Robinson <crobinso@redhat.com>
Signed-off-by: Pavel Mores <pmores@redhat.com>
2019-11-25 08:47:08 -05:00
Pavel Mores
b648d96289 qemu: default video device type selection algoritm moved into its own function
The default video device type selection algorithm we're about to deploy will
increase the amount of code dedicated to the task by amount enough to warrant
factoring the whole thing into its own function so as not to pollute the
caller qemuDomainDeviceVideoDefPostParse().  Do it now so that the actual
algorithm change later on is in a clean commit by itself and easy to review.

Reviewed-by: Cole Robinson <crobinso@redhat.com>
Signed-off-by: Pavel Mores <pmores@redhat.com>
2019-11-25 08:47:08 -05:00
Erik Skultety
36a01c2a47 Revert "network: Check for QOS before blindly using it"
This reverts commit f4db846c32c0a1e99a0f62b340273e48f8a98ed3.

This patch results in the following error when trying to start
essentially any VM with default network:

unsupported configuration: QOS must be defined for network 'default'

Coverity didn't see that the bandwidth == NULL it complained about in
virNetDevBandwidthPlug was already checked properly in
networkCheckBandwidth, thus causing networkPlugBandwidth to return 0
and finish before a call to virNetDevBandwidthPlug would have been even
made.

Signed-off-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
2019-11-25 09:41:40 +01:00
Daniel P. Berrangé
bc7e72914a util: consolidate on one free callback for hash data
This previous commit introduced a simpler free callback for
hash data with only 1 arg, the value to free:

  commit 49288fac965f0ee23db45d83ae4ef3a9a71dafd0
  Author: Peter Krempa <pkrempa@redhat.com>
  Date:   Wed Oct 9 15:26:37 2019 +0200

    util: hash: Add possibility to use simpler data free function in virHash

It missed two functions in the hash table code which need
to call the alternate data free function, virHashRemoveEntry
and virHashRemoveSet.

After the previous patch though, there is no code that
makes functional use of the 2nd key arg in the data
free function. There is merely one log message that can
be dropped.

We can thus purge the current virHashDataFree callback
entirely, and rename virHashDataFreeSimple to replace
it.

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
2019-11-22 14:21:28 +00:00
Daniel P. Berrangé
feef23e130 conf: stop using hash key when free'ing hash entries
The virChrdevHashEntryFree method uses the hash 'key'
as the name of the logfile it has to remove. By storing
a struct as the value which contains the stream and
the dev path, we can avoid relying on the hash key
when free'ing entries.

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
2019-11-22 14:21:16 +00:00
Peter Krempa
c6a9e54ce3 qemu: enable blockdev support
Now that all pieces are in place (hopefully) let's enable -blockdev.

We base the capability on presence of the fix for 'auto-read-only' on
files so that blockdev works properly, mandate that qemu supports
explicit SCSI id strings to avoid ABI regression and that the fix for
'savevm' is present so that internal snapshots work.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
2019-11-22 12:51:27 +01:00
Peter Krempa
5b4b503be6 qemu: capabilities: Add detection of the 'savevm' fix for -blockdev
The 'savevm' HMP command didn't work properly with blockdev as it tried
to do snapshot of everything including the protocol nodes accessing
files which are not snapshottable. Qemu fixed this bug so now we need to
detect it to allow enabling blockdev.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
2019-11-22 12:51:27 +01:00
Peter Krempa
e0300f92fc qemu: qapi: Add support for command features
The top level commands now can have 'feature' flags for fixes so add
support for querying those as well.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
2019-11-22 12:51:27 +01:00
Peter Krempa
3460fef5a0 qemu: caps: Add capability for dynamic 'auto-read-only' support for files
Initial implementation of 'auto-read-only' didn't reopen the backing
files when needed. For '-blockdev' to work we need to be able to tel
qemu to open a file read-only and change it during blockjobs as we label
backing chains with a sVirt label which does not allow writing. The
dynamic auto-read-only supports this as it reopens files when writing
is demanded.

Add a capability to detect that the posix file based backends support
the dynamic part.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
2019-11-22 12:51:27 +01:00
Peter Krempa
48e57cd632 qemu: caps: Base support of 'backingStoreInput' domain feature on QEMU_CAPS_BLOCKDEV
The qemu driver will obey <backingStore> when we support blockdev.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
2019-11-22 12:51:27 +01:00
Peter Krempa
757203ee52 conf: domcaps: Add 'backingStoreInput' domain capability
Historically we've only supported the <backingStore> as an output-only
element for domain disks. The documentation states that it may become
supported on input. To allow management apps detectin once that happens
add a domain capability which will be asserted if the hypervisor driver
will be able to obey the <backingStore> as configured on input.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
2019-11-22 12:51:27 +01:00
Michal Privoznik
8fa0374c5b qemuProcessStop: Remove image metadata for running mirror jobs
If user starts a blockcommit or a blockcopy then we modify access
for qemu on both images and leave it like that until the job
terminates.  So far so good. Problem is, if user instead of
terminating the job (where we would modify the access again so
that the state before the job is restored) calls destroy on the
domain or if qemu dies whilst executing the block job.  In this
case we don't ever clear the access we granted at the beginning.
To fix this, maybe a bit harsh approach is used, but it works:
after all labels were restored (that is after
qemuSecurityRestoreAllLabel() was called), we iterate over each
disk in the domain and remove XATTRs from the whole backing chain
and also from any file the disk is being mirrored to.

This would have been done at the time of pivot, but it isn't
because user decided to kill the domain instead. If we don't do
this and leave some XATTRs behind the domain might be unable to
start.

Also, secdriver can't do this because it doesn't know if there is
any job running. It's outside of its scope - the hypervisor
driver is responsible for calling secdriver's APIs.

Moreover, this is safe to call because we don't remember labels
for any member of a backing chain except of the top layer. But
that one was restored in qemuSecurityRestoreAllLabel() call done
earlier. Therefore, not only we don't remember labels (and thus
this is basically a NOP for other images in the backing chain) it
is also safe to call this when no blockjob was started in the
first place, or if some parts of the backing chain are shared
with some other domains - this is NOP, unless a block job is
active at the time of domain destroy.

https://bugzilla.redhat.com/show_bug.cgi?id=1741456#c19

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
2019-11-22 10:48:14 +01:00
Michal Privoznik
1c12b86185 qemu: Separate image metadata removal into a function
There are four places where we remove image XATTRs and in all of
them we have the same for() loop with the same body. Move it into
a separate function because I'm about to introduce fifth place
where the same needs to be done.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
2019-11-22 10:48:04 +01:00
Peter Krempa
86085c9a2f qemu: Instantiate pflash via -machine when using blockdev
Install the convertor function which enables the internals that will use
-blockdev to make qemu open the firmware image and stop using -drive.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
2019-11-22 08:32:25 +01:00
Peter Krempa
c8eb99eebf qemu: command: Build the 'pflash' drives via -machine
The old way to instantiate a pflash device via -drive was a hack since
it's a platform device.

The modern approach calls for configuring it via -machine and takes the
node name as an argument.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
2019-11-22 08:32:25 +01:00
Peter Krempa
7d2f942af9 qemu: command: Build -blockdev-s for backing of pflash
As a first step we will build the blockdevs which will be supposed to
back the pflash drives when moving away from -drive.

This code is similar to the way we build the blockdevs for the disk, but
skips the copy-on-read layer and doesn't implement any legacy approach.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
2019-11-22 08:32:25 +01:00
Peter Krempa
11d13ad8cf qemu: domain: Introduce helper to convert <loader> into virStorageSource
Add a helper which will covert the PFLASH code file and variable file
into the virStorageSource objects stored in private data so that we can
use them with -blockdev while keeping the infrastructure to determine
the path to the loaders intact.

This is a temporary solution until we will want to do snapshots of the
pflash where we will be forced do track the full backing chain in the
XML.

In the meanwhile just convert it partially so that we can stop using
-drive.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
2019-11-22 08:32:25 +01:00
Peter Krempa
07675b0100 qemu: domain: Store virStorageSources representing pflash backing
To allow converting the pflash drives to blockdev we will need a
virStorageSource to allow using our helpers. Temporarily prior to
coverting loader data to a virStorageSoruce add private data which will
house this.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
2019-11-22 08:32:25 +01:00
Peter Krempa
316223b6ad qemu: command: Extract formatting of -drive for pflash
Extract the old way to instantiate pflash devices to hold the firmware
via -drive to a separate function so that it can later be conditionally
disabled when -blockdev will be used.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
2019-11-22 08:32:23 +01:00
Peter Krempa
c78fadb57c domcaps: Remove function initializing domain caps as unsupported
Commit 5751a0b6b1968bb2354b2ac21cc5938b93009590 added a helper function
called virDomainCapsFeaturesInitUnsupported which initialized all domain
capability features as unsupported.

When adding a new feature this would initialize it as unsupported also
for hypervisor drivers which the original author possibly didn't intend
to modify. To prevent accidental wrong value being reported in such case
revert back to initializing individual features in the hypervisor
drivers themselves.

This is not a straight revert as additonal patches modified how we store
the capabilities.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
2019-11-22 08:16:51 +01:00
Erik Skultety
d6064e2759 libvirt-<module>: Check caller-provided buffers to be NULL with size > 0
Pre-Glib era which used malloc allowed the size of the client-side
buffers to be declared as 0, because malloc documents that it can either
return 0 or a unique pointer on 0 size allocations.
With glib this doesn't work anymore, because glib documents that for
such allocation requests NULL is always returned which results in an
error in our public API checks server-side.
This patch complements the fix in the RPC layer by explicitly erroring
out on the following combination of args used by our legacy APIs (their
moder equivalents don't suffer from this):

function(caller-allocated-array, size, ...) {
    if (!caller-allocated-array && size > 0)
        return error;
}

treating everything else as a valid input and potentially let that fail
on the server-side rather than client-side.

https://bugzilla.redhat.com/show_bug.cgi?id=1772842

Signed-off-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
2019-11-21 18:16:35 +01:00
Erik Skultety
bf2988235c rpc: gendispatch: Fix a couple of places adding trailing spaces
Signed-off-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
2019-11-21 18:16:18 +01:00
Ján Tomko
5e067ba4e1 conf: remove NULL check from virDomainQemuMonitorEventNew
The qemu_domain_monitor_event_msg struct in qemu_protocol.x
defines event as a nonnull_string and qemuMonitorJSONIOProcessEvent
also errors out on a non-NULL event.

Drop the check to fix the build with static analysis.

This essentially reverts commit d343e8203d248635f5e7a0d86dd077f74d49e3af

Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
2019-11-21 12:44:23 +01:00