If the job has finished, but we didn't yet process the completion fake
that it's still incomplete so that apps which decided to poll
qemuDomainGetBlockJobInfo rather than use events can be sure that the
XML update was completed.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Replace qemuMonitorGetBlockJobInfo by qemuMonitorGetAllBlockJobInfo and
hash table lookup. This basically open-codes qemuMonitorGetBlockJobInfo,
but it will be removed in next patch.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
If starting an container fails, the virLXCProcessStop() is
called. But since vm->def->id is not set until libvirt_lxc is
spawned (the domain's ID is PID of that process),
virLXCProcessStop() returns early as virDomainObjIsActive()
returns false. But doing so leaves behind resources reserved for
the containers during the startup process. Most notably, hostdevs
are not re-attached to the host, the domain's transient XML is
not removed, etc.
To resolve this, virLXCProcessCleanup() is called in this case.
However, it is modified to accept @flags which allows caller to
run only specific cleanups (depending how far in container
creation the failure occurred). There is plenty of cleanups which
don't need this guard because either they detect a NULL pointer
or try to release an unique resource.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
In recent commit of bf8bd93df0 (and friends) we switched the way
we process queried command line arguments: from string lists to
virJSONValue stored in a hash table. To achieve this
qemuMonitorJSONGetCommandLineOptions() helper was introduced
which executes the "query-command-line-options" monitor command
and then calls virJSONValueArrayForeachSteal() to process the
output. The array process function is also given
qemuMonitorJSONGetCommandLineOptionsWorker() as the callback
which is called over each item of the returned array. This
callback then steals "parameters" attribute of each array iteam
storing it in the hash table, but it leaves behind "option"
attribute (because it's g_strdup()-ed). After all of this, the
callback returns 0 which is a signal to the array processing
function that the callback took ownership of the array item. But
this is not true. While it removed "parameters" it did not take
the rest ("option" for instance). And therefore, it leads to a
memory leak:
5,347 (1,656 direct, 3,691 indirect) bytes in 69 blocks are definitely lost in loss record 2,752 of 2,794
at 0x483BEC5: calloc (vg_replace_malloc.c:760)
by 0x4E25A10: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.6400.5)
by 0x4943317: virJSONValueNewObject (virjson.c:569)
by 0x4945692: virJSONParserHandleStartMap (virjson.c:1768)
by 0x5825A86: yajl_do_parse (in /usr/lib64/libyajl.so.2.1.0)
by 0x4945BFA: virJSONValueFromString (virjson.c:1896)
by 0xAF5C115: qemuMonitorJSONIOProcessLine (qemu_monitor_json.c:224)
by 0xAF5C45E: qemuMonitorJSONIOProcess (qemu_monitor_json.c:279)
by 0xAF4BB6C: qemuMonitorIOProcess (qemu_monitor.c:342)
by 0xAF4C444: qemuMonitorIO (qemu_monitor.c:574)
by 0x4FEF846: socket_source_dispatch (in /usr/lib64/libgio-2.0.so.0.6400.5)
by 0x4E1F727: g_main_context_dispatch (in /usr/lib64/libglib-2.0.so.0.6400.5)
The callback must return 1 so that the array item is properly
freed.
Fixes: ebeff6cd57
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Since the function is now only used in qemu_domain.c, move it from
domain_conf.c and rename it.
This reverts the work done in commit ace5931553
(conf, qemu: move qemuDomainNVDimmAlignSizePseries to domain_conf.c).
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
qemuDomainAlignMemorySizes() has an operation order problem. We are
calculating 'initialmem' without aligning the memory modules first.
Since we're aligning the dimms afterwards this can create inconsistencies
in the end result. x86 has alignment of 1-2MiB and it's not severely
impacted by it, but pSeries works with 256MiB alignment and the difference
is noticeable.
This is the case of the existing 'memory-hotplug-ppc64-nonuma' test.
The test consists of a 2GiB (aligned value) guest with 2 ~520MiB dimms,
both unaligned. 'initialmem' is calculated by taking total_mem and
subtracting the dimms size (via virDomainDefGetMemoryInitial()), which
wil give us 2GiB - 520MiB - 520MiB, ending up with a little more than
an 1GiB of 'initialmem'. Note that this value is now unaligned, and
will be aligned up via VIR_ROUND_UP(), and we'll end up with 'initialmem'
of 1GiB + 256MiB. Given that the dimms are aligned later on, the end
result for QEMU is that the guest will have a 'mem' size of 1310720k,
plus the two 512 MiB dimms, exceeding in 256MiB the desired 2GiB
memory and currentMemory specified in the XML.
Existing guests can't be fixed without breaking ABI, but we have
code already in place to align pSeries NVDIMM modules for new guests.
Let's extend it to align all pSeries mem modules.
A new test, 'memory-hotplug-ppc64-nonuma-abi-update', a copy of the
existing 'memory-hotplug-ppc64-nonuma', was added to demonstrate the
result for new pSeries guests. For the same unaligned XML mentioned
above, after applying this patch:
- starting QEMU mem size without PARSE_ABI_UPDATE:
-m size=1310720k,slots=16,maxmem=4194304k \ (no changes)
- starting QEMU mem size with PARSE_ABI_UPDATE:
-m size=1048576k,slots=16,maxmem=4194304k \ (size fixed)
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
A previous patch removed the pSeries NVDIMM align that wasn't
being done properly. This patch reintroduces it in the right
fashion, making it reliant on VIR_DOMAIN_DEF_PARSE_ABI_UPDATE.
This makes it complying with the intended design defined by
commit c7d7ba85a6.
Since the PARSE_ABI_UPDATE is more restrictive than checking for
!migrate && !snapshot, like is being currently done with
qemuDomainAlignMemorySizes(), this means that we'll align the
pSeries NVDIMMs in two places - in post parse time for new
guests, and in qemuDomainAlignMemorySizes() for all guests
that aren't migrating or in a snapshot.
Another difference is that the logic is now in the QEMU driver
instead of domain_conf.c. This was necessary because all
considerations made about the PARSE_ABI_UPDATE flag were done
under QEMU. Given that no other driver supports ppc64 there is no
impact in this change.
A new test was added to exercise what we're doing. It consists
of a a copy of the existing 'memory-hotplug-nvdimm-ppc64' xml2xml
test, called with the PARSE_ABI_UPDATE flag. As intended, we're
not changing QEMU command line or any XML without the flag,
while the pseries NVDIMM memory is being aligned when the
flag is used.
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
The code to align ppc64 NVDIMMs on post parse was introduced in
commit d3f3c2c97f. That commit failed to realize that we
can't align memory unconditionally. As of commit c7d7ba85a6
("qemu: command: Align memory sizes only on fresh starts"),
all memory alignment should be executed only when we're not
migrating or in a snapshot.
This revert does not break any guests in the wild, given that
ppc64 NVDIMMs are still being aligned in qemuDomainAlignMemorySizes().
Next patch will introduce a mechanism where we can have post
parse NVDIMM alignment for pSeries without breaking the
intended design, as defined by c7d7ba85a6.
This reverts commit d3f3c2c97f.
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
The AppArmorSetMemoryLabel() is a callback that is called from
qemuSecuritySetMemoryLabel() which never passes NULL as @mem.
Therefore, there is no need to check whether @mem is NULL. Also,
no other driver does that and just dereference it immediately.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
After previous cleanup the @qemuCaps argument in
qemuDomainDefValidateMemoryHotplug() is unused and thus doesn't
need to be passed.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Loop for multiple PATH arguments to support shell pattern expansion.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
In some cases you want to fix a certain directory while you don't really
care whether there are other VMs running. Add a option to disable the
check.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Linux and FreeBSD have different prefix. In the current state we've
tried to reset the labels for both systems which resulted in errors like
this:
Fixing /tmp/bitmaps2.qcow2
setfattr: /tmp/bitmaps2.qcow2: Operation not supported
setfattr: /tmp/bitmaps2.qcow2: Operation not supported
setfattr: /tmp/bitmaps2.qcow2: Operation not supported
setfattr: /tmp/bitmaps2.qcow2: Operation not supported
setfattr: /tmp/bitmaps2.qcow2: Operation not supported
setfattr: /tmp/bitmaps2.qcow2: Operation not supported
The 6 failed 'setfattrs' correspond to the wrong prefix.
Select the correct prefix based on the kernel name and modify the code
appropriately.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
So far our memory modules could go only into DIMM slots. But with
virtio model this assumption is no longer true - virtio-pmem goes
onto PCI bus. But for formatting PCI address onto command line we
already have a function - qemuBuildDeviceAddressStr(). Therefore,
mode DIMM address generation into it so that we don't have to
special case address building later on.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Tested-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Tested-by: Han Han <hhan@redhat.com>
There is this function qemuDomainDefValidateMemoryHotplug() which
is called explicitly from hotplug path and the qemu's domain def
validator. This is not really necessary because we can move the
part that validates feature against qemuCaps into device
validator which is called implicitly (from qemu driver's POV).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Tested-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Tested-by: Han Han <hhan@redhat.com>
The virDomainMemoryTargetDefFormat() uses good old style of
formatting child buffer (virBufferAdjustIndent()). When switched
to virXMLFormatElement() we can save a couple of lines
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Tested-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Tested-by: Han Han <hhan@redhat.com>
The virDomainMemorySourceDefFormat() uses good old style of
formatting child buffer (virBufferAdjustIndent()). When switched
to virXMLFormatElement() we can save a couple of lines.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Tested-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Tested-by: Han Han <hhan@redhat.com>
The virDomainMemoryModel structure has a @type member which is
really type of virDomainMemoryModel but we store it as int
because the virDomainMemoryModelTypeFromString() call stores its
retval right into it. Then, to have compiler do compile time
check for us, every switch() typecasts the @type. This is
needlessly verbose because the parses already has @val - a
variable to store temporary values. Switch @type in the struct to
virDomainMemoryModel and drop all typecasts.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Tested-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Tested-by: Han Han <hhan@redhat.com>
Our code expects that a nvdimm has a path defined always. And the
parser does check for that. Well, not fully - only when parsing
<source/> (which is an optional element). So if the element is
not in the XML then the check is not performed and the assumption
is broken. Verify in the memory def validator that a path was
set.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Tested-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Tested-by: Han Han <hhan@redhat.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Tested-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Tested-by: Han Han <hhan@redhat.com>
The UUID is guest visible and thus shouldn't change if we want to
not break guest ABI.
Fixes: 08ed673901
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Tested-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Tested-by: Han Han <hhan@redhat.com>
On PPC platform it is required that a NVDIMM has an UUID. If none
is provided then libvirt generates one during parsing (see
v6.2.0-rc1~96 and friends). However, the example provided in our
documentation is not valid XML.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Tested-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Tested-by: Han Han <hhan@redhat.com>
This macro checks whether given number is an integer power of
two. At the same time, I've identified two places where we check
for pow2 and I'm replacing them with the macro.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Tested-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Tested-by: Han Han <hhan@redhat.com>
The only test we do when checking for UUID validity is that
whether all bytes are the same (invalid UUID) or not (valid
UUID). The algorithm we use is needlessly complicated.
Also, the checked UUID is not modified and hence the argument can
be of 'const' type.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Tested-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Tested-by: Han Han <hhan@redhat.com>
The domain definition stored with a checkpoint isn't used currently
apart from matching disks when creating a new checkpoints.
As some users of the incremental backup API want to provide backups in
offline mode under their control (obviously while compying with our
documentation on how the on-disk state should be handled) and then want
to define the checkpoint for live use, supplying a <domain> sub-element
is overly complex and not actually needed by the code.
Relax the restriction when re-defining a checkpoint so that <domain> is
not necessary and add (alibistic) documentation saying that future
actions may not work if it's missing.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Conditionalize code which assumes that the domain definition stored in
the checkpoint is present.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Checking the definition ABI when redefining checkpoints doesn't make
much sense for the following reasons:
* the domain definition in the checkpoint is mostly unused (a relic
adopted from the snapshot code)
* can be very easily overridden by deleting the checkpoint metadata
before redefinition
Rather than complicating the logic when we'll be taking into account
that the domain definition may be missing, let's just remove the check.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Fix the type for a variable holding flags to the usual 'unsigned int'
and change the name to be more appropriate to its use.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
We can extract './domain' directly and let the parser deal with the
type.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
We can leave out things like USB controller, memballoon device,
kernel and initrd since they're not the focus of the tests.
Propagating some information from the output files back to the
input files makes it easier to compare them, as it reduces the
resulting diff, and in the case of the qemuxml2xml test for
memory-hotplug-ppc64-nonuma it allows us to convert the output
file into a symlink, since in the specific case the XML doesn't
change at all.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
The ppc64 tests
memory-hotplug-ppc64-nonuma
memory-hotplug-nvdimm-ppc64
are not passed the same information for qemuxml2argv and
qemuxml2xml tests; the former, in particular, doesn't show up
at all in qemuxml2xml. Address this inconsistency.
Note that one of the new output files had been introduced with
5540acb9a2 despite not being actually used as of that commit.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
See arch/x86/include/asm/intel-family.h in the Kernel:
#define INTEL_FAM6_ICELAKE_X 0x6A
Signed-off-by: Tim Wiederhake <twiederh@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
LXC processes confined by apparmor are not permitted to receive signals
from libvirtd. Attempting to destroy such a process fails
virsh --connect lxc:/// destroy distro_apparmor
error: Failed to destroy domain distro_apparmor
error: Failed to kill process 29491: Permission denied
And from /var/log/audit/audit.log
type=AVC msg=audit(1606949706.142:6345): apparmor="DENIED"
operation="signal" profile="libvirt-314b7109-fdce-48dc-ad28-7c47958a27c1"
pid=29390 comm="libvirtd" requested_mask="receive" denied_mask="receive"
signal=term peer="libvirtd"
Similar to the libvirt-qemu abstraction, add a rule to the libvirt-lxc
abstraction allowing reception of signals from libvirtd.
Signed-off-by: Jim Fehlig <jfehlig@suse.com>
Reviewed-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
At this moment, it is not possible to create a test specifying
ARG_PARSEFLAGS because info->parseFlags is not being forwarded to
testCompareDomXML2XMLFiles(). Let's fix it now so next patch can
make use of it.
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
qemuBuildCommandLine() is calling qemuDomainAlignMemorySizes(),
which is an operation that changes live XML and domain and has
little to do with the command line build process.
Move it to qemuProcessPrepareDomain() where we're supposed to
make live XML and domain changes before launch. qemuProcessStart()
is setting VIR_QEMU_PROCESS_START_NEW if !migrate && !snapshot,
same conditions used in qemuBuildCommandLine() to call
qemuDomainAlignMemorySizes(), making this change seamless.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
qemuProcessCreatePretendCmdPrepare() is setting the
VIR_QEMU_PROCESS_START_NEW regardless of whether this is
a migration case or not. This behavior differs from what we're
doing in qemuProcessStart(), where the flag is set only
if !migrate && !snapshot.
Fix it by making the flag setting consistent with what we're
doing in qemuProcessStart().
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Let's pass along / fill @niothreads rather than trying to make dual
use as a return value and thread count.
This resolves a Coverity issue detected in qemuDomainGetIOThreadsMon
where if qemuDomainObjExitMonitor failed, then a -1 was returned and
overwrite @niothreads causing a memory leak.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
In this previous commit:
commit 65491a2dfe
Author: Martin Kletzander <mkletzan@redhat.com>
Date: Thu Nov 12 13:58:53 2020 +0100
Do not disable incompatible-pointer-types-discards-qualifiers
We selectively rewrite G_DEFINE_TYPE to avoid warnings about
mismatched volatile/non-volatile pointers that appeared with
CLang when using GLib2 >= 2.67
We have now just hit the reverse problem, GCC >= 11 has started
warning about mismatched volatile/non-volatile pointers but only
with GLib2 < 2.67. The new GLib2 avoids the warning, as does
older GCC.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Existing practice with the filesystem fields reported for the
virDomainGetGuestInfo API is to use the singular form for
field names. Ensure the disk info follows this practice.
Fixes
commit 05a75ca2ce
Author: Marc-André Lureau <marcandre.lureau@redhat.com>
Date: Fri Nov 20 22:09:46 2020 +0400
domain: add disk informations to virDomainGetGuestInfo
commit 0cb2d9f05d
Author: Marc-André Lureau <marcandre.lureau@redhat.com>
Date: Fri Nov 20 22:09:47 2020 +0400
qemu_driver: report guest disk informations
commit 172b830435
Author: Marc-André Lureau <marcandre.lureau@redhat.com>
Date: Fri Nov 20 22:09:48 2020 +0400
virsh: add --disk informations to guestinfo command
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
We don't need the index that virDomainDiskIndexByName returns.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
We don't need the index that virDomainDiskIndexByName returns.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Similarly to d3c029bb10 where we've refactored
virDomainSnapshotAlignDisks, modify the extension algorithm to avoid use
of the 'idx' variable and sorting of the array.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
