Commit 76f4788932 made qemuNodeDeviceDetachFlags() unusable due to an
'if then else if' chain that will always results in a 'return -1',
regardless of 'driverName' input.
Found by Coverity.
Fixes: 76f4788932
Reported-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Just return when alias is null and Remove the 'ret' variable.
Signed-off-by: Yi Li <yili@winhong.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Ján Tomko <jtomko@redhat.com>
This script works under two specific conditions. For each opened file,
search for all functions that has ACL calls and store them, and see
if there is a vir*DriverPtr struct declared in it. For each implementation
found, check if there is an ACL verification inside it, and error out if
none was found. The script also supports the concept of stub, where another
function takes the responsibility for the ACL call instead of the
original API.
Unfortunately this is not enough to cover the new scenario we have now,
with domain_driver.c containing helper functions that execute the ACL
calls. The script does not store state between files because, until now,
it wasn't needed to - APIs and stubs and vir*DriverPtr declarations were
always in the same file. Also, the script will not check for ACL in functions
that does not belong to a vir*DriverPtr interface. What we have now in
domain_driver.c breaks both assumptions: the functions are in a different
file, and there is no vir*DriverPtr being implemented in the file that
uses these functions.
This patch changes check-aclrules.py to accomodate this scenario. The helpers
that have ACL checks are stored beforehand in aclFuncHelpers, allowing other
files to use them to recognize a stub situation. In case the current file
being analyzed is domain_driver.c itself, we'll do a manual check using
aclFuncHelpers to verify that these functions indeed have ACL checks.
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
libxlNodeDeviceDetachFlags() and qemuNodeDeviceDetachFlags() are mostly
equal, aside from how the virHostdevmanager pointer is retrieved and
the PCI stub driver used.
Now that the PCI stub driver verification is done early in both functions,
we can use the virDomainDriverNodeDeviceDetachFlags() helper to reduce
code duplication between them. 'driverName' is checked inside the helper
to set the appropriate stub driver.
The helper is named with the 'Flags' suffix, even when the helper itself
isn't receiving the flags from the callers, to be compliant with the
ACL function virNodeDeviceDetachFlagsEnsureACL() that is being called
inside it and was called from the original functions. Renaming the helper
would implicate in renaming REMOTE_PROC_NODE_DEVICE_DETACH_FLAGS, and all the
related structs inside remote_protocol.x, to be compliant with the ACL
rules.
This is not being checked at this moment, but we'll fix check-aclrules.py to
verify all the helpers that calls ACL functions in domain_driver.c shortly.
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
The validation of 'driverName' does not depend on any other state and can be
done right on the start of the function. We can fail earlier while avoiding
a cleanup jump.
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
The validation of 'driverName' does not depend on any other state and can be
done right on the start of the function. We can fail earlier while avoiding
a cleanup jump.
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
libxlNodeDeviceReAttach() and qemuNodeDeviceReAttach() are mostly equal,
differing only how the virHostdevManager pointer is retrieved.
Put the common code into virDomainDriverNodeDeviceReAttach() to reduce
code duplication.
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Next patch will use g_autoptr() with virNodeDevicePtr for cleanups.
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
libxlNodeDeviceReset() and qemuNodeDeviceReset() are mostly equal,
differing only how the virHostdevManager pointer is retrieved.
Put the common code into virDomainDriverNodeDeviceReset() to reduce
code duplication.
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Setting the system time backward would lead to a
multiplication overflow in function virKeepAliveStart.
The function virKeepAliveTimerInternal got the same bug too.
Backtrace below:
#0 0x0000ffffae898470 in raise () from /usr/lib64/libc.so.6
#1 0x0000ffffae89981c in abort () from /usr/lib64/libc.so.6
#2 0x0000ffffaf9a36a8 in __mulvsi3 () from /usr/lib64/libvirt.so.0
#3 0x0000ffffaf8fd9e8 in virKeepAliveStart (ka=0xaaaaf954ce10, interval=interval entry=0,
count=count entry=0) at ../../src/rpc/virkeepalive.c:283
#4 0x0000ffffaf908560 in virNetServerClientStartKeepAlive (client=0xaaaaf954cbe0)
at ../../src/rpc/virnetserverclient.c:1628
#5 0x0000aaaac57eb6dc in remoteDispatchConnectSupportsFeature (server=0xaaaaf95309d0,
msg=0xaaaaf9549d90, ret=0xffff8c007fc0, args=0xffff8c002e70, rerr=0xffff9ea054a0,
client=0xaaaaf954cbe0) at ../../src/remote/remote_daemon_dispatch.c:5063
#6 remoteDispatchConnectSupportsFeatureHelper (server=0xaaaaf95309d0, client=0xaaaaf954cbe0,
msg=0xaaaaf9549d90, rerr=0xffff9ea054a0, args=0xffff8c002e70, ret=0xffff8c007fc0)
at ./remote/remote_daemon_dispatch_stubs.h:3503
#7 0x0000ffffaf9053a4 in virNetServerProgramDispatchCall(msg=0xaaaaf9549d90, client=0xaaaaf954cbe0,
server=0x0, prog=0xaaaaf953a170) at ../../src/rpc/virnetserverprogram.c:451
#8 virNetServerProgramDispatch (prog=0xaaaaf953a170, server=0x0, server entry=0xaaaaf95309d0,
client=0xaaaaf954cbe0, msg=0xaaaaf9549d90) at ../../src/rpc/virnetserverprogram.c:306
#9 0x0000ffffaf90a6bc in virNetServerProcessMsg (msg=<optimized out>, prog=<optimized out>,
client=<optimized out>, srv=0xaaaaf95309d0) at ../../src/rpc/virnetserver.c:137
#10 virNetServerHandleJob (jobOpaque=0xaaaaf950df80, opaque=0xaaaaf95309d0)
at ../../src/rpc/virnetserver.c:154
#11 0x0000ffffaf812e14 in virThreadPoolWorker (opaque=<optimized out>)
at ../../src/util/virthreadpool.c:163
#12 0x0000ffffaf81237c in virThreadHelper (data=<optimized out>) at ../../src/util/virthread.c:246
#13 0x0000ffffaea327ac in ?? () from /usr/lib64/libpthread.so.0
#14 0x0000ffffae93747c in ?? () from /usr/lib64/libc.so.6
(gdb) frame 3
#3 0x0000ffffaf8fd9e8 in virKeepAliveStart (ka=0xaaaaf954ce10, interval=interval entry=0,
count=count entry=0) at ../../src/rpc/virkeepalive.c:283
283 timeout = ka->interval - delay;
(gdb) list
278 now = time(NULL);
279 delay = now - ka->lastPacketReceived; <='delay' got a negative value
280 if (delay > ka->interval)
281 timeout = 0;
282 else
283 timeout = ka->interval - delay;
284 ka->intervalStart = now - (ka->interval - timeout);
285 ka->timer = virEventAddTimeout(timeout * 1000, virKeepAliveTimer, <= multiplication overflow
286 ka, virObjectFreeCallback);
287 if (ka->timer < 0)
(gdb) p now
$2 = 18288001
(gdb) p ka->lastPacketReceived
$3 = 1609430405
Signed-off-by: BiaoXiang Ye <yebiaoxiang@huawei.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Ján Tomko <jtomko@redhat.com>
All of these strings are allocated once, freed once, and are never
returned out of the function where they are declared.
Signed-off-by: Jim Fehlig <jfehlig@suse.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
* src/util/virsocket.c (virSocketRecvFD): Set msg.msg_controllen as documented
in the man pages.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Using locally built images is a useful feature; our commentaries even
mention overriding them may be useful in some scenarios. Expose the
variables in the help to let users know they can use the feature.
Formatting would definitely break, so this patch adds more spacing for
proper alignment.
Signed-off-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
In commit 321293e2 I dropped the prepare.sh script, but forgot to
remove the corresponding variable from the Makefile.
Signed-off-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
According to meson.build the minimal version of curl needed is
7.18.0 which was released in January 2008. If the minimal version
is bumped to 7.19.1 (released in November 2008) we can drop some
workarounds because this newer version provides APIs we need.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
This completer offers completion for --codeset argument of
send-key command.
Signed-off-by: Kristina Hanicova <khanicov@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
These are all cases when 1) the pointer is passed by reference from
the caller (ie.e. **) and expects it to be NULL on return if there is
an error, or 2) the variable holding the pointer is being checked or
re-used in the same function, but not right away.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
switching to g_autofree left many cleanup: sections empty.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Or when it will be immediately have a new value assigned to it.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
mimeType is initialized to NULL, and then only set in one place, just
before a check (not involving mimeType) that then VIR_FREEs mimeType
if it fails. If we just reorder the code to do the check prior to
setting mimeType, then there won't be any need to VIR_FREE(mimeType)
on failure (because it will already be empty/NULL).
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
If we put the potential return string into the g_autofreed tmpResult,
and the move it to the returned "result" only as a final step ater, we
can avoid the need to explicitly VIR_FREE (or g_free) on failure.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Although the three functions esxFreePrivate(), esxFreeStreamPrivate(),
and esxUtil_FreeParsedUri() are calling VIR_FREE on *object, and so in
theory the caller of the function might rely on "object" (the free
function's arg) being set to NULL, in practice these functions are
only called from a couple places each, and in all cases the pointer
that is passed is a local variable, and goes out of scope almost
immediately after calling the Free function, so it is safe to change
VIR_FREE() into g_free().
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
volumeName was defined at the top of the function, then a new string
was assigned to it each time through a loop, but after the first
iteration of the loop, the previous string wasn't freed before
allocating a new string the next time. By reducing the scope of
volumeName to be just the loop, and making it g_autofree, we eliminate
the leak.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
These strings were being VIR_FREEd multiple times because they were
defined at the top of a function, but then set each time through a
loop. But they are only used inside that loop, so they can be
converted to use g_autofree if their definition is also placed inside
that loop.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
All of these strings are allocated once, freed once, and are never
returned out of the function where they are created, used, and are
freed.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
All callers are now using the on|off syntax, so yes|no is a unreachable
code path.
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
QEMU has long accepted many different values for boolean properties, but
set accepted has been different depending on which QEMU parser you hit.
The on|off values were supported by all QEMU parsers. The yes|no, y|n,
true|false values were only partially supported:
https://lists.gnu.org/archive/html/qemu-devel/2020-11/msg01012.html
Thus we should standardize on on|off everywhere since that is most
widely supported in QEMU.
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
QEMU has long accepted many different values for boolean properties, but
set accepted has been different depending on which QEMU parser you hit.
The on|off values were supported by all QEMU parsers. The yes|no, y|n,
true|false values were only partially supported:
https://lists.gnu.org/archive/html/qemu-devel/2020-11/msg01012.html
Thus we should standardize on on|off everywhere since that is most
widely supported in QEMU.
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The preferred syntax for boolean options is to set the value "on" or
"off". QEMU 7.1.0 will deprecate the short format we currently use.
The long format has been supported with -vnc since the change to use
QemuOpts in 2.2.0, so we check based on the new capability flag.
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
This was introduced in QEMU 2.2.0, and is visible by -vnc appearing in
the "query-command-line-options" data.
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
When virDomainGetFSInfo() is called over a QEMU/KVM domain it
results into calling of 'guest-get-fsinfo' guest agent command to
which it replies with info on guest (mounted) filesystems. When
filling return structure we also try to do basic lookup and
translate guest agent provided disk address into disk target (as
seen in domain XML). This can of course fail - guest can have
variety of disks not recorded in domain XML (iSCSI, scsi_debug,
NFS to name a few). If that's the case, a debug message is logged
and no disk target is added into the return structure.
However, due to the way our code is written the caller is led to
believe that the target was added into the structure. This may
lead to a situation where the array of disk targets (strings)
contains NULL. But our RPC structure says the array contains only
non-NULL strings. This results in somewhat 'cryptic' (at least to
users) error message:
error: Unable to get filesystem information
error: Unable to encode message payload
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1919783
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
After previous commit, the freeing of @info_ret inside of
virDomainFSInfoFormat() looks like this:
for () {
if (info_ret)
virDomainFSInfoFree(info_ret[i]);
}
It is needless to compare @info_ret against NULL in each
iteration. We can switch the order and do the comparison first
followed by the loop.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
When qemuDomainGetFSInfo() is called it calls
qemuDomainGetFSInfoAgent() which executes 'guest-get-fsinfo'
guest agent command, parses returned JSON and returns an array of
qemuAgentFSInfo structures (well, pointers to those structs).
Then it grabs a domain job and tries to do some matching of guest
returned info against domain definition. This matching is done in
virDomainFSInfoFormat() which also frees the array of
qemuAgentFSInfo structures allocated earlier.
But this is not just. If acquiring the domain job fails (or
domain activeness check executed right after that fails) then
virDomainFSInfoFormat() is not called, leaking the array of
structs.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
As the very first thing, this function checks whether the number
of items inside @agentinfo array is not negative. This is
redundant as the only caller - qemuDomainGetFSInfo() already
checked for that and would not even call this function if that
was the case.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The preferred syntax for boolean options is to set the value "on" or
"off". QEMU 7.1.0 will deprecate the short format we currently use.
The long format has been supported with -spice since at least 1.5.3,
so we don't need to check for it.
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The preferred syntax for boolean options is to set the value "on" or
"off". QEMU 7.1.0 will deprecate the short format we currently use.
The long format has been supported with -chardev since at least 1.5.3,
so we don't need to check for it.
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The -2 value is misleading because if 'qemuAgentFSFreeze' fails it
doesn't necessarily mean that the command was sent to the agent.
Since callers don't care about the -2 value specifically, remove it.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
If we didn't freeze any filesystems we should not even attempt thawing
them. Additionally 'guest-fsfreeze-freeze' fails if the filesystems are
already frozen, where thawing them may break users data integrity if
they used VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE accidentally after an
explicit virDomainFSFreeze and the next snapshot without that flag would
be taken with already thawed filesystems.
This effectively reverts 7c736bab06 .
Libvirt nowadays checks whether the guest agent is connected and pings
it before issuing an command so it's very unlikely that we'd end up in a
situation where qemuSnapshotCreateActiveExternal froze filesystems and
didn't thaw them.
Additionally we now discourage the use of
VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE since users have better control if
they freeze the FS themselves.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
The flag creates additional points of failure which are hard to recover
from, such as when thawing of the filesystems fails after an otherwise
successful snapshot.
Encourage use of explicit virDomainFSFreeze/virDomainFSThaw.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
Use scripts/test-wrap-argv.py to rewrap the output files so that any
further changes don't introduce churn since we are rewrapping the output
automatically now.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
The suffix is used for output files of 'storagevolxml2argvtest.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
Use g_autofree for 'dom_xml' to free it on some of the (unlikely) code
paths jumping to cleanup prior to the deallocation which is done right
after it's not needed any more since it's a big string.
Noticed when running under valgrind:
==2204780== 8,192 bytes in 1 blocks are definitely lost in loss record 2,539 of 2,551
==2204780== at 0x483BCE8: realloc (vg_replace_malloc.c:834)
==2204780== by 0x4D890DF: g_realloc (in /usr/lib64/libglib-2.0.so.0.6600.4)
==2204780== by 0x4DA3AF0: g_string_append_vprintf (in /usr/lib64/libglib-2.0.so.0.6600.4)
==2204780== by 0x4917293: virBufferAsprintf (virbuffer.c:307)
==2204780== by 0x49B0B75: virDomainChrDefFormat (domain_conf.c:26109)
==2204780== by 0x49E25EF: virDomainDefFormatInternalSetRootName (domain_conf.c:28956)
==2204780== by 0x15F81D24: qemuDomainDefFormatBufInternal (qemu_domain.c:6204)
==2204780== by 0x15F8270D: qemuDomainDefFormatXMLInternal (qemu_domain.c:6229)
==2204780== by 0x15F8270D: qemuDomainDefFormatLive (qemu_domain.c:6279)
==2204780== by 0x15FD8100: qemuMigrationSrcBeginPhase (qemu_migration.c:2395)
==2204780== by 0x15FE0F0D: qemuMigrationSrcPerformPeer2Peer3 (qemu_migration.c:4640)
==2204780== by 0x15FE0F0D: qemuMigrationSrcPerformPeer2Peer (qemu_migration.c:5093)
==2204780== by 0x15FE0F0D: qemuMigrationSrcPerformJob (qemu_migration.c:5168)
==2204780== by 0x15FE280E: qemuMigrationSrcPerform (qemu_migration.c:5372)
==2204780== by 0x15F9BA3D: qemuDomainMigratePerform3Params (qemu_driver.c:11841)
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
