There is no need to do that since both fallible functions do that already.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Callers that already do this anyway can be cleaned up thanks to this and the one
that does not (daemon startup) gains the benefit of the error being printed to
standard error output changing:
LIBVIRT_LOG_OUTPUTS=1:invalid libvirtd
/home/nert/dev/libvirt/upstream/build/src/libvirtd: initialisation failed
into:
LIBVIRT_LOG_OUTPUTS=1:invalid libvirtd
libvirt: error : invalid argument: Invalid destination 'invalid' for output '1:invalid'
/home/nert/dev/libvirt/upstream/build/src/libvirtd: initialisation failed
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
And make callers check the return value as well. This helps error out early for
invalid environment variables.
That is desirable because it could lead to deadlocks. This can happen when
resetting logging after fork() reports translated errors because gettext
functions are not reentrant. Well, it is not limited to resetting logging after
fork(), it can be any translation at that phase, but parsing environment
variables is easy to make fail on purpose to show the result, it can also happen
just due to a typo.
Before this commit it is possible to deadlock the daemon on startup
with something like:
LIBVIRT_LOG_FILTERS='1:*' LIBVIRT_LOG_OUTPUTS=1:stdout libvirtd
where filters are used to enable more logging and hence make the race less rare
and outputs are set to invalid
Combined with the previous patches this changes
the following from:
...
<deadlock>
to:
...
libvirtd: initialisation failed
The error message is improved in future commits and is also possible thanks to
this patch.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
This prevents starting any daemons with improper logging settings. This is
desirable on its own, but will be even more beneficial when more functions start
reporting errors and failing on them, coming up in following patches
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
The only difference is that we are not going to be guaranteed that the mutex is
normal (as opposed to recursive, although there is no system known to me that
would default to recursive mutexes), but that was done only to find occasional
errors (during runtime, back in 2010, commit 336fd879c00b). Functions using
this mutex are mostly stable and unchanging, and it makes the virLogOnceInit()
function only return 0 (or possibly abort in glib calls). On top of that we can
assume that the virLogMutex is always initialized which enables us to be more
consistent in some early error reporting.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Set a launch secret in guest memory using the sev-inject-launch-secret
QMP API. Only supported with qemu >= 6.0.0 and SEV-enabled guests in a
paused state.
Signed-off-by: Jim Fehlig <jfehlig@suse.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
The 'sev-inject-launch-secret' qmp command is only available with
qemu >= 6.0.0. Introduce a capability for sev-inject-launch-secret.
Signed-off-by: Jim Fehlig <jfehlig@suse.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
This API allows setting a launch secret within a guests's memory. The
launch secret is created by the guest owner after retrieving and
verifying the launch measurement with virDomainGetLaunchSecurityInfo.
The API uses virTypedParameter for input, allowing it to be expanded
to support other confidential computing technologies. In the case of
SEV, a basic guest launch workflow is described in the SEV API spec
in section "1.3.1 Launch"
https://www.amd.com/system/files/TechDocs/55766_SEV-KM_API_Specification.pdf
Signed-off-by: Jim Fehlig <jfehlig@suse.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
As described in the previous commit, the units for 'burst' are
kibibytes and not kilobytes, i.e. multiples of 1024 not 1000.
Therefore, when constructing ovs-vsctl command the burst value
must be multiplied by 1024 and not just 1000. And because ovs
expects this size in bits the value has to be multiplied again by
8.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1510237#c26
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
The burst attribute for bandwidth specifies how much bytes can be
transmitted in a single burst. Therefore, the unit is in
multiples of 1024 (thus kibibytes) not SI-like 1000. It has
always been like that.
The 'tc' output is still confusing though, for instance:
# tc class add dev $DEV parent 1: classid 1:1 htb rate 1000kbps burst 2097152
# tc class show dev vnet2
class htb 1:1 root rate 8Mbit ceil 8Mbit burst 2Mb cburst 1600b
Please note that 2097152 = 2*1024*1024. Even the man page is
confusing. From tc(8):
kb or k Kilobytes
mb or m Megabytes
But I guess this is because 'tc' predates IEC standardisation of
binary multiples and thus can't change without breaking scripts
parsing its output.
And while at it, adjust _virNetDevBandwidthRate struct member
description, to make it obvious which members use SI/IEC units.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
There are few places in virDomainDefFormatFeatures() which can
use virXMLFormatElement() or virXMLFormatElementEmpty() instead
of writing directly into the output buffer.
After this, there are still a lot of places left, but that is
much bigger task.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
According to ioctl_ficlonerange(2)
These ioctl operations [FICLONE and FICLONERANGE] first
appeared in Linux 4.5. They were previously known as
BTRFS_IOC_CLONE and BTRFS_IOC_CLONE_RANGE, and were private
to Btrfs.
We no longer target any distro that comes with a kernel older
than 4.5, so we can stop looking for the btrfs and xfs
specific versions of the constant and just use the generic
version directly.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Neal Gompa <ngompa13@gmail.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
The virt_socket_lib is built from virnetsocket.c (among others).
But this file includes virprobe.h which includes libvirt_probes.h
which is a generated file. But this dependency is not recorded in
meson which may lead to a failed build.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
The virDomainTPMDefFormat() function can't fail really. There's
no point in it returning an integer then. Make it return void and
fix both places which check for its retval.
And while at it, turn @def into a const pointer to make it
obvious the function does not modify passed struct.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
The <tpm/> element formatting is handled in
virDomainTPMDefFormat() which uses the "old style" - appending
strings directly into the output buffer. With this, it's easy to
get conditions that tell when an element has ended wrong. In this
particular case, if both <encryption/> and <active_pcr_banks/>
are to be formatted the current code puts a stray '>' into the
output buffer, resulting in invalid XML.
Rewrite the function to use virXMLFormatElement() which is more
clever.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2016599#c15
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Changing <active_pcr_banks/> means changing the guest ABI and as
such must be prevented on both restoring from a file or
migration.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2035888
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
The new helper replaces the 'value' part of the key-value tuple in an
object. The advantage of this new helper is that it preserves the
ordering of the key in the object when compared to a combination of
stealing the old key and adding a new value. This will be needed for a
new test/helper for validating and modifying qemu capabilities data.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
We need libparted to be available at build time otherwise we
can't link against it; we don't, however, need the parted
command to be present until runtime and, just as is the case
for other commands, we already perform a lookup through the
virCommand API so making sure it's available at build time
is unnecessary.
This doesn't make any difference for platform such as Fedora
and CentOS, where both the library and the command are in the
same package, but others like Debian, Ubuntu and openSUSE
have separate packages for the two components and this change
means that we can install one less package at build time.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
As encryption norms get more strict it's easy to fall on the
insecure side. For instance, so far we are generating 2048 bits
long prime for Diffie-Hellman keys. Some systems consider this
not long enough. While we may just keep increasing the value
passed to the corresponding gnutls_* function, that is not well
maintainable. Instead, we may do what's recommended in the
gnutls_* manpage. From gnutls_dh_params_generate2(3):
It is recommended not to set the number of bits directly, but
use gnutls_sec_param_to_pk_bits() instead.
Looking into the gnutls_sec_param_to_pk_bits() then [1], 2048
bits corresponds to parameter MEDIUM.
1: https://www.gnutls.org/manual/gnutls.html#tab_003akey_002dsizes
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ani Sinha <ani@anisinha.ca>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
According to the gnutls_dh_set_prime_bits() manpage:
The function has no effect in server side.
Therefore, don't call it when creating server side context.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ani Sinha <ani@anisinha.ca>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
If needed 'virJSONValueIsNull' can be easily replaced by
'virJSONValueGetType(obj) == VIR_JSON_TYPE_NULL'.
'virJSONValueObjectIsNull' has confusing name because it checks that a
virJSONValue of OBJECT type has a key which is NULL, not that the object
itself is NULL. This can be replaced according to the needs e.g. by
virJSONValueObjectHasKey or the above check.
Both are unused.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Tim Wiederhake <twiederh@redhat.com>
Replace the function by a call to virJSONValueNewString, when we copy
the string using g_strndup. Remove the unused helper.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Tim Wiederhake <twiederh@redhat.com>
With 'g_strdup' not needing error handling we can ask callers to pass a
copy of the string which will be adopted by the JSON value.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Tim Wiederhake <twiederh@redhat.com>
In two instances we've created a string virJSONValue just to append it
to the array. Replace it by use of the virJSONValueArrayAppendString
helper.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Tim Wiederhake <twiederh@redhat.com>
The auth mode array is static, parse it from a JSON string.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Tim Wiederhake <twiederh@redhat.com>
In cases where the qcow2 image is using subclusters/extended_l2 entries
we should propagate them to the new images which are based on such
images.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
In order to be able to propagate image configuration to newly formatted
images we need to be able to query it.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Allow creating the qcow2 with the new subcluster format if required.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Add machinery for probing the incompatible feature flags field and
specifically extract whether the extended l2 feature (1 << 4) is
present.
For now we don't care abot the other features.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Prepare for extraction of features from the 'incompatible features'
group.
This is done by moving the extraction loop into a new function called
qcow2GetFeaturesProcessGroup. The new function also allows to ingore
features we don't care about by passing VIR_STORAGE_FILE_FEATURE_LAST as
the target flag.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
QCOW2 images now support 'extended_l2' which splits the default clusters
into 32 subcluster allocation units. This allows the allocation units to
be smaller without increasing the size of L2 table too much and thus also
the cache requirements for holding the full L2 table in memory.
Unfortunately it's incompatible with qemu versions older than 5.2 thus
can't be used as default.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The function was formatted weirdly which prompted additions to conform
to the unusual style.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
After previous commit, it's no longer possible to change nodeset
for strict numatune. Therefore, we can start generating
host-nodes onto command line again.
This partially reverts d73265af6ec41104c20633b5c0a23688a62105e6.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
Let's imagine a guest that's configured with strict numatune:
<numatune>
<memory mode='strict' nodeset='0'/>
</numatune>
For guests with NUMA:
Depending on machine type used (see commit v6.4.0-rc1~75) we
generate either:
1) -object '{"qom-type":"memory-backend-ram","id":"ram-node0",\
"size":20971520,"host-nodes":[0],"policy":"preferred"}' \
-numa node,nodeid=0,cpus=0,memdev=ram-node0
or
2) -numa node,nodeid=0,cpus=0,mem=20480
Later, when QEMU boots up and cpuset CGroup controller is
available we further restrict QEMU there too. But there's a
behaviour difference hidden: while in case 1) QEMU is restricted
from beginning, in case 2) it is not and thus it may happen that
it will allocate memory from different NUMA node and even though
CGroup will try to migrate it, it may fail to do so (e.g. because
memory is locked). Therefore, one can argue that case 2) is
broken. NB, case 2) is exactly what mode 'restrictive' is for.
However, in case 1) we are unable to update QEMU with new
host-nodes, simply because it's lacking a command to do so.
For guests without NUMA:
It's very close to case 2) from above. We have commit
v7.10.0-rc1~163 that prevents us from outputting host-nodes when
generating memory-backend-* for system memory, but that simply
allows QEMU to allocate memory anywhere and then relies on
CGroups to move it to desired location.
Due to all of this, there is no reliable way to change nodeset
for mode 'strict'. Let's forbid it.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
The whole idea of VIR_DOMAIN_NUMATUNE_MEM_RESTRICTIVE is that the
memory location is restricted only via CGroups and thus can be
changed on the fly (which is exactly what
qemuDomainSetNumaParamsLive() does. Allow this mode there then.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
Set the kernel-hashes property on the sev-guest object if the config
asked for it explicitly. While QEMU machine types currently default to
having this setting off, it is not guaranteed to remain this way.
We can't assume that the QEMU capabilities were generated on an AMD host
with SEV, so we must force set the QEMU_CAPS_SEV_GUEST. This also means
that the 'sev' info in the qemuCaps struct might be NULL, but this is
harmless from POV of testing the CLI generator.
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
This sev-guest object property indicates whether QEMU should
expose the kernel, ramdisk, cmdline hashes to the firmware
for measurement.
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Normally the SEV measurement only covers the firmware
loader contents. When doing a direct kernel boot, however,
with new enough OVMF it is possible to ask for the
measurement to cover the kernel, ramdisk and command line.
It can't be done automatically as that would break existing
guests using direct kernel boot with old firmware, so there
is a new XML setting allowing this behaviour to be toggled.
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The VNC password authentication scheme is quite horrendous in that it
takes the user password and directly uses it as a DES case. DES is a
byte 8 keyed cipher, so the VNC password can never be more than 8
characters long. Anything over that length will be silently dropped.
We should validate this length restriction when accepting user XML
configs and report an error. For the global VNC password we don't
really want to break daemon startup by reporting an error, but
logging a warning is worthwhile.
https://bugzilla.redhat.com/show_bug.cgi?id=1506689
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Over time, the code using them got split into other files.
(Mostly qemu_interface.c and qemu_process.c)
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Ani Sinha <ani@anisinha.ca>
'virDomainDiskInsert' orders the inserted disks by target. If the target
is not provided though it would try to parse it anyways. This lead to a
crash when parsing a definition where there are multiple disks and of
two disks sharing the bus at least one also misses the target.
Since we want to actually use the parser for stuff which doesn't
necessarily need the disk target, we make virDomainDiskInsert tolerant
of missing target instead. The definition will be rejected by the
validator regardless of the order the disks were inserted in.
Fixes: 61fd7174
Closes: https://gitlab.com/libvirt/libvirt/-/issues/257
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Commit 52521de8332c2323bd ("qemu: Use qemuDomainSaveStatus") replaced a call
to virDomainObjSave() with qemuDomainSaveStatus() as a part of cleanup. Since
qemuDomainSaveStatus() does not indicate any failure through its return code,
the error handling cleanup code got eliminated in the process. Thus upon
failure, we will no longer killing the started qemu process. This commit fixes
this by reverting the change that was introduced with the above commit.
Fixes: 52521de8332c2323bd ("qemu: Use qemuDomainSaveStatus")
Signed-off-by: Ani Sinha <ani@anisinha.ca>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
Now that we only check whether the dnsmasq version is new enough,
there is no need for the caps field.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Laine Stump <laine@redhat.com>
