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 c7d7ba85a624.
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>
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>
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 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>
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>
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>
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>
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>
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 05a75ca2ce743bc0bb119fb8d532ff84646fafa3
Author: Marc-André Lureau <marcandre.lureau@redhat.com>
Date: Fri Nov 20 22:09:46 2020 +0400
domain: add disk informations to virDomainGetGuestInfo
commit 0cb2d9f05d00497a715352f6ea28cf8fb6921731
Author: Marc-André Lureau <marcandre.lureau@redhat.com>
Date: Fri Nov 20 22:09:47 2020 +0400
qemu_driver: report guest disk informations
commit 172b8304352b1945e328394e61290a24446280dd
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>
Currently it is simply ignored.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Currently it is simply ignored.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Commit c4f4e195 fixed a double free, but if the code returns before
we realloc the list and virFirmwareFreeList was called with cfg->nfirmwares
> 0 (e.g. during virQEMUDriverConfigDispose), then it would be rather
disastrous. So let's reinitialize that too to indicate the list is empty.
Coverity pointed out that using nvram[0] as a guard to reallocating the
list could lead to a possible NULL deref. While nvram[0] may always be
true in this case, if it wasn't then the subsequent for loop would fail.
Just reallocate always regardless - even if nfirmwares == 0 as
virFirmwareFreeList will free it for us anyway.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Glib provides g_auto(GStrv) which is in-place replacement of our
VIR_AUTOSTRINGLIST.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
If there is an error getting info from guest agent, then the
control on qemuDomainGetGuestInfo() jumps onto 'exitagent' label
and subsequently continues on 'endagentjob'. Both labels are hit
also in success case too. The control then continues by
attempting to match fetched info (e.g. disk addresses) with
domain def. But this is needless - the API will return error
regardless.
To return early from the function move both 'exitagent' and
'endagentjob' labels at the end of the function and jump straight
onto 'cleanup' afterwards. This allows us to set 'ret = 0' later
- only when we know we succeeded.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Remove the function along with helpers for caching the reply and tests.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Use the new handler to fetch the required data and do the extraction
locally without conversion to string list.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Add a new set hander for getting the data for
'query-command-line-options' which returns everything at once and lets
the caller extract the data. This way we don't need to cache the output
of the monitor command for repeated calls.
Note that we will have enough testing of this code path via
qemucapabilitiestest.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Do not look up the index of the passed FD in places where
we already have it.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
An alternative to qemuVirCommandGetFDSet that takes the index
into the passed FD set as an argument and does not try to look it up.
Use it as well ass virCommandPassFDIndex in qemuBuildChrChardevFileStr
and qemuBuildInterfaceCommandLine.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
In a few commit back (v6.10.0-5-gb3dad96972) a new helper for
obtaining string arrays from a virJSONObject was introduced:
virJSONValueObjectGetStringArray(). I've identified three places
where it can be used instead of open coding it:
qemuAgentSSHGetAuthorizedKeys(),
qemuMonitorJSONGetStringListProperty() and
qemuMonitorJSONGetCPUDefinitions().
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
cfg->firmwares still points to the original memory address after being
freed by virFirmwareFreeList(). As cfg get freed, it will be freed again
even if cfg->nfirmwares=0 which eventually lead to crash.
The patch fix it by setting cfg->firmwares to NULL explicitly after
virFirmwareFreeList() returns
Signed-off-by: Guoyi Tu<tu.guoyi@h3c.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Tested-by: Han Han <hhan@redhat.com>
guest-get-disks is available since QEMU 5.2:
https://wiki.qemu.org/ChangeLog/5.2#Guest_agent
Note that the test response was manually edited based on a reply on my
bare-metal computer. It shows partial results due to pcieport driver not
being currently supported by QGA.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Tested-by: Han Han <hhan@redhat.com>
There might be more potential users around, I haven't looked thoroughly.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Tested-by: Han Han <hhan@redhat.com>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Tested-by: Han Han <hhan@redhat.com>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Tested-by: Han Han <hhan@redhat.com>
To match the QGA schema name (we are introducing a qemuAgentDiskInfo
struct again for different purpose).
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Tested-by: Han Han <hhan@redhat.com>
Even though it is technically possible, when running the migrations QEMU's
nbd-server-start errors out with:
"TLS is only supported with IPv4/IPv6"
We can always enable it when QEMU adds this feature, but for now it is safer to
show our error message rather than rely on QEMU to error out properly.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
When executing the hypervisor-cpu-baseline command and if there is
only a single CPU definition present in the XML file, then the
baseline handler will exit early and libvirt will print an unhelpful
message:
"error: An error occurred, but the cause is unknown"
This is due to no CPU definition ever being "baselined", since the
handler expects at least two CPU models.
Let's fix this by performing a CPU model expansion on the single CPU
definition and returning the result to the caller. This will also
ensure the CPU model's feature set is sane if any were provided in
the file.
Signed-off-by: Collin Walling <walling@linux.ibm.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
Check the provided CPU models against the CPU models
known by the hypervisor before baselining and print
an error if an unrecognized model is found.
Signed-off-by: Collin Walling <walling@linux.ibm.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
When executing the hypervisor-cpu-baseline command and the
XML file contains a CPU definition without a model name, or
an invalid CPU definition, then the commands will fail and
return an error message from the QMP response.
Let's clean this up by checking for a valid definition and
presence of a model name.
This code is copied from virCPUBaseline.
Signed-off-by: Collin Walling <walling@linux.ibm.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
Hypervisor-cpu-baseline requires the cpu-model-expansion
capability when expanding CPU model features if the
--features flag is provided.
Signed-off-by: Collin Walling <walling@linux.ibm.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
Forgetting to use the VIR_MIGRATE_TLS flag with migration can lead to
leak of sensitive information. Add an administrative knob to force use
of the flag.
Note that without VIR_MIGRATE_PEER2PEER, the migration is driven by an
instance of the client library which doesn't necessarily run on either
of the hosts so the flag can't be used to assume VIR_MIGRATE_TLS even
if it wasn't provided by the user instead of rejecting if it's not.
Resolves: https://gitlab.com/libvirt/libvirt/-/issues/67
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
qemu's internals were not prepared for switching to -blockdev for the
legacy storage migration. Add a proper error message since qemu is
unlikely to attempt fixing the old protocol.
Resolves: https://gitlab.com/libvirt/libvirt/-/issues/65
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Move and aggregate all the logic which is switched based on whether the
migration is tunnelled or not before other checks. Further checks will
be added later.
While the code is being moved the error message is put on a single line
per new coding style.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Our streams are not the best transport for migration data and we support
TLS for security now. It's unlikely that there will be enough motivation
to add a new migration protocol to tunnel NBD too.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Similarly to previous commit dealing with snapshots we must rewrite the
metadata of the previously-'current' checkpoint when changing which
checkpoint is considered 'current'.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Whether a snapshot definition is considered 'current' or active is
stored in the metadata XML libvirt writes when we create metadata.
This means that if we are changing the 'current' snapshot we must
re-write the metadata of the previously 'current' snapshot to update the
field to prevent having multiple active snapshots.
Unfortunately the snapshot creation code didn't do this properly, which
resulted in the following error:
error : qemuDomainSnapshotLoad:430 : internal error: Too many snapshots claiming to be current for domain snapshot-test
being printed if libvirtd was terminated and restarted.
Introduce qemuSnapshotSetCurrent which writes out the old snapshot's
metadata when updating the current snapshot.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
In some cases such as when creating an internal inactive snapshot we
know that the domain definition in the snapshot is equivalent to the
current definition. Additionally we set up the current definition for
the snapshotting but not the one contained in the snapshot. Thus in some
cases the caller knows better which def to use.
Make qemuDomainSnapshotForEachQcow2 take the definition by the caller
and copy the logic for selecting the definition to callers where we
don't know for sure that the above claim applies.
This fixes internal inactive snapshots when <disk type='volume'> is used
as we translate the pool/vol combo only in the current def.
Resolves: https://gitlab.com/libvirt/libvirt/-/issues/97
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Don't try to manipulate snapshots on network or unresolved volume backed
storage.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
'continue' the loop if the device is not a disk. Saving the level makes
one of the error messages fit on a single line.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The ESP SCSI controllers (NCR53C90, DC390, AM53C974) have the same
requirement as the LSI Logic controller for each disk to be set via
the scsi-id=NNN property, not the lun=NNN property.
Switching the code to use an enum will force authors to pay attention
to this difference when adding future SCSI controllers.
Reviewed-by: Laine Stump <laine@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
When introducing the API I've mistakenly used 'int' type for
@nkeys argument which does nothing more than tells the API how
many items there are in @keys array. Obviously, negative values
are not expected and therefore 'unsigned int' should have been
used.
Reported-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
