qemuDomainSnapshotDiskDataCollect copies the source of the disk from the
live config into the inactive config. Move this operation earlier so
that if we initialize it for use for the particular instance the
run-time-only data is not copied.
Signed-off-by: NPeter Krempa <pkrempa@redhat.com>
Reviewed-by: NJán Tomko <jtomko@redhat.com>

In case when the backing store can be represented with something
simpler such as a URI we can use it rather than falling back to the
json: pseudo-protocol.

In cases when it's not worth it (e.g. with the old ugly NBD or RBD
strings) let's switch to json.

The function is exported as we'll need it when overwriting the ugly
strings qemu would come up with during blockjobs.
Signed-off-by: NPeter Krempa <pkrempa@redhat.com>
Reviewed-by: NJán Tomko <jtomko@redhat.com>

The block commit API checked 'disk->src->path' to see whether there
is a reasonable disk source to be committed. As the top image can be
e.g. backed by NBD the check is not good enough. Replace it by
virStorageSourceIsEmpty.
Signed-off-by: NPeter Krempa <pkrempa@redhat.com>
Reviewed-by: NJán Tomko <jtomko@redhat.com>

For the modern use cases we are going to use 'blockdev-snapshot' instead
of 'blockdev-snapshot-sync'.
Signed-off-by: NPeter Krempa <pkrempa@redhat.com>
Reviewed-by: NJán Tomko <jtomko@redhat.com>

qemuBuildStorageSourceChainAttachPrepareBlockdev prepares the full
backing chain for attachment via blockdev. For snapshots we'll need to
prepare one image only as it needs to be plugged on top of the existing
chain.

This patch introduces qemuBuildStorageSourceChainAttachPrepareBlockdevTop
which prepares only @top similarly to the original function by splitting
out the functionality into an internal function so that the API does not
change.
Signed-off-by: NPeter Krempa <pkrempa@redhat.com>
Reviewed-by: NJán Tomko <jtomko@redhat.com>

In commit 042c95bd qemuBuildStorageSourceChainAttachPrepareBlockdev
was added but the comment for the function mentions
qemuBuildStorageSourceChainAttachPrepareDrive. Fix the mistake.
Signed-off-by: NPeter Krempa <pkrempa@redhat.com>
Reviewed-by: NJán Tomko <jtomko@redhat.com>

Now that we track the job separately we watch only for the abort of the
one single block job so the comment is no longer accurate. Also
describing asynchronous operation is not really necessary.
Signed-off-by: NPeter Krempa <pkrempa@redhat.com>
Reviewed-by: NJán Tomko <jtomko@redhat.com>

With -blockdev:

- we track the job and check it after restart
- have the ability to ask qemu to persist it to collect result
- have the ability to report errors.

This solves all points the comment outlined so remove it. Also all jobs
handle the disk state modification along with the event so there's
nothing special the comment says.
Signed-off-by: NPeter Krempa <pkrempa@redhat.com>
Reviewed-by: NJán Tomko <jtomko@redhat.com>

Use job-complete/job-abort instead of the blockjob-* variants for
blockdev.
Signed-off-by: NPeter Krempa <pkrempa@redhat.com>
Reviewed-by: NJán Tomko <jtomko@redhat.com>

As the error message is now available and we know whether the job failed
we can report an error straight away rather than having the user check
the event.
Signed-off-by: NPeter Krempa <pkrempa@redhat.com>
Reviewed-by: NJán Tomko <jtomko@redhat.com>

Set the correct job states after the operation is requested in qemu.
Signed-off-by: NPeter Krempa <pkrempa@redhat.com>
Reviewed-by: NJán Tomko <jtomko@redhat.com>

When initiating a pivot or abort of a block job we need to track which
one was initiated. Currently it was done via data stashed in
virDomainDiskDef. Add possibility to track this also together with the
job itself.
Signed-off-by: NPeter Krempa <pkrempa@redhat.com>
Reviewed-by: NJán Tomko <jtomko@redhat.com>

Do decisions based on the configuration of the job rather than the data
stored with the disk.
Signed-off-by: NPeter Krempa <pkrempa@redhat.com>
Reviewed-by: NJán Tomko <jtomko@redhat.com>

Use the stored job name rather than passing in the disk alias when
referring to the job which allows the same code to work also when
-blockdev will be used.

Note that this API does not require the change to use 'query-job' as it
will ever only work with blockjobs bound to disks due to the arguments
which allow only referring to a disk. For the disk-less jobs we'll need
to add a separate API later.

The change to qemuMonitorGetBlockJobInfo is required as the API was
stripping the 'drive-' prefix when returning the data which is not
desired any more.
Signed-off-by: NPeter Krempa <pkrempa@redhat.com>
Reviewed-by: NJán Tomko <jtomko@redhat.com>

Signed-off-by: NPavel Hrdina <phrdina@redhat.com>
Acked-by: NPeter Krempa <pkrempa@redhat.com>

In cgroups v2 when a new group is created by default no controller is
enabled so the detection code will not detect any controllers.

When enabling the controllers we should also store them for the group.
Signed-off-by: NPavel Hrdina <phrdina@redhat.com>
Acked-by: NPeter Krempa <pkrempa@redhat.com>

When creating new group for cgroups v2 the we cannot check
cgroups.controllers for that cgroup because the directory is created
later.  In that case we should check cgroups.subtree_control of parent
group to get list of controllers enabled for child cgroups.

In order to achieve that we will prefer the parent group if it exists,
the current group will be used only for root group.
Signed-off-by: NPavel Hrdina <phrdina@redhat.com>
Acked-by: NPeter Krempa <pkrempa@redhat.com>

The virsh-optparse test broke after commit 6ac402c4 because it
always assumed the max memory limit can be adjusted on a running domain
which used to be the case in the old code.
This is only a hot fix for the CI build. The proper fix here is to
re-write the whole test in a self-test/unit-test manner where we only
test virsh's ability to parse various values, not running actual
commands.
Signed-off-by: NErik Skultety <eskultet@redhat.com>
Reviewed-by: NJán Tomko <jtomko@redhat.com>
Acked-by: NPeter Krempa <pkrempa@redhat.com>

When commit 6ac402c4 added the API whenever VIR_DOMAIN_MEM_MAXIMUM
was passed the code always checked whether the domain was active and
therefore failed with an error even though only a config change was
requested. Fix the issue by replacing virDomainObjGetOneDef with
virDomainObjGetOneDefState which tells us what definition we're
performing the change on.
Signed-off-by: NErik Skultety <eskultet@redhat.com>
Reviewed-by: NJán Tomko <jtomko@redhat.com>
Acked-by: NPeter Krempa <pkrempa@redhat.com>

Commit d5572f62 forgot to add maxthreads to the non-Linux definition
of the function, thus breaking the MinGW build.
Signed-off-by: NErik Skultety <eskultet@redhat.com>
Reviewed-by: NJán Tomko <jtomko@redhat.com>
Acked-by: NPeter Krempa <pkrempa@redhat.com>

Changes noticed while copying to similar aspects of checkpoints.
Signed-off-by: NEric Blake <eblake@redhat.com>
Reviewed-by: NDaniel P. Berrangé <berrange@redhat.com>

Any message that is easy to trigger (as evidenced by the testsuite
update) should not use 'internal error' as its category.
Signed-off-by: NEric Blake <eblake@redhat.com>
Reviewed-by: NDaniel P. Berrangé <berrange@redhat.com>

virDomainSnapshotFindByName(list, NULL) should return NULL, rather
than the internal-use-only metaroot.  Most existing callers pass in a
non-NULL name; the few external callers that don't are immediately
calling virDomainMomentSetParent (which indeed needs the metaroot
rather than NULL if the parent name is NULL); but as the leaky
abstraction is ugly, it is worth instead making
virDomainMomentSetParent static and adding a new function for
resolving the parent link of a brand new moment within its list.  The
existing external uses of virDomainMomentSetParent always succeed
(either the new moment has parent_name of NULL to become a new root,
or has parent_name set to a strdup of the previous current moment);
hence, our new function does not need a return value (but it still has
a VIR_WARN in case future uses break our assumptions about failure
being impossible).

Missed when commit 02c4e24d refactored things to attempt to remove
direct metaroot manipulations out of the qemu and test drivers into
internal-only details, and made more obvious when commit dc8d3dc6
factored it out into a separate file.
Signed-off-by: NEric Blake <eblake@redhat.com>
Reviewed-by: NDaniel P. Berrangé <berrange@redhat.com>

Some VM configurations may result in a large number of threads created by
the associated qemu process which can exceed the system default limit. The
maximum number of threads allowed per process is controlled by the pids
cgroup controller and is set to 16k when creating VMs with systemd's
machined service. The maximum number of threads per process is recorded
in the pids.max file under the machine's pids controller cgroup hierarchy,
e.g.

$cgrp-mnt/pids/machine.slice/machine-qemu\\x2d1\\x2dtest.scope/pids.max

Maximum threads per process is controlled with the TasksMax property of
the systemd scope for the machine. This patch adds an option to qemu.conf
which can be used to override the maximum number of threads allowed per
qemu process. If the value of option is greater than zero, it will be set
in the TasksMax property of the machine's scope after creating the machine.
Signed-off-by: NJim Fehlig <jfehlig@suse.com>
Reviewed-by: NDaniel P. Berrangé <berrange@redhat.com>

Signed-off-by: NIlias Stamatis <stamatis.iliass@gmail.com>
Reviewed-by: NErik Skultety <eskultet@redhat.com>

Signed-off-by: NIlias Stamatis <stamatis.iliass@gmail.com>
Reviewed-by: NErik Skultety <eskultet@redhat.com>

Update the current or max memory, on the persistent or live definition
depending on the flags which are currently ignored.
Signed-off-by: NIlias Stamatis <stamatis.iliass@gmail.com>
Reviewed-by: NErik Skultety <eskultet@redhat.com>

Signed-off-by: NIlias Stamatis <stamatis.iliass@gmail.com>
Reviewed-by: NErik Skultety <eskultet@redhat.com>

Make the test driver only support the VIR_TYPED_PARAM_STRING flag for
now.
Signed-off-by: NIlias Stamatis <stamatis.iliass@gmail.com>
Reviewed-by: NErik Skultety <eskultet@redhat.com>

Signed-off-by: NIlias Stamatis <stamatis.iliass@gmail.com>
Reviewed-by: NErik Skultety <eskultet@redhat.com>

Signed-off-by: NIlias Stamatis <stamatis.iliass@gmail.com>
Reviewed-by: NErik Skultety <eskultet@redhat.com>

Signed-off-by: NIlias Stamatis <stamatis.iliass@gmail.com>
Reviewed-by: NErik Skultety <eskultet@redhat.com>

If the dup2 fails, then we aren't going to get the correct result.

Found by Coverity
Signed-off-by: NJohn Ferlan <jferlan@redhat.com>
ACKed-by: NPeter Krempa <pkrempa@redhat.com>

Avoid the chance that sysconf(_SC_OPEN_MAX) returns -1 and thus
would cause virBitmapNew would attempt to allocate a very large
bitmap.

Found by Coverity
Signed-off-by: NJohn Ferlan <jferlan@redhat.com>
ACKed-by: NPeter Krempa <pkrempa@redhat.com>

Avoid the chance that qemuMonitorTestNewSimple could return NULL

Found by Coverity
Signed-off-by: NJohn Ferlan <jferlan@redhat.com>
ACKed-by: NPeter Krempa <pkrempa@redhat.com>

It's already dereffed in the initialization and shouldn't be NULL
unless virJSONValueArraySize after a virJSONValueObjectGetArray
could return a NULL data entry.

Found by Coverity
Signed-off-by: NJohn Ferlan <jferlan@redhat.com>
ACKed-by: NPeter Krempa <pkrempa@redhat.com>

Ever since --parallel-connections option for virsh migrate was
introduced we did not properly check the return value of
vshCommandOptInt. We would set VIR_MIGRATE_PARAM_PARALLEL_CONNECTIONS
parameter even if vshCommandOptInt returned 0 (which means
--parallel-connections was not specified) when another int option which
was checked earlier was specified with a nonzero value.

Specifically, running virsh migrate with either
--auto-converge-increment, --auto-converge-initial, --comp-mt-dthreads,
--comp-mt-threads, or --comp-mt-level would set
VIR_MIGRATE_PARAM_PARALLEL_CONNECTIONS parameter and if --parallel
option was not used, libvirt would complain

    error: invalid argument: Turn parallel migration on to tune it

even though --parallel-connections option was not used at all.

https://bugzilla.redhat.com/show_bug.cgi?id=1726643Signed-off-by: NJiri Denemark <jdenemar@redhat.com>
Reviewed-by: NMichal Privoznik <mprivozn@redhat.com>

Report in logs when we don't find existing block job data and create it
just to handle the job.
Signed-off-by: NPeter Krempa <pkrempa@redhat.com>
Reviewed-by: NJán Tomko <jtomko@redhat.com>

While this function does start a block job in case when we'd not be able
to get our internal data for it, the handler sets the job state to
QEMU_BLOCKJOB_STATE_RUNNING anyways, thus qemuBlockJobStartupFinalize
would just unref the job.

Since the other usage of qemuBlockJobStartupFinalize in the other part
of the event handler was a bug replace this one anyways even if it would
not cause problems.
Signed-off-by: NPeter Krempa <pkrempa@redhat.com>
Reviewed-by: NJán Tomko <jtomko@redhat.com>

The block job event handler qemuProcessHandleBlockJob looks at the block
job data to see whether the job requires synchronous handling. Since the
block job event may arrive before we continue the job handling (if the
job has no data to copy) we could hit the state when the job is still
set as QEMU_BLOCKJOB_STATE_NEW (as we move it to the
QEMU_BLOCKJOB_STATE_RUNNING state only after returning from monitor).

If the event handler uses qemuBlockJobStartupFinalize it would
unregister and free the job. Thankfully this is not a big problem for
legacy blockjobs as we don't need much data for them but since we'd
re-instantiate the job data structure we'd report wrong job type for
active commit as qemu reports it as a regular commit job.

Fix it by not using qemuBlockJobStartupFinalize function in
qemuProcessHandleBlockJob as it is not starting the job anyways.

https://bugzilla.redhat.com/show_bug.cgi?id=1721375Signed-off-by: NPeter Krempa <pkrempa@redhat.com>
Reviewed-by: NJán Tomko <jtomko@redhat.com>