Introduced in c8007fdc5d, it should use 'greater than max' instead of
'equal or greater than max' for the condition of checking invalid scsi
unit.
Signed-off-by: Han Han <hhan@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
If we static link to libvirt_util.la then we can't override functions in
this file by simply implementing them in the test code. Any tests should
dynamic link to the main libvirt.la and ensure symbols are exported.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
We clear some capabilities here so the lockouts need to be
re-evaluated.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
Blockdev is required to do incremental backups properly. Add a helper
function for locking out capabilities and export it to allow re-doing
the processing if a different code path modifies capabilities.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
Checking whether a qemu capability set right before clearing it without
any other logic doesn't make sense.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
Do all post-processing of capabilities in qemuProcessPrepareQEMUCaps.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
Move the post-processing of the QEMU_CAPS_CHARDEV_FD_PASS flag to the
new function.
The clearing of the capability is based on the presence of
VIR_QEMU_PROCESS_START_STANDALONE so we must also pass in the process
start flags.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
Start aggregating all capability post-processing code in one place.
The comment was modified while moving it as it was mentioning floppies
which are no longer clearing the blockdev capability.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The function is now used only in qemu_process.c so move it there and
name it 'qemuProcessPrepareQEMUCaps' which is more appropriate to what
it's doing.
The reworded comment now mentions that it will also post-process the
caps for VM startup.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The redetection was originally added in 43c01d3838 as a way to recover
from libvirtd upgrade from the time when we didn't persist the qemu
capabilities in the status XML. Also this the oldest supported qemu by
more than two years.
Even if somebody would have a running VM running at least qemu 1.5 with
such an old libvirt we certainly wouldn't do the right thing by
redetecting the capabilities and then trying to communicate with qemu.
For now it will be the best to just stop considering this scenario any
more and error out for such VM.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Commit c90fb5a828 added explicit use of the private copy of the qemu
capabilities to various places. The change to qemuProcessInit was bogus
though as at the point where we re-initiate the post parse callbacks
priv->qemuCaps is still NULL as we clear it after shutdown of the VM and
don't initiate it until a later point.
Using the value from priv->qemuCaps might mislead readers of the code
into thinking that something useful is being passed at that point so go
with an explicit NULL instead.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
qemuDomainGetJobInfo didn't always reset the return data in @info.
Thankfully this wouldn't be a problem as the RPC layer does it but we
should do it anyways.
Since we reset the struct we don't have to set the type to
VIR_DOMAIN_JOB_NONE as the value is 0.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
virDomainGetJobStats destroys the completed statistics on the first
read. Give the user possibility to keep them around if they wish so.
Add a flag VIR_DOMAIN_JOB_STATS_KEEP_COMPLETED which will read the stats
without destroying them.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
For managed save we can choose between various compression
methods. I randomly tested the 'xz' program on a 8 GB guest
and was surprised to have to wait > 50 minutes for it to
finish compressing, with 'xz' burning 100% cpu for the
entire time. Despite the impressive compression, this is
completely useless in the real world as it is far too long
to wait to save the VM.
The 'xz' binary defaults to '-6' optimization level which
aims for high compression, with moderate memory usage,
at the expense of speed.
This change switches it to use the '-3' optimization level
which is documented as being the one that optimizes speed
at expense of compression. Even with this, it will still
outperform all the other options in terms of compression
level. It is a little less than x4 faster than '-6' which
means it starts to be a viable choice to use 'xz' for
people who really want best compression.
The test results on a 1 GB, fairly freshly booted VM are
as follows
format | save | restore size
=======+=======+=============
raw | 05s | 1s | 428 MB
lzop | 05s | 3s | 160 MB
gzip | 29s | 5s | 118 MB
bz2 | 54s | 22s | 114 MB
xz | 4m37s | 13s | 86 MB
xz -3 | 1m20s | 12s | 95 MB
Based on this we can say
* For moderate compression with no noticable loss in speed
=> use lzop
* For high compression with moderate loss in speed
=> use gzip
* For best compression with significant loss in speed
=> use xz
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
This is a very simple and straightforward implementation of the opposite
what buildPool does for the disk backend.
The background for this change comes from an existing test case in TCK
which does use the delete method for a pool of type disk, but it
truly could not have ever worked since the implementation simply
wasn't there for the pool of type disk.
Signed-off-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Commit 4b58fdf280 which enabled block copy also for network
destinations needed to limit when the 'mirror' storage source is
initialized in cases when we e.g. don't have an appropriate backend.
Limiting it just to virStorageFileSupportsCreate is too restrictive as
for example we can't precreate block devices and thus wouldn't
initialize the 'mirror' but since it's a local source we'd try to
examine it. This would fail since it wouldn't be initialized.
Fix it by introducing a more granular check whether certain operations
are supported and fix the check interlocks.
https://bugzilla.redhat.com/show_bug.cgi?id=1778058
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
We tolerate image format detection during block copy in very specific
circumstances, but the code didn't error out on failure of the format
detection.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The API XML files are generated files, so live in the build dir not the
source dir.
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
In v5.9.0-273-g8ecab214de I've tried to fix a lock ordering
problem, but introduced a crasher. Problem is that because the
client lock is unlocked (in order to honour lock ordering) the
stream we are currently checking in daemonStreamFilter() might be
freed and thus stream->priv might not even exist when the control
get to virMutexLock() call.
To resolve this, grab an extra reference to the stream and handle
its cleanup should the refcounter reach zero after the deref.
If that's the case and we are the only ones holding a reference
to the stream, we MUST return a positive value to make
virNetServerClientDispatchRead() break its loop where it iterates
over filters. The problem is, if we did not do so, then
"filter = filter->next" line will read from a memory that was
just freed (freeing a stream also unregisters its filter).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
In commit 2ccb5335dc I've refactored how we fill the typed parameters
for domain statistics. The commit introduced a regression in the
formating of stats for IOthreads by using the array index to label the
entries as it's common for all other types of statistics rather than
the iothread IDs used for iothreads.
Since only the design of iothread deviates from the common approach used
in all other statistic types this was not caught.
https://bugzilla.redhat.com/show_bug.cgi?id=1778014
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The original implementation used QEMU_ADD_COUNT_PARAM which added the
'count' suffix, but 'cnt' was documented. Fix the documentation to
conform with the original implementation.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
Commit ed56851f1b 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>
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>
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>
This reverts commit 421c9550f5
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>
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>
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>
<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>
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>
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>
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>
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>
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>
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>
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>
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>
