The "max" model can be treated the same way as "host" model in general.
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
This is a special CPU model similar to "-cpu host", so won't use our
normal CPU model detection logic.
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The logic applied in the ppc64 case isn't quite correct, as the
interpretation of maximum mode depends on whether hardware virt
is used or not. This is information the CPU driver doesn't have.
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The data reported is the same as for "host-passthrough"
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
For hardware virtualization this is functionally identical to the
existing host-passthrough mode so the same caveats apply.
For emulated guest this exposes the maximum featureset supported by
the emulator. Note that despite being emulated this is not guaranteed
to be migration safe, especially if different emulator software versions
are used on each host.
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The @vhostuser member of virStorageSource structure is allocated
during parsing in virDomainDiskSourceVHostUserParse() but never
freed leading to a memleak. Since the member is an object it has
to be unrefed instead of g_free()-d.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
There are few cases where STREQLEN() is called like this:
STREQLEN(var, string, strlen(string))
which is the same as STRPREFIX(var, string). Use STRPREFIX()
because it is more obvious what the check is doing.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>
A <machine/> element can have "deprecated" attribute that
corresponds to 'deprecated' member of _virQEMUCapsMachineType
struct. But the member is of boolean type. Therefore, the string
returned by virXMLPropString() must be freed.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
If parsing "maxCpus" attribute of <machine/> element fails an
error is printed but the corresponding string is not freed. While
it is very unlikely to happen (parsed XML is not user provided
and we are the ones generating it), it is possible. Instead of
freeing the variable in the error path explicitly, let's declare
it as g_autofree. And while I'm at it, let's bring it into the
loop where it's used.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The various virtproxyd socket files are generated with invalid syntax,
e.g. from virtproxyd.socket
[Unit]
Description=Libvirt proxy local socket
Before=virtproxyd.service
libvirtd.socket libvirtd-ro.socket libvirtd-admin.socket libvirtd-tcp.socket libvirtd-tls.socket
Note the missing 'Conflicts=' in the last line. Fix it by prepending
'Conflicts=' to libvirtd_socket_conflicts when adding virtproxyd
to virt_daemon_units.
Signed-off-by: Jim Fehlig <jfehlig@suse.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
On platforms that lack both getauxval() and elf_aux_info(),
such as OpenBSD and macOS, host CPU detection can't work.
https://gitlab.com/libvirt/libvirt/-/issues/121
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
No need to fetch the same information twice.
As a side effect, this solves a bug where, on platforms where
elf_aux_info() is used instead of getauxval(), we would not
make sure the CPUID feature is available before attempting to
use it.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
This header is not present on several non-Linux targets that
nonetheless support aarch64.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
A few commits back I've introduced new 'virtio-pmem' <memory/>
device. Since it's virtio it goes onto PCI bus. Therefore, on
hotplug new PCI address is generated (or provided one is
reserved). However, if hotplug fails (for whatever reason) the
address needs to be released. This is different to 'dimm' type of
address because for that type we don't keep a map of used slots
rather generate one on each address assign request. The map is
then thrown away. But for PCI addresses we keep internal state
and thus has to keep it updated. Therefore, this new
qemuDomainReleaseMemoryDeviceSlot() function is NOP for those
models which use 'dimm' address type ('dimm' and 'nvdimm').
While I'm at it, let's release the address in case of hot unplug.
Not that is supported (any such attempt fails with the following
error:
"virtio based memory devices cannot be unplugged"
But if QEMU ever implements hot unplug then we don't have to
remember to fix our code.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
Currently, nmdm console device requires user to specify master and slave
path attributes (such as /dev/nmdm0A and /dev/nmdm0B respectively).
However, making user find a non-occupied device name might be not
convenient, especially for the remote connections.
Update the logic to make these attributes optional. In case if not
specified, use /dev/nmdm$UUID[AB], where $UUID is a domain's UUID.
With this schema it's unlikely nmdm device will clash with other domains
or even other non-bhyve nmdm devices.
Signed-off-by: Roman Bogorodskiy <bogorodskiy@gmail.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
All of these options are actually supported by vhostuser disk so
we should allow them to be usable.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Currently, requesting domain capabilities fails when the specified
emulator binary does not equal to "/usr/sbin/bhyve". As we're
not using user-specified emulator anyway, drop this check to avoid
showing errors for values like "bhyve" (without absolute path).
Signed-off-by: Roman Bogorodskiy <bogorodskiy@gmail.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Get rid of the 'need_release' variable. The code can be rewritten
so that it is not needed.
Signed-off-by: Yi Li <yili@winhong.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
When deleting the vhostuserclient interface, OVS prompts that the interface does not exist,
Through the XML file, I found that the "target dev" has a '\n', results in an XML parsing error.
XML file:
<target dev='vm-20ac9c030a47
'/>
That is because 'ovs-vsctl' returns a newline result, always come with a '\n',
and the vircommandrun function puts it in ifname.
So virNetDevOpenvswitchGetVhostuserIfname should remove '\n' from ifname.
Signed-off-by: Yalei Li <liyl43@chinatelecom.cn>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
This function is only called from one place, and has, well... not a
*misleading* name, but it doesn't fit the standard frame of functions
that end in "Free" (it doesn't actually free the object pointed to by
its argument, but frees *some parts* of the content of the object).
Rather than try to think up an appropriate name, let's just move the
meat of this function into its one and only caller,
virNetLibsshSessionDispose(), which will allow us to convert its
VIR_FREEs into g_free in a future patch.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
virDomainCapsDispose() was the only caller of
virDomainCapsStringValuesFree(), which 1) didn't actually free the
object it was called with, but only cleared it, making it less
mechanical to convert from VIR_FREE to g_free (since it's not
immediately obvious from looking at virDomainCapsStringValuesFree()
that the pointers being cleared will never again be used).
We could have renamed the function to virDomainCapsStringValuesClear()
to side-step the confusion of what the function actually does, but
that would just make the upcoming switch from VIR_FREE to g_free
require more thought. But since there is only a single caller to the
function, and it is a vir*Dispose() function (indicating that the
object containing the virDomainCapsStringValues is going to be freed
immediately after the function finishes), and thus VIR_FREE() *could*
be safely replaced by g_free()), we instead just move the contents of
virDomainCapsStringValuesFree() into virDomainCapsDispose() (and
*that* function will be trivially converted in an upcoming
"mechanical" patch).
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
This is another *Free() function that doesn't free the object it is
passed. Instead it frees and clears some parts of the object.
In this case, the function is actually called from two places, and one
of them (virNetSSHSessionAuthReset) appears to be assuming that the
pointers actually *will* be cleared. So the proper thing to do here
(?) is to rename the function to virNetSSHSesionAuthMethodsClear().
(NB: virNetSSHSessionAuthReset is seemingly never called from
anywhere. Is this one of those functions that actually *is* called by
some strange MACRO invocation? Or it is truly one of those
"written-but-never-used" functions that can be deleted? (if the latter
is the case, then I would rather move the contents of
virNetSessionAuthMethodsFree() into its only other caller,
virNetSSHSessionDispose(), so that the VIR_FREEs could be replaced
with g_free.)
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
These functions are all only called as a part of qemuFirmwareFree(),
which frees the qemuFirmware object before return, so we can be sure
none of the pointers is referenced after freeing (and thus there is no
need to clear any of them).
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
These functions all cooperate to free memory pointed to by a single
object that contains (doesn't *point to*, but actually contains)
several sub-objects. They were written to send copies of these
sub-objects to subordinate functions, rather than just sending
pointers to the sub-objects.
Let's change these functions to just send pointers to the objects
they're cleaning out rather than all the wasteful and pointless
copying.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Several functions had the names virFirmware[something]Free(), but they
aren't taking a pointer to some object and freeing it. Instead, they
are making a copy of the content of an entire object, then Freeing the
objects pointed to by that content.
As a first step in a too-complicated cleanup just to eliminate a few
occurrences of VIR_FREE(), this patch renames those functions to more
accurately reflect what they do - they Free the *Content* of their
arguments.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
dhcpHostFree() and addnHostFree() don't follow the normal pattern of
*Free functions in the rest of libvirt code - they are actually more
similar to the *Dispose() functions, in that they free all subordinate
objects, but not the object pointed to by the argument
itself. However, the arguments aren't virObjects, so it wouldn't be
proper to name them *Dispose() either.
They *currently* behave similar to a *Clear() function, in that they
free all the subordinate objects and nullify the pointers of those
objects. HOWEVER, we don't actually need or want that behavior - the
two functions in question are only called as part of a higher level
*Free() function, and the pointers are not referenced in any way
between the time they are freed and when the parent object is freed.
So, since the current name isn't correct, nor is *Dispose(), and we
want to change the behavior in such a way that *Clear() also wouldn't
be correct, lets name the functions *FreeContent(), which is an
accurate description of what the functions do, and what we *want* them
to do.
And since it's such a small patch, we can go ahead and change that
behavior - replacing the VIR_FREEs with g_free.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Commit <318d807a0bd3372b634d1952b559c5c627ccfa5b> added a fix to skip
most of the block stat code to not log error message for missing storage
sources but forgot to increase the recordnr counter.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
The correct backend type is 'vc', same as in qemuBuildChrChardevStr()
where we generate qemu command line.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
A memory leak was identified in
virCgroupEnableMissingControllers():
==11680== at 0x483EAE5: calloc (vg_replace_malloc.c:760)
==11680== by 0x4E51780: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.6701.0)
==11680== by 0x4908618: virCgroupNew (vircgroup.c:701)
==11680== by 0x49096F4: virCgroupEnableMissingControllers (vircgroup.c:1146)
==11680== by 0x4909B17: virCgroupNewMachineSystemd (vircgroup.c:1228)
==11680== by 0x4909E94: virCgroupNewMachine (vircgroup.c:1313)
==11680== by 0x1694FDBC: qemuInitCgroup (qemu_cgroup.c:946)
==11680== by 0x1695046B: qemuSetupCgroup (qemu_cgroup.c:1083)
==11680== by 0x16A60126: qemuProcessLaunch (qemu_process.c:7077)
==11680== by 0x16A61504: qemuProcessStart (qemu_process.c:7384)
==11680== by 0x169B84C2: qemuDomainObjStart (qemu_driver.c:6590)
==11680== by 0x169B8776: qemuDomainCreateWithFlags (qemu_driver.c:6641)
What happens is that new virCgroup is created and stored into
@parent. Then, if @tokens is not empty the for() loop is entered
into where another virCgroup is created and @parent is replaced
with this new virCgroup. But nothing freed the old @parent.
Fixes: 77291414c7
Reported-by: Andrea Bolognani <abologna@redhat.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
Implements QEMU support for vhost-user-blk together with live
hotplug/unplug.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Make the function reusable by other vhost-user based devices.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Hypervisors are capable of reporting that some features are deprecated.
This should be used to mark a domain as tainted.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
QEMU has the ability to mark machine types as deprecated. This should be
exposed to management applications in the capabilities.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
QEMU has the ability to mark CPUs as deprecated. This should be exposed
to management applications in the domain capabilities.
This attribute is only set when the model is actually deprecated.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The use case VIR_ALLOC_VAR deals with is very unlikely. We had just 2
legitimate uses, which were reimplemented locally using g_malloc0 and
sizeof instead as they used a static number of members of the trailing
array.
Remove VIR_ALLOC_VAR since in most cases the direct implementation is
shorter and clearer and there are no users of it currently.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
In this case we need a 'struct ethtool_gfeatures' followed by two
'struct ethtool_get_features_block' so there's no risk of overflow.
Use g_malloc0 and sizeof() to allocate the memory instead of
VIR_ALLOC_VAR.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
In both cases we need memory for a 'struct sanlk_resource' followed by
one 'struct sanlk_disk', thus there's no risk of overflow.
Use g_malloc0 and sizeof() to allocate the memory instead of
VIR_ALLOC_VAR.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Use g_autofree to allow removal of 'cleanup:' and the 'ret' variable.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Use g_autofree and remove the 'cleanup' section and 'ret' variable.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Switch to the more common approach of having arrays allocated separately
rather than trailing the struct.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Users were replaced with virSecureEraseString with explicit freeing of
the memory.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
There are no users any more. The replacement is to use g_auto and
virSecureEraseString explicitly.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
In this instance attempting to be correct is really pointless since the
secret is formatted into another string which is not erased securely and
then put on the commandline.
Keep the secure handling for correctness.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
The macros are unused now and callers who care about clearing the memory
they use should use memset() appropriately.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Clear out the value using virSecureErase and free it with g_free so
that VIR_DISPOSE_N can be phased out.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Clear the key and IV structs using virSecureErase.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Clear out the value using virSecureErase and free it with g_free so
that VIR_DISPOSE_N can be phased out.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Switch the secret value to 'g_autofree' for handling of the memory and
clear it out using virSecureErase.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Phase out use of VIR_DISPOSE_N from the qemu driver. Use memset in the
appropriate cases.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Clear the secret right after use with virSecureErase.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
The code pretends that it cares about clearing the secret values, but
passes the secret value to a realloc, which may copy the value somewhere
else and doesn't sanitize the original location when it does so.
Since we want to construct a string from the value, let's copy it to a
new piece of memory which has the space for the 'NUL' byte ourselves, to
prevent a random realloc keeping the data around.
While at it, use virSecureErase instead of VIR_DISPOSE_N since it's
being phased out.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
The module will provide functions for disposing secrets stored in
memory.
Note that for now it's implemented using memset, which is not really
secure.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Shuffle the code around to remove the need for temporary variables and
labels for cleaning them.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
The check whether @keyfile is non-NULL is before locking @sess, but uses
the 'error' label which unlocks '@sess'.
While touching the error path, update the error message to be on one
line.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
When virRandomBytes fails we don't get any random bytes and even if we
did they don't have to be treated as secret as they weren't used in any
way.
Add a temporary variable with automatic freeing for the secret buffer
and assign it only on success.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
The list isn't secret which would need being disposed of. Just expand
the array and return failure when adding the NULL terminator similarly
to how we expand the list for adding devices in a loop.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
The struct doesn't contain any secrets to clear before freeing and even
if it did VIR_DISPOSE_N wouldn't help as the struct contains only
pointers thus the actual memory pointing to isn't sanitized.
Just free the params array pointer and then the struct itself.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Pass the parameter clock rt to qemu to ensure that the
virtual machine is not synchronized with the host time
Signed-off-by: gongwei <gongwei@smartx.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
The headers weren't removed after use of VIR_STRDUP was removed.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
"f + 0.5" does not round correctly for values very close to
".5" for every integer multiple, e.g. "0.499999975".
Found by clang-tidy's "bugprone-incorrect-roundings" check.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
`udevGetIntSysfsAttr` does not necessarily write to the third parameter,
even when it returns 0.
This was found by clang-tidy's
"clang-analyzer-core.UndefinedBinaryOperatorResult" check.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
This was found by clang-tidy's
"clang-analyzer-security.insecureAPI.bzero" check.
bzero is marked as deprecated ("LEGACY") in POSIX.1-2001 and
removed in POSIX.1-2008.
Besides its deprecation, bzero can be unsafe to use under certain
circumstances, e.g. when used to zero-out memory containing secrects.
These calls can be optimized away by the compiler, if it concludes no
further access happens to the memory, thus leaving the secrets still
in memory. Hence its classification as "insecureAPI".
Signed-off-by: Tim Wiederhake <twiederh@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
This was found by clang-tidy's "readability-misleading-indentation"
check.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
This was found by clang-tidy's "readability-misleading-indentation"
check.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
This section is guarded by "#ifndef WIN32" in line 2109--2808.
Found by clang-tidy's "readability-redundant-preprocessor" check.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Co-authored-by: Sri Ramanujam <sramanujam@datto.com>
Signed-off-by: Matt Coleman <matt@datto.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
This patch takes on one set of examples of unnecessary use of
VIR_FREE() when g_free() is adequate - it modifies only vir*Free()
functions within the conf directory that take a single pointer and
free the object pointed to by that argument before returning. The
modification is to replace VIR_FREE() with g_free() for the object
itself *and* for all subordinate chunks of memory pointed to by that
object.
(NB: there are other functions that VIR_FREE subordinate memory of
objects that end up being freed before return (also sometimes with
VIR_FREE); I am purposefully ignoring those to reduce scope and focus
on a sub class where the pointlessness is obvious.)
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
usually a function call vir*Free() will take a single pointer to an
object as its argument, and will then free all resources associated
with that object, including the object
itself. virStorageEnctyptionInfoDefFree() doesn't do that - it frees
all the subordinate resources of the ojbect, but doesn't free the
object itself; usually a function like that is called
vir*Clear(). Let's rename this function to not be misleading.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
There is no point in setting the interface model to unknown during
virDomainNetDefFree(), since we are about to free the object anyway
(and the model isn't used anywhere in the rest of the function).
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
The memory containing the pointer is going to be freed momentarily anyway.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
This function clears out and frees a virDomainZPCIAddressIds object,
so that's that's what it should take as its argument, *not* the
pointer to a parent object that contains the object we want to free.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Back in commit 2c71d3826, which appeared in libvirt-1.2.3 in April
2014, the location used to store saved MAC addresses and vlan tags of
SRIOV VFs was changed from /var/run/libvirt/qemu to
/var/run/libvirt/hostdevmgr. For backward compatibility the code was
made to continue looking in the old location for the files when it
didn't find them in the new location.
It's now been 6 years, and even if there was somebody still running
libvirt-1.2.3 on their system, that system would now be out of support
for libvirt, so there would be no way for them to upgrade to a new
libvirt that no longer looks in "oldStateDir" for the files. So
let's no longer look in "oldStateDir" for the files!
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
virGetConnectNetwork() calls
virGetConnectGeneric(), which calls
virConnecCacheInitialize(), which is actually a call (only once) to
virConnectCacheOnceInit() which calls
virThreadLocalInit() several times, which calls
pthread_key_create()
If pthread_key_create() fails, it (of course) doesn't log an error
(because it's not a part of libvirt), nor does any other function on
the call chain all the way up to virGetConnectNetwork(). But none of
the callers of virGetConnectNetwork() log an error either, so it is
possible that an API could fail due to virGetConnectNetwork() failing,
but would only log "an error was encountered, but the cause is
unknown. Deal with it." (paraphrasing).
(In all likelyhood, virConnectCacheOnceInit() is going to be called at
some earlier time, and almost certainly pthread_key_create() will
never fail (and if it does, the user will have *much* bigger problems
than an obtuse error message from libvirt)).
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
posix_fallocate() might be not supported by a filesystem, for example,
it's not supported by ZFS. In that case it fails with
return code 22 (EINVAL), and thus safezero_posix_fallocate() returns -1.
As safezero_posix_fallocate() is the first function tried by safezero()
and it tries other functions only when it returns -2, it fails
immediately without falling back to other methods, such as
safezero_slow().
Fix that by returning -2 if posix_fallocate() returns EINVAL, to give
safezero() a chance to try other functions.
Signed-off-by: Roman Bogorodskiy <bogorodskiy@gmail.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
The current virPCIDeviceNew() signature, receiving 4 uints in sequence
(domain, bus, slot, function), is not neat.
We already have a way to represent a PCI address in virPCIDeviceAddress
that is used in the code. Aside from the test files, most of
virPCIDeviceNew() callers have access to a virPCIDeviceAddress reference,
but then we need to retrieve the 4 required uints (addr.domain, addr.bus,
addr.slot, addr.function) to satisfy virPCIDeviceNew(). The result is
that we have extra verbosity/boilerplate to retrieve an information that
is already available in virPCIDeviceAddress.
A better way is presented by virNVMEDeviceNew(), where the caller just
supplies a virPCIDeviceAddress pointer and the function handles the
details internally.
This patch changes virPCIDeviceNew() to receive a virPCIDeviceAddress
pointer instead of 4 uints.
Reviewed-by: Laine Stump <laine@redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Instead of receiving 4 uints in order and write domain/bus/slot/function,
receive a virPCIDeviceAddressPtr instead and write into it.
This change will allow us to simplify the API for virPCIDeviceNew()
in the next patch.
Reviewed-by: Laine Stump <laine@redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
libxlNodeDeviceGetPCIInfo() and qemuNodeDeviceGetPCIInfo() are equal.
Let's move the logic to a new virDomainDriverNodeDeviceGetPCIInfo()
info to be used by libxl_driver.c and qemu_driver.c.
Reviewed-by: Laine Stump <laine@redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
There is no need to open code the PCI address string format
when we have a function that does exactly that.
Reviewed-by: Laine Stump <laine@redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Further testing revealed commit f035f53baa regresses Debian bug 955216
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=955216
Restarting libvirt-guests on libvirtd restart is worse than the original
dependency issue, so revert the commit until a better solution is found.
This reverts commit f035f53baa.
Signed-off-by: Jim Fehlig <jfehlig@suse.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Wire up the QEMU command line for this option.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Tested-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Rename virDomainCheckVirtioOptions into
virDomainCheckVirtioOptionsAreAbsent since it checks if all
virtio options are absent. The old name was very misleading.
Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Add virtio related options iommu, ats and packed as driver element attributes
to vsock devices. Ex:
<vsock model='virtio'>
<cid auto='no' address='3'/>
<driver iommu='on'/>
</vsock>
Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
The virDomainVirtioOptionsCheckABIStability() function is called
from various ABI stability check functions. Every caller checks
if both old and new definitions have virtio options set and only
after that they call the function. This is suboptimal because:
a) this check can be done in the function itself (making all
callers shorter),
b) is inherently wrong, because it doesn't catch case where one
definition has virtio options set and the other doesn't.
Do proper checks at the beginning of the function and simplify
its calls.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The previous commit rendered this function empty and needless.
Remove it.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The aim of virDomainCheckVirtioOptions() function is to check
whether no virtio options are set, i.e. no @iommu no @ats and no
@packed attributes were present in given device's XML (yeah, the
function has very misleading name). Nevertheless, this kind of
check belongs to validation phase, but now is done in post parse
phase. Move the function and its calls to domain_validate.c so
that future code is not tempted to repeat this mistake.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The qemuDomainObjFromDomain() API must be paired with
the virDomainObjEndAPI API. The qemuDomainAuthorizedSSHKeysGet
method simply did 'return -1' leaking a reference and lock
in two paths.
The qemuDomainAuthorizedSSHKeysSet method marked the object
as an autoptr while also have some code paths that will call
virDomainObjEndAPI. As a result the object will be released
but not unlocked in error paths.
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
If QEMU driver fails to initialize for whatever reason (it can be
as trivial as a typo on qemu.conf), the control jumps to error
label in qemuStateInitialize() where qemuStateCleanup() is called
which frees the driver. But the daemon then asks drivers to
prepare for shutdown, which in case of QEMU driver is implemented
in qemuStateShutdownPrepare(). In here, the driver is
dereferenced but since it was freed earlier, the pointer is NULL
which leads to instant crash.
Solution is simple - just check if qemu_driver is not NULL. But
doing so only in qemuStateShutdownPrepare() would push the
problem down to virStateShutdownWait(), well
qemuStateShutdownWait(). Therefore, duplicate the trick there
too.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1895359#c14
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
All callers of this function called virStorageFileParseChainIndex
before. Internalize the logic of that function to prevent multiple calls
and passing around unnecessary temporary variables.
This is achieved by calling virStorageFileParseBackingStoreStr and using
it to fill the values internally.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
A terminated chain has a virStorageSource with type ==
VIR_STORAGE_TYPE_NONE at the end. Since virStorageSourceHasBacking
is explicitly returning false in that case we'd probe the chain
needlessly. Just check whether src->backingStore is non-NULL.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
'virDomainDiskGetSource' returns src->path effectively. Checking whether
a disk is empty is done via 'virStorageSourceIsEmpty'.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
Use g_autoptr for the hash table and remove the 'ret' variable.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
The parsers for the backing store strings are relatively self-contained
and rather massive piece of code. Move them to a new module called
storage_source_backingstore.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
There are no other files using it. Move it and make the functions
static.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
Rename the function to virStorageSourceFetchRelativeBackingPath and
return relative paths only. The function is only used to restore the
relative relationship between images so there's no need for it to be
universal.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
Move it together with virStorageSourceGetRelativeBackingPath which is
the main reason why it exists. Upcoming patch will modify the comment
and arguments refering to virStorageSourceGetRelativeBackingPath so it's
better if they are together.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
As explained in QEMU commit 4c257911dcc7c4189768e9651755c849ce9db4e8
intel-pt features should never be included in the CPU models as it was
not supported by KVM back then and even once it started to be supported,
users have to enable it by passing pt_mode=1 parameter to kvm_intel
module. The Icelake-* CPU models with intel-pt included were added to
QEMU 3.1.0 and removed right in the following 4.0.0 release (and even in
3.1.1 maintenance release).
In libvirt 6.10.0 I introduced 'removed' attribute for features included
in our CPU model definitions which we can use to drop intel-pt from
Icelake-* CPU models. Back then I explained we can safely do so only for
features which could never be enabled, which is not the case of intel-pt.
Theoretically, it could be possible to create an environment in which
QEMU would enable intel-pt without asking for it explicitly: it would
need to use a new enough kernel (not available at the time of QEMU
3.1.0) and pt_mode KVM parameter in combination with QEMU 3.1.0 running
a domain with q35 machine type and all that on a CPU which didn't really
exist at that time.
Migrating such domain to a host with newer SW stack including libvirt
with this patch applied would result in incompatible guest ABI (the
virtual CPU would lose intel-pt). However, QEMU changed its CPU models
unconditionally and thus migration would not work even without this
patch. That said, it is safe to follow QEMU and remove the feature from
Icelake-* CPU models in our cpu_map.
https://bugzilla.redhat.com/show_bug.cgi?id=1853972
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Tim Wiederhake <twiederh@redhat.com>
dtrace invokes the C compiler, so when cross-building we need
to make sure that $CC is set in the environment and that it
points to the cross-compiler rather than the native one.
Until https://github.com/mesonbuild/meson/issues/266
is addressed, the workaround is to call dtrace via env(1).
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=980334
Signed-off-by: Helmut Grohne <helmut@subdivi.de>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
virPCIGetNetName is used to get the name of the netdev associated with
a particular PCI device. This is used when we have a VF name, but need
the PF name in order to send a netlink command (e.g. in order to
get/set the MAC address of the VF).
In simple cases there is a single netdev associated with any PCI
device, so it is easy to figure out the PF netdev for a VF - just look
for the PCI device that has the VF listed in its "virtfns" directory;
the only name in the "net" subdirectory of that PCI device's sysfs
directory is the PF netdev that is upstream of the VF in question.
In some cases there can be more than one netdev in a PCI device's net
directory though. In the past, the only case of this was for SR-IOV
NICs that could have multiple PF's per PCI device. In this case, all
PF netdevs associated with a PCI address would be listed in the "net"
subdirectory of the PCI device's directory in sysfs. At the same time,
all VF netdevs and all PF netdevs have a phys_port_id in their sysfs,
so the way to learn the correct PF netdev for a particular VF netdev
is to search through the list of devices in the net subdirectory of
the PF's PCI device, looking for the one netdev with a "phys_port_id"
matching that of the VF netdev.
But starting in kernel 5.8, the NVIDIA Mellanox driver began linking
the VFs' representor netdevs to the PF PCI address [1], and so the VF
representor netdevs would also show up in the net
subdirectory. However, all of the devices that do so also only have a
single PF netdev for any given PCI address.
This means that the net directory of the PCI device can still hold
multiple net devices, but only one of them will be the PF netdev (the
others are VF representors):
$ ls '/sys/bus/pci/devices/0000:82:00.0/net'
ens1f0 eth0 eth1
In this case the way to find the PF device is to look at the
"phys_port_name" attribute of each netdev in sysfs. All PF devices
have a phys_port_name matching a particular regex
(p[0-9]+$)|(p[0-9]+s[0-9]+$)
Since there can only be one PF in the entire list of devices, once we
match that regex, we've found the PF netdev.
[1] - https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/
commit/?id=123f0f53dd64b67e34142485fe866a8a581f12f1
Co-Authored-by: Moshe Levi <moshele@nvidia.com>
Signed-off-by: Dmytro Linkin <dlinkin@nvidia.com>
Reviewed-by: Adrian Chiris <adrianc@nvidia.com>
Reviewed-by: Laine Stump <laine@redhat.com>
This commit add virNetDevGetPhysPortName to read netdevice
phys_port_name from sysfs. It also refactor the code so
virNetDevGetPhysPortName and virNetDevGetPhysPortID will use
same method to read the netdevice sysfs.
Signed-off-by: Moshe Levi <moshele@nvidia.com>
Reviewed-by: Laine Stump <laine@redhat.com>
Fixes a memory leak when hypervCreateInvokeParamsList() fails.
Signed-off-by: Matt Coleman <matt@datto.com>
Reviewed-by: Laine Stump <laine@redhat.com>
