Re: [PATCH V3 3/4] qemu: Implement the virDomainSetLaunchSecurityState API
On 12/14/21 23:46, Jim Fehlig wrote:
Set a launch secret in guest memory using the sev-inject-launch-secret
QMP API. Only supported for SEV-enabled guests in a paused state.
Signed-off-by: Jim Fehlig
---
src/qemu/qemu_driver.c | 88
src/qemu/qemu_monitor.c | 14 ++
src/qemu/qemu_monitor.h | 7 +++
src/qemu/qemu_monitor_json.c | 45 ++
src/qemu/qemu_monitor_json.h | 6 +++
tests/qemumonitorjsontest.c | 3 ++
6 files changed, 163 insertions(+)
Just checking - not active day to day, but this technology is what some
of my team works on now... I only realized this because Dan pointed it
out and I was trying to help someone new on my team see how commands are
created in libvirt. He's more familiar with sevctl processing, but will
need to contribute to libvirt/qemu in the coming months.
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index aae622ea5d..889892a097 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -20070,6 +20070,93 @@ qemuDomainGetLaunchSecurityInfo(virDomainPtr domain,
return ret;
}
+
+static int
+qemuDomainSetLaunchSecurityState(virDomainPtr domain,
+ virTypedParameterPtr params,
+ int nparams,
+ unsigned int flags)
+{
+virQEMUDriver *driver = domain->conn->privateData;
+virDomainObj *vm;
+int ret = -1;
+int rc;
+size_t i;
+g_autofree char *secrethdr = NULL;
+g_autofree char *secret = NULL;
+unsigned long long setaddr = 0;
+bool hasSetaddr = false;
+int state;
+
+virCheckFlags(0, -1);
+if (virTypedParamsValidate(params, nparams,
+ VIR_DOMAIN_LAUNCH_SECURITY_SEV_SECRET_HEADER,
+ VIR_TYPED_PARAM_STRING,
+ VIR_DOMAIN_LAUNCH_SECURITY_SEV_SECRET,
+ VIR_TYPED_PARAM_STRING,
+
VIR_DOMAIN_LAUNCH_SECURITY_SEV_SECRET_SET_ADDRESS,
+ VIR_TYPED_PARAM_ULLONG,
+ NULL) < 0)
+return -1;
+
+if (!(vm = qemuDomainObjFromDomain(domain)))
+goto cleanup;
+
+if (virDomainSetLaunchSecurityStateEnsureACL(domain->conn, vm->def) < 0)
+goto cleanup;
+
+/* Currently only SEV is supported */
+if (!vm->def->sec ||
+vm->def->sec->sectype != VIR_DOMAIN_LAUNCH_SECURITY_SEV) {
+virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+ _("setting a launch secret is only supported in SEV-enabled
domains"));
+goto cleanup;
+}
Would we need a specific capability check here before calling
sev-inject-launch-secret since it's only available since qemu-6.0?
Seems to me VIR_DOMAIN_LAUNCH_SECURITY_SEV would be keyed off the
presence of the sev-guest object only. That would seem to be a qemu 2.12
type check. It's been a while since I looked at sources, so I'm rusty,
but I think qemu_validate.c/qemuValidateDomainDef would check for
QEMU_CAPS_SEV_GUEST only.
This is unlike the query-sev-capabilities and query-sev-launch-measure
which are present in 2.12.
John
+
+for (i = 0; i < nparams; i++) {
+virTypedParameterPtr param = [i];
+
+if (STREQ(param->field, VIR_DOMAIN_LAUNCH_SECURITY_SEV_SECRET_HEADER))
{
+secrethdr = g_strdup(param->value.s);
+} else if (STREQ(param->field, VIR_DOMAIN_LAUNCH_SECURITY_SEV_SECRET))
{
+secret = g_strdup(param->value.s);
+} else if (STREQ(param->field,
VIR_DOMAIN_LAUNCH_SECURITY_SEV_SECRET_SET_ADDRESS)) {
+setaddr = param->value.ul;
+hasSetaddr = true;
+}
+}
+
+if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
+goto cleanup;
+
+if (virDomainObjCheckActive(vm) < 0)
+goto endjob;
+
+state = virDomainObjGetState(vm, NULL);
+if (state != VIR_DOMAIN_PAUSED) {
+virReportError(VIR_ERR_OPERATION_INVALID,
+ "%s", _("domain must be in a paused state"));
+goto endjob;
+}
+
+qemuDomainObjEnterMonitor(driver, vm);
+rc = qemuMonitorSetLaunchSecurityState(QEMU_DOMAIN_PRIVATE(vm)->mon,
+ secrethdr, secret, setaddr,
hasSetaddr);
+qemuDomainObjExitMonitor(driver, vm);
+if (rc < 0)
+goto endjob;
+
+ret = 0;
+
+ endjob:
+qemuDomainObjEndJob(driver, vm);
+
+ cleanup:
+virDomainObjEndAPI();
+return ret;
+}
+
+
static const unsigned int qemuDomainGetGuestInfoSupportedTypes =
VIR_DOMAIN_GUEST_INFO_USERS |
VIR_DOMAIN_GUEST_INFO_OS |
@@ -20943,6 +21030,7 @@ static virHypervisorDriver qemuHypervisorDriver = {
.domainAuthorizedSSHKeysSet = qemuDomainAuthorizedSSHKeysSet, /* 6.10.0 */
.domainGetMessages = qemuDomainGetMessages, /* 7.1.0 */
Re: [PATCH] vmx: Parse vm.genid
On 8/2/21 7:00 AM, Michal Prívozník wrote: On 7/30/21 2:02 PM, Richard W.M. Jones wrote: On Thu, Jul 29, 2021 at 10:30:30AM +0200, Michal Privoznik wrote: The VMware metadata file contains genid but we are not parsing and thus reporting it in domain XML. However, it's not as straightforward as one might think. The UUID reported by VMware is not in its usual string form, but split into two signed long longs. That means, we have to do a bit of trickery when parsing. But looking around it's the same magic that libguestfs does: https://github.com/libguestfs/virt-v2v/blob/master/v2v/input_vmx.ml#L421 It's also explained by Rich on qemu-devel: https://lists.nongnu.org/archive/html/qemu-devel/2018-07/msg02019.html Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1598348 Signed-off-by: Michal Privoznik --- I've successfully ran vmx2xmltest on an s390x machine which means that there shouldn't be any endiandness problem. src/vmx/vmx.c | 30 +++ .../vmx2xml-esx-in-the-wild-10.xml| 1 + 2 files changed, 31 insertions(+) Looked reasonable and seems to match the description here: https://lists.nongnu.org/archive/html/qemu-devel/2018-07/msg02019.html Reviewed-by: Richard W.M. Jones Pushed, thanks. Out of interest, what is this being consumed by? I will add this to virt-v2v when it goes upstream. I don't recall all the specifics (it was John who implemented it), but IIRC it was needed for Windows guests. Something about identifying them uniquely. John? Tugging at a memory strand that's 3+ years old results in a blank stare followed by some amount of panic ;-) I do have vague recollections of issues w/ snapshots though - scanning the libvirt code for 'genid' or 'GEN_VMID' would seem to give some hints. There were some very specific options / processing. I recall a few qemu patches after the initial implementation, but the specifics have long since been erased from the recall buffer. I do see Gal who is still at Red Hat did some reviews for the original implementation - whether he has any recollections. John Here's the commit that implemented it in libvirt: https://gitlab.com/libvirt/libvirt/-/commit/b50efe97ad1357f9dff26450daf68a7a53201bea Michal
Re: [libvirt PATCH v2 2/2] storage_file: add support to probe cluster_size from QCOW2 images
On 5/20/21 11:14 AM, Pavel Hrdina wrote:
From QEMU docs/interop/qcow2.txt :
Byte 20 - 23: cluster_bits
Number of bits that are used for addressing an offset
within a cluster (1 << cluster_bits is the cluster size).
With this patch libvirt will be able to report the current cluster_size
for all existing storage volumes managed by storage driver.
Signed-off-by: Pavel Hrdina
---
Changes in v2:
- Reworkded to use callback.
src/storage/storage_util.c| 3 ++
src/storage_file/storage_file_probe.c | 70 ---
2 files changed, 56 insertions(+), 17 deletions(-)
[...]
+static unsigned long long
+qcow2GetClusterSize(const char *buf,
+size_t buf_size,
+int endian)
+{
+int clusterBits = 0;
+
+if ((QCOWX_HDR_CLUSTER_BITS_OFFSET + 4) > buf_size)
+return 0;
+
+if (endian == LV_LITTLE_ENDIAN)
+clusterBits = virReadBufInt32LE(buf + QCOWX_HDR_CLUSTER_BITS_OFFSET);
+else
+clusterBits = virReadBufInt32BE(buf + QCOWX_HDR_CLUSTER_BITS_OFFSET);
+
+if (clusterBits > 0)
+return 1 << clusterBits;
+
Coverity showed me a new error today: OVERFLOW_BEFORE_WIDEN
1) Event overflow_before_widen: Potentially overflowing expression "1 <<
clusterBits" with type "int" (32 bits, signed) is evaluated using 32-bit
arithmetic, and then used in a context that expects an expression of
type "unsigned long long" (64 bits, unsigned).
(2) Event remediation: To avoid overflow, cast "1" to type "unsigned
long long".
John
+return 0;
+}
+
+
[...]
Re: [libvirt PATCH v3 08/10] src: set identity when opening secondary drivers
On 5/12/21 9:33 AM, Daniel P. Berrangé wrote:
The drivers can all call virGetConnectXXX to open a connection to a
secondary driver. For example, when creating a encrypted storage volume,
the storage driver has to open a secret driver connection, or when
starting a guest, the QEMU driver has to open the network driver to
lookup a virtual network.
When using monolithic libvirtd, the connection has the same effective
identity as the client, since everything is still in the same process.
When using the modular daemons, however, the remote daemon sees the
identity of the calling daemon. This is a mistake as it results in
the modular daemons seeing the client with elevated privileges.
We need to pass on the current identity explicitly when opening the
secondary drivers. This is the same thing that is done by daemon RPC
dispatcher code when it is directly forwarding top level API calls
from virtproxyd and other daemons.
Reviewed-by: Michal Privoznik
Signed-off-by: Daniel P. Berrangé
---
src/driver.c | 27 +++
1 file changed, 27 insertions(+)
diff --git a/src/driver.c b/src/driver.c
index f8022d2522..227bb56e48 100644
--- a/src/driver.c
+++ b/src/driver.c
@@ -33,6 +33,8 @@
#include "virstring.h"
#include "virthread.h"
#include "virutil.h"
+#include "viridentity.h"
+#include "datatypes.h"
#include "configmake.h"
VIR_LOG_INIT("driver");
@@ -136,6 +138,7 @@ static virConnectPtr
virGetConnectGeneric(virThreadLocal *threadPtr, const char *name)
{
virConnectPtr conn;
+virErrorPtr saved;
if (virConnectCacheInitialize() < 0)
return NULL;
@@ -153,8 +156,32 @@ virGetConnectGeneric(virThreadLocal *threadPtr, const char
*name)
conn = virConnectOpen(uri);
VIR_DEBUG("Opened new %s connection %p", name, conn);
+if (!conn)
+return NULL;
+
+if (conn->driver->connectSetIdentity != NULL) {
+g_autoptr(virIdentity) ident = NULL;
+virTypedParameterPtr identparams = NULL;
+int nidentparams = 0;
+
+VIR_DEBUG("Attempting to delegate current identity");
+if (!(ident = virIdentityGetCurrent()))
+goto error;
+
+if (virIdentityGetParameters(ident, , ) <
0)
+goto error;
+
+if (virConnectSetIdentity(conn, identparams, nidentparams, 0) < 0)
+goto error;
+}
}
return conn;
+
+ error:
+saved = virSaveLastError();
+virConnectClose(conn);
+virSetError(saved);
Coverity complains about leak here
Need a virFreeError(saved);
John
+return NULL;
}
Re: [PATCH 2/5] virCapabilitiesHostNUMAInitReal: Free @cpus properly
On 5/5/21 4:02 AM, Michal Privoznik wrote: The @cpus variable is an array of structs in which each item contains a virBitmap member. As such it is not enough to just VIR_FREE() the array - each bitmap has to be freed too. Signed-off-by: Michal Privoznik --- src/conf/capabilities.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c index 084e09286d..4d509a6d28 100644 --- a/src/conf/capabilities.c +++ b/src/conf/capabilities.c @@ -1648,6 +1648,7 @@ virCapabilitiesHostNUMAInitReal(virCapsHostNUMA *caps) cleanup: virBitmapFree(cpumap); +virCapabilitiesClearHostNUMACellCPUTopology(cpus, ncpus); There's some coding axiom about for every bug you fix you introduce another ;-)... Anyway Coverity notes you can get to cleanup from within the for loop when ncpus < 0 and that will not be very good for this call. Yes, -1 can no longer be returned, but -2 can be and we could fall to cleanup (at least theoretically). Another tweak could be to only check -2 and continue in the if statement since -1 is no longer possible. John VIR_FREE(cpus); VIR_FREE(siblings); VIR_FREE(pageinfo);
Re: [libvirt PATCH v2 07/12] nodedev: driver: Create a generic mdevctl command translator
On 4/13/21 4:39 PM, Jonathon Jongsma wrote:
From: Erik Skultety
Currently there are dedicated wrappers to construct mdevctl command.
These are mostly fine except for the one that translates both "start"
and "define" commands, only because mdevctl takes the same set of
arguments. Instead, keep the wrappers, but let them call a single
global translator that handles all the mdevctl command differences and
commonalities.
Signed-off-by: Erik Skultety
Signed-off-by: Jonathon Jongsma
---
src/node_device/node_device_driver.c | 125 ---
src/node_device/node_device_driver.h | 6 +-
tests/nodedevmdevctltest.c | 4 +-
3 files changed, 79 insertions(+), 56 deletions(-)
Coverity found some issues...
diff --git a/src/node_device/node_device_driver.c
b/src/node_device/node_device_driver.c
index da55e386f0..bbb01c3967 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -702,42 +702,75 @@ nodeDeviceFindAddressByName(const char *name)
}
-/* the mdevctl 'start' and 'define' commands accept almost the exact same
- * arguments, so provide a common implementation that can be wrapped by a more
- * specific function */
-static virCommand*
-nodeDeviceGetMdevctlDefineCreateCommand(virNodeDeviceDef *def,
-const char *subcommand,
-char **uuid_out,
-char **errmsg)
+static virCommand *
+nodeDeviceGetMdevctlCommand(virNodeDeviceDef *def,
+virMdevctlCommand cmd_type,
+char **outbuf,
+char **errbuf)
{
-virCommand *cmd;
-g_autofree char *json = NULL;
-g_autofree char *parent_addr = nodeDeviceFindAddressByName(def->parent);
-
-if (!parent_addr) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
- _("unable to find parent device '%s'"), def->parent);
-return NULL;
+g_autofree char *parent_addr = NULL;
+virCommand *cmd = NULL;
+const char *subcommand = virMdevctlCommandTypeToString(cmd_type);
+g_autofree char *inbuf = NULL;
+
+switch (cmd_type) {
+case MDEVCTL_CMD_CREATE:
+/* now is the time to make sure "create" is replaced with "start" on
+ * mdevctl cmdline */
+cmd = virCommandNewArgList(MDEVCTL, "start", NULL);
+break;
+case MDEVCTL_CMD_STOP:
+case MDEVCTL_CMD_START:
+case MDEVCTL_CMD_DEFINE:
+case MDEVCTL_CMD_UNDEFINE:
+cmd = virCommandNewArgList(MDEVCTL, subcommand, NULL);
+break;
+case MDEVCTL_CMD_LAST:
+default:
+/* SHOULD NEVER HAPPEN */
Shouldn't happen, but allows code to fall thru w/ later access to @cmd
which will fail. It's a false positive technically...
+break;
}
-if (nodeDeviceDefToMdevctlConfig(def, ) < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("couldn't convert node device def to mdevctl JSON"));
-return NULL;
-}
+switch (cmd_type) {
+case MDEVCTL_CMD_CREATE:
+case MDEVCTL_CMD_DEFINE:
+parent_addr = nodeDeviceFindAddressByName(def->parent);
-cmd = virCommandNewArgList(MDEVCTL, subcommand, NULL);
-virCommandAddArgPair(cmd, "--parent", parent_addr);
-virCommandAddArgPair(cmd, "--jsonfile", "/dev/stdin");
+if (!parent_addr) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("unable to find parent device '%s'"),
def->parent);
Leaks @cmd
+return NULL;
+}
+
+if (nodeDeviceDefToMdevctlConfig(def, ) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("couldn't convert node device def to mdevctl
JSON"));
Leaks @cmd
+return NULL;
+}
-virCommandSetInputBuffer(cmd, json);
+virCommandAddArgPair(cmd, "--parent", parent_addr);
+virCommandAddArgPair(cmd, "--jsonfile", "/dev/stdin");
+
+virCommandSetInputBuffer(cmd, inbuf);
+virCommandSetOutputBuffer(cmd, outbuf);
+break;
+
+case MDEVCTL_CMD_UNDEFINE:
+case MDEVCTL_CMD_STOP:
+case MDEVCTL_CMD_START:
+/* No special handling here, we only need to pass UUID with these */
+break;
+case MDEVCTL_CMD_LAST:
+default:
+/* SHOULD NEVER HAPPEN */
+break;
+}
[...]
Hopefully I haven't missed some other patch along the way as I don't
keep up to date as much any more. Coverity just keeps running though...
John
Re: [libvirt PATCH v6 27/30] nodedev: fix hang when destroying an mdev in use
On 3/26/21 12:48 PM, Jonathon Jongsma wrote:
Calling `mdevctl stop` for a mediated device that is in use by an active
domain will block until that vm exits (or the vm closes the device).
Since the nodedev driver cannot query the hypervisor driver to see
whether any active domains are using the device, we resort to a
workaround that relies on the fact that a vfio group can only be opened
by one user at a time. If we get an EBUSY error when attempting to open
the group file, we assume the device is in use and refuse to try to
destroy that device.
Signed-off-by: Jonathon Jongsma
Reviewed-by: Erik Skultety
---
src/node_device/node_device_driver.c | 19 +++
1 file changed, 19 insertions(+)
diff --git a/src/node_device/node_device_driver.c
b/src/node_device/node_device_driver.c
index b1b3217afb..31322e3afb 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -1177,8 +1177,27 @@ nodeDeviceDestroy(virNodeDevicePtr device)
ret = 0;
} else if (nodeDeviceHasCapability(def, VIR_NODE_DEV_CAP_MDEV)) {
+/* If this mediated device is in use by a vm, attempting to stop it
+ * will block until the vm closes the device. The nodedev driver
+ * cannot query the hypervisor driver to determine whether the device
+ * is in use by any active domains, since that would introduce circular
+ * dependencies between daemons and add a risk of deadlocks. So we need
+ * to resort to a workaround. vfio only allows the group for a device
+ * to be opened by one user at a time. So if we get EBUSY when opening
+ * the group, we infer that the device is in use and therefore we
+ * shouldn't try to remove the device. */
+g_autofree char *vfiogroup =
+virMediatedDeviceGetIOMMUGroupDev(def->caps->data.mdev.uuid);
FWIW: Coverity points out @vfiogroup could be returned as NULL here (w/
virReportError called) thus making the subsequent call possibly fail...
John
+VIR_AUTOCLOSE fd = open(vfiogroup, O_RDONLY);
g_autofree char *errmsg = NULL;
+if (fd < 0 && errno == EBUSY) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Unable to destroy '%s': device in use"),
+ def->name);
+goto cleanup;
+}
+
if (virMdevctlStop(def, ) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Unable to destroy '%s': %s"), def->name,
Re: [PATCH] qemu_driver.c: Coverity fix in qemuNodeDeviceDetachFlags()
On 2/18/21 7:33 AM, Daniel Henrique Barboza wrote: > Commit 76f47889326c4 made qemuNodeDeviceDetachFlags() unusable due to an > 'if then else if' chain that will always results in a 'return -1', > regardless of 'driverName' input. This slipped through review process > and Gitlab CI, making it clear now that there is no unit tests > exercising this code ATM. > > LUckily John Ferlan caught this up with his Coverity scan and spared us > from an unneeded headache later on. > > Fixes: 76f47889326c45d2732711bc6dd5751aaf6e5194 > Reported-by: John Ferlan > Signed-off-by: Daniel Henrique Barboza > --- > src/qemu/qemu_driver.c | 26 +++--- > 1 file changed, 15 insertions(+), 11 deletions(-) > I can vouch this does make Coverity happy again. I do agree w/ Jano no need to be too elaborate on slipping thru cracks And when I do create patches (rarely) now for Coverity, I'll just attribute it to "Found by Coverity" and be done with it. You already put the Reported-by so that's probably sufficient! Reviewed-by: John Ferlan John
Re: [PATCH v2 07/10] qemu_driver.c: validate 'driverName' earlier in qemuNodeDeviceDetachFlags()
On 2/2/21 8:06 PM, Daniel Henrique Barboza wrote:
> 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
> Signed-off-by: Daniel Henrique Barboza
> ---
> src/qemu/qemu_driver.c | 41 -
> 1 file changed, 20 insertions(+), 21 deletions(-)
> > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 64ae8fafc0..c6ba33e4ad 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -11982,6 +11982,25 @@ qemuNodeDeviceDetachFlags(virNodeDevicePtr dev,
>
> virCheckFlags(0, -1);
>
> +if (!driverName)
> +driverName = "vfio";
> +
> +if (STREQ(driverName, "vfio") && !vfio) {
> +virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
> + _("VFIO device assignment is currently not "
> + "supported on this system"));
> + return -1;
> +} else if (STREQ(driverName, "kvm")) {
> +virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
> + _("KVM device assignment is no longer "
> + "supported on this system"));
> +return -1;
> +} else {
> +virReportError(VIR_ERR_INVALID_ARG,
> + _("unknown driver name '%s'"), driverName);
> +return -1;
> +}
> +
Coverity points out the rest of this is unreachable now (even with the
subsequent patch).
> if (!(nodeconn = virGetConnectNodeDev()))
> goto cleanup;
>
> @@ -12013,27 +12032,7 @@ qemuNodeDeviceDetachFlags(virNodeDevicePtr dev,
> if (!pci)
> goto cleanup;
>
> -if (!driverName)
> -driverName = "vfio";
> -
> -if (STREQ(driverName, "vfio")) {
> -if (!vfio) {
> -virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
> - _("VFIO device assignment is currently not "
> - "supported on this system"));
> -goto cleanup;
> -}
> -virPCIDeviceSetStubDriver(pci, VIR_PCI_STUB_DRIVER_VFIO);
This section seems to be more than just code motion... it used to pass
thru here when vfio = true, but with the movement it would fall into the
catch all else.
John
> -} else if (STREQ(driverName, "kvm")) {
> -virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
> - _("KVM device assignment is no longer "
> - "supported on this system"));
> -goto cleanup;
> -} else {
> -virReportError(VIR_ERR_INVALID_ARG,
> - _("unknown driver name '%s'"), driverName);
> -goto cleanup;
> -}
> +virPCIDeviceSetStubDriver(pci, VIR_PCI_STUB_DRIVER_VFIO);
>
> ret = virHostdevPCINodeDeviceDetach(hostdev_mgr, pci);
> cleanup:
>
Re: [libvirt PATCH v2 5/6] qemu: implement virDomainGetMessages API
On 2/9/21 11:01 AM, Daniel P. Berrangé wrote:
> Signed-off-by: Daniel P. Berrangé
> ---
> src/conf/domain_conf.c | 17
> src/conf/domain_conf.h | 1 +
> src/libvirt_private.syms | 2 ++
> src/qemu/qemu_domain.h | 3 +++
> src/qemu/qemu_driver.c | 58
> 5 files changed, 81 insertions(+)
>
[...]
> +static int
> +qemuDomainGetMessages(virDomainPtr dom,
> + char ***msgs,
> + unsigned int flags)
> +{
> +virDomainObjPtr vm = NULL;
> +int rv = -1;
> +size_t i, n;
> +int nmsgs;
> +
> +virCheckFlags(VIR_DOMAIN_MESSAGE_DEPRECATION |
> + VIR_DOMAIN_MESSAGE_TAINTING, -1);
> +
> +if (!(vm = qemuDomainObjFromDomain(dom)))
> +return -1;
> +
> +if (virDomainGetMessagesEnsureACL(dom->conn, vm->def) < 0)
> +goto cleanup;
> +
> +*msgs = NULL;
> +nmsgs = 0;
> +n = 0;
> +
> +if (!flags || (flags & VIR_DOMAIN_MESSAGE_TAINTING)) {
> +nmsgs += __builtin_popcount(vm->taint);
> +*msgs = g_renew(char *, *msgs, nmsgs+1);
> +
> +for (i = 0; i < VIR_DOMAIN_TAINT_LAST; i++) {
> +if (vm->taint & (1 << i)) {
> +(*msgs)[n++] = g_strdup_printf(
> +_("tainted: %s"),
> +_(virDomainTaintMessageTypeToString(i)));
> +}
> +}
> +}
> +
> +if (!flags || (flags & VIR_DOMAIN_MESSAGE_DEPRECATION)) {
> +nmsgs += vm->ndeprecations;
> +*msgs = g_renew(char *, *msgs, nmsgs+1);
> +
> +for (i = 0; i < vm->ndeprecations; i++) {
> +(*msgs)[n++] = g_strdup_printf(
> +_("deprecated configuration: %s"),
> +vm->deprecations[i]);
> +}
> +}
> +
> +(*msgs)[nmsgs] = NULL;
FYI: Coverity got grumpy right here as it believes *msgs could be NULL
because of the !flags || condition and not realizing that flags could
only be one of the two or 0.
Wasn't sure whether being safe and adding a if (*msgs) is desired, but
figured I'd at least note it.
John
> +
> +rv = nmsgs;
> +
> + cleanup:
> +virDomainObjEndAPI();
> +return rv;
> +}
> +
[...]
Re: [PATCH v3 1/4] src: adopt to VIR_DRV_SUPPORTS_FEATURE return -1
On 12/18/20 1:56 AM, Nikolay Shirokovskiy wrote:
> Otherwise in some places we can mistakenly report 'unsupported' error instead
> of root cause. So let's handle root cause explicitly from the macro.
>
> Signed-off-by: Nikolay Shirokovskiy
> ---
> src/libvirt-domain.c | 511
> ++-
> src/libvirt-host.c | 18 +-
> src/libvirt.c| 7 +-
> 3 files changed, 365 insertions(+), 171 deletions(-)
>
[...]
> @@ -3005,8 +3019,11 @@ virDomainMigrateVersion3Full(virDomainPtr domain,
> return NULL;
> params = tmp;
>
> -if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
> - VIR_DRV_FEATURE_MIGRATE_CHANGE_PROTECTION))
> +ret = VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
> +
> VIR_DRV_FEATURE_MIGRATE_CHANGE_PROTECTION);
> +if (ret < 0)
> +return NULL;
> +if (ret)
Coverity complains this is a RESOURCE_LEAK for @tmp (or essentially @params)
Perhaps the hunk for VIR_DRV_SUPPORTS_FEATURE should go before
virTypedParamsCopy or use goto done (similar if !dom_xml)?
John
> protection = VIR_MIGRATE_CHANGE_PROTECTION;
>
> VIR_DEBUG("Begin3 %p", domain->conn);
> @@ -3403,6 +3420,8 @@ virDomainMigrateUnmanagedParams(virDomainPtr domain,
[...]
Re: [PATCH 08/10] network: Rework networkGetDHCPLeases()
On 12/18/20 10:09 AM, Michal Privoznik wrote:
> Firstly, bring variables that are used only within loops into
> their respective loops. Secondly, drop 'error' label which is
> redundant since we have @rv which holds the return value.
> Thirdly, fix indendation in one case, the rest is indented
> properly.
>
> Signed-off-by: Michal Privoznik
> ---
> src/network/bridge_driver.c | 47 +
> 1 file changed, 22 insertions(+), 25 deletions(-)
>
[...]
> @@ -4145,7 +4146,7 @@ networkGetDHCPLeases(virNetworkPtr net,
> /* A lease without ip-address makes no sense */
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> _("found lease without ip-address"));
> -goto error;
> +goto cleanup;
> }
>
> /* Unlike IPv4, IPv6 uses ':' instead of '.' as separator */
> @@ -4154,7 +4155,7 @@ networkGetDHCPLeases(virNetworkPtr net,
>
> /* Obtain prefix */
> for (j = 0; j < def->nips; j++) {
> -ipdef_tmp = >ips[j];
> +virNetworkIPDefPtr ipdef_tmp = ipdef_tmp = >ips[j];
Coverity notes... Department of redundancy department
Simple/trivial fix for someone I'm sure
John
>
> if (ipv6 && VIR_SOCKET_ADDR_IS_FAMILY(_tmp->address,
>AF_INET6)) {
> @@ -4162,7 +4163,7 @@ networkGetDHCPLeases(virNetworkPtr net,
[...]
Re: [PATCHv3 3/3] lxc: fix a memory leak
Coverity reminds us of the ancient software engineering proverb related
to being stuck with ownership because you touched the code last :-) - I
know this patch didn't cause the problem, but because the code was
touched Coverity decided to look harder and found another leak.
On 12/16/20 1:01 AM, Shi Lei wrote:
> In virLXCProcessSetupInterfaceTap, containerVeth needs to be freed on
> failure.
>
> Signed-off-by: Shi Lei
> ---
> src/lxc/lxc_process.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
> index 85d0287a..0f7c9295 100644
> --- a/src/lxc/lxc_process.c
> +++ b/src/lxc/lxc_process.c
> @@ -303,7 +303,7 @@ virLXCProcessSetupInterfaceTap(virDomainDefPtr vm,
> const char *brname)
> {
> char *parentVeth;
Coverity complains that @parentVeth is leaked too - although it's far
more opaque and ornery.
Let's assume on input that net->ifname != NULL - that means that in
virNetDevVethCreate the call to virNetDevGenerateName and the memory in
@**ifname (e.g. @parentVeth's copy of net->ifname) will be g_free()'d
when !virNetDevExists(try) succeeds and @*ifname gets the memory from
@try. In this case @try is a g_strdup_printf of @*ifname. So this code
essentially free's memory pointed to by net->ifname, but since
@parentVeth is used in the call net->ifname is *NOT* updated which is a
second problem that Coverity did not note.
Then let's say the call to virNetDevGenerateName fails for @veth2...
Since on input @orig1 != NULL (e.g. @parentVeth != NULL), we will not
VIR_FREE(*veth1); - that's fine given the original assumption, but when
we return to virLXCProcessSetupInterfaceTap that means net->ifname will
point at memory that was g_free'd and @parentVeth will not be free'd,
thus Coverity squawks loud and proud about the resource leak - although
it doesn't complain about the fact that net->ifname now points to memory
that was free'd.
As an aside, I see no way on input @*veth2 could not be NULL so in the
cleanup path the check for @orig2 would seem to be dead code. Although,
sure future changes could alter that reality.
As a test if I replace all @parentVeth refs w/ net->ifname - then
Coverity is happy again. I will leave it up to Laine or Shi Lei to
generate the "real fix" and/or validate that my reading of the logic is
right or not ;-).
John
> -char *containerVeth = NULL;
> +g_autofree char *containerVeth = NULL;
> const virNetDevVPortProfile *vport =
> virDomainNetGetActualVirtPortProfile(net);
>
> VIR_DEBUG("calling vethCreate()");
> @@ -357,7 +357,7 @@ virLXCProcessSetupInterfaceTap(virDomainDefPtr vm,
> virDomainConfNWFilterInstantiate(vm->name, vm->uuid, net, false) < 0)
> return NULL;
>
> -return containerVeth;
> +return g_steal_pointer();
> }
>
>
>
Re: [PATCH libvirt v4 09/12] node_device: refactor address retrieval of node device
On 12/3/20 12:59 PM, Shalini Chellathurai Saroja wrote:
> Use switch statements instead of if-else condition in the method
> nodeDeviceFindAddressByName to retrieve address of a node device.
>
> Signed-off-by: Shalini Chellathurai Saroja
> Reviewed-by: Bjoern Walk
> Reviewed-by: Boris Fiuczynski
> ---
> src/node_device/node_device_driver.c | 30 ++--
> 1 file changed, 28 insertions(+), 2 deletions(-)
>
> diff --git a/src/node_device/node_device_driver.c
> b/src/node_device/node_device_driver.c
> index f5ea973c..65c647f5 100644
> --- a/src/node_device/node_device_driver.c
> +++ b/src/node_device/node_device_driver.c
> @@ -638,7 +638,8 @@ nodeDeviceFindAddressByName(const char *name)
>
> def = virNodeDeviceObjGetDef(dev);
> for (caps = def->caps; caps != NULL; caps = caps->next) {
> -if (caps->data.type == VIR_NODE_DEV_CAP_PCI_DEV) {
> +switch (caps->data.type) {
> +case VIR_NODE_DEV_CAP_PCI_DEV: {
Coverity got pretty grumpy this morning with RESOURCE_LEAKS due to
override of @addr as changing from a break; inside a switch/case
statement sent us back to the for statement rather than outside the loop
which would occur on the break; inside the if {} else if {}
Suggestion... add "&& addr != NULL" as an additional for loop exit.
John
> virPCIDeviceAddress pci_addr = {
> .domain = caps->data.pci_dev.domain,
> .bus = caps->data.pci_dev.bus,
> @@ -648,7 +649,9 @@ nodeDeviceFindAddressByName(const char *name)
>
> addr = virPCIDeviceAddressAsString(_addr);
> break;
> -} else if (caps->data.type == VIR_NODE_DEV_CAP_CSS_DEV) {
> +}
> +
> +case VIR_NODE_DEV_CAP_CSS_DEV: {
> virDomainDeviceCCWAddress ccw_addr = {
> .cssid = caps->data.ccw_dev.cssid,
> .ssid = caps->data.ccw_dev.ssid,
> @@ -657,6 +660,29 @@ nodeDeviceFindAddressByName(const char *name)
>
> addr = virDomainCCWAddressAsString(_addr);
> break;
> +}
> +
> +case VIR_NODE_DEV_CAP_SYSTEM:
> +case VIR_NODE_DEV_CAP_USB_DEV:
> +case VIR_NODE_DEV_CAP_USB_INTERFACE:
> +case VIR_NODE_DEV_CAP_NET:
> +case VIR_NODE_DEV_CAP_SCSI_HOST:
> +case VIR_NODE_DEV_CAP_SCSI_TARGET:
> +case VIR_NODE_DEV_CAP_SCSI:
> +case VIR_NODE_DEV_CAP_STORAGE:
> +case VIR_NODE_DEV_CAP_FC_HOST:
> +case VIR_NODE_DEV_CAP_VPORTS:
> +case VIR_NODE_DEV_CAP_SCSI_GENERIC:
> +case VIR_NODE_DEV_CAP_DRM:
> +case VIR_NODE_DEV_CAP_MDEV_TYPES:
> +case VIR_NODE_DEV_CAP_MDEV:
> +case VIR_NODE_DEV_CAP_CCW_DEV:
> +case VIR_NODE_DEV_CAP_VDPA:
> +case VIR_NODE_DEV_CAP_AP_CARD:
> +case VIR_NODE_DEV_CAP_AP_QUEUE:
> +case VIR_NODE_DEV_CAP_AP_MATRIX:
> +case VIR_NODE_DEV_CAP_LAST:
> +break;
> }
> }
>
>
[PATCH v2] qemu: Pass / fill niothreads for qemuMonitorGetIOThreads
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
---
Since v1, updated the logic to pass @niothreads around rather than
rely on the dual meaning. Took the full plunge.
src/qemu/qemu_driver.c | 23 +++
src/qemu/qemu_monitor.c | 8 +---
src/qemu/qemu_monitor.h | 3 ++-
src/qemu/qemu_monitor_json.c | 6 --
src/qemu/qemu_monitor_json.h | 3 ++-
src/qemu/qemu_process.c | 4 ++--
tests/qemumonitorjsontest.c | 4 ++--
7 files changed, 28 insertions(+), 23 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index bca1c84630..65725b2ef2 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4972,17 +4972,18 @@ qemuDomainGetMaxVcpus(virDomainPtr dom)
static int
qemuDomainGetIOThreadsMon(virQEMUDriverPtr driver,
virDomainObjPtr vm,
- qemuMonitorIOThreadInfoPtr **iothreads)
+ qemuMonitorIOThreadInfoPtr **iothreads,
+ int *niothreads)
{
qemuDomainObjPrivatePtr priv = vm->privateData;
-int niothreads = 0;
+int ret = -1;
qemuDomainObjEnterMonitor(driver, vm);
-niothreads = qemuMonitorGetIOThreads(priv->mon, iothreads);
-if (qemuDomainObjExitMonitor(driver, vm) < 0 || niothreads < 0)
+ret = qemuMonitorGetIOThreads(priv->mon, iothreads, niothreads);
+if (qemuDomainObjExitMonitor(driver, vm) < 0)
return -1;
-return niothreads;
+return ret;
}
@@ -5014,7 +5015,7 @@ qemuDomainGetIOThreadsLive(virQEMUDriverPtr driver,
goto endjob;
}
-if ((niothreads = qemuDomainGetIOThreadsMon(driver, vm, )) < 0)
+if ((ret = qemuDomainGetIOThreadsMon(driver, vm, , ))
< 0)
goto endjob;
/* Nothing to do */
@@ -5314,8 +5315,7 @@ qemuDomainHotplugAddIOThread(virQEMUDriverPtr driver,
* IOThreads thread_id's, adjust the cgroups, thread affinity,
* and add the thread_id to the vm->def->iothreadids list.
*/
-if ((new_niothreads = qemuMonitorGetIOThreads(priv->mon,
- _iothreads)) < 0)
+if (qemuMonitorGetIOThreads(priv->mon, _iothreads, _niothreads) <
0)
goto exit_monitor;
if (qemuDomainObjExitMonitor(driver, vm) < 0)
@@ -5425,8 +5425,7 @@ qemuDomainHotplugDelIOThread(virQEMUDriverPtr driver,
if (rc < 0)
goto exit_monitor;
-if ((new_niothreads = qemuMonitorGetIOThreads(priv->mon,
- _iothreads)) < 0)
+if (qemuMonitorGetIOThreads(priv->mon, _iothreads, _niothreads) <
0)
goto exit_monitor;
if (qemuDomainObjExitMonitor(driver, vm) < 0)
@@ -18507,7 +18506,7 @@ qemuDomainGetStatsIOThread(virQEMUDriverPtr driver,
qemuDomainObjPrivatePtr priv = dom->privateData;
size_t i;
qemuMonitorIOThreadInfoPtr *iothreads = NULL;
-int niothreads;
+int niothreads = 0;
int ret = -1;
if (!HAVE_JOB(privflags) || !virDomainObjIsActive(dom))
@@ -18516,7 +18515,7 @@ qemuDomainGetStatsIOThread(virQEMUDriverPtr driver,
if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD))
return 0;
-if ((niothreads = qemuDomainGetIOThreadsMon(driver, dom, )) < 0)
+if (qemuDomainGetIOThreadsMon(driver, dom, , ) < 0)
return -1;
/* qemuDomainGetIOThreadsMon returns a NULL-terminated list, so we must
free
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index ce1a06c4c8..551b65e778 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -4211,22 +4211,24 @@ qemuMonitorRTCResetReinjection(qemuMonitorPtr mon)
* qemuMonitorGetIOThreads:
* @mon: Pointer to the monitor
* @iothreads: Location to return array of IOThreadInfo data
+ * @niothreads: Count of the number of IOThreads in the array
*
* Issue query-iothreads command.
* Retrieve the list of iothreads defined/running for the machine
*
- * Returns count of IOThreadInfo structures on success
+ * Returns 0 on success
*-1 on error.
*/
int
qemuMonitorGetIOThreads(qemuMonitorPtr mon,
-qemuMonitorIOThreadInfoPtr **iothreads)
+qemuMonitorIOThreadInfoPtr **iothreads,
+int *niothreads)
{
VIR_DEBUG("iothreads=%p", iothreads);
QEMU_CHECK_MONITOR(mon);
-return qemuMonitorJSONGetIOThreads(mon, iothreads);
+return qemuMonitorJSONGetIOThreads(mon, iothreads, niothreads);
}
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index 8bc092870b..49be2d5412 100644
--- a/src/qemu/qemu_monitor.
Re: [PATCH 4/7] qemu: Fix resource leak and coding error in qemuDomainGetIOThreadsMon
On 12/2/20 9:44 AM, Ján Tomko wrote:
> The commit summary uses 'and' which makes me think there are two
> issues, but the commit message and code only seem to fix one.
>
True - I changed my mind multiple times on this one w/r/t how involved
the fix should be. Originally I did have cleanup added, but then changed
my mind to allow the caller to do it and pass instead.
I can rework this one - thanks for the reviews/pushes. I'll also drop
the pidfilefd one and keep it in my false positive list.
Tks -
John
> On a Wednesday in 2020, John Ferlan wrote:
>> If qemuDomainGetIOThreadsMon fails because qemuDomainObjExitMonitor
>> fails, then a -1 is returned which overwrites @niothreads causing a
>> memory leak. Let's pass @niothreads instead.
>>
>> Found by Coverity.
>>
>> Signed-off-by: John Ferlan
>> ---
>> src/qemu/qemu_driver.c | 19 +--
>> 1 file changed, 9 insertions(+), 10 deletions(-)
>>
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index 8eaa3ce68f..870159de47 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -4972,17 +4972,16 @@ qemuDomainGetMaxVcpus(virDomainPtr dom)
>> static int
>> qemuDomainGetIOThreadsMon(virQEMUDriverPtr driver,
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â virDomainObjPtr vm,
>> -Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â
>> qemuMonitorIOThreadInfoPtr **iothreads)
>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â
>> qemuMonitorIOThreadInfoPtr **iothreads,
>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â int *niothreads)
>
> Returning niothreads separately from success/error might clear things
> up,
>
>> {
>> Â Â Â qemuDomainObjPrivatePtr priv = vm->privateData;
>> -Â Â Â int niothreads = 0;
>>
>> Â Â Â qemuDomainObjEnterMonitor(driver, vm);
>> -Â Â Â niothreads = qemuMonitorGetIOThreads(priv->mon, iothreads);
>> -Â Â Â if (qemuDomainObjExitMonitor(driver, vm) < 0 || niothreads < 0)
>> +Â Â Â *niothreads = qemuMonitorGetIOThreads(priv->mon, iothreads);
>
> but qemuMonitorGetIOThreads can also return -1. In that case we
> should not:
> a) return 0 in this function
> b) compare the return value against unsigned size_t in the cleanup
> Â section of qemuDomainGetStatsIOThread
>
>> +Â Â Â if (qemuDomainObjExitMonitor(driver, vm) < 0)
>
> I think that now that we take a job even for destroying the domain
> in processMonitorEOFEvent, this should not happen.
>
> But if it still can, it would be more consistent if
> qemuDomainGetIOThreadsMon
> cleaned up after itself if it returns -1, instead of leaving it up
> to the caller.
>
> (Other callers of qemuMonitorGetIOThreads and qemuDomainGetIOThreadsMon
> Â seem to handle it properly and check if (iothreads) in their cleanup
> Â sections, it's only qemuDomainGetStatsIOThread that relies on
> Â niothreads being right)
>
> Jano
>
>> Â Â Â Â Â Â Â return -1;
>> -
>> -Â Â Â return niothreads;
>> +Â Â Â return 0;
>> }
>>
>>
>> @@ -5014,7 +5013,7 @@ qemuDomainGetIOThreadsLive(virQEMUDriverPtr driver,
>> Â Â Â Â Â Â Â goto endjob;
>> Â Â Â }
>>
>> -Â Â Â if ((niothreads = qemuDomainGetIOThreadsMon(driver, vm,
>> )) < 0)
>> +Â Â Â if (qemuDomainGetIOThreadsMon(driver, vm, ,
>> ) < 0)
>> Â Â Â Â Â Â Â goto endjob;
>>
>> Â Â Â /* Nothing to do */
>> @@ -18507,7 +18506,7 @@ qemuDomainGetStatsIOThread(virQEMUDriverPtr
>> driver,
>> Â Â Â qemuDomainObjPrivatePtr priv = dom->privateData;
>> Â Â Â size_t i;
>> Â Â Â qemuMonitorIOThreadInfoPtr *iothreads = NULL;
>> -Â Â Â int niothreads;
>> +Â Â Â int niothreads = 0;
>> Â Â Â int ret = -1;
>>
>> Â Â Â if (!HAVE_JOB(privflags) || !virDomainObjIsActive(dom))
>> @@ -18516,8 +18515,8 @@ qemuDomainGetStatsIOThread(virQEMUDriverPtr
>> driver,
>> Â Â Â if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD))
>> Â Â Â Â Â Â Â return 0;
>>
>> -Â Â Â if ((niothreads = qemuDomainGetIOThreadsMon(driver, dom,
>> )) < 0)
>> -Â Â Â Â Â Â Â return -1;
>> +Â Â Â if (qemuDomainGetIOThreadsMon(driver, dom, ,
>> ) < 0)
>> +Â Â Â Â Â Â Â goto cleanup;
>>
>> Â Â Â /* qemuDomainGetIOThreadsMon returns a NULL-terminated list, so we
>> must free
>> Â Â Â Â * it even if it returns 0 */
>> --Â
>> 2.28.0
>>
[PATCH 7/7] qemu: Fix some issues in virQEMUDriverConfigLoadNVRAMEntry
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
disasterous. So let's reinitialze 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
---
src/qemu/qemu_conf.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index cbdde0c0dc..690cfd39f9 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -835,6 +835,7 @@ virQEMUDriverConfigLoadNVRAMEntry(virQEMUDriverConfigPtr
cfg,
virFirmwareFreeList(cfg->firmwares, cfg->nfirmwares);
cfg->firmwares = NULL;
+cfg->nfirmwares = 0;
if (qemuFirmwareFetchConfigs(, privileged) < 0)
return -1;
@@ -843,13 +844,11 @@ virQEMUDriverConfigLoadNVRAMEntry(virQEMUDriverConfigPtr
cfg,
VIR_WARN("Obsolete nvram variable is set while firmware metadata "
"files found. Note that the nvram config file variable is
"
"going to be ignored.");
-cfg->nfirmwares = 0;
return 0;
}
cfg->nfirmwares = virStringListLength((const char *const *)nvram);
-if (nvram[0])
-cfg->firmwares = g_new0(virFirmwarePtr, cfg->nfirmwares);
+cfg->firmwares = g_new0(virFirmwarePtr, cfg->nfirmwares);
for (i = 0; nvram[i] != NULL; i++) {
cfg->firmwares[i] = g_new0(virFirmware, 1);
--
2.28.0
[PATCH 3/7] docs: Fix link for virConnectGetStoragePoolCapabilities
The API is in the storage family not the domain family Signed-off-by: John Ferlan --- docs/formatstoragecaps.html.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/formatstoragecaps.html.in b/docs/formatstoragecaps.html.in index 900303aef7..a9ecc371fa 100644 --- a/docs/formatstoragecaps.html.in +++ b/docs/formatstoragecaps.html.in @@ -20,7 +20,7 @@ (Since 5.2.0): -virConnectGetStoragePoolCapabilities +virConnectGetStoragePoolCapabilities The root element that emulator capability XML document starts with is -- 2.28.0
[PATCH 5/7] logging: Resolve mem leak in virLogDaemonPreExecRestart
Initialize and free @magic since virJSONValueObjectAppendString
does not free it for us eventually.
Signed-off-by: John Ferlan
---
src/logging/log_daemon.c | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/src/logging/log_daemon.c b/src/logging/log_daemon.c
index be93c63eb5..6b8f3b6fe5 100644
--- a/src/logging/log_daemon.c
+++ b/src/logging/log_daemon.c
@@ -508,7 +508,7 @@ virLogDaemonPreExecRestart(const char *state_file,
virJSONValuePtr child;
char *state = NULL;
virJSONValuePtr object = virJSONValueNewObject();
-char *magic;
+char *magic = NULL;
VIR_DEBUG("Running pre-restart exec");
@@ -523,10 +523,8 @@ virLogDaemonPreExecRestart(const char *state_file,
if (!(magic = virLogDaemonGetExecRestartMagic()))
goto cleanup;
-if (virJSONValueObjectAppendString(object, "magic", magic) < 0) {
-VIR_FREE(magic);
+if (virJSONValueObjectAppendString(object, "magic", magic) < 0)
goto cleanup;
-}
if (!(child = virLogHandlerPreExecRestart(logDaemon->handler)))
goto cleanup;
@@ -559,6 +557,7 @@ virLogDaemonPreExecRestart(const char *state_file,
abort(); /* This should be impossible to reach */
cleanup:
+VIR_FREE(magic);
VIR_FREE(state);
virJSONValueFree(object);
return -1;
--
2.28.0
[PATCH 1/7] util: Fix memory leak in virNetDevOpenvswitchInterfaceGetMaster
Since 032548c4 @cmd was never autofree'd. Perhaps as a result of
VIR_AUTOPTR type changes occurring at roughly the same time so the
copy pasta missed this.
Found by Coverity.
Signed-off-by: John Ferlan
---
src/util/virnetdevopenvswitch.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c
index 7452527f49..d380b0cf22 100644
--- a/src/util/virnetdevopenvswitch.c
+++ b/src/util/virnetdevopenvswitch.c
@@ -428,7 +428,7 @@ virNetDevOpenvswitchInterfaceStats(const char *ifname,
int
virNetDevOpenvswitchInterfaceGetMaster(const char *ifname, char **master)
{
-virCommandPtr cmd = virNetDevOpenvswitchCreateCmd();
+g_autoptr(virCommand) cmd = virNetDevOpenvswitchCreateCmd();
int exitstatus;
*master = NULL;
--
2.28.0
[PATCH 2/7] util: Resolve resource leak in virExec error path
On error, the @pidfilefd was not released
Found by Coverity
Signed-off-by: John Ferlan
---
src/util/vircommand.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/util/vircommand.c b/src/util/vircommand.c
index e47dd6b932..1716225aeb 100644
--- a/src/util/vircommand.c
+++ b/src/util/vircommand.c
@@ -792,12 +792,14 @@ virExec(virCommandPtr cmd)
if (virSetInherit(pidfilefd, true) < 0) {
virReportSystemError(errno, "%s",
_("Cannot disable close-on-exec flag"));
+virPidFileReleasePath(cmd->pidfile, pidfilefd);
goto fork_error;
}
c = '1';
if (safewrite(pipesync[1], , sizeof(c)) != sizeof(c)) {
virReportSystemError(errno, "%s", _("Unable to notify child
process"));
+virPidFileReleasePath(cmd->pidfile, pidfilefd);
goto fork_error;
}
VIR_FORCE_CLOSE(pipesync[0]);
--
2.28.0
[PATCH 6/7] locking: Resolve mem leak in virLockDaemonPreExecRestart
Initialize and free @magic since virJSONValueObjectAppendString
does not free it for us eventually.
Signed-off-by: John Ferlan
---
src/locking/lock_daemon.c | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c
index 57c7fb088f..851e9fc6f0 100644
--- a/src/locking/lock_daemon.c
+++ b/src/locking/lock_daemon.c
@@ -700,7 +700,7 @@ virLockDaemonPreExecRestart(const char *state_file,
virJSONValuePtr child;
char *state = NULL;
virJSONValuePtr object = virJSONValueNewObject();
-char *magic;
+char *magic = NULL;
virHashKeyValuePairPtr pairs = NULL, tmp;
virJSONValuePtr lockspaces;
@@ -748,10 +748,8 @@ virLockDaemonPreExecRestart(const char *state_file,
if (!(magic = virLockDaemonGetExecRestartMagic()))
goto cleanup;
-if (virJSONValueObjectAppendString(object, "magic", magic) < 0) {
-VIR_FREE(magic);
+if (virJSONValueObjectAppendString(object, "magic", magic) < 0)
goto cleanup;
-}
if (!(state = virJSONValueToString(object, true)))
goto cleanup;
@@ -775,6 +773,7 @@ virLockDaemonPreExecRestart(const char *state_file,
abort(); /* This should be impossible to reach */
cleanup:
+VIR_FREE(magic);
VIR_FREE(pairs);
VIR_FREE(state);
virJSONValueFree(object);
--
2.28.0
[PATCH 4/7] qemu: Fix resource leak and coding error in qemuDomainGetIOThreadsMon
If qemuDomainGetIOThreadsMon fails because qemuDomainObjExitMonitor
fails, then a -1 is returned which overwrites @niothreads causing a
memory leak. Let's pass @niothreads instead.
Found by Coverity.
Signed-off-by: John Ferlan
---
src/qemu/qemu_driver.c | 19 +--
1 file changed, 9 insertions(+), 10 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 8eaa3ce68f..870159de47 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4972,17 +4972,16 @@ qemuDomainGetMaxVcpus(virDomainPtr dom)
static int
qemuDomainGetIOThreadsMon(virQEMUDriverPtr driver,
virDomainObjPtr vm,
- qemuMonitorIOThreadInfoPtr **iothreads)
+ qemuMonitorIOThreadInfoPtr **iothreads,
+ int *niothreads)
{
qemuDomainObjPrivatePtr priv = vm->privateData;
-int niothreads = 0;
qemuDomainObjEnterMonitor(driver, vm);
-niothreads = qemuMonitorGetIOThreads(priv->mon, iothreads);
-if (qemuDomainObjExitMonitor(driver, vm) < 0 || niothreads < 0)
+*niothreads = qemuMonitorGetIOThreads(priv->mon, iothreads);
+if (qemuDomainObjExitMonitor(driver, vm) < 0)
return -1;
-
-return niothreads;
+return 0;
}
@@ -5014,7 +5013,7 @@ qemuDomainGetIOThreadsLive(virQEMUDriverPtr driver,
goto endjob;
}
-if ((niothreads = qemuDomainGetIOThreadsMon(driver, vm, )) < 0)
+if (qemuDomainGetIOThreadsMon(driver, vm, , ) < 0)
goto endjob;
/* Nothing to do */
@@ -18507,7 +18506,7 @@ qemuDomainGetStatsIOThread(virQEMUDriverPtr driver,
qemuDomainObjPrivatePtr priv = dom->privateData;
size_t i;
qemuMonitorIOThreadInfoPtr *iothreads = NULL;
-int niothreads;
+int niothreads = 0;
int ret = -1;
if (!HAVE_JOB(privflags) || !virDomainObjIsActive(dom))
@@ -18516,8 +18515,8 @@ qemuDomainGetStatsIOThread(virQEMUDriverPtr driver,
if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD))
return 0;
-if ((niothreads = qemuDomainGetIOThreadsMon(driver, dom, )) < 0)
-return -1;
+if (qemuDomainGetIOThreadsMon(driver, dom, , ) < 0)
+goto cleanup;
/* qemuDomainGetIOThreadsMon returns a NULL-terminated list, so we must
free
* it even if it returns 0 */
--
2.28.0
[PATCH 0/7] A few Coverity related issues
Fix some of the issues that have built up. Also a docs link fix that I tripped across at some point in time. Again as a reminder, I no longer have push access ;-) John Ferlan (7): util: Fix memory leak in virNetDevOpenvswitchInterfaceGetMaster util: Resolve resource leak in virExec error path docs: Fix link for virConnectGetStoragePoolCapabilities qemu: Fix resource leak and coding error in qemuDomainGetIOThreadsMon logging: Resolve mem leak in virLogDaemonPreExecRestart locking: Resolve mem leak in virLockDaemonPreExecRestart qemu: Fix some issues in virQEMUDriverConfigLoadNVRAMEntry docs/formatstoragecaps.html.in | 2 +- src/locking/lock_daemon.c | 7 +++ src/logging/log_daemon.c| 7 +++ src/qemu/qemu_conf.c| 5 ++--- src/qemu/qemu_driver.c | 19 +-- src/util/vircommand.c | 2 ++ src/util/virnetdevopenvswitch.c | 2 +- 7 files changed, 21 insertions(+), 23 deletions(-) -- 2.28.0
Re: [libvirt PATCH 1/2] src: rework static analysis detection
On 11/20/20 8:39 AM, Pavel Hrdina wrote:
> On Fri, Nov 20, 2020 at 06:57:10AM -0500, John Ferlan wrote:
>>
>>
>> On 11/16/20 10:36 AM, Pavel Hrdina wrote:
>>> Inspired by QEMU code.
>>>
>>> Signed-off-by: Pavel Hrdina
>>> ---
>>> meson.build| 14 --
>>> src/internal.h | 4
>>> 2 files changed, 4 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/meson.build b/meson.build
>>> index cecaad199d..dbbc9632f1 100644
>>> --- a/meson.build
>>> +++ b/meson.build
>>> @@ -142,20 +142,6 @@ if get_option('test_coverage')
>>> endif
>>>
>>>
>>> -# Detect when running under the clang static analyzer's scan-build driver
>>> -# or Coverity-prevent's cov-build. Define STATIC_ANALYSIS accordingly.
>>> -
>>> -rc = run_command(
>>> - 'sh', '-c',
>>> - 'test -n "${CCC_ANALYZER_HTML}"' +
>>> -' -o -n "${CCC_ANALYZER_ANALYSIS+set}"' +
>>> -' -o -n "$COVERITY_BUILD_COMMAND$COVERITY_LD_PRELOAD"',
>>> -)
>>> -if rc.returncode() == 0
>>> - conf.set('STATIC_ANALYSIS', 1)
>>> -endif
>>> -
>>> -
>>> # Add RPATH information when building for a non-standard prefix, or
>>> # when explicitly requested to do so
>>>
>>> diff --git a/src/internal.h b/src/internal.h
>>> index d167e56b48..5226667d3d 100644
>>> --- a/src/internal.h
>>> +++ b/src/internal.h
>>> @@ -29,6 +29,10 @@
>>> #include
>>> #include "glibcompat.h"
>>>
>>> +#if defined __clang_analyzer__ || defined __COVERITY__
>>
>> ^^ Bah humbug ... what defines __COVERITY__ then?
>>
>> In my Coverity environment, nothing... OK, sure I can add it, no problem
>> but something in QEMU's coverity build environment must do that as it's
>> not predefined as far as I can tell.
>
> Hi John,
>
> There is no need to add it anywhere. When building something using
> coverity it is indeed not defined so GCC will ignore any code guarded
> with __COVERITY__.
>
> The __COVERITY__ is defined by cov-emit binary which is executed by
> cov-build internally which creates the files that are later analyzed.
>
> This is directly from cov-emit --help:
>
> Description
>
> The cov-emit command parses a source file and outputs it into a directory
> (emit repository) that can later be analyzed with cov-analyze. The
> cov-emit command is typically called by cov-translate, which is in turn
> typically called by cov-build (cov-emit is a low-level command and is not
> normally called directly). The cov-emit command defines the __COVERITY__
> preprocessor symbol.
>
> Pavel
>
Well it didn't seem to get defined in my case; otherwise, I wouldn't
have noted it here. ;-) Of course more research ends up figuring this
out .
Prior to this change, STATIC_ANALYSIS was defined in my /
meson-config.h file because COVERITY_LD_PRELOAD is defined. After this
change it is not defined there.
After this change, I got a build error because I have some extra
sa_assert's in my local sources to avoid various Coverity warnings. One
of those is in qemu_agent.c in the qemuAgentCommandFull where I had to add:
+if (ret == 0)
+sa_assert(*reply);
just prior to the return. This resulted in the build error:
../src/qemu/qemu_agent.c: In function ‘qemuAgentCommandFull’:
../src/qemu/qemu_agent.c:1137:26: error: suggest braces around empty
body in an ‘if’ statement [-Werror=empty-body]
1137 | sa_assert(*reply);
| ^
cc1: all warnings being treated as errors
[10/305] Compiling C object
src/qemu/l..._driver_qemu_impl.a.p/qemu_hotplug.c.o
ninja: build stopped: subcommand failed.
which implied to me that sa_assert wasn't defined as a result of a
coding error if sa_assert wasn't defined.
FWIW:
This was done to avoid callers complaining because *reply was set to
NULL before a goto cleanup; and before being set to msg.rxObject.
Coverity has a "hard time" deciding when one variable (in this case
@ret) controls two conditions (in this case @*reply would be filled in).
So in the caller it runs a pass where the return value would 0 and
*reply = NULL - it's an impossible condition when you read the code.
Hence I added the sa_assert, which interestingly enough in this case
tripped because of missing { ... }, but that ended up being showing me
that STATIC_ANALYSIS was not defined in my build env any more.
What got a bit more confusing to me on this though is that if I fix my
local patch to have the { }, then I don't get any Coverity warnings like
I was getting when the sa_assert wasn't there. So that perhaps implies
there's a pass running with *and* without STATIC_ANALYSIS being defined.
If I look at the Coverity created build-log.txt I see two passes being made:
...
[1182705] EXECUTING: /bin/sh -c "gcc ... qemu_agent.c"
...
[1182707] COMPILING: /opt/cov-analysis-linux64-2020.09/bin/cov-translate
gcc ... qemu_agent.c
...
So I believe I have my answer ...
John
Re: [libvirt PATCH 1/2] src: rework static analysis detection
On 11/16/20 10:36 AM, Pavel Hrdina wrote:
> Inspired by QEMU code.
>
> Signed-off-by: Pavel Hrdina
> ---
> meson.build| 14 --
> src/internal.h | 4
> 2 files changed, 4 insertions(+), 14 deletions(-)
>
> diff --git a/meson.build b/meson.build
> index cecaad199d..dbbc9632f1 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -142,20 +142,6 @@ if get_option('test_coverage')
> endif
>
>
> -# Detect when running under the clang static analyzer's scan-build driver
> -# or Coverity-prevent's cov-build. Define STATIC_ANALYSIS accordingly.
> -
> -rc = run_command(
> - 'sh', '-c',
> - 'test -n "${CCC_ANALYZER_HTML}"' +
> -' -o -n "${CCC_ANALYZER_ANALYSIS+set}"' +
> -' -o -n "$COVERITY_BUILD_COMMAND$COVERITY_LD_PRELOAD"',
> -)
> -if rc.returncode() == 0
> - conf.set('STATIC_ANALYSIS', 1)
> -endif
> -
> -
> # Add RPATH information when building for a non-standard prefix, or
> # when explicitly requested to do so
>
> diff --git a/src/internal.h b/src/internal.h
> index d167e56b48..5226667d3d 100644
> --- a/src/internal.h
> +++ b/src/internal.h
> @@ -29,6 +29,10 @@
> #include
> #include "glibcompat.h"
>
> +#if defined __clang_analyzer__ || defined __COVERITY__
^^ Bah humbug ... what defines __COVERITY__ then?
In my Coverity environment, nothing... OK, sure I can add it, no problem
but something in QEMU's coverity build environment must do that as it's
not predefined as far as I can tell.
John
> +#define STATIC_ANALYSIS 1
> +#endif
> +
> #if STATIC_ANALYSIS
> # undef NDEBUG /* Don't let a prior NDEBUG definition cause trouble. */
> # include
>
Re: [PATCH 1/1] qemu_driver.c: do not redefine 'event' in qemuDomainDefineXMLFlags()
On 11/13/20 12:57 PM, Daniel Henrique Barboza wrote: > A bad merge while rebasing 74b2834333a caused the @event variable > to be defined twice, inside the 'cleanup' label, causing coverity > errors. > > This code was originally moved outside of the label by commit > 773c7c43611a. Delete the unintended code in the 'cleanup' > label. > > Fixes: 74b2834333ab3bf500f870e0a6d4e8309379d96a > Reported-by: John Ferlan > Signed-off-by: Daniel Henrique Barboza > --- > src/qemu/qemu_driver.c | 9 - > 1 file changed, 9 deletions(-) > Reviewed-by: John Ferlan Tks - John
Re: [PATCH v1 01/10] qemu_driver.c: use g_autoptr() with virDomainDef pointers
Coverity found a bad merge conflict resolution...
On 11/12/20 4:48 PM, Daniel Henrique Barboza wrote:
> Signed-off-by: Daniel Henrique Barboza
> ---
> src/qemu/qemu_driver.c | 54 ++
> 1 file changed, 18 insertions(+), 36 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 05f8eb2cb7..fdbac9d21d 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
[...]
> ret = -1;
> @@ -6689,8 +6682,8 @@ qemuDomainDefineXMLFlags(virConnectPtr conn,
> unsigned int flags)
> {
> virQEMUDriverPtr driver = conn->privateData;
> -virDomainDefPtr def = NULL;
> -virDomainDefPtr oldDef = NULL;
> +g_autoptr(virDomainDef) def = NULL;
> +g_autoptr(virDomainDef) oldDef = NULL;
> virDomainObjPtr vm = NULL;
> virDomainPtr dom = NULL;
> virObjectEventPtr event = NULL;
> @@ -6751,8 +6744,6 @@ qemuDomainDefineXMLFlags(virConnectPtr conn,
> dom = virGetDomain(conn, vm->def->name, vm->def->uuid, vm->def->id);
>
> cleanup:
> -virDomainDefFree(oldDef);
> -virDomainDefFree(def);
> virDomainObjEndAPI();
> virObjectEventStateQueue(driver->domainEventState, event);
> return dom;
> @@ -7755,7 +7746,7 @@ qemuDomainAttachDeviceLiveAndConfig(virDomainObjPtr vm,
The code that was pushed added:
+
+event = virDomainEventLifecycleNewFromObj(vm,
+ VIR_DOMAIN_EVENT_DEFINED,
+ !oldDef ?
+ VIR_DOMAIN_EVENT_DEFINED_ADDED :
+ VIR_DOMAIN_EVENT_DEFINED_UPDATED);
+
+VIR_INFO("Creating domain '%s'", vm->def->name);
+dom = virGetDomain(conn, vm->def->name, vm->def->uuid, vm->def->id);
+
within the cleanup: label even though recent commit 773c7c43611 had
moved the code up higher.
The coverity error is that @event gets overwritten...
Hopefully easy enough to figure out.
John
[...]
Re: [PATCH 1/2] virnetdevopenvswitch: Get names for dpdkvhostuserclient too
On 11/11/20 3:38 AM, Michal Privoznik wrote: > There are two type of vhostuser ports: > > dpdkvhostuser - OVS creates the socket and QEMU connects to it > dpdkvhostuserclient - QEMU creates the socket and OVS connects to it > > But of course ovs-vsctl syntax for fetching ifname is different. > So far, we've implemented the former. The lack of implementation > for the latter means that we are not detecting the interface name > and thus not reporting it in domain XML, or failing to get > interface statistics. > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1767013 > > Signed-off-by: Michal Privoznik > --- > src/qemu/qemu_command.c | 1 + > src/qemu/qemu_hotplug.c | 1 + > src/util/virnetdevopenvswitch.c | 60 +++-- > src/util/virnetdevopenvswitch.h | 1 + > tests/qemuxml2argvmock.c| 1 + > 5 files changed, 46 insertions(+), 18 deletions(-) > [...] > diff --git a/src/util/virnetdevopenvswitch.h b/src/util/virnetdevopenvswitch.h > index c9ea592058..5cd1d22ae3 100644 > --- a/src/util/virnetdevopenvswitch.h > +++ b/src/util/virnetdevopenvswitch.h > @@ -61,6 +61,7 @@ int virNetDevOpenvswitchInterfaceGetMaster(const char > *ifname, char **master) > ATTRIBUTE_NONNULL(1) G_GNUC_WARN_UNUSED_RESULT; > > int virNetDevOpenvswitchGetVhostuserIfname(const char *path, > + bool server, > char **ifname) > ATTRIBUTE_NONNULL(2) G_GNUC_WARN_UNUSED_RESULT G_GNUC_NO_INLINE; ^^ A coverity build breaker... s/(2)/(3) will resolve. John
Re: [libvirt PATCH 21/25] vircgroup: drop @create from virCgroupNewDomainPartition
On 11/3/20 7:41 AM, Pavel Hrdina wrote: > All callers pass true. > > Signed-off-by: Pavel Hrdina > --- > src/util/vircgroup.c | 5 + > src/util/vircgrouppriv.h | 1 - > tests/vircgrouptest.c| 4 ++-- > 3 files changed, 3 insertions(+), 7 deletions(-) > [...] > diff --git a/src/util/vircgrouppriv.h b/src/util/vircgrouppriv.h > index 7248bc1d35..77e01519b0 100644 > --- a/src/util/vircgrouppriv.h > +++ b/src/util/vircgrouppriv.h > @@ -123,7 +123,6 @@ int virCgroupNewPartition(const char *path, > int virCgroupNewDomainPartition(virCgroupPtr partition, > const char *driver, > const char *name, > -bool create, > virCgroupPtr *group) > ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(5); ^^ FYI: This breaks the Coverity build - need to use ATTRIBUTE_NONNULL(4) John [...]
Re: [libvirt PATCH v5 6/6] Include vdpa devices in node device list
On 10/14/20 1:08 PM, Jonathon Jongsma wrote:
> The current udev node device driver ignores all events related to vdpa
> devices. Since libvirt now supports vDPA network devices, include these
> devices in the device list.
>
> Example output:
>
> virsh # nodedev-list
> [...ommitted long list of nodedevs...]
> vdpa_vdpa0
>
> virsh # nodedev-dumpxml vdpa_vdpa0
>
> vdpa_vdpa0
> /sys/devices/vdpa0
> computer
>
> vhost_vdpa
>
>
> /dev/vhost-vdpa-0
>
>
>
> NOTE: normally the 'parent' would be a PCI device instead of 'computer',
> but this example output is from the vdpa_sim kernel module, so it
> doesn't have a normal parent device.
>
> Signed-off-by: Jonathon Jongsma
> ---
> docs/formatnode.html.in| 9 +
> docs/schemas/nodedev.rng | 10 ++
> include/libvirt/libvirt-nodedev.h | 1 +
> src/conf/node_device_conf.c| 14
> src/conf/node_device_conf.h| 11 ++-
> src/conf/virnodedeviceobj.c| 4 ++-
> src/node_device/node_device_udev.c | 53 ++
> tools/virsh-nodedev.c | 3 ++
> 8 files changed, 103 insertions(+), 2 deletions(-)
>
Coverity notes a RESOURCE_LEAK
> diff --git a/src/node_device/node_device_udev.c
> b/src/node_device/node_device_udev.c
> index 29a7eaa07c..b1b8427c05 100644
> --- a/src/node_device/node_device_udev.c
> +++ b/src/node_device/node_device_udev.c
> @@ -1142,6 +1142,55 @@ udevProcessCSS(struct udev_device *device,
> return 0;
> }
>
> +
> +static int
> +udevGetVDPACharDev(const char *sysfs_path,
> + virNodeDevCapDataPtr data)
> +{
> +struct dirent *entry;
> +DIR *dir = NULL;
> +int direrr;
> +
> +if (virDirOpenIfExists(, sysfs_path) <= 0)
> +return -1;
Any return after this leaks @dir - need a VIR_CLOSE_DIR(dir)
> +
> +while ((direrr = virDirRead(dir, , NULL)) > 0) {
> +if (g_str_has_prefix(entry->d_name, "vhost-vdpa")) {
> +g_autofree char *chardev = g_strdup_printf("/dev/%s",
> entry->d_name);
> +
> +if (!virFileExists(chardev)) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("vDPA chardev path '%s' does not exist"),
> + chardev);> +return -1;
> +}
> +VIR_DEBUG("vDPA chardev is at '%s'", chardev);
> +
> +data->vdpa.chardev = g_steal_pointer();
> +break;
> +}
> +}
> +
> +if (direrr < 0)
> +return -1;
> +
> +return 0;
> +}
> +
John
[...]
Re: [libvirt PATCH v5 4/6] qemu: add monitor functions for handling file descriptors
On 10/14/20 1:08 PM, Jonathon Jongsma wrote:
> add-fd, remove-fd, and query-fdsets provide functionality that can be
> used for passing fds to qemu and closing fdsets that are no longer
> necessary.
>
> Signed-off-by: Jonathon Jongsma
> ---
> src/qemu/qemu_monitor.c | 93 +++
> src/qemu/qemu_monitor.h | 41 +
> src/qemu/qemu_monitor_json.c | 173 +++
> src/qemu/qemu_monitor_json.h | 12 +++
> 4 files changed, 319 insertions(+)
>
Coverity indicated a possible RESOURCE_LEAK
> +/* if fdset is negative, qemu will create a new fdset and add the fd to that
> */
> +int qemuMonitorJSONAddFileHandleToSet(qemuMonitorPtr mon,
> + int fd,
> + int fdset,
> + const char *opaque,
> + qemuMonitorAddFdInfoPtr fdinfo)
> +{
> +virJSONValuePtr args = NULL;
> +g_autoptr(virJSONValue) reply = NULL;
> +g_autoptr(virJSONValue) cmd = NULL;
> +
> +if (virJSONValueObjectCreate(, "S:opaque", opaque, NULL) < 0)
> +return -1;
> +
> +if (fdset >= 0)
> +if (virJSONValueObjectAdd(args, "j:fdset-id", fdset, NULL) < 0)
Leaks @args
> +return -1;
I'm surprised the code style gremlins didn't complain about not having {
} or combining the conditions
> +
> +if (!(cmd = qemuMonitorJSONMakeCommandInternal("add-fd", args)))
> +return -1;
I think at this point @args is consumed within @cmd ... which really
confuses Coverity, but I have a bunch of hacks to handle that...
John
> +
> +if (qemuMonitorJSONCommandWithFd(mon, cmd, fd, ) < 0)
> +return -1;
> +
> +if (qemuMonitorJSONCheckError(cmd, reply) < 0)
> +return -1;
> +
> +if (qemuAddfdInfoParse(reply, fdinfo) < 0)
> +return -1;
> +
> +return 0;
> +}
> +
[...]
Re: [libvirt PATCH v5 3/6] qemu: add vdpa support
On 10/14/20 1:08 PM, Jonathon Jongsma wrote:
> Enable for qemu domains. This provides basic
> support and does not support hotplug or migration.
>
> Signed-off-by: Jonathon Jongsma
> ---
> src/qemu/qemu_command.c | 35 +++--
> src/qemu/qemu_command.h | 3 +-
> src/qemu/qemu_domain.c| 6 ++-
> src/qemu/qemu_hotplug.c | 14 ---
> src/qemu/qemu_interface.c | 23 +++
> src/qemu/qemu_interface.h | 2 +
> src/qemu/qemu_migration.c | 10 -
> src/qemu/qemu_validate.c | 14 +++
> .../net-vdpa.x86_64-latest.args | 38 +++
> tests/qemuxml2argvdata/net-vdpa.xml | 28 ++
> tests/qemuxml2argvmock.c | 11 +-
> tests/qemuxml2argvtest.c | 1 +
> tests/qemuxml2xmloutdata/net-vdpa.xml | 34 +
> tests/qemuxml2xmltest.c | 1 +
> 14 files changed, 206 insertions(+), 14 deletions(-)
> create mode 100644 tests/qemuxml2argvdata/net-vdpa.x86_64-latest.args
> create mode 100644 tests/qemuxml2argvdata/net-vdpa.xml
> create mode 100644 tests/qemuxml2xmloutdata/net-vdpa.xml
Coverity indicated a possible RESOURCE_LEAK
[...]
> @@ -8203,13 +8212,17 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver,
>
> break;
>
> +case VIR_DOMAIN_NET_TYPE_VDPA:
> +if ((vdpafd = qemuInterfaceVDPAConnect(net)) < 0)
> +goto cleanup;
> +break;
> +
Between here and where it gets used/consumed, it's possible to jump to
cleanup. Whether it's technically possible based on various tests made,
I'm not 100% sure. The cleanup code would need to account for
VIR_CLOSE_FORCE(vdpafd) if (vdpafd >= 0)...
> case VIR_DOMAIN_NET_TYPE_USER:
> case VIR_DOMAIN_NET_TYPE_SERVER:
> case VIR_DOMAIN_NET_TYPE_CLIENT:
> case VIR_DOMAIN_NET_TYPE_MCAST:
> case VIR_DOMAIN_NET_TYPE_INTERNAL:
> case VIR_DOMAIN_NET_TYPE_UDP:
> -case VIR_DOMAIN_NET_TYPE_VDPA:
> case VIR_DOMAIN_NET_TYPE_LAST:
> /* nada */
> break;
> @@ -8327,13 +8340,29 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver,
> vhostfd[i] = -1;
> }
>
> +if (vdpafd > 0) {
> +g_autofree char *fdset = NULL;
> +g_autofree char *addfdarg = NULL;
> +
> +virCommandPassFD(cmd, vdpafd, VIR_COMMAND_PASS_FD_CLOSE_PARENT);
> +fdset = qemuVirCommandGetFDSet(cmd, vdpafd);
> +if (!fdset)
> +goto cleanup;
> +vdpafdName = qemuVirCommandGetDevSet(cmd, vdpafd);
> +/* set opaque to the devicepath so that we can look up the fdset
> later
> + * if necessary */
> +addfdarg = g_strdup_printf("%s,opaque=%s", fdset,
> + net->data.vdpa.devicepath);
> +virCommandAddArgList(cmd, "-add-fd", addfdarg, NULL);
> +}
> +
As long as the above code consumes vdpafd, then just set it to -1 right
after consumption to avoid double cleanup when it is really closed.
John
> if (chardev)
> virCommandAddArgList(cmd, "-chardev", chardev, NULL);
>
> if (!(hostnetprops = qemuBuildHostNetStr(net,
> tapfdName, tapfdSize,
> vhostfdName, vhostfdSize,
> - slirpfdName)))
> + slirpfdName, vdpafdName)))
> goto cleanup;
>
> if (!(host = virQEMUBuildNetdevCommandlineFromJSON(hostnetprops,
[...]
Re: [libvirt PATCH] remote: remove leftover goto
On 10/9/20 7:09 AM, Ján Tomko wrote: > Signed-off-by: Ján Tomko > Reported-by: John Ferlan > Fixes: 8487595bee0c04e56b0d8e866c5c71318faf1689 > Signed-off-by: Ján Tomko > --- > /me gets the paper bag > > src/remote/remote_driver.c | 1 - > 1 file changed, 1 deletion(-) > Reviewed-by: John Ferlan Although you may want to clean up the double signoff John
Re: [libvirt PATCH 3/8] remote: use g_new0 instead of VIR_ALLOC
On 10/8/20 8:12 AM, Ján Tomko wrote: > Signed-off-by: Ján Tomko > --- > src/remote/remote_daemon_config.c | 3 +- > src/remote/remote_daemon_dispatch.c | 225 ++-- > src/remote/remote_daemon_stream.c | 6 +- > src/remote/remote_driver.c | 102 - > 4 files changed, 108 insertions(+), 228 deletions(-) > [...] > diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c > index 6e0c04f168..49769ac04d 100644 > --- a/src/remote/remote_driver.c > +++ b/src/remote/remote_driver.c [...] > @@ -7676,14 +7635,13 @@ remoteDomainGetFSInfo(virDomainPtr dom, > goto cleanup; > } > > -if (VIR_ALLOC_N(info_ret, ret.info.info_len) < 0) > +info_ret = g_new0(virDomainFSInfoPtr, ret.info.info_len); > goto cleanup; Coverity notes a simple / trivial cleanup [...]
Re: [libvirt PATCH] Fix incorrect g_new0 conversions
On 10/6/20 7:15 AM, Ján Tomko wrote: > I left in a 'return' or 'goto cleanup' in a few places that > where I did the conversion manually. > > Signed-off-by: Ján Tomko > Reported-by: John Ferlan > --- > src/esx/esx_storage_backend_vmfs.c | 1 - > src/libxl/libxl_capabilities.c | 2 -- > src/libxl/libxl_driver.c | 1 - > src/qemu/qemu_hotplug.c| 1 - > src/storage/storage_backend_iscsi_direct.c | 1 - > tools/virt-login-shell-helper.c| 1 - > 6 files changed, 7 deletions(-) > Thanks - Reviewed-by: John Ferlan John
Re: [libvirt PATCHv2] qemu: process: use g_new0
[...]
Coverity notes...
> @@ -1074,20 +1070,17 @@ qemuProcessHandleGraphics(qemuMonitorPtr mon
> G_GNUC_UNUSED,
> virDomainEventGraphicsSubjectPtr subject = NULL;
> size_t i;
>
> -if (VIR_ALLOC(localAddr) < 0)
> -goto error;
> +localAddr = g_new0(virDomainEventGraphicsAddress, 1);
> localAddr->family = localFamily;
> localAddr->service = g_strdup(localService);
> localAddr->node = g_strdup(localNode);
>
> -if (VIR_ALLOC(remoteAddr) < 0)
> -goto error;
> +remoteAddr = g_new0(virDomainEventGraphicsAddress, 1);
> remoteAddr->family = remoteFamily;
> remoteAddr->service = g_strdup(remoteService);
> remoteAddr->node = g_strdup(remoteNode);
>
> -if (VIR_ALLOC(subject) < 0)
> -goto error;
> +subject = g_new0(virDomainEventGraphicsSubject, 1);
> if (x509dname) {
> if (VIR_REALLOC_N(subject->identities, subject->nidentity+1) < 0)
> goto error;
There's no way to error: now w/o @localAddr, @remoteAddr, & @subject
being allocated, thus there's no need to check whether they're non-null
before accessing.
John
[...]
Re: [libvirt PATCH 9/9] qemu: use g_new0
Coverity notes ... > diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c > index 11b549b12b..09f8525cfa 100644 > --- a/src/qemu/qemu_hotplug.c > +++ b/src/qemu/qemu_hotplug.c > @@ -928,8 +928,7 @@ qemuDomainFindOrCreateSCSIDiskController(virQEMUDriverPtr > driver, > > /* No SCSI controller present, for backward compatibility we > * now hotplug a controller */ > -if (VIR_ALLOC(cont) < 0) > -return NULL; > +cont = g_new0(virDomainControllerDef, 1); > cont->type = VIR_DOMAIN_CONTROLLER_TYPE_SCSI; > cont->idx = controller; > if (model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DEFAULT) > @@ -1243,11 +1242,9 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, > if (!tapfdSize) > tapfdSize = vhostfdSize = 1; > queueSize = tapfdSize; > -if (VIR_ALLOC_N(tapfd, tapfdSize) < 0) > -goto cleanup; > +tapfd = g_new0(int, tapfdSize); > memset(tapfd, -1, sizeof(*tapfd) * tapfdSize); > -if (VIR_ALLOC_N(vhostfd, vhostfdSize) < 0) > -goto cleanup; > +vhostfd = g_new0(int, vhostfdSize); > memset(vhostfd, -1, sizeof(*vhostfd) * vhostfdSize); > if (qemuInterfaceBridgeConnect(vm->def, driver, net, > tapfd, ) < 0) > @@ -1262,11 +1259,9 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, > if (!tapfdSize) > tapfdSize = vhostfdSize = 1; > queueSize = tapfdSize; > -if (VIR_ALLOC_N(tapfd, tapfdSize) < 0) > -goto cleanup; > +tapfd = g_new0(int, tapfdSize); > memset(tapfd, -1, sizeof(*tapfd) * tapfdSize); > -if (VIR_ALLOC_N(vhostfd, vhostfdSize) < 0) > -goto cleanup; > +vhostfd = g_new0(int, vhostfdSize); > memset(vhostfd, -1, sizeof(*vhostfd) * vhostfdSize); > if (qemuInterfaceDirectConnect(vm->def, driver, net, > tapfd, tapfdSize, > @@ -1282,10 +1277,9 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, > if (!tapfdSize) > tapfdSize = vhostfdSize = 1; > queueSize = tapfdSize; > -if (VIR_ALLOC_N(tapfd, tapfdSize) < 0) > -goto cleanup; > +tapfd = g_new0(int, tapfdSize); > memset(tapfd, -1, sizeof(*tapfd) * tapfdSize); > -if (VIR_ALLOC_N(vhostfd, vhostfdSize) < 0) > +vhostfd = g_new0(int, vhostfdSize); > goto cleanup; ^^^ Everything below here is unreachable. FWIW: Similar issues after g_new0 calls in: libxlCapsInitNuma libxlConnectDomainXMLToNative virStorageBackendISCSIDirectVolWipeZero virLoginShellGetShellArgv John > memset(vhostfd, -1, sizeof(*vhostfd) * vhostfdSize); > if (qemuInterfaceEthernetConnect(vm->def, driver, net, > @@ -1381,9 +1375,8 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, > goto cleanup; > } > [...]
Re: [libvirt PATCH v2 4/4] storage: add support for qcow2 LUKS encryption
Coverity was unhappy On 9/17/20 7:59 AM, Daniel P. Berrangé wrote: > The storage driver was wired up to support creating raw volumes in LUKS > format, but was never adapted to support LUKS-in-qcow2. This is trivial > as it merely requires the encryption properties to be prefixed with > the "encrypt." prefix, and "encrypt.format=luks" when creating the > volume. > > Signed-off-by: Daniel P. Berrangé [...] > diff --git a/src/util/virqemu.h b/src/util/virqemu.h > index b1296cb657..be14c04d51 100644 > --- a/src/util/virqemu.h > +++ b/src/util/virqemu.h > @@ -60,6 +60,7 @@ char *virQEMUBuildDriveCommandlineFromJSON(virJSONValuePtr > src); > > void virQEMUBuildBufferEscapeComma(virBufferPtr buf, const char *str); > void virQEMUBuildQemuImgKeySecretOpts(virBufferPtr buf, > + int format, >virStorageEncryptionInfoDefPtr enc, >const char *alias) > ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); New argument (2) is an int, so (2) and (3) have to change... John [...]
Re: [libvirt PATCH] docs: virConnectGetCapabilities do not provide pool types
On 7/21/20 10:14 AM, Daniel P. Berrangé wrote: > On Tue, Jul 21, 2020 at 08:46:55AM -0400, John Ferlan wrote: >> >> Upon further reflection and some memory jiggling... >> >> virsh -c storage:///system capabilities >> >> >> >> >> dir >> fs >> netfs >> logical >> iscsi >> iscsi-direct >> scsi >> mpath >> disk >> rbd >> sheepdog >> gluster >> zfs >> >> >> >> >> >> But yeah, without the -c storage:///system one won't see the results >> because 'npools == 0' when virCapabilitiesFormatStoragePoolXML is called >> from virCapabilitiesFormatXML unless it's the storage driver URI. > > Err, don't you mean "virsh pool-capabilities". > pool-capabilities takes a different path ending in virStoragePoolCapsFormat as opposed to capabilities which ends up in virCapabilitiesFormatXML > The "capabilities" command is exclusively for the virt driver usage, > not storage. # virsh version Compiled against library: libvirt 6.6.0 Using library: libvirt 6.6.0 Using API: QEMU 6.6.0 Running hypervisor: QEMU 5.0.50 # virsh -c storage:///system capabilities [as above] This work is/was done Jan/Feb 2019 - Perhaps I'm only getting asked (again) about it now as a result of some downstream documenting :-/. Far too long ago for me to recall why both options were done. But separable enough in commit 05fe03505a to be undone. John > > In the new modular daemon work, "capabilities" API will never be > sent to the storage daemon. > > Regards, > Daniel >
Re: [libvirt PATCH] docs: virConnectGetCapabilities do not provide pool types
Upon further reflection and some memory jiggling... virsh -c storage:///system capabilities dir fs netfs logical iscsi iscsi-direct scsi mpath disk rbd sheepdog gluster zfs But yeah, without the -c storage:///system one won't see the results because 'npools == 0' when virCapabilitiesFormatStoragePoolXML is called from virCapabilitiesFormatXML unless it's the storage driver URI. John On 7/20/20 1:31 PM, John Ferlan wrote: > > > On 7/20/20 3:50 AM, Pino Toscano wrote: >> Remove the paragraph in the storage pool page that mentions >> virConnectGetCapabilities, as virConnectGetCapabilities does not return >> any information about pools. >> >> Signed-off-by: Pino Toscano >> --- >> docs/formatstoragecaps.html.in | 6 -- >> 1 file changed, 6 deletions(-) >> > > I was asked by Pino to review to be "ok" with removal, so yeah: > > Reviewed-by: John Ferlan > > John > > If anyone cares to dredge up history back in Jan/Feb 2019 I believe it > was a true statement in at least a patches I had locally, but reviews, > conversations, and other adjustments to the code changed that. Been too > long to recall all those details. > >> diff --git a/docs/formatstoragecaps.html.in b/docs/formatstoragecaps.html.in >> index ee3888f44d..d8a1cacd96 100644 >> --- a/docs/formatstoragecaps.html.in >> +++ b/docs/formatstoragecaps.html.in >> @@ -13,12 +13,6 @@ >> supported, and if relevant the source format types, the required >> source elements, and the target volume format types. >> >> -The Storage Pool Capabilities XML provides more information than the >> - >> -virConnectGetCapabilities >> - >> -which only provides an enumerated list of supported pool types. >> - >> Element and attribute overview >> >> A query interface was added to the virConnect API's to retrieve the >> >
Re: [libvirt PATCH v2 06/15] network: eliminate unnecessary labels
On 7/7/20 5:08 PM, Laine Stump wrote:
> All these cleanup/error labels were reduced to having just "return
> ret" by a previous patch, so get rid of them and return directly.
>
> Signed-off-by: Laine Stump
> ---
> src/network/bridge_driver.c | 264 --
> src/network/bridge_driver_linux.c | 15 +-
> 2 files changed, 113 insertions(+), 166 deletions(-)
>
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index 31bd0dd92c..79b2ca3330 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
[...]
Coverity noted there's a leak with this part of the change for @field...
>
> @@ -2207,7 +2164,6 @@ networkSetIPv6Sysctls(virNetworkObjPtr obj)
> {
> virNetworkDefPtr def = virNetworkObjGetDef(obj);
> g_autofree char *field = NULL;
> -int ret = -1;
> bool enableIPv6 = !!virNetworkDefGetIPByIndex(def, AF_INET6, 0);
>
> /* set disable_ipv6 if there are no ipv6 addresses defined for the
> @@ -2221,15 +2177,14 @@ networkSetIPv6Sysctls(virNetworkObjPtr obj)
> if (!enableIPv6)
> VIR_DEBUG("ipv6 appears to already be disabled on %s",
>def->bridge);
> -ret = 0;
> -goto cleanup;
> +return 0;
> }
Below here doesn't match w/ current source, but I assume you know that.
Looks like a mis-merge with the review from the previous patch.
>
> if (virFileWriteStr(field, enableIPv6 ? "0" : "1", 0) < 0) {
> virReportSystemError(errno,
> _("cannot write to %s to enable/disable IPv6 "
> "on bridge %s"), field, def->bridge);
> -goto cleanup;
> +return -1;
> }
>
> /* The rest of the ipv6 sysctl tunables should always be set the
> @@ -2246,7 +2201,7 @@ networkSetIPv6Sysctls(virNetworkObjPtr obj)
> if (virFileWriteStr(field, "0", 0) < 0) {
> virReportSystemError(errno,
> _("cannot disable %s"), field);
> -goto cleanup;
> +return -1;
> }
>
> /* All interfaces used as a gateway (which is what this is, by
> @@ -2258,12 +2213,10 @@ networkSetIPv6Sysctls(virNetworkObjPtr obj)
> if (virFileWriteStr(field, "0", 0) < 0) {
> virReportSystemError(errno,
> _("cannot disable %s"), field);
> -goto cleanup;
> +return -1;
> }
>
> -ret = 0;
> - cleanup:
> -return ret;
> +return 0;
> }
[...]
Re: [libvirt PATCH] docs: virConnectGetCapabilities do not provide pool types
On 7/20/20 3:50 AM, Pino Toscano wrote: > Remove the paragraph in the storage pool page that mentions > virConnectGetCapabilities, as virConnectGetCapabilities does not return > any information about pools. > > Signed-off-by: Pino Toscano > --- > docs/formatstoragecaps.html.in | 6 -- > 1 file changed, 6 deletions(-) > I was asked by Pino to review to be "ok" with removal, so yeah: Reviewed-by: John Ferlan John If anyone cares to dredge up history back in Jan/Feb 2019 I believe it was a true statement in at least a patches I had locally, but reviews, conversations, and other adjustments to the code changed that. Been too long to recall all those details. > diff --git a/docs/formatstoragecaps.html.in b/docs/formatstoragecaps.html.in > index ee3888f44d..d8a1cacd96 100644 > --- a/docs/formatstoragecaps.html.in > +++ b/docs/formatstoragecaps.html.in > @@ -13,12 +13,6 @@ > supported, and if relevant the source format types, the required > source elements, and the target volume format types. > > -The Storage Pool Capabilities XML provides more information than the > - > -virConnectGetCapabilities > - > -which only provides an enumerated list of supported pool types. > - > Element and attribute overview > > A query interface was added to the virConnect API's to retrieve the >
Re: [PATCH 2/3] virNetSocketCheckProtocols: lookup IPv6 only if suspecting IPv6
On 7/15/20 7:48 AM, Michal Privoznik wrote:
> There is not much sense trying to disprove host is IPv6 capable
> if we know after first round (getifaddrs()) that is is not.
>
> Signed-off-by: Michal Privoznik
> ---
> src/rpc/virnetsocket.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c
> index b6bc3edc3b..b0d63f0f2c 100644
> --- a/src/rpc/virnetsocket.c
> +++ b/src/rpc/virnetsocket.c
> @@ -205,7 +205,8 @@ int virNetSocketCheckProtocols(bool *hasIPv4,
> freeifaddrs(ifaddr);
>
>
> -if (virNetSocketCheckProtocolByLookup("::1", AF_INET6, hasIPv6) < 0)
> +if (hasIPv6 &&
For this and the subsequent hasIPv4 patch, Coverity complains that
checking "if (hasIPv6)" is a REVERSE_INULL issue since a few lines above
we have "*hasIPv4 = *hasIPv6 = false;" or individual setting to true.
This then should be "if (!*hasIPv6..." and of course the same for hasIPv4.
/me wonders what kind of slight optimization exists or anyone cares
about in the ifaddr loop once both has* bool ptrs are proven true - it's
not like there's a set to false...
John
> +virNetSocketCheckProtocolByLookup("::1", AF_INET6, hasIPv6) < 0)
> return -1;
>
> VIR_DEBUG("Protocols: v4 %d v6 %d", *hasIPv4, *hasIPv6);
>
Re: [PATCH 0/9] Some Coverity memory leak patches
On 6/16/20 8:25 AM, Peter Krempa wrote: > On Tue, Jun 16, 2020 at 08:07:01 -0400, John Ferlan wrote: >> Even though I changed roles, the cron job still runs. I recently >> updated to a newer coverity version and it found some new stuff and >> even more false positives. It's still very unhappy with the usage >> of GLIB_DEPRECATED_ENUMERATOR_IN_* macros for *glib/gspawn.h and >> *gobject/gparam.h, but I can work around that. I had to disable >> the use_after_free type checking because the virObject to GObject >> change just caused a large number of possible false positives for >> virObjectUnref usages (coverity cannot keep track of all the counts). >> >> Anyway, I had a few spare cycles, so I figured I'd pass along at >> least the memory leak and usage things that were found. I haven't >> posted in a while so hopefully I haven't missed some subtlety. >> >> FWIW: I no longer have commit access so I'm at the mercy of someone >> else pushing my patches now. >> >> >> John Ferlan (9): >> util: Fix memory leak in virAuthGetCredential >> util: Fix memory leak in virAuthConfigLookup >> lxc: Fix memory leak in virLXCControllerPopulateDevices >> util: Fix memory leak in virPCIProbeStubDriver >> test: Fix memory leak in testParseXMLDocFromFile >> conf: Fix memory leak in openvzWriteConfigParam >> conf: Fix memory leak in openvzReadFSConf >> conf: Fix memory leak in virCPUDefParseXML > > Patches 1-8: > > Reviewed-by: Peter Krempa > > And I'll push them shortly with the fix mentioned in one of them. > OK - thanks! John I see why I commented the way I did for @absFile , but yeah it's a problem either way... Probably was more focused on the fact it was my original blunder... >> util: Avoid using wrong free function >> >> src/conf/cpu_conf.c| 2 +- >> src/lxc/lxc_controller.c | 3 ++- >> src/openvz/openvz_conf.c | 7 ++- >> src/remote/remote_driver.c | 2 +- >> src/test/test_driver.c | 2 +- >> src/util/iohelper.c| 7 +++ >> src/util/virauth.c | 5 + >> src/util/virauthconfig.c | 4 ++-- >> src/util/virauthconfig.h | 2 +- >> src/util/virpci.c | 1 + >> tests/virauthconfigtest.c | 2 +- >> 11 files changed, 20 insertions(+), 17 deletions(-) >> >> -- >> 2.25.4 >>
Re: [PATCH 9/9] util: Avoid using wrong free function
On 6/16/20 8:16 AM, Daniel P. Berrangé wrote: > On Tue, Jun 16, 2020 at 08:07:10AM -0400, John Ferlan wrote: >> Since 1e2ae2e31, changes to use the automagic free logic didn't take >> into account that one path uses posix_memalign and the other uses >> VIR_ALLOC_N - the former requires using VIR_FREE() and not g_free() >> to free the memory. > > VIR_FREE calls g_free, so they're semantically identical > Hmm... this one was something that had been on my list of false positives (~60 patches) and wait for enough other changes before posting list since March, but I see that reverting it doesn't cause the error any more. So sure dropping is understandable... Tks - John
[PATCH 4/9] util: Fix memory leak in virPCIProbeStubDriver
Since 9ea90206, @drvpath could be overwritten if we jumped to recheck Found by Coverity. Signed-off-by: John Ferlan --- src/util/virpci.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/util/virpci.c b/src/util/virpci.c index 6c7e6bbcab..16936c4be9 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -1018,6 +1018,7 @@ virPCIProbeStubDriver(virPCIStubDriver driver) goto cleanup; } +g_free(drvpath); goto recheck; } -- 2.25.4
[PATCH 5/9] test: Fix memory leak in testParseXMLDocFromFile
Since ceb3255c, @absFile could be leaked if we jumped to error.
Found by Coverity.
Signed-off-by: John Ferlan
---
src/test/test_driver.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 7a1db21718..993f405f3c 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -781,7 +781,7 @@ testParseXMLDocFromFile(xmlNodePtr node, const char *file,
const char *type)
{
xmlNodePtr ret = NULL;
xmlDocPtr doc = NULL;
-char *absFile = NULL;
+g_autofree char *absFile = NULL;
g_autofree char *relFile = NULL;
if ((relFile = virXMLPropString(node, "file"))) {
--
2.25.4
[PATCH 9/9] util: Avoid using wrong free function
Since 1e2ae2e31, changes to use the automagic free logic didn't take
into account that one path uses posix_memalign and the other uses
VIR_ALLOC_N - the former requires using VIR_FREE() and not g_free()
to free the memory.
Found by Coverity.
Signed-off-by: John Ferlan
---
src/util/iohelper.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/src/util/iohelper.c b/src/util/iohelper.c
index 342bae229b..64b7a13f61 100644
--- a/src/util/iohelper.c
+++ b/src/util/iohelper.c
@@ -45,7 +45,11 @@
static int
runIO(const char *path, int fd, int oflags)
{
+#if HAVE_POSIX_MEMALIGN
+void *base = NULL; /* Location to be freed */
+#else
g_autofree void *base = NULL; /* Location to be freed */
+#endif
char *buf = NULL; /* Aligned location within base */
size_t buflen = 1024*1024;
intptr_t alignMask = 64*1024 - 1;
@@ -168,6 +172,9 @@ runIO(const char *path, int fd, int oflags)
ret = 0;
cleanup:
+#if HAVE_POSIX_MEMALIGN
+VIR_FREE(base);
+#endif
if (VIR_CLOSE(fd) < 0 &&
ret == 0) {
virReportSystemError(errno, _("Unable to close %s"), path);
--
2.25.4
[PATCH 3/9] lxc: Fix memory leak in virLXCControllerPopulateDevices
Since 5b82f7f3, @path should have been placed inside the for loop
since it'd need to be free'd for each pass through the loop; otherwise,
we'd leak like a sieve.
Found by Coverity.
Signed-off-by: John Ferlan
---
src/lxc/lxc_controller.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
index 4672920574..734ac73210 100644
--- a/src/lxc/lxc_controller.c
+++ b/src/lxc/lxc_controller.c
@@ -1474,7 +1474,6 @@ static int virLXCControllerSetupDev(virLXCControllerPtr
ctrl)
static int virLXCControllerPopulateDevices(virLXCControllerPtr ctrl)
{
size_t i;
-g_autofree char *path = NULL;
const struct {
int maj;
int min;
@@ -1494,6 +1493,8 @@ static int
virLXCControllerPopulateDevices(virLXCControllerPtr ctrl)
/* Populate /dev/ with a few important bits */
for (i = 0; i < G_N_ELEMENTS(devs); i++) {
+g_autofree char *path = NULL;
+
path = g_strdup_printf("/%s/%s.dev/%s", LXC_STATE_DIR, ctrl->def->name,
devs[i].path);
--
2.25.4
[PATCH 2/9] util: Fix memory leak in virAuthConfigLookup
Since 5084091a, @authcred is filled by a g_key_file_get_string which is
now an allocated string as opposed to some hash table lookup value, so
we need to treat it as so.
Found by Coverity
Signed-off-by: John Ferlan
---
src/util/virauthconfig.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/virauthconfig.c b/src/util/virauthconfig.c
index 8289b28d34..2e50609531 100644
--- a/src/util/virauthconfig.c
+++ b/src/util/virauthconfig.c
@@ -103,7 +103,7 @@ int virAuthConfigLookup(virAuthConfigPtr auth,
{
g_autofree char *authgroup = NULL;
g_autofree char *credgroup = NULL;
-const char *authcred;
+g_autofree char *authcred = NULL;
*value = NULL;
--
2.25.4
[PATCH 7/9] conf: Fix memory leak in openvzReadFSConf
Since 1f5deed9, @veid_str has been leaked in the error path.
Found by Coverity.
Signed-off-by: John Ferlan
---
src/openvz/openvz_conf.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c
index c06bfa13e3..190c57b622 100644
--- a/src/openvz/openvz_conf.c
+++ b/src/openvz/openvz_conf.c
@@ -330,7 +330,7 @@ openvzReadFSConf(virDomainDefPtr def,
{
int ret;
virDomainFSDefPtr fs = NULL;
-char *veid_str = NULL;
+g_autofree char *veid_str = NULL;
char *temp = NULL;
const char *param;
unsigned long long barrier, limit;
@@ -365,8 +365,6 @@ openvzReadFSConf(virDomainDefPtr def,
fs->type = VIR_DOMAIN_FS_TYPE_MOUNT;
if (!(fs->src->path = virStringReplace(temp, "$VEID", veid_str)))
goto error;
-
-VIR_FREE(veid_str);
}
fs->dst = g_strdup("/");
--
2.25.4
[PATCH 8/9] conf: Fix memory leak in virCPUDefParseXML
Since a08669c31, @tsc is not automatically free'd by any g_auto* method. Found by Coverity. Signed-off-by: John Ferlan --- src/conf/cpu_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c index b40737e407..e1b0a5653f 100644 --- a/src/conf/cpu_conf.c +++ b/src/conf/cpu_conf.c @@ -335,7 +335,7 @@ virCPUDefParseXML(xmlXPathContextPtr ctxt, g_autofree char *vendor_id = NULL; g_autofree char *tscScaling = NULL; g_autofree char *migratable = NULL; -virHostCPUTscInfoPtr tsc = NULL; +g_autofree virHostCPUTscInfoPtr tsc = NULL; *cpu = NULL; -- 2.25.4
[PATCH 6/9] conf: Fix memory leak in openvzWriteConfigParam
Since 60623a7c, @temp_file was not properly free'd on the non error path.
Found by Coverity.
Signed-off-by: John Ferlan
---
src/openvz/openvz_conf.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c
index 78547b8b28..c06bfa13e3 100644
--- a/src/openvz/openvz_conf.c
+++ b/src/openvz/openvz_conf.c
@@ -616,7 +616,7 @@ int openvzLoadDomains(struct openvz_driver *driver)
static int
openvzWriteConfigParam(const char * conf_file, const char *param, const char
*value)
{
-char * temp_file = NULL;
+g_autofree char *temp_file = NULL;
int temp_fd = -1;
FILE *fp;
char *line = NULL;
@@ -666,7 +666,6 @@ openvzWriteConfigParam(const char * conf_file, const char
*param, const char *va
VIR_FORCE_CLOSE(temp_fd);
if (temp_file)
unlink(temp_file);
-VIR_FREE(temp_file);
return -1;
}
--
2.25.4
[PATCH 0/9] Some Coverity memory leak patches
Even though I changed roles, the cron job still runs. I recently updated to a newer coverity version and it found some new stuff and even more false positives. It's still very unhappy with the usage of GLIB_DEPRECATED_ENUMERATOR_IN_* macros for *glib/gspawn.h and *gobject/gparam.h, but I can work around that. I had to disable the use_after_free type checking because the virObject to GObject change just caused a large number of possible false positives for virObjectUnref usages (coverity cannot keep track of all the counts). Anyway, I had a few spare cycles, so I figured I'd pass along at least the memory leak and usage things that were found. I haven't posted in a while so hopefully I haven't missed some subtlety. FWIW: I no longer have commit access so I'm at the mercy of someone else pushing my patches now. John Ferlan (9): util: Fix memory leak in virAuthGetCredential util: Fix memory leak in virAuthConfigLookup lxc: Fix memory leak in virLXCControllerPopulateDevices util: Fix memory leak in virPCIProbeStubDriver test: Fix memory leak in testParseXMLDocFromFile conf: Fix memory leak in openvzWriteConfigParam conf: Fix memory leak in openvzReadFSConf conf: Fix memory leak in virCPUDefParseXML util: Avoid using wrong free function src/conf/cpu_conf.c| 2 +- src/lxc/lxc_controller.c | 3 ++- src/openvz/openvz_conf.c | 7 ++- src/remote/remote_driver.c | 2 +- src/test/test_driver.c | 2 +- src/util/iohelper.c| 7 +++ src/util/virauth.c | 5 + src/util/virauthconfig.c | 4 ++-- src/util/virauthconfig.h | 2 +- src/util/virpci.c | 1 + tests/virauthconfigtest.c | 2 +- 11 files changed, 20 insertions(+), 17 deletions(-) -- 2.25.4
[PATCH 1/9] util: Fix memory leak in virAuthGetCredential
Since 5084091a, @tmp is filled by a g_key_file_get_string which is
now an allocated string as opposed to some hash table lookup value,
so we need to treat it as so.
Found by Coverity
Signed-off-by: John Ferlan
---
src/remote/remote_driver.c | 2 +-
src/util/virauth.c | 5 +
src/util/virauthconfig.c | 2 +-
src/util/virauthconfig.h | 2 +-
tests/virauthconfigtest.c | 2 +-
5 files changed, 5 insertions(+), 8 deletions(-)
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index 0aeab9db27..653c68472a 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -4097,7 +4097,7 @@ static int remoteAuthFillFromConfig(virConnectPtr conn,
}
for (ninteract = 0; state->interact[ninteract].id != 0; ninteract++) {
-const char *value = NULL;
+char *value = NULL;
switch (state->interact[ninteract].id) {
case SASL_CB_USER:
diff --git a/src/util/virauth.c b/src/util/virauth.c
index f75e674586..105fca16eb 100644
--- a/src/util/virauth.c
+++ b/src/util/virauth.c
@@ -107,7 +107,6 @@ virAuthGetCredential(const char *servicename,
char **value)
{
g_autoptr(virAuthConfig) config = NULL;
-const char *tmp;
*value = NULL;
@@ -121,11 +120,9 @@ virAuthGetCredential(const char *servicename,
servicename,
hostname,
credname,
-) < 0)
+value) < 0)
return -1;
-*value = g_strdup(tmp);
-
return 0;
}
diff --git a/src/util/virauthconfig.c b/src/util/virauthconfig.c
index 1c007757c7..8289b28d34 100644
--- a/src/util/virauthconfig.c
+++ b/src/util/virauthconfig.c
@@ -99,7 +99,7 @@ int virAuthConfigLookup(virAuthConfigPtr auth,
const char *service,
const char *hostname,
const char *credname,
-const char **value)
+char **value)
{
g_autofree char *authgroup = NULL;
g_autofree char *credgroup = NULL;
diff --git a/src/util/virauthconfig.h b/src/util/virauthconfig.h
index de28b1ff28..b6f5b5c110 100644
--- a/src/util/virauthconfig.h
+++ b/src/util/virauthconfig.h
@@ -37,6 +37,6 @@ int virAuthConfigLookup(virAuthConfigPtr auth,
const char *service,
const char *hostname,
const char *credname,
-const char **value);
+char **value);
G_DEFINE_AUTOPTR_CLEANUP_FUNC(virAuthConfig, virAuthConfigFree);
diff --git a/tests/virauthconfigtest.c b/tests/virauthconfigtest.c
index 20855f004e..a88b453543 100644
--- a/tests/virauthconfigtest.c
+++ b/tests/virauthconfigtest.c
@@ -42,7 +42,7 @@ struct ConfigLookupData {
static int testAuthLookup(const void *args)
{
const struct ConfigLookupData *data = args;
-const char *actual = NULL;
+g_autofree char *actual = NULL;
int rv;
rv = virAuthConfigLookup(data->config,
--
2.25.4
Re: [libvirt PATCH 1/2] lxc: replace VIR_FREE with g_autofree / g_free
First time in a while - Coverity complained this morning [...] > diff --git a/src/lxc/lxc_fuse.c b/src/lxc/lxc_fuse.c > index e73b4d0690..c4223f4e06 100644 > --- a/src/lxc/lxc_fuse.c > +++ b/src/lxc/lxc_fuse.c > @@ -326,10 +326,10 @@ int lxcSetupFuse(virLXCFusePtr *f, virDomainDefPtr def) > *f = fuse; ^^ Event use_after_free: Using freed pointer "fuse". Also see events:[alias][freed_arg] > return ret; > cleanup1: > -VIR_FREE(fuse->mountpoint); > +g_free(fuse->mountpoint); > virMutexDestroy(>lock); > cleanup2: > -VIR_FREE(fuse); > +g_free(fuse); ^^ Event freed_arg:"g_free" frees "fuse". A fuse = NULL; here will make coverity happy, but not sure if that's standard any more... The VIR_FREE would have done thta for us IIRC. John [...]
Re: [PATCH v3 1/2] conf: Add option for settings
On 4/22/20 4:05 PM, Julio Faracco wrote:
> If an user is trying to configure a dhcp neetwork settings, it is not
> possible to change the leasetime of a range or a host entry. This is
> available using dnsmasq extra options, but they are associated with
> dhcp-range or dhcp-hosts fields. This patch implements a leasetime for
> range and hosts tags. They can be defined under that settings:
>
>
>
>
>
>
>
>
>
>
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=913446
>
> Signed-off-by: Julio Faracco
> ---
[...]
> static int
> -virSocketAddrRangeParseXML(const char *networkName,
> - virNetworkIPDefPtr ipdef,
> - xmlNodePtr node,
> - virSocketAddrRangePtr range)
> +virNetworkDHCPLeaseTimeDefParseXML(virNetworkDHCPLeaseTimeDefPtr *lease,
> + xmlNodePtr node)
> +{
> +virNetworkDHCPLeaseTimeDefPtr new_lease = *lease;
> +g_autofree char *expiry = NULL, *unit = NULL;
> +
> +if (!(expiry = virXMLPropString(node, "expiry")))
> +return 0;
> +
> +if (VIR_ALLOC(new_lease) < 0)
> +return -1;
> +
> +if (virStrToLong_ul(expiry, NULL, 10, _lease->expiry) < 0)
> +return -1;
> +
> +if (!(unit = virXMLPropString(node, "unit")))
> +new_lease->unit = VIR_NETWORK_DHCP_LEASETIME_UNIT_MINUTES;
> +else
> +new_lease->unit = virNetworkDHCPLeaseTimeUnitTypeFromString(unit);
> +
> +/* infinite */
> +if (new_lease->expiry > 0) {
> +/* This boundary check is related to dnsmasq man page settings:
> + * "The minimum lease time is two minutes." */
> +if ((new_lease->unit == VIR_NETWORK_DHCP_LEASETIME_UNIT_SECONDS &&
> + new_lease->expiry < 120) ||
> +(new_lease->unit == VIR_NETWORK_DHCP_LEASETIME_UNIT_MINUTES &&
> + new_lease->expiry < 2)) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("The minimum lease time should be greater "
> + "than 2 minutes"));
> +return -1;
Coverity pointed out there's a @new_lease leak here. In just a quick
scan of the code it's not the only place and I wonder about the
initialization of new_lease = *lease and then just VIR_ALLOC right over
that... Probably should be initialized to NULL.
Anyway - perhaps consider changing @expiry to @expirystr and @unit to
@unitstr, then filling/using @expiry & @unit (instead of unitInt) for
comparisons before the VIR_ALLOC(new_lease) and filling in the two
fields once all the checks are done - probably allows those boundary
checks to not span 4 lines... JMO...
John
> +}
> +}
> +
> +*lease = new_lease;
> +
> +return 0;
> +}
> +
> +
[...]
Re: [libvirt PATCH 0/3] fix ATTRIBUTE_NONNULL issues
On 3/11/20 8:34 AM, Pavel Hrdina wrote: > On Wed, Mar 11, 2020 at 01:25:50PM +0100, Ján Tomko wrote: >> Missing blurb in the cover letter. >> >> On a Wednesday in 2020, Pavel Hrdina wrote: >>> Pavel Hrdina (3): >>> domain_conf: fix ATTRIBUTE_NONNULL for virDomainDefFormat >>> libvirt_internal: fix ATTRIBUTE_NONNULL for virStateInitialize >>> vircommand: fix ATTRIBUTE_NONNULL for virCommandAddArg* >>> >> >> Do these actually help Coverity do its job or all they're good for >> is finding out where we failed to update the attributes? > > Now that I'm trying to setup coverity scans using scan.coverity.com they > actually help coverity do its job and I managed to run into these issues > while building libvirt using coverity. > > Pavel > Good luck with Coverity issues - if you'd like my list of work-around hacks I don't mind sending you the list of 55 or so local patches in my build environment. There's even more with more recent coverity releases, but I haven't taken the time to work through them. These types of build issues can also be seen by enabling STATIC_ANALYSIS for a build - IOW: outside of coverity... FWIW: I've tried this for vircommand.h a while ago, see: https://www.redhat.com/archives/libvir-list/2018-December/msg00548.html The only other one I had in my backlog of hacks around things that in general the community hasn't accepted historically is for virnetdevtap.h and the virNetDevTapReattachBridge prototype for argument 2 because in the source code the @brname is used in a STREQ_NULLABLE thus it also gets flagged. John
[libvirt] [PATCH 3/3] virsh: Adjust logic checks in virshUpdateDiskXML
Make it clearer that what we're trying to do is find @source and @target_node so that the unattentive or code analysis utility doesn't believe 'source' and 'target' could be found in the same node element. Signed-off-by: John Ferlan --- tools/virsh-domain.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 56137bdd74..9d4cdd26dd 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -12476,10 +12476,9 @@ virshUpdateDiskXML(xmlNodePtr disk_node, if (tmp->type != XML_ELEMENT_NODE) continue; -if (virXMLNodeNameEqual(tmp, "source")) +if (!source && virXMLNodeNameEqual(tmp, "source")) source = tmp; - -if (virXMLNodeNameEqual(tmp, "target")) +else if (!target_node && virXMLNodeNameEqual(tmp, "target")) target_node = tmp; /* -- 2.23.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/3] vbox: Reset @ret after xmlFreeNode
In the error path, if we xmlFreeNode @ret, then the return ret;
a few lines later returns something that's already been free'd
and could be reused, so let's reinit it.
Found by Coverity
Signed-off-by: John Ferlan
---
src/vbox/vbox_snapshot_conf.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/vbox/vbox_snapshot_conf.c b/src/vbox/vbox_snapshot_conf.c
index 5a0abd6d0e..db6c389a64 100644
--- a/src/vbox/vbox_snapshot_conf.c
+++ b/src/vbox/vbox_snapshot_conf.c
@@ -352,6 +352,7 @@
virVBoxSnapshotConfCreateHardDiskNode(virVBoxSnapshotConfHardDiskPtr hardDisk)
if (result < 0) {
xmlUnlinkNode(ret);
xmlFreeNode(ret);
+ret = NULL;
}
VIR_FREE(uuid);
return ret;
--
2.23.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/3] conf: Fix ATTRIBUTE_NONNULL usages
Recent changes removed the virCapsPtr, but didn't adjust/remove the corresponding ATTRIBUTE_NONNULL resulting in a build failure to build in my Coverity environment. Signed-off-by: John Ferlan --- src/conf/domain_conf.h | 15 ++- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 11fafe46b3..e012975fca 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3089,27 +3089,24 @@ unsigned int virDomainDefFormatConvertXMLFlags(unsigned int flags); char *virDomainDefFormat(virDomainDefPtr def, virDomainXMLOptionPtr xmlopt, unsigned int flags) -ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) -ATTRIBUTE_NONNULL(3); +ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); char *virDomainObjFormat(virDomainObjPtr obj, virDomainXMLOptionPtr xmlopt, unsigned int flags) -ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) -ATTRIBUTE_NONNULL(3); +ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); int virDomainDefFormatInternal(virDomainDefPtr def, virDomainXMLOptionPtr xmlopt, virBufferPtr buf, unsigned int flags) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) -ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4); +ATTRIBUTE_NONNULL(3); int virDomainDefFormatInternalSetRootName(virDomainDefPtr def, virDomainXMLOptionPtr xmlopt, virBufferPtr buf, const char *rootname, unsigned int flags) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) -ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) -ATTRIBUTE_NONNULL(5); +ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4); int virDomainDiskSourceFormat(virBufferPtr buf, virStorageSourcePtr src, @@ -3286,14 +3283,14 @@ int virDomainDefSave(virDomainDefPtr def, const char *configDir) G_GNUC_WARN_UNUSED_RESULT ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) -ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4); +ATTRIBUTE_NONNULL(3); int virDomainObjSave(virDomainObjPtr obj, virDomainXMLOptionPtr xmlopt, const char *statusDir) G_GNUC_WARN_UNUSED_RESULT ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) -ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4); +ATTRIBUTE_NONNULL(3); typedef void (*virDomainLoadConfigNotify)(virDomainObjPtr dom, int newDomain, -- 2.23.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/3] Some coverity adjustments
I upgraded to f31 and it resulted in an essentially hosed Coverity build/analysis environment with the following message during cov-emit processing (a preprocessing of sorts): "/usr/include/glib-2.0/glib/gspawn.h", line 76: error #67: expected a "}" G_SPAWN_ERROR_2BIG GLIB_DEPRECATED_ENUMERATOR_IN_2_32_FOR(G_SPAWN_ERROR_TOO_BIG) = G_SPAWN_ERROR_TOO_BIG, ^ So instead, I'm using a guest to run Coverity "when I remember/can". I also found that my f31 environment doesn't like building w/ docs as I get the following messages while running the convert command: ... usr/bin/mv: cannot stat '/tmp/magick-1191987h12h27ex0lZD.svg': No such file or directory GEN kbase.html.tmp convert: delegate failed `'uniconvertor' '%i' '%o.svg'; /usr/bin/mv '%o.svg' '%o'' @ error/delegate.c/InvokeDelegate/1958. convert: unable to open file `/tmp/magick-1191987OqYJwrq8isaG': No such file or directory @ error/constitute.c/ReadImage/605. convert: no images defined `migration-managed-p2p.png' @ error/convert.c/ConvertImageCommand/3235. I haven't followed along as closely as I used to, but my vpath env uses obj as a subdirectory of my main git tree/target. Whether the new build env has anything to do with it or it's just f31, I haven't been able to determine. Beyond these 3 patches here - there is one other adjustment that is necessary to build libvirt under Coverity and that's removing the ATTRIBUTE_NONNULL(2) from the virDomainDefFormat definition in src/conf/domain_conf.h. This was added in commit 92d412149 which also included two calls to virDomainDefFormat with NULL as the 2nd argument (hyperv_driver.c and security_apparmor.c); however, the commit message notes preparation for future work, so I'll keep a hack for that local for now at least. The virsh change below is innocuous yes, but it showed up in a coverity analysis because it wasn't sure if the resulting variables could point to the same address and if they did, then there was a possible use after free because the @source is free'd even though the @target_node is later referenced. The patch here avoids that and provides a slight adjustment to not search for either node by name if it was already found. Whether there's a weird latent issue because can be repeated while cannot be is something I suppose a reviewer can warn me about ;-). John Ferlan (3): conf: Fix ATTRIBUTE_NONNULL usages vbox: Reset @ret after xmlFreeNode virsh: Adjust logic checks in virshUpdateDiskXML src/conf/domain_conf.h| 15 ++- src/vbox/vbox_snapshot_conf.c | 1 + tools/virsh-domain.c | 5 ++--- 3 files changed, 9 insertions(+), 12 deletions(-) -- 2.23.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] util: virNetDevBandwidthPlug: Drop ATTRIBUTE_UNUSED(4)
On 11/25/19 3:40 AM, Erik Skultety wrote: > On Mon, Nov 25, 2019 at 09:30:26AM +0100, Jiri Denemark wrote: >> On Fri, Nov 22, 2019 at 17:09:00 +0100, Erik Skultety wrote: >>> Since we know for sure that the @bandwidth parameter is properly handled >>> in networkCheckBandwidth (@ifaceBand), it cannot ever be NULL, but >>> coverity doesn't see this fact. In order to prevent coverity from >>> reporting a false positive here, drop ATTRIBUTE_UNUSED for this specific >> >> s/ATTRIBUTE_UNUSED/ATTRIBUTE_NONNULL/ both in the subject and the commit >> message, however... > > Oops, don't know how that happened... :) > >> >>> argument - virNetDevBandwidthPlug is also called from a single place only >>> >>> Signed-off-by: Erik Skultety >>> --- >>> src/util/virnetdevbandwidth.h | 3 +-- >>> 1 file changed, 1 insertion(+), 2 deletions(-) >>> >>> diff --git a/src/util/virnetdevbandwidth.h b/src/util/virnetdevbandwidth.h >>> index 19323c5ed2..59d7513286 100644 >>> --- a/src/util/virnetdevbandwidth.h >>> +++ b/src/util/virnetdevbandwidth.h >>> @@ -55,8 +55,7 @@ int virNetDevBandwidthPlug(const char *brname, >>> const virMacAddr *ifmac_ptr, >>> virNetDevBandwidthPtr bandwidth, >>> unsigned int id) >>> -ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) >>> -G_GNUC_WARN_UNUSED_RESULT; >>> +ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3) G_GNUC_WARN_UNUSED_RESULT; >>> >>> int virNetDevBandwidthUnplug(const char *brname, >>> unsigned int id) >> >> I don't think this is a good solution. From the point of view of this >> function ATTRIBUTE_NONNULL is correct for all three parameters and > > Well, first of all ATTRIBUTE_NONNULL has a very limited use case and most of > the time only coverity complains about something which relates to that > attribute. My reasoning was that since in this case we can guarantee that the > argument will never be NULL, we can drop it. However... > Enable static analysis in your build environment, e.g.: ./autogen.sh --system lv_cv_static_analysis=yes then try a build... The *_NONNULL has been debated before - at least w/r/t passing a NULL vs. a value being NULL. In this case I could have tried removing the _NONNNUL, but chose not to mainly because that hasn't been accepted in the past and the check seemed right (and of course got rid of the Coverity error). >> removing it for one of them does not make sense. >> >> Since we just want to silence coverity's false positive, we could use >> sa_assert in the right place to teach coverity virNetDevBandwidthPlug is >> not ever called with bandwidth == NULL: > > ...I actually like ^your suggestion here and if it turns out it works, I'll > happily go with that one instead. Putting John on CC. I'll push 1/2 in the > meantime. > > John, could you please try the patch below and see whether coverity stops > complaining about the issue that your original patch tried to address? > Anyway, apologies for the issue and excess noise (or as a pun bandwidth) Suffice to say the solution for Coverity is "complicated". It turns out to be not at as easy as an sa_assert because of the _NONNULL(4) as other checks fail. I'll just keep it local and out of the mainline libvirt. John > Erik > >> >> diff --git i/src/network/bridge_driver.c w/src/network/bridge_driver.c >> index 9b12b98d88..52c8ba4c73 100644 >> --- i/src/network/bridge_driver.c >> +++ w/src/network/bridge_driver.c >> @@ -5263,6 +5263,8 @@ networkPlugBandwidth(virNetworkObjPtr obj, >> /* no QoS needs to be set; claim success */ >> return 0; >> >> +sa_assert(ifaceBand); >> + >> virMacAddrFormat(mac, ifmac); >> >> if (networkPlugBandwidthImpl(obj, mac, ifaceBand, class_id, >> new_rate) < 0) >> >> Jirka -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/3] util: Remove unnecessary check in virFileRewrite
Since g_strdup_printf will abort, we know @newfile won't be NULL. Found by Coverity Signed-off-by: John Ferlan --- src/util/virfile.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/util/virfile.c b/src/util/virfile.c index cb6cfc0fe7..fca7ff9d35 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -543,8 +543,7 @@ virFileRewrite(const char *path, cleanup: VIR_FORCE_CLOSE(fd); -if (newfile) -unlink(newfile); +unlink(newfile); return ret; } -- 2.20.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 3/3] network: Check for QOS before blindly using it
If networkAllocatePort calls networkPlugBandwidth eventually the
port->bandwidth would be passed to virNetDevBandwidthPlug which
requires that the parameter is non-NULL. Coverity additionally
notes that since (!port->bandwidth) is checked earlier in the
networkAllocatePort method that the subsequent call to blindly
use if for a function that requires it needs to check.
Signed-off-by: John Ferlan
---
src/network/bridge_driver.c | 14 ++
1 file changed, 14 insertions(+)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 68bb916501..9c49c70564 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -4567,6 +4567,13 @@ networkAllocatePort(virNetworkObjPtr obj,
return -1;
}
+if (!port->bandwidth) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("QOS must be defined for network '%s'"),
+ netdef->name);
+return -1;
+}
+
if (networkPlugBandwidth(obj, >mac, port->bandwidth,
>class_id) < 0)
return -1;
break;
@@ -4633,6 +4640,13 @@ networkAllocatePort(virNetworkObjPtr obj,
}
}
+if (!port->bandwidth) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("QOS must be defined for network '%s'"),
+ netdef->name);
+return -1;
+}
+
if (networkPlugBandwidth(obj, >mac, port->bandwidth,
>class_id) < 0)
return -1;
break;
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/3] A few more Coverity patches
Finally got Coverity to build with the recent build env changes. With doing that Coverity re-examines everything and bridge_driver generated a couple of longer term things. The virFileRewrite is from more recent changes. John Ferlan (3): util: Remove unnecessary check in virFileRewrite network: Use local variables in networkUpdatePortBandwidth network: Check for QOS before blindly using it src/network/bridge_driver.c | 31 ++- src/util/virfile.c | 3 +-- 2 files changed, 23 insertions(+), 11 deletions(-) -- 2.20.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/3] network: Use local variables in networkUpdatePortBandwidth
We go through the trouble of checking {old|new}Bandwidth[->in] and
storing the result in local @old_floor and @new_floor, but then
we don't use them. Instead we make derefs to the longer name. This
caused Coverity to note dereferencing newBandwidth->in without first
checking @newBandwidth like was done for new_floor could cause a
NULL dereference.
Signed-off-by: John Ferlan
---
src/network/bridge_driver.c | 17 -
1 file changed, 8 insertions(+), 9 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 7ee5d7ee53..68bb916501 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -5380,19 +5380,18 @@ networkUpdatePortBandwidth(virNetworkObjPtr obj,
/* Okay, there are three possible scenarios: */
-if (oldBandwidth && oldBandwidth->in && oldBandwidth->in->floor &&
-newBandwidth->in && newBandwidth->in->floor) {
+if (old_floor > 0 && new_floor > 0) {
/* Either we just need to update @floor .. */
if (virNetDevBandwidthUpdateRate(def->bridge,
*class_id,
def->bandwidth,
- newBandwidth->in->floor) < 0)
+ new_floor) < 0)
return -1;
tmp_floor_sum = virNetworkObjGetFloorSum(obj);
-tmp_floor_sum -= oldBandwidth->in->floor;
-tmp_floor_sum += newBandwidth->in->floor;
+tmp_floor_sum -= old_floor;
+tmp_floor_sum += new_floor;
virNetworkObjSetFloorSum(obj, tmp_floor_sum);
new_rate -= tmp_floor_sum;
@@ -5401,17 +5400,17 @@ networkUpdatePortBandwidth(virNetworkObjPtr obj,
virNetworkObjSaveStatus(driver->stateDir,
obj, network_driver->xmlopt) < 0) {
/* Ouch, rollback */
-tmp_floor_sum -= newBandwidth->in->floor;
-tmp_floor_sum += oldBandwidth->in->floor;
+tmp_floor_sum -= new_floor;
+tmp_floor_sum += old_floor;
virNetworkObjSetFloorSum(obj, tmp_floor_sum);
ignore_value(virNetDevBandwidthUpdateRate(def->bridge,
*class_id,
def->bandwidth,
-
oldBandwidth->in->floor));
+ old_floor));
return -1;
}
-} else if (newBandwidth->in && newBandwidth->in->floor) {
+} else if (new_floor > 0) {
/* .. or we need to plug in new .. */
if (networkPlugBandwidthImpl(obj, mac, newBandwidth,
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 8/8] tests: Fix logic to not have possible NULL deref
It's possible that virBitmapNewString returns NULL with an error
string (and not an allocation failure that would abort); however, if
virBitmapToString is called with a NULL @bitmap, then it will fail
in an ugly manner. So rather than have if (!map && !str) logic, split
the checks for each variable.
Found by Coverity
Signed-off-by: John Ferlan
---
tests/virbitmaptest.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/tests/virbitmaptest.c b/tests/virbitmaptest.c
index 545e9272df..2808d9c880 100644
--- a/tests/virbitmaptest.c
+++ b/tests/virbitmaptest.c
@@ -682,9 +682,11 @@ test13(const void *opaque G_GNUC_UNUSED)
for (i = 0; i < G_N_ELEMENTS(strings); i++) {
map = virBitmapNewString(strings[i]);
-str = virBitmapToString(map, false, true);
+if (!map)
+goto cleanup;
-if (!map || !str)
+str = virBitmapToString(map, false, true);
+if (!str)
goto cleanup;
if (STRNEQ(strings[i], str)) {
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 7/8] tests: Add return value check in checkUserInfo
Commit 1c8113f9c added the call to virTypedParamsGetString without
a return value check which caused Coverity to complain especially
since other checks for the same function are made.
Found by Coverity
Signed-off-by: John Ferlan
---
tests/qemuagenttest.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/tests/qemuagenttest.c b/tests/qemuagenttest.c
index cef9ae5fee..ae55086d17 100644
--- a/tests/qemuagenttest.c
+++ b/tests/qemuagenttest.c
@@ -1127,7 +1127,9 @@ checkUserInfo(virTypedParameterPtr params,
snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
"user.%zu.domain", nth);
-virTypedParamsGetString(params, nparams, param_name, );
+if (virTypedParamsGetString(params, nparams, param_name, ) < 0)
+return -1;
+
if (STRNEQ_NULLABLE(expDomain, domain)) {
virReportError(VIR_ERR_INTERNAL_ERROR,
"Expected domain '%s', got '%s'",
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/8] Coverity related patches
Some related to "new-ish" changes that have caused Coverity to discover new issues and some from older changes from a pile of 50 or so that are not essentially "false positives". John Ferlan (8): vbox: Fix possible NULL deref conf: Remove ATTRIBUTE_NONNULL for virDomainQemuMonitorEventNew tests: Fix memory leak in mymain lxc: Remove unnecessary comment tests: Remove _NULLABLE in virNetDevExists mock qemu: Fix possible NULL deref in qemuDomainSaveImageStartVM tests: Add return value check in checkUserInfo tests: Fix logic to not have possible NULL deref src/conf/domain_event.c | 6 +- src/conf/domain_event.h | 2 +- src/lxc/lxc_container.c | 5 - src/qemu/qemu_driver.c | 2 +- src/vbox/vbox_common.c | 4 ++-- tests/qemuagenttest.c| 4 +++- tests/qemuhotplugtest.c | 3 +-- tests/qemuxml2argvmock.c | 2 +- tests/virbitmaptest.c| 6 -- 9 files changed, 18 insertions(+), 16 deletions(-) -- 2.20.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 4/8] lxc: Remove unnecessary comment
Commit 66e2adb2ba moved the code and the coverity comment which now was useless since the context was in lxcContainerSetupPivotRoot. Signed-off-by: John Ferlan --- src/lxc/lxc_container.c | 5 - 1 file changed, 5 deletions(-) diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index a18b214a25..1fb9049c96 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -1658,11 +1658,6 @@ static int lxcContainerUnmountForSharedRoot(const char *stateDir, /* Some versions of Linux kernel don't let you overmount * the selinux filesystem, so make sure we kill it first */ -/* Filed coverity bug for false positive 'USE_AFTER_FREE' due to swap - * of root->src with root->dst and the VIR_FREE(root->src) prior to the - * reset of root->src in lxcContainerPrepareRoot() - */ -/* coverity[deref_arg] */ if (lxcContainerUnmountSubtree(SELINUX_MOUNT, false) < 0) goto cleanup; #endif -- 2.20.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/8] conf: Remove ATTRIBUTE_NONNULL for virDomainQemuMonitorEventNew
Commit 17561eb36 modified the logic to check "if (!event)" for an
attribute that was not supposed to be passed as NULL. This causes
the static checker/Coverity build to fail. Since the check is made,
alter the header.
Also add an error message since returning -1 without some sort of
error message as previously would have happened with the failed
VIR_STRDUP so that the eventual error doesn't get the default
for some reason message.
Signed-off-by: John Ferlan
---
src/conf/domain_event.c | 6 +-
src/conf/domain_event.h | 2 +-
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/src/conf/domain_event.c b/src/conf/domain_event.c
index 644f6eb595..900d8f745e 100644
--- a/src/conf/domain_event.c
+++ b/src/conf/domain_event.c
@@ -1935,8 +1935,12 @@ virDomainQemuMonitorEventNew(int id,
return NULL;
/* event is mandatory, details are optional */
-if (!event)
+if (!event) {
+virReportError(VIR_ERR_INVALID_ARG,
+ _("unexpected event=NULL name=%s uuid=%s details=%s"),
+ name, uuidstr, NULLSTR(details));
goto error;
+}
ev->event = g_strdup(event);
ev->seconds = seconds;
diff --git a/src/conf/domain_event.h b/src/conf/domain_event.h
index d1cfb81d62..0a4bce3d04 100644
--- a/src/conf/domain_event.h
+++ b/src/conf/domain_event.h
@@ -321,4 +321,4 @@ virDomainQemuMonitorEventNew(int id,
long long seconds,
unsigned int micros,
const char *details)
-ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4);
+ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/8] vbox: Fix possible NULL deref
The @valueTypeUtf8 references need to use the STREQ_NULLABLE since
they're variantly filled in by @valueTypeUtf16.
Found by Coverity.
Signed-off-by: John Ferlan
---
src/vbox/vbox_common.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
index 5486e5ff3b..cdbec15dae 100644
--- a/src/vbox/vbox_common.c
+++ b/src/vbox/vbox_common.c
@@ -3509,13 +3509,13 @@ vboxDumpDisplay(virDomainDefPtr def, vboxDriverPtr
data, IMachine *machine)
VBOX_UTF8_FREE(valueDisplayUtf8);
}
-if (STREQ(valueTypeUtf8, "sdl")) {
+if (STREQ_NULLABLE(valueTypeUtf8, "sdl")) {
graphics->type = VIR_DOMAIN_GRAPHICS_TYPE_SDL;
graphics->data.sdl.display = valueDisplayUtf8;
valueDisplayUtf8 = NULL;
}
-if (STREQ(valueTypeUtf8, "gui")) {
+if (STREQ_NULLABLE(valueTypeUtf8, "gui")) {
graphics->type = VIR_DOMAIN_GRAPHICS_TYPE_DESKTOP;
graphics->data.desktop.display = valueDisplayUtf8;
valueDisplayUtf8 = NULL;
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 6/8] qemu: Fix possible NULL deref in qemuDomainSaveImageStartVM
Commit 075523438 added a direct reference to @cookie even though it may be NULL as shown by a comment a few lines previous - so add the check here as well. Found by Coverity Signed-off-by: John Ferlan --- src/qemu/qemu_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d17c18705b..56fcba8b2c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6831,7 +6831,7 @@ qemuDomainSaveImageStartVM(virConnectPtr conn, qemuDomainFixupCPUs(vm, >cpu) < 0) goto cleanup; -if (!cookie->slirpHelper) +if (cookie && !cookie->slirpHelper) priv->disableSlirp = true; if (qemuProcessStart(conn, driver, vm, cookie ? cookie->cpu : NULL, -- 2.20.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 5/8] tests: Remove _NULLABLE in virNetDevExists mock
The @ifname is listed as an ATTRIBUTE_NONNULL(1) parameter, so
checking for _NULLABLE causes a coverity build failure - remove
that and if it's NULL for the test let's fail miserably.
Signed-off-by: John Ferlan
---
tests/qemuxml2argvmock.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tests/qemuxml2argvmock.c b/tests/qemuxml2argvmock.c
index 914d2bcf9f..8143de1618 100644
--- a/tests/qemuxml2argvmock.c
+++ b/tests/qemuxml2argvmock.c
@@ -169,7 +169,7 @@ virNetDevSetMAC(const char *ifname G_GNUC_UNUSED,
int
virNetDevExists(const char *ifname)
{
-return STREQ_NULLABLE(ifname, "mytap0");
+return STREQ(ifname, "mytap0");
}
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 3/8] tests: Fix memory leak in mymain
Commit 944a35d7f0 added @fakerootdir; however, there are multiple
paths out of mymain that didn't free the memory - so just use the
g_autofree to resolve the potential leak.
Found by Coverity
Signed-off-by: John Ferlan
---
tests/qemuhotplugtest.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c
index 4ff2b38c83..ebf4582ac1 100644
--- a/tests/qemuhotplugtest.c
+++ b/tests/qemuhotplugtest.c
@@ -595,7 +595,7 @@ mymain(void)
int ret = 0;
struct qemuHotplugTestData data = {0};
struct testQemuHotplugCpuParams cpudata;
-char *fakerootdir;
+g_autofree char *fakerootdir = NULL;
fakerootdir = g_strdup(FAKEROOTDIRTEMPLATE);
@@ -875,7 +875,6 @@ mymain(void)
if (getenv("LIBVIRT_SKIP_CLEANUP") == NULL)
virFileDeleteTree(fakerootdir);
-VIR_FREE(fakerootdir);
qemuTestDriverFree();
virObjectUnref(data.vm);
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 10/34] conf: use g_strdup instead of VIR_STRDUP
[...]
> virObjectEventPtr
> @@ -1635,11 +1587,8 @@ virDomainEventBlockThresholdNew(int id,
> id, name, uuid)))
> return NULL;
>
> -if (VIR_STRDUP(ev->dev, dev) < 0 ||
> -VIR_STRDUP(ev->path, path) < 0) {
> -virObjectUnref(ev);
> -return NULL;
> -}
> +ev->dev = g_strdup(dev);
> +ev->path = g_strdup(path);
> ev->threshold = threshold;
> ev->excess = excess;
>
> @@ -1986,12 +1935,13 @@ virDomainQemuMonitorEventNew(int id,
> return NULL;
>
> /* event is mandatory, details are optional */
> -if (VIR_STRDUP(ev->event, event) <= 0)
> +if (!event)
@event is defined w/ ATTRIBUTE_NONNULL in src/conf/domain_event.h and
thus causes a coverity build error.
Perhaps the ATTRIBUTE_NONNULL can be removed.. at the very least should
an error be generated if it was removed? In order to avoid the failed
for some reason due to a NULL return. I didn't dig that deep - your call
on how to handle.
John
[...]
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] build: Update .gitignore for build-aux files
On 10/16/19 10:52 AM, Ján Tomko wrote: > On Wed, Oct 16, 2019 at 03:23:41PM +0100, Daniel P. Berrangé wrote: >> On Wed, Oct 16, 2019 at 10:17:16AM -0400, John Ferlan wrote: >>> >>> >>> On 10/16/19 10:08 AM, Daniel P. Berrangé wrote: >>> > On Wed, Oct 16, 2019 at 08:54:26AM -0400, John Ferlan wrote: >>> >> Recent changes moved some files to build-aux and created at least >>> >> one more. >>> > >>> > Really ? I wasn't aware that we'd moved the files listed in >>> > this change. >>> > >>> >>> Well that's where they now show up... I dunno. I had to clean my >>> environment more than once due to recent changes and these files now >>> show up in the noted/changed locations. I also use a clean environment >>> for my coverity builds every day and this is where they show up. >> >> Oh in fact I see them in the same place build-aux, but git is not >> reporting them despite not being listed in gitignore. >> > > Note that there is another .gitignore in build-aux that lists the > filenames mentioned in the patch > > Jano > $ cat build-aux/.gitignore /config.rpath /useless-if-before-free /vc-list-files $ hmm... commit 144c06d4ee4cdf9c2035b9912844ab42bcd4dd9a Author: Eric Blake Date: Tue Nov 16 12:29:09 2010 -0700 maint: update to latest gnulib ... There's an 'interesting' change to build-aux/.gitignore John >> I wonder if it has some default ignore list its using ? >> >> >> Regards, >> Daniel >> -- >> |: https://berrange.com -o- >> https://www.flickr.com/photos/dberrange :| >> |: https://libvirt.org -o- >> https://fstop138.berrange.com :| >> |: https://entangle-photo.org -o- >> https://www.instagram.com/dberrange :| >> >> -- >> libvir-list mailing list >> libvir-list@redhat.com >> https://www.redhat.com/mailman/listinfo/libvir-list -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] build: Update .gitignore for build-aux files
On 10/16/19 10:08 AM, Daniel P. Berrangé wrote: > On Wed, Oct 16, 2019 at 08:54:26AM -0400, John Ferlan wrote: >> Recent changes moved some files to build-aux and created at least >> one more. > > Really ? I wasn't aware that we'd moved the files listed in > this change. > Well that's where they now show up... I dunno. I had to clean my environment more than once due to recent changes and these files now show up in the noted/changed locations. I also use a clean environment for my coverity builds every day and this is where they show up. If that's not right, then maybe someone else can figure out why - I was just going for the "clean" look of/for git status. If I need to change the commit message that works too. I didn't think too long about it. John >> >> Signed-off-by: John Ferlan >> --- >> .gitignore | 7 --- >> 1 file changed, 4 insertions(+), 3 deletions(-) >> >> diff --git a/.gitignore b/.gitignore >> index 85ead5c907..30610a37c9 100644 >> --- a/.gitignore >> +++ b/.gitignore >> @@ -43,20 +43,22 @@ >> /autom4te.cache >> /build-aux/.gitignore >> /build-aux/compile >> +/build-aux/config.guess >> +/build-aux/config.sub >> /build-aux/depcomp >> +/build-aux/install-sh >> +/build-aux/ltmain.sh >> /build-aux/missing >> /build-aux/test-driver >> /build/ >> /ci/scratch/ >> /confdefs.h >> /config.cache >> -/config.guess >> /config.h >> /config.h.in >> /config.log >> /config.rpath >> /config.status >> -/config.sub >> /configure >> /configure.lineno >> /conftest.* >> @@ -96,7 +98,6 @@ >> /libvirt*.pc >> /libvirt.spec >> /ltconfig >> -/ltmain.sh >> /m4/* >> /mingw-libvirt.spec >> /mkinstalldirs >> -- >> 2.20.1 >> >> -- >> libvir-list mailing list >> libvir-list@redhat.com >> https://www.redhat.com/mailman/listinfo/libvir-list > > Regards, > Daniel > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] build: Update .gitignore for build-aux files
Recent changes moved some files to build-aux and created at least one more. Signed-off-by: John Ferlan --- .gitignore | 7 --- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/.gitignore b/.gitignore index 85ead5c907..30610a37c9 100644 --- a/.gitignore +++ b/.gitignore @@ -43,20 +43,22 @@ /autom4te.cache /build-aux/.gitignore /build-aux/compile +/build-aux/config.guess +/build-aux/config.sub /build-aux/depcomp +/build-aux/install-sh +/build-aux/ltmain.sh /build-aux/missing /build-aux/test-driver /build/ /ci/scratch/ /confdefs.h /config.cache -/config.guess /config.h /config.h.in /config.log /config.rpath /config.status -/config.sub /configure /configure.lineno /conftest.* @@ -96,7 +98,6 @@ /libvirt*.pc /libvirt.spec /ltconfig -/ltmain.sh /m4/* /mingw-libvirt.spec /mkinstalldirs -- 2.20.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 10/10] tools: Use consistent error preservation and restoration calls
Provide some consistency over error message variable name and usage
when saving error messages across possible other errors or possibility
of resetting of the last error.
Instead of virSaveLastError paired up with virSetError and virFreeError,
we should use the newer virErrorPreserveLast and virRestoreError.
Signed-off-by: John Ferlan
---
tools/virsh.c | 7 ++-
tools/virt-admin.c | 7 ++-
2 files changed, 4 insertions(+), 10 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c
index 4aae877160..a3553ddd36 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -93,7 +93,7 @@ virshCatchDisconnect(virConnectPtr conn,
virErrorPtr error;
char *uri;
-error = virSaveLastError();
+virErrorPreserveLast();
uri = virConnectGetURI(conn);
switch ((virConnectCloseReason) reason) {
@@ -114,10 +114,7 @@ virshCatchDisconnect(virConnectPtr conn,
vshError(ctl, _(str), NULLSTR(uri));
VIR_FREE(uri);
-if (error) {
-virSetError(error);
-virFreeError(error);
-}
+virErrorRestore();
disconnected++;
vshEventDone(ctl);
}
diff --git a/tools/virt-admin.c b/tools/virt-admin.c
index c0cc0999cb..a820e7241d 100644
--- a/tools/virt-admin.c
+++ b/tools/virt-admin.c
@@ -124,7 +124,7 @@ vshAdmCatchDisconnect(virAdmConnectPtr conn G_GNUC_UNUSED,
if (reason == VIR_CONNECT_CLOSE_REASON_CLIENT)
return;
-error = virSaveLastError();
+virErrorPreserveLast();
uri = virAdmConnectGetURI(conn);
switch ((virConnectCloseReason) reason) {
@@ -146,10 +146,7 @@ vshAdmCatchDisconnect(virAdmConnectPtr conn G_GNUC_UNUSED,
vshError(ctl, _(str), NULLSTR(uri));
VIR_FREE(uri);
-if (error) {
-virSetError(error);
-virFreeError(error);
-}
+virErrorRestore();
}
static int
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 05/10] qemu: Use consistent error preservation and restoration calls
Provide some consistency over error message variable name and usage
when saving error messages across possible other errors or possibility
of resetting of the last error.
Instead of virSaveLastError paired up with virSetError and virFreeError,
we should use the newer virErrorPreserveLast and virRestoreError.
Signed-off-by: John Ferlan
---
src/qemu/qemu_cgroup.c | 14 +++---
src/qemu/qemu_command.c | 12 ++---
src/qemu/qemu_domain.c | 7 +--
src/qemu/qemu_driver.c | 36 ++
src/qemu/qemu_migration.c| 85 +++-
src/qemu/qemu_migration_params.c | 9 ++--
src/qemu/qemu_monitor.c | 19 +++
src/qemu/qemu_process.c | 7 +--
8 files changed, 72 insertions(+), 117 deletions(-)
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index f1776a7c0b..8b915d124c 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -1149,12 +1149,11 @@ qemuSetupCgroupVcpuBW(virCgroupPtr cgroup,
error:
if (period) {
-virErrorPtr saved = virSaveLastError();
+virErrorPtr saved;
+
+virErrorPreserveLast();
ignore_value(virCgroupSetCpuCfsPeriod(cgroup, old_period));
-if (saved) {
-virSetError(saved);
-virFreeError(saved);
-}
+virErrorRestore();
}
return -1;
@@ -1362,10 +1361,9 @@
qemuCgroupEmulatorAllNodesRestore(qemuCgroupEmulatorAllNodesDataPtr data)
if (!data)
return;
-err = virSaveLastError();
+virErrorPreserveLast();
virCgroupSetCpusetMems(data->emulatorCgroup, data->emulatorMemMask);
-virSetError(err);
-virFreeError(err);
+virErrorRestore();
qemuCgroupEmulatorAllNodesDataFree(data);
}
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index c2a19689c5..2e5e2a75bf 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -8644,10 +8644,11 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver,
ret = 0;
cleanup:
if (ret < 0) {
-virErrorPtr saved_err = virSaveLastError();
+virErrorPtr saved_err;
+
+virErrorPreserveLast(_err);
virDomainConfNWFilterTeardown(net);
-virSetError(saved_err);
-virFreeError(saved_err);
+virErrorRestore(_err);
}
for (i = 0; vhostfd && i < vhostfdSize; i++) {
if (ret < 0)
@@ -8730,11 +8731,10 @@ qemuBuildNetCommandLine(virQEMUDriverPtr driver,
error:
/* free up any resources in the network driver
* but don't overwrite the original error */
-originalError = virSaveLastError();
+virErrorPreserveLast();
for (i = 0; last_good_net != -1 && i <= last_good_net; i++)
virDomainConfNWFilterTeardown(def->nets[i]);
-virSetError(originalError);
-virFreeError(originalError);
+virErrorRestore();
return -1;
}
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index c638077aa8..43aec2e3d7 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -9174,7 +9174,7 @@ void qemuDomainObjTaint(virQEMUDriverPtr driver,
/* We don't care about errors logging taint info, so
* preserve original error, and clear any error that
* is raised */
-orig_err = virSaveLastError();
+virErrorPreserveLast(_err);
if (!(timestamp = virTimeStringNow()))
goto cleanup;
@@ -9198,10 +9198,7 @@ void qemuDomainObjTaint(virQEMUDriverPtr driver,
cleanup:
VIR_FREE(timestamp);
-if (orig_err) {
-virSetError(orig_err);
-virFreeError(orig_err);
-}
+virErrorRestore(_err);
}
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 6ce6348593..c662676be3 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -3436,7 +3436,8 @@ qemuDomainSaveInternal(virQEMUDriverPtr driver,
endjob:
if (ret < 0) {
if (was_running && virDomainObjIsActive(vm)) {
-virErrorPtr save_err = virSaveLastError();
+virErrorPtr save_err;
+virErrorPreserveLast(_err);
if (qemuProcessStartCPUs(driver, vm,
VIR_DOMAIN_RUNNING_SAVE_CANCELED,
QEMU_ASYNC_JOB_SAVE) < 0) {
@@ -3446,8 +3447,7 @@ qemuDomainSaveInternal(virQEMUDriverPtr driver,
VIR_DOMAIN_EVENT_SUSPENDED,
VIR_DOMAIN_EVENT_SUSPENDED_API_ERROR));
}
-virSetError(save_err);
-virFreeError(save_err);
+virErrorRestore(_err);
}
}
qemuDomainObjEndAsyncJob(driver, vm);
@@ -6729,7 +6729,9 @@ qemuDomainSaveImageUpdateDef(virQEMUDriverPtr driver,
goto cleanup;
if (!virDomainDefCheckABIStability(def, newdef_migr, driver->xmlopt)) {
-virErrorPtr err = virSaveLastError();
+virErrorPtr save_err;
+
+virErrorP
[libvirt] [PATCH 00/10] Clean up error preservation/restoration
I was going through some old git trees and found this one started before I 'altered' my Red Hat career path. Seeing as no one has picked it up from https://wiki.libvirt.org/page/BiteSizedTasks - figured I may as well post it rather than throw it away ;-) The change essentially alters virSaveLastError ... virSetError and virFreeError paths to use virErrorPreserveLast & virErrorRestore. John Ferlan (10): conf: Use consistent error preservation and restoration calls src: Use consistent error preservation and restoration calls libxl: Use consistent error preservation and restoration calls lxc: Use consistent error preservation and restoration calls qemu: Use consistent error preservation and restoration calls remote: Use consistent error preservation and restoration calls storage: Use consistent error preservation and restoration calls util: Use consistent error preservation and restoration calls vz: Use consistent error preservation and restoration calls tools: Use consistent error preservation and restoration calls src/conf/domain_conf.c| 7 +-- src/libvirt-stream.c | 36 +--- src/libxl/libxl_migration.c | 18 ++ src/lxc/lxc_driver.c | 9 ++- src/lxc/lxc_process.c | 7 +-- src/qemu/qemu_cgroup.c| 14 ++--- src/qemu/qemu_command.c | 12 ++-- src/qemu/qemu_domain.c| 7 +-- src/qemu/qemu_driver.c| 36 +--- src/qemu/qemu_migration.c | 85 +-- src/qemu/qemu_migration_params.c | 9 ++- src/qemu/qemu_monitor.c | 19 +++--- src/qemu/qemu_process.c | 7 +-- src/remote/remote_daemon_stream.c | 17 +++--- src/storage/storage_backend_disk.c| 5 +- src/storage/storage_backend_logical.c | 5 +- src/storage/storage_driver.c | 8 +-- src/util/vircgroup.c | 18 +++--- src/util/virfirewall.c| 6 +- src/vz/vz_driver.c| 9 +-- tools/virsh.c | 7 +-- tools/virt-admin.c| 7 +-- 22 files changed, 137 insertions(+), 211 deletions(-) -- 2.20.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 09/10] vz: Use consistent error preservation and restoration calls
Provide some consistency over error message variable name and usage
when saving error messages across possible other errors or possibility
of resetting of the last error.
Instead of virSaveLastError paired up with virSetError and virFreeError,
we should use the newer virErrorPreserveLast and virRestoreError.
Signed-off-by: John Ferlan
---
src/vz/vz_driver.c | 9 +++--
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c
index 917acdae57..b4049261ae 100644
--- a/src/vz/vz_driver.c
+++ b/src/vz/vz_driver.c
@@ -3206,7 +3206,7 @@ vzDomainMigratePerformP2P(virDomainObjPtr dom,
*/
if (uri && virTypedParamsReplaceString(, ,
VIR_MIGRATE_PARAM_URI, uri) < 0) {
-orig_err = virSaveLastError();
+virErrorPreserveLast(_err);
goto finish;
}
@@ -3216,7 +3216,7 @@ vzDomainMigratePerformP2P(virDomainObjPtr dom,
cookieoutlen = 0;
if (vzDomainMigratePerformStep(dom, driver, params, nparams, cookiein,
cookieinlen, flags) < 0) {
-orig_err = virSaveLastError();
+virErrorPreserveLast(_err);
goto finish;
}
@@ -3242,10 +3242,7 @@ vzDomainMigratePerformP2P(virDomainObjPtr dom,
/* confirm step is NOOP thus no need to call it */
done:
-if (orig_err) {
-virSetError(orig_err);
-virFreeError(orig_err);
-}
+virErrorRestore(_err);
VIR_FREE(dom_xml);
VIR_FREE(uri);
VIR_FREE(cookiein);
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 08/10] util: Use consistent error preservation and restoration calls
Provide some consistency over error message variable name and usage
when saving error messages across possible other errors or possibility
of resetting of the last error.
Instead of virSaveLastError paired up with virSetError and virFreeError,
we should use the newer virErrorPreserveLast and virRestoreError.
Signed-off-by: John Ferlan
---
src/util/vircgroup.c | 18 --
src/util/virfirewall.c | 6 --
2 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
index c6e4cf2dde..4c83e37ec4 100644
--- a/src/util/vircgroup.c
+++ b/src/util/vircgroup.c
@@ -1168,13 +1168,12 @@ virCgroupNewMachineSystemd(const char *name,
}
if (virCgroupAddProcess(*group, pidleader) < 0) {
-virErrorPtr saved = virSaveLastError();
+virErrorPtr saved;
+
+virErrorPreserveLast();
virCgroupRemove(*group);
virCgroupFree(group);
-if (saved) {
-virSetError(saved);
-virFreeError(saved);
-}
+virErrorRestore();
}
return 0;
@@ -1220,13 +1219,12 @@ virCgroupNewMachineManual(const char *name,
goto cleanup;
if (virCgroupAddProcess(*group, pidleader) < 0) {
-virErrorPtr saved = virSaveLastError();
+virErrorPtr saved;
+
+virErrorPreserveLast();
virCgroupRemove(*group);
virCgroupFree(group);
-if (saved) {
-virSetError(saved);
-virFreeError(saved);
-}
+virErrorRestore();
}
done:
diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c
index 0d4bfae8f8..62f404afd6 100644
--- a/src/util/virfirewall.c
+++ b/src/util/virfirewall.c
@@ -838,7 +838,9 @@ virFirewallApply(virFirewallPtr firewall)
if (virFirewallApplyGroup(firewall, i) < 0) {
VIR_DEBUG("Rolling back groups up to %zu for %p", i, firewall);
size_t first = i;
-VIR_AUTOPTR(virError) saved_error = virSaveLastError();
+virErrorPtr saved_error;
+
+virErrorPreserveLast(_error);
/*
* Look at any inheritance markers to figure out
@@ -858,7 +860,7 @@ virFirewallApply(virFirewallPtr firewall)
virFirewallRollbackGroup(firewall, j);
}
-virSetError(saved_error);
+virErrorRestore(_error);
VIR_DEBUG("Done rolling back groups for %p", firewall);
goto cleanup;
}
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 07/10] storage: Use consistent error preservation and restoration calls
Provide some consistency over error message variable name and usage
when saving error messages across possible other errors or possibility
of resetting of the last error.
Instead of virSaveLastError paired up with virSetError and virFreeError,
we should use the newer virErrorPreserveLast and virRestoreError.
Signed-off-by: John Ferlan
---
src/storage/storage_backend_disk.c| 5 ++---
src/storage/storage_backend_logical.c | 5 ++---
src/storage/storage_driver.c | 8 +++-
3 files changed, 7 insertions(+), 11 deletions(-)
diff --git a/src/storage/storage_backend_disk.c
b/src/storage/storage_backend_disk.c
index 5bf704bcc8..d2f7b94224 100644
--- a/src/storage/storage_backend_disk.c
+++ b/src/storage/storage_backend_disk.c
@@ -918,10 +918,9 @@ virStorageBackendDiskCreateVol(virStoragePoolObjPtr pool,
/* Best effort to remove the partition. Ignore any errors
* since we could be calling this with vol->target.path == NULL
*/
-save_err = virSaveLastError();
+virErrorPreserveLast(_err);
ignore_value(virStorageBackendDiskDeleteVol(pool, vol, 0));
-virSetError(save_err);
-virFreeError(save_err);
+virErrorRestore(_err);
return -1;
}
diff --git a/src/storage/storage_backend_logical.c
b/src/storage/storage_backend_logical.c
index 6e468b3579..bfedb06a91 100644
--- a/src/storage/storage_backend_logical.c
+++ b/src/storage/storage_backend_logical.c
@@ -969,10 +969,9 @@ virStorageBackendLogicalCreateVol(virStoragePoolObjPtr
pool,
return 0;
error:
-err = virSaveLastError();
+virErrorPreserveLast();
virStorageBackendLogicalDeleteVol(pool, vol, 0);
-virSetError(err);
-virFreeError(err);
+virErrorRestore();
return -1;
}
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index 6516b0943d..e8551ba57e 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -81,18 +81,16 @@ storagePoolRefreshFailCleanup(virStorageBackendPtr backend,
virStoragePoolObjPtr obj,
const char *stateFile)
{
-virErrorPtr orig_err = virSaveLastError();
+virErrorPtr orig_err;
+virErrorPreserveLast(_err);
virStoragePoolObjClearVols(obj);
if (stateFile)
unlink(stateFile);
if (backend->stopPool)
backend->stopPool(obj);
-if (orig_err) {
-virSetError(orig_err);
-virFreeError(orig_err);
-}
+virErrorRestore(_err);
}
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 04/10] lxc: Use consistent error preservation and restoration calls
Provide some consistency over error message variable name and usage
when saving error messages across possible other errors or possibility
of resetting of the last error.
Instead of virSaveLastError paired up with virSetError and virFreeError,
we should use the newer virErrorPreserveLast and virRestoreError.
Signed-off-by: John Ferlan
---
src/lxc/lxc_driver.c | 9 -
src/lxc/lxc_process.c | 7 ++-
2 files changed, 6 insertions(+), 10 deletions(-)
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index 5c7a9140b2..204a3ed522 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -1857,12 +1857,11 @@ static int lxcSetVcpuBWLive(virCgroupPtr cgroup,
unsigned long long period,
error:
if (period) {
-virErrorPtr saved = virSaveLastError();
+virErrorPtr saved;
+
+virErrorPreserveLast();
virCgroupSetCpuCfsPeriod(cgroup, old_period);
-if (saved) {
-virSetError(saved);
-virFreeError(saved);
-}
+virErrorRestore();
}
return -1;
diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
index da0b055b85..d9939f102d 100644
--- a/src/lxc/lxc_process.c
+++ b/src/lxc/lxc_process.c
@@ -1568,7 +1568,7 @@ int virLXCProcessStart(virConnectPtr conn,
rc = -1;
}
if (rc != 0) {
-err = virSaveLastError();
+virErrorPreserveLast();
virLXCProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED);
}
virCommandFree(cmd);
@@ -1582,10 +1582,7 @@ int virLXCProcessStart(virConnectPtr conn,
virObjectUnref(cfg);
virObjectUnref(caps);
-if (err) {
-virSetError(err);
-virFreeError(err);
-}
+virErrorRestore();
return rc;
}
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 03/10] libxl: Use consistent error preservation and restoration calls
Provide some consistency over error message variable name and usage
when saving error messages across possible other errors or possibility
of resetting of the last error.
Instead of virSaveLastError paired up with virSetError and virFreeError,
we should use the newer virErrorPreserveLast and virRestoreError.
Signed-off-by: John Ferlan
---
src/libxl/libxl_migration.c | 18 ++
1 file changed, 6 insertions(+), 12 deletions(-)
diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
index 8a41e9374d..770a300316 100644
--- a/src/libxl/libxl_migration.c
+++ b/src/libxl/libxl_migration.c
@@ -1049,7 +1049,7 @@ libxlDoMigrateSrcP2P(libxlDriverPrivatePtr driver,
if (uri_out) {
if (virTypedParamsReplaceString(, ,
VIR_MIGRATE_PARAM_URI, uri_out) <
0) {
-orig_err = virSaveLastError();
+virErrorPreserveLast(_err);
goto finish;
}
} else {
@@ -1067,7 +1067,7 @@ libxlDoMigrateSrcP2P(libxlDriverPrivatePtr driver,
uri_out, NULL, flags);
if (ret < 0) {
notify_source = false;
-orig_err = virSaveLastError();
+virErrorPreserveLast(_err);
}
cancelled = (ret < 0);
@@ -1094,7 +1094,7 @@ libxlDoMigrateSrcP2P(libxlDriverPrivatePtr driver,
* one we need to preserve it in case confirm3 overwrites
*/
if (!orig_err)
-orig_err = virSaveLastError();
+virErrorPreserveLast(_err);
confirm:
if (notify_source) {
@@ -1119,10 +1119,7 @@ libxlDoMigrateSrcP2P(libxlDriverPrivatePtr driver,
ret = -1;
}
-if (orig_err) {
-virSetError(orig_err);
-virFreeError(orig_err);
-}
+virErrorRestore(_err);
VIR_FREE(cookieout);
VIR_FREE(dom_xml);
@@ -1200,15 +1197,12 @@ libxlDomainMigrationSrcPerformP2P(libxlDriverPrivatePtr
driver,
}
cleanup:
-orig_err = virSaveLastError();
+virErrorPreserveLast(_err);
virObjectUnlock(vm);
virObjectUnref(dconn);
virObjectUnref(cfg);
virObjectLock(vm);
-if (orig_err) {
-virSetError(orig_err);
-virFreeError(orig_err);
-}
+virErrorRestore(_err);
return ret;
}
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 06/10] remote: Use consistent error preservation and restoration calls
Provide some consistency over error message variable name and usage
when saving error messages across possible other errors or possibility
of resetting of the last error.
Instead of virSaveLastError paired up with virSetError and virFreeError,
we should use the newer virErrorPreserveLast and virRestoreError.
Signed-off-by: John Ferlan
---
src/remote/remote_daemon_stream.c | 17 +
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/src/remote/remote_daemon_stream.c
b/src/remote/remote_daemon_stream.c
index d7fcb1bf42..c4f14a27ef 100644
--- a/src/remote/remote_daemon_stream.c
+++ b/src/remote/remote_daemon_stream.c
@@ -229,15 +229,18 @@ daemonStreamEvent(virStreamPtr st, int events, void
*opaque)
int ret;
virNetMessagePtr msg;
virNetMessageError rerr;
-virErrorPtr origErr = virSaveLastError();
+virErrorPtr origErr;
+
+virErrorPreserveLast();
memset(, 0, sizeof(rerr));
stream->closed = true;
virStreamEventRemoveCallback(stream->st);
virStreamAbort(stream->st);
if (origErr && origErr->code != VIR_ERR_OK) {
-virSetError(origErr);
+virErrorRestore();
} else {
+virFreeError(origErr);
if (events & VIR_STREAM_EVENT_HANGUP)
virReportError(VIR_ERR_RPC,
"%s", _("stream had unexpected termination"));
@@ -245,7 +248,6 @@ daemonStreamEvent(virStreamPtr st, int events, void *opaque)
virReportError(VIR_ERR_RPC,
"%s", _("stream had I/O failure"));
}
-virFreeError(origErr);
msg = virNetMessageNew(false);
if (!msg) {
@@ -549,7 +551,9 @@ daemonStreamHandleWriteData(virNetServerClientPtr client,
return 1;
} else if (ret < 0) {
virNetMessageError rerr;
-virErrorPtr err = virSaveLastError();
+virErrorPtr err;
+
+virErrorPreserveLast();
memset(, 0, sizeof(rerr));
@@ -558,10 +562,7 @@ daemonStreamHandleWriteData(virNetServerClientPtr client,
virStreamEventRemoveCallback(stream->st);
virStreamAbort(stream->st);
-if (err) {
-virSetError(err);
-virFreeError(err);
-}
+virErrorRestore();
return virNetServerProgramSendReplyError(stream->prog,
client,
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 01/10] conf: Use consistent error preservation and restoration calls
Provide some consistency over error message variable name and usage
when saving error messages across possible other errors or possibility
of resetting of the last error.
Instead of virSaveLastError paired up with virSetError and virFreeError,
we should use the newer virErrorPreserveLast and virRestoreError.
Signed-off-by: John Ferlan
---
src/conf/domain_conf.c | 7 ++-
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 10d6bf0eea..188ea80c3d 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -23624,17 +23624,14 @@ virDomainDefCheckABIStabilityFlags(virDomainDefPtr
src,
return true;
error:
-err = virSaveLastError();
+virErrorPreserveLast();
strSrc = virDomainDefFormat(src, NULL, 0);
strDst = virDomainDefFormat(dst, NULL, 0);
VIR_DEBUG("XMLs that failed stability check were: src=\"%s\", dst=\"%s\"",
NULLSTR(strSrc), NULLSTR(strDst));
-if (err) {
-virSetError(err);
-virFreeError(err);
-}
+virErrorRestore();
return false;
}
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 02/10] src: Use consistent error preservation and restoration calls
Provide some consistency over error message variable name and usage
when saving error messages across possible other errors or possibility
of resetting of the last error.
Instead of virSaveLastError paired up with virSetError and virFreeError,
we should use the newer virErrorPreserveLast and virRestoreError.
Signed-off-by: John Ferlan
---
src/libvirt-stream.c | 36
1 file changed, 16 insertions(+), 20 deletions(-)
diff --git a/src/libvirt-stream.c b/src/libvirt-stream.c
index ef83696bcd..fb646d0aef 100644
--- a/src/libvirt-stream.c
+++ b/src/libvirt-stream.c
@@ -622,12 +622,11 @@ virStreamSendAll(virStreamPtr stream,
VIR_FREE(bytes);
if (ret != 0) {
-virErrorPtr orig_err = virSaveLastError();
+virErrorPtr orig_err;
+
+virErrorPreserveLast(_err);
virStreamAbort(stream);
-if (orig_err) {
-virSetError(orig_err);
-virFreeError(orig_err);
-}
+virErrorRestore(_err);
virDispatchError(stream->conn);
}
@@ -794,12 +793,11 @@ int virStreamSparseSendAll(virStreamPtr stream,
VIR_FREE(bytes);
if (ret != 0) {
-virErrorPtr orig_err = virSaveLastError();
+virErrorPtr orig_err;
+
+virErrorPreserveLast(_err);
virStreamAbort(stream);
-if (orig_err) {
-virSetError(orig_err);
-virFreeError(orig_err);
-}
+virErrorRestore(_err);
virDispatchError(stream->conn);
}
@@ -900,12 +898,11 @@ virStreamRecvAll(virStreamPtr stream,
VIR_FREE(bytes);
if (ret != 0) {
-virErrorPtr orig_err = virSaveLastError();
+virErrorPtr orig_err;
+
+virErrorPreserveLast(_err);
virStreamAbort(stream);
-if (orig_err) {
-virSetError(orig_err);
-virFreeError(orig_err);
-}
+virErrorRestore(_err);
virDispatchError(stream->conn);
}
@@ -1034,12 +1031,11 @@ virStreamSparseRecvAll(virStreamPtr stream,
VIR_FREE(bytes);
if (ret != 0) {
-virErrorPtr orig_err = virSaveLastError();
+virErrorPtr orig_err;
+
+virErrorPreserveLast(_err);
virStreamAbort(stream);
-if (orig_err) {
-virSetError(orig_err);
-virFreeError(orig_err);
-}
+virErrorRestore(_err);
virDispatchError(stream->conn);
}
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 16/23] qemu: add a flag to the cookie to prevent slirp-helper setup
On 8/8/19 10:55 AM, marcandre.lur...@redhat.com wrote:
> From: Marc-André Lureau
>
> For VM started and migrated/saved without slirp-helpers, let's prevent
> the automatic setup (as it would fail to migrate otherwise).
>
> Signed-off-by: Marc-André Lureau
> ---
> src/qemu/qemu_domain.c | 30 --
> src/qemu/qemu_domain.h | 2 ++
> src/qemu/qemu_driver.c | 8
> 3 files changed, 38 insertions(+), 2 deletions(-)
>
[...]
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index ff83d1c024..4b49203738 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -6971,6 +6971,7 @@ qemuDomainSaveImageStartVM(virConnectPtr conn,
> bool start_paused,
> qemuDomainAsyncJob asyncJob)
> {
> +qemuDomainObjPrivatePtr priv = vm->privateData;
> int ret = -1;
> bool restored = false;
> virObjectEventPtr event;
> @@ -7011,6 +7012,9 @@ qemuDomainSaveImageStartVM(virConnectPtr conn,
> qemuDomainFixupCPUs(vm, >cpu) < 0)
> goto cleanup;
>
> +if (!cookie->slirpHelper)
> +priv->disableSlirp = true;
> +
Coverity lets me know that the above will need to have a "cookie &&" in
the if statement (similar to the lines just above it and of course the
check just below for @cookie being NULL...
John
> if (qemuProcessStart(conn, driver, vm, cookie ? cookie->cpu : NULL,
> asyncJob, "stdio", *fd, path, NULL,
> VIR_NETDEV_VPORT_PROFILE_OP_RESTORE,
> @@ -16654,6 +16658,10 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr
> snapshot,
> virCPUDefFree(priv->origCPU);
> VIR_STEAL_PTR(priv->origCPU, origCPU);
> }
> +
> +if (cookie && !cookie->slirpHelper)
> +priv->disableSlirp = true;
hmm... yeah, just like this ;-)
> +
> } else {
> /* Transitions 2, 3 */
> load:
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/7] util: make allocation functions abort on OOM
On 8/29/19 2:02 PM, Daniel P. Berrangé wrote:
> The functions are left returning an "int" to avoid an immediate
> big-bang cleanup. They'll simply never return anything other
> than 0, except for virInsertN which can still return an error
> if the requested insertion index is out of range. Interestingly
> in that case, the _QUIET function would none the less report
> an error.
>
> Signed-off-by: Daniel P. Berrangé
> ---
> src/util/viralloc.c | 201 +++-
> src/util/viralloc.h | 145 +++-
> 2 files changed, 97 insertions(+), 249 deletions(-)
>
FWIW: I applied the series to my Coverity branch to see what (if
anything) gets shaken there. Good news is - not much. There were a
couple of things I already reported on that were fixed for 5.7.0 (commit
ed7e342b0 and be9d259eb).
There's one more false positive in qemuDomainGetNumaParameters that
probably would be fixed by AUTOFREE stuff (nodeset gets set to NULL
after virTypedParameterAssign).
Beyond that the only thing is two CHECKED_RETURN's (e.g. N callers check
the return value, but these 2 don't). I consider both false positives;
however, it could also be argued that unless any of the functions where
either 0 or abort occurs, then make them just void, but that's a sh*load
more work...
[...]
> /**
> @@ -204,50 +143,35 @@ int virExpandN(void *ptrptr,
> * @allocptr: pointer to number of elements allocated in array
> * @count: number of elements currently used in array
> * @add: minimum number of additional elements to support in array
> - * @report: whether to report OOM error, if there is one
> - * @domcode: error domain code
> - * @filename: caller's filename
> - * @funcname: caller's funcname
> - * @linenr: caller's line number
> *
> * If 'count' + 'add' is larger than '*allocptr', then resize the
> * block of memory in 'ptrptr' to be an array of at least 'count' +
> * 'add' elements, each 'size' bytes in length. Update 'ptrptr' and
> * 'allocptr' with the details of the newly allocated memory. On
> * failure, 'ptrptr' and 'allocptr' are not changed. Any newly
> - * allocated memory in 'ptrptr' is zero-filled. If @report is true,
> - * OOM errors are reported automatically.
> - *
> + * allocated memory in 'ptrptr' is zero-filled.
> *
> - * Returns -1 on failure to allocate, zero on success
> + * Returns zero on success, aborts on OOM
> */
> int virResizeN(void *ptrptr,
> size_t size,
> size_t *allocptr,
> size_t count,
> - size_t add,
> - bool report,
> - int domcode,
> - const char *filename,
> - const char *funcname,
> - size_t linenr)
> + size_t add)
> {
> size_t delta;
>
> -if (count + add < count) {
> -if (report)
> -virReportOOMErrorFull(domcode, filename, funcname, linenr);
> -errno = ENOMEM;
> -return -1;
> -}
> +if (count + add < count)
> +abort();
> +
> if (count + add <= *allocptr)
> return 0;
>
> delta = count + add - *allocptr;
> if (delta < *allocptr / 2)
> delta = *allocptr / 2;
> -return virExpandN(ptrptr, size, allocptr, delta, report,
> - domcode, filename, funcname, linenr);
> +virExpandN(ptrptr, size, allocptr, delta);
> +return 0;
Could just return virExpandN here...
> }
[...]
> @@ -312,12 +229,7 @@ int
> virInsertElementsN(void *ptrptr, size_t size, size_t at,
> size_t *countptr,
> size_t add, void *newelems,
> - bool clearOriginal, bool inPlace,
> - bool report,
> - int domcode,
> - const char *filename,
> - const char *funcname,
> - size_t linenr)
> + bool clearOriginal, bool inPlace)
> {
> if (at == -1) {
> at = *countptr;
> @@ -330,9 +242,8 @@ virInsertElementsN(void *ptrptr, size_t size, size_t at,
>
> if (inPlace) {
> *countptr += add;
> -} else if (virExpandN(ptrptr, size, countptr, add, report,
> - domcode, filename, funcname, linenr) < 0) {
> -return -1;
> +} else {
> +virExpandN(ptrptr, size, countptr, add);
> }
>
This one's more painful, with using ignore_value() wrapper to just
pacify Coverity.
[...]
I'm not saying anything has to be done, but I figured it could be a
useful data point for you -
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: domain: Fix potential NULL deref when parsing job private data
On 9/2/19 10:13 AM, Peter Krempa wrote: > A specially crafted XML which would reference a non-existing disk but > request the mirror to be registered with the blockjob could potentially > make the parser dereference NULL. Fix it by moving the code slightly and > just treat it as a wrong job XML. Found by Coverity. > > Reported-by: John Ferlan > Signed-off-by: Peter Krempa > --- > src/qemu/qemu_domain.c | 10 +++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > Reviewed-by: John Ferlan SFF John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Validate arg in qemuAgentErrorComandUnsupported()
On 8/30/19 12:09 PM, Jonathon Jongsma wrote: > Coverity noted that 'reply' can be NULL after calling > qemuAgentCommand(). Avoid dereferencing reply in > qemuAgentErrorComandUnsupported() in that case. > > Signed-off-by: Jonathon Jongsma > --- > src/qemu/qemu_agent.c | 7 ++- > 1 file changed, 6 insertions(+), 1 deletion(-) > Reviewed-by: John Ferlan John (I've pushed as a bugfix during freeze) -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 2/4] qemu: guestinfo: handle unsupported agent commands
On 8/27/19 4:35 PM, Jonathon Jongsma wrote:
> When we're collecting guest information, older agents may not support
> all agent commands. In the case where the user requested all info
> types (i.e. types == 0), ignore unsupported command errors and gather as
> much information as possible. If the agent command failed for some other
> reason, or if the user explciitly requested a specific info type (i.e.
> types != 0), abort on the first error.
>
> Signed-off-by: Jonathon Jongsma
> ---
> src/qemu/qemu_agent.c | 70 ++
> src/qemu/qemu_driver.c | 33 ++--
> tests/qemuagenttest.c | 2 +-
> 3 files changed, 82 insertions(+), 23 deletions(-)
>
> diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
> index f2a8bb6263..c63db968c6 100644
> --- a/src/qemu/qemu_agent.c
> +++ b/src/qemu/qemu_agent.c
> @@ -995,6 +995,26 @@ qemuAgentStringifyErrorClass(const char *klass)
> return "unknown QEMU command error";
> }
>
> +/* Checks whether the agent reply msg is an error caused by an unsupported
> + * command.
> + *
> + * Returns true when reply is CommandNotFound or CommandDisabled
> + * false otherwise
> + */
> +static bool
> +qemuAgentErrorCommandUnsupported(virJSONValuePtr reply)
> +{
> +const char *klass;
> +virJSONValuePtr error = virJSONValueObjectGet(reply, "error");> +
Coverity notes it's possible to enter this function with @reply ==
NULL... Calls to qemuAgentCommand will set @*reply = NULL immediately
and only fill in @*reply when/if qemuAgentSend returns 0 *and*
@msg.rxObject != NULL.
When virJSONValueObjectGet is called it derefs it's first parameter
(@object) immediately
John
[...]
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 6/9] qemu: add support for new fields in FSInfo
[...]
>
>> int
>> qemuAgentGetFSInfo(qemuAgentPtr mon, virDomainFSInfoPtr **info,
>> virDomainDefPtr vmdef)
>> +{
>> + int ret = -1;
>> + qemuAgentFSInfoPtr *agentinfo = NULL;
>> + virDomainFSInfoPtr *info_ret = NULL;
>> + size_t i;
>> + int nfs;
>> +
>> + nfs = qemuAgentGetFSInfoInternal(mon, , vmdef);
>> + if (nfs < 0)
>> + return ret;
>> + if (VIR_ALLOC_N(info_ret, nfs) < 0)
>> + goto cleanup;
>> +
>> + for (i = 0; i < nfs; i++) {
>> + if (!(info_ret[i] = qemuAgentFSInfoToPublic(agentinfo[i])))
>> + goto cleanup;
>> + }
>> +
>> + *info = info_ret;
>> + info_ret = NULL;
>> + ret = nfs;
>> +
>> + cleanup:
>> + for (i = 0; i < nfs; i++) {
>> + qemuAgentFSInfoFree(agentinfo[i]);
>> + /* if there was an error, free any memory we've allocated for
>> the
>> + * return value */
>> + if (info_ret)
>> + virDomainFSInfoFree(info_ret[i]);
>
>
> Dont' forget to free @info_ret itself.
>
Seems this review comment was missed/forgotten as my Coverity checker
triggered yesterday (just didn't get to it until today)... Although
there was an addition of a VIR_FREE on @agentinfo.
John
>> + }
>> + return ret;
>> +}
>> +
>> +
[...]
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 3/9] qemu: add helper for getting guest users
[...]
>> +
>> +static int getUserInfo(virTypedParameterPtr params, int nparams,
>> size_t nth,
>> + const char **username, const char **domain,
>> + unsigned long long *logintime)
>> +{
>> + char param_name[VIR_TYPED_PARAM_FIELD_LENGTH];
>> +
>> + snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
>> + "user.%zu.name", nth);
>> + if (username &&
>> + virTypedParamsGetString(params, nparams, param_name,
>> username) < 0)
>> + return -1;
>> +
>> + snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
>> + "user.%zu.domain", nth);
>> + if (domain &&
>> + virTypedParamsGetString(params, nparams, param_name, domain)
>> < 0)
>> + return -1;
>> +
>> + snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
>> + "user.%zu.login-time", nth);
>> + if (logintime &&
>> + virTypedParamsGetULLong(params, nparams, param_name,
>> logintime) < 0)
>> + return -1;
>> +
>> + return 0;
>
> This function can be renamed to checkUserInfo() and it can check the
> values directly. It saves us couple of more lines.
>
Changes made to this function after review, but not posted AFAICT
neglected to check the return value of virTypedParamsGetString for
"user.%zu.domain" like the other two calls checked, so Coverity noted that.
John
>> +}
>> +
>
> Michal
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v1 10/11] storage_driver: Protect pool def during startup and build
On 5/24/19 10:35 AM, Michal Privoznik wrote:
> In near future the storage pool object lock will be released
> during startPool and buildPool callback (in some backends). But
> this means that another thread may acquire the pool object lock
> and change its definition rendering the former thread access not
> only stale definition but also access freed memory
> (virStoragePoolObjAssignDef() will free old def when setting a
> new one).
>
> One way out of this would be to have the pool appear as active
> because our code deals with obj->def and obj->newdef just fine.
> But we can't declare a pool as active if it's not started or
> still building up. Therefore, have a boolean flag that is very
> similar and forces virStoragePoolObjAssignDef() to store new
> definition in obj->newdef even for an inactive pool. In turn, we
> have to move the definition to correct place when unsetting the
> flag. But that's as easy as calling
> virStoragePoolUpdateInactive().
>
> Technically speaking, change made to
> storageDriverAutostartCallback() is not needed because until
> storage driver is initialized no storage API can run therefore
> there can't be anyone wanting to change the pool's definition.
> But I'm doing the change there for consistency anyways.
>
> Signed-off-by: Michal Privoznik
> ---
> src/conf/virstorageobj.c | 26 +-
> src/conf/virstorageobj.h | 6 ++
> src/libvirt_private.syms | 2 ++
> src/storage/storage_driver.c | 31 ++-
> 4 files changed, 63 insertions(+), 2 deletions(-)
>
[...]
> diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
> index def4123b82..60bfa48e25 100644
> --- a/src/storage/storage_driver.c
> +++ b/src/storage/storage_driver.c
> @@ -201,12 +201,14 @@ storageDriverAutostartCallback(virStoragePoolObjPtr obj,
>
> if (virStoragePoolObjIsAutostart(obj) &&
> !virStoragePoolObjIsActive(obj)) {
> +
> +virStoragePoolObjSetStarting(obj, true);
> if (backend->startPool &&
> backend->startPool(obj) < 0) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("Failed to autostart storage pool '%s': %s"),
> def->name, virGetLastErrorMessage());
> -return;
> +goto cleanup;
> }
> started = true;
> }
> @@ -225,6 +227,13 @@ storageDriverAutostartCallback(virStoragePoolObjPtr obj,
> virStoragePoolObjSetActive(obj, true);
> }
> }
> +
> + cleanup:
> +if (virStoragePoolObjIsStarting(obj)) {
> +if (!virStoragePoolObjIsActive(obj))
> +virStoragePoolUpdateInactive(obj);
> +virStoragePoolObjSetStarting(obj, false);
> +}
> }
>
>
> @@ -750,6 +759,8 @@ storagePoolCreateXML(virConnectPtr conn,
> newDef = NULL;
> def = virStoragePoolObjGetDef(obj);
>
Coverity let me know this morning that there's quite a few lines above
here which goto cleanup; however, cleanup expects @obj != NULL (or at
least virStoragePoolObjIsStarting does). Probably need similar logic
like you added in storagePool{Create|Build}.
John
> +virStoragePoolObjSetStarting(obj, true);
> +
> if (backend->buildPool) {
> if (flags & VIR_STORAGE_POOL_CREATE_WITH_BUILD_OVERWRITE)
> build_flags |= VIR_STORAGE_POOL_BUILD_OVERWRITE;
> @@ -786,6 +797,11 @@ storagePoolCreateXML(virConnectPtr conn,
> pool = virGetStoragePool(conn, def->name, def->uuid, NULL, NULL);
>
> cleanup:
> +if (virStoragePoolObjIsStarting(obj)) {
> +if (!virStoragePoolObjIsActive(obj))
> +virStoragePoolUpdateInactive(obj);
> +virStoragePoolObjSetStarting(obj, false);
> +}
> virObjectEventStateQueue(driver->storageEventState, event);
> virStoragePoolObjEndAPI();
> return pool;
[...]
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
