[libvirt] [PATCH 0/6] A few misc fixes from LGTM static analysis
There is an online service call LGTM (Looks Good To Me) which does static analysis of open source projects and I happened to learn that they include coverage of libvirt https://lgtm.com/projects/g/libvirt/libvirt I looked at the alerts they reported. Currently no errors, 41 warnings and 90 recommendations (79 of which are FIXME comments :-). There's nothing particularly important they identify right now, but I felt like addressing a few of them anyway, hence this series. Daniel P. Berrangé (6): conf: remove pointless check on enum value remote: remove variable whose value is a constant storage: pass struct _virStorageBackendQemuImgInfo by reference qemu: pass virDomainDeviceInfo by reference hyperv: remove unused 'total' variable hyperv: use "is None" not "== None" for PEP-8 compliance src/conf/domain_conf.c | 20 ++++++++--------- src/hyperv/hyperv_wmi_generator.py | 3 +-- src/qemu/qemu_command.c | 4 ++-- src/qemu/qemu_domain.c | 10 ++++----- src/qemu/qemu_domain.h | 9 ++++---- src/qemu/qemu_domain_address.c | 2 +- src/remote/remote_daemon_dispatch.c | 8 ++----- src/storage/storage_util.c | 35 ++++++++++++++--------------- 8 files changed, 42 insertions(+), 49 deletions(-) -- 2.20.1
'val' is initialized from virDomainCapsFeatureTypeFromString and a few lines earlier there was already a check for 'val < 0'. The 'val >= 0' is thus always true. The enum conversion similarly ensures that the val will be less than VIR_DOMAIN_CAPS_FEATURE_LAST, so "val < VIR_DOMAIN_CAPS_FEATURE_LAST' is thus always true too. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/conf/domain_conf.c | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 9409d93c23..f2ef53a9a3 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -20524,18 +20524,16 @@ virDomainDefParseXML(xmlDocPtr xml, goto error; } - if (val >= 0 && val < VIR_DOMAIN_CAPS_FEATURE_LAST) { - if ((tmp = virXMLPropString(nodes[i], "state"))) { - if ((def->caps_features[val] = virTristateSwitchTypeFromString(tmp)) == -1) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown state attribute '%s' of feature capability '%s'"), - tmp, virDomainFeatureTypeToString(val)); - goto error; - } - VIR_FREE(tmp); - } else { - def->caps_features[val] = VIR_TRISTATE_SWITCH_ON; + if ((tmp = virXMLPropString(nodes[i], "state"))) { + if ((def->caps_features[val] = virTristateSwitchTypeFromString(tmp)) == -1) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown state attribute '%s' of feature capability '%s'"), + tmp, virDomainFeatureTypeToString(val)); + goto error; } + VIR_FREE(tmp); + } else { + def->caps_features[val] = VIR_TRISTATE_SWITCH_ON; } } VIR_FREE(nodes); -- 2.20.1
On 1/30/19 12:40 PM, Daniel P. Berrangé wrote:
'val' is initialized from virDomainCapsFeatureTypeFromString and a few lines earlier there was already a check for 'val < 0'.
The 'val >= 0' is thus always true. The enum conversion similarly ensures that the val will be less than VIR_DOMAIN_CAPS_FEATURE_LAST, so "val < VIR_DOMAIN_CAPS_FEATURE_LAST' is thus always true too.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/conf/domain_conf.c | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> However, do see below for a change that either could be made now or as a separate patch afterwards with an implied R-by as well. John
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 9409d93c23..f2ef53a9a3 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -20524,18 +20524,16 @@ virDomainDefParseXML(xmlDocPtr xml, goto error; }
- if (val >= 0 && val < VIR_DOMAIN_CAPS_FEATURE_LAST) { - if ((tmp = virXMLPropString(nodes[i], "state"))) { - if ((def->caps_features[val] = virTristateSwitchTypeFromString(tmp)) == -1) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown state attribute '%s' of feature capability '%s'"), - tmp, virDomainFeatureTypeToString(val)); - goto error; - } - VIR_FREE(tmp); - } else { - def->caps_features[val] = VIR_TRISTATE_SWITCH_ON; + if ((tmp = virXMLPropString(nodes[i], "state"))) { + if ((def->caps_features[val] = virTristateSwitchTypeFromString(tmp)) == -1) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown state attribute '%s' of feature capability '%s'"), + tmp, virDomainFeatureTypeToString(val)); ^^^^ This caught my eye...
Since @val was sourced from virDomainCapsFeatureTypeFromString so one would think it should use virDomainCapsFeatureTypeToString(val) at the very least. Or use 'nodes[i]->name' directly. As a test I modified tests/domainschemadata/domain-caps-features.xml to set state to "foo" and then creating the lxc guest would result in : error: unsupported configuration: unknown state attribute 'foo' of feature capability 'hpt' fixing to use DomainCapsFeature got: error: unsupported configuration: unknown state attribute 'foo' of feature capability 'mknod' I checked other virDomainFeatureTypeToString callers and they all seemed OK - it was just this one.
+ goto error; } + VIR_FREE(tmp); + } else { + def->caps_features[val] = VIR_TRISTATE_SWITCH_ON; } } VIR_FREE(nodes);
On Wed, Jan 30, 2019 at 04:07:13PM -0500, John Ferlan wrote:
On 1/30/19 12:40 PM, Daniel P. Berrangé wrote:
'val' is initialized from virDomainCapsFeatureTypeFromString and a few lines earlier there was already a check for 'val < 0'.
The 'val >= 0' is thus always true. The enum conversion similarly ensures that the val will be less than VIR_DOMAIN_CAPS_FEATURE_LAST, so "val < VIR_DOMAIN_CAPS_FEATURE_LAST' is thus always true too.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/conf/domain_conf.c | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com>
However, do see below for a change that either could be made now or as a separate patch afterwards with an implied R-by as well.
John
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 9409d93c23..f2ef53a9a3 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -20524,18 +20524,16 @@ virDomainDefParseXML(xmlDocPtr xml, goto error; }
- if (val >= 0 && val < VIR_DOMAIN_CAPS_FEATURE_LAST) { - if ((tmp = virXMLPropString(nodes[i], "state"))) { - if ((def->caps_features[val] = virTristateSwitchTypeFromString(tmp)) == -1) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown state attribute '%s' of feature capability '%s'"), - tmp, virDomainFeatureTypeToString(val)); - goto error; - } - VIR_FREE(tmp); - } else { - def->caps_features[val] = VIR_TRISTATE_SWITCH_ON; + if ((tmp = virXMLPropString(nodes[i], "state"))) { + if ((def->caps_features[val] = virTristateSwitchTypeFromString(tmp)) == -1) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown state attribute '%s' of feature capability '%s'"), + tmp, virDomainFeatureTypeToString(val)); ^^^^ This caught my eye...
Since @val was sourced from virDomainCapsFeatureTypeFromString so one would think it should use virDomainCapsFeatureTypeToString(val) at the very least. Or use 'nodes[i]->name' directly.
As a test I modified tests/domainschemadata/domain-caps-features.xml to set state to "foo" and then creating the lxc guest would result in :
error: unsupported configuration: unknown state attribute 'foo' of feature capability 'hpt'
fixing to use DomainCapsFeature got:
error: unsupported configuration: unknown state attribute 'foo' of feature capability 'mknod'
I checked other virDomainFeatureTypeToString callers and they all seemed OK - it was just this one.
Oh fun. I'll fix this in a separate patch with your R-b.
+ goto error; } + VIR_FREE(tmp); + } else { + def->caps_features[val] = VIR_TRISTATE_SWITCH_ON; } } VIR_FREE(nodes);
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 :|
The 'rv' variable is never changed after being declared, so can be removed. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/remote/remote_daemon_dispatch.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c index fcd602304f..df28259042 100644 --- a/src/remote/remote_daemon_dispatch.c +++ b/src/remote/remote_daemon_dispatch.c @@ -3659,7 +3659,6 @@ remoteDispatchAuthSaslStart(virNetServerPtr server, const char *serverout; size_t serveroutlen; int err; - int rv = -1; struct daemonClientPrivate *priv = virNetServerClientGetPrivateData(client); const char *identity; @@ -3739,8 +3738,7 @@ remoteDispatchAuthSaslStart(virNetServerPtr server, virResetLastError(); virReportError(VIR_ERR_AUTH_FAILED, "%s", _("authentication failed")); - if (rv < 0) - virNetMessageSaveError(rerr); + virNetMessageSaveError(rerr); virMutexUnlock(&priv->lock); return -1; } @@ -3757,7 +3755,6 @@ remoteDispatchAuthSaslStep(virNetServerPtr server, const char *serverout; size_t serveroutlen; int err; - int rv = -1; struct daemonClientPrivate *priv = virNetServerClientGetPrivateData(client); const char *identity; @@ -3837,8 +3834,7 @@ remoteDispatchAuthSaslStep(virNetServerPtr server, virResetLastError(); virReportError(VIR_ERR_AUTH_FAILED, "%s", _("authentication failed")); - if (rv < 0) - virNetMessageSaveError(rerr); + virNetMessageSaveError(rerr); virMutexUnlock(&priv->lock); return -1; } -- 2.20.1
On 1/30/19 12:40 PM, Daniel P. Berrangé wrote:
The 'rv' variable is never changed after being declared, so can be removed.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/remote/remote_daemon_dispatch.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
The struct _virStorageBackendQemuImgInfo is quite large so it is preferrable to pass it by reference instead of by value. This requires us to stop modifying the "compat" field. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/storage/storage_util.c | 35 +++++++++++++++++------------------ 1 file changed, 17 insertions(+), 18 deletions(-) diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index 37b3d58667..de6f8ec2bd 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -742,38 +742,40 @@ struct _virStorageBackendQemuImgInfo { static int storageBackendCreateQemuImgOpts(virStorageEncryptionInfoDefPtr encinfo, char **opts, - struct _virStorageBackendQemuImgInfo info) + struct _virStorageBackendQemuImgInfo *info) { virBuffer buf = VIR_BUFFER_INITIALIZER; - if (info.backingPath) + if (info->backingPath) virBufferAsprintf(&buf, "backing_fmt=%s,", - virStorageFileFormatTypeToString(info.backingFormat)); + virStorageFileFormatTypeToString(info->backingFormat)); if (encinfo) - virQEMUBuildQemuImgKeySecretOpts(&buf, encinfo, info.secretAlias); + virQEMUBuildQemuImgKeySecretOpts(&buf, encinfo, info->secretAlias); - if (info.preallocate) { - if (info.size_arg > info.allocation) + if (info->preallocate) { + if (info->size_arg > info->allocation) virBufferAddLit(&buf, "preallocation=metadata,"); else virBufferAddLit(&buf, "preallocation=falloc,"); } - if (info.nocow) + if (info->nocow) virBufferAddLit(&buf, "nocow=on,"); - if (info.compat) - virBufferAsprintf(&buf, "compat=%s,", info.compat); + if (info->compat) + virBufferAsprintf(&buf, "compat=%s,", info->compat); + else if (info->format == VIR_STORAGE_FILE_QCOW2) + virBufferAddLit(&buf, "compat=0.10,"); - if (info.features && info.format == VIR_STORAGE_FILE_QCOW2) { - if (virBitmapIsBitSet(info.features, + if (info->features && info->format == VIR_STORAGE_FILE_QCOW2) { + if (virBitmapIsBitSet(info->features, VIR_STORAGE_FILE_FEATURE_LAZY_REFCOUNTS)) { - if (STREQ_NULLABLE(info.compat, "0.10")) { + if (STREQ_NULLABLE(info->compat, "0.10")) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("lazy_refcounts not supported with compat" " level %s"), - info.compat); + info->compat); goto error; } virBufferAddLit(&buf, "lazy_refcounts,"); @@ -942,13 +944,10 @@ storageBackendCreateQemuImgSetBacking(virStoragePoolObjPtr pool, static int storageBackendCreateQemuImgSetOptions(virCommandPtr cmd, virStorageEncryptionInfoDefPtr encinfo, - struct _virStorageBackendQemuImgInfo info) + struct _virStorageBackendQemuImgInfo *info) { char *opts = NULL; - if (info.format == VIR_STORAGE_FILE_QCOW2 && !info.compat) - info.compat = "0.10"; - if (storageBackendCreateQemuImgOpts(encinfo, &opts, info) < 0) return -1; if (opts) @@ -1196,7 +1195,7 @@ virStorageBackendCreateQemuImgCmdFromVol(virStoragePoolObjPtr pool, } if (convertStep != VIR_STORAGE_VOL_ENCRYPT_CONVERT) { - if (storageBackendCreateQemuImgSetOptions(cmd, encinfo, info) < 0) + if (storageBackendCreateQemuImgSetOptions(cmd, encinfo, &info) < 0) goto error; if (info.inputPath) virCommandAddArg(cmd, info.inputPath); -- 2.20.1
On 1/30/19 12:40 PM, Daniel P. Berrangé wrote:
The struct _virStorageBackendQemuImgInfo is quite large so it is preferrable to pass it by reference instead of by value. This requires us to stop modifying the "compat" field.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/storage/storage_util.c | 35 +++++++++++++++++------------------ 1 file changed, 17 insertions(+), 18 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
The virDomainDeviceInfo parameter is a large struct so it is preferrable to pass it by reference instead of by value. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/qemu/qemu_command.c | 4 ++-- src/qemu/qemu_domain.c | 10 +++++----- src/qemu/qemu_domain.h | 9 +++++---- src/qemu/qemu_domain_address.c | 2 +- 4 files changed, 13 insertions(+), 12 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 3e46f3ced3..a59583fb75 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1894,7 +1894,7 @@ qemuBuildDiskDeviceStr(const virDomainDef *def, if (qemuCheckDiskConfig(disk, qemuCaps) < 0) goto error; - if (!qemuDomainCheckCCWS390AddressSupport(def, disk->info, qemuCaps, disk->dst)) + if (!qemuDomainCheckCCWS390AddressSupport(def, &disk->info, qemuCaps, disk->dst)) goto error; if (disk->iothread && !qemuCheckIOThreads(def, disk)) @@ -5961,7 +5961,7 @@ qemuBuildRNGDevStr(const virDomainDef *def, { virBuffer buf = VIR_BUFFER_INITIALIZER; - if (!qemuDomainCheckCCWS390AddressSupport(def, dev->info, qemuCaps, + if (!qemuDomainCheckCCWS390AddressSupport(def, &dev->info, qemuCaps, dev->source.file)) goto error; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 63879e3e4c..6a78c023c8 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -5807,7 +5807,7 @@ qemuDomainDeviceDefValidateController(const virDomainControllerDef *controller, { int ret = 0; - if (!qemuDomainCheckCCWS390AddressSupport(def, controller->info, qemuCaps, + if (!qemuDomainCheckCCWS390AddressSupport(def, &controller->info, qemuCaps, "controller")) return -1; @@ -5861,7 +5861,7 @@ qemuDomainDeviceDefValidateVsock(const virDomainVsockDef *vsock, return -1; } - if (!qemuDomainCheckCCWS390AddressSupport(def, vsock->info, qemuCaps, + if (!qemuDomainCheckCCWS390AddressSupport(def, &vsock->info, qemuCaps, "vsock")) return -1; @@ -13474,11 +13474,11 @@ qemuDomainGetMachineName(virDomainObjPtr vm) */ bool qemuDomainCheckCCWS390AddressSupport(const virDomainDef *def, - virDomainDeviceInfo info, + const virDomainDeviceInfo *info, virQEMUCapsPtr qemuCaps, const char *devicename) { - if (info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) { + if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) { if (!qemuDomainIsS390CCW(def)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("cannot use CCW address type for device " @@ -13491,7 +13491,7 @@ qemuDomainCheckCCWS390AddressSupport(const virDomainDef *def, "this QEMU")); return false; } - } else if (info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390) { + } else if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390) { if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_S390)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("virtio S390 address type is not supported by " diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index defbffbf94..462da8224d 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -1064,10 +1064,11 @@ int qemuDomainObjPrivateXMLParseAllowReboot(xmlXPathContextPtr ctxt, virTristateBool *allowReboot); -bool qemuDomainCheckCCWS390AddressSupport(const virDomainDef *def, - virDomainDeviceInfo info, - virQEMUCapsPtr qemuCaps, - const char *devicename); +bool +qemuDomainCheckCCWS390AddressSupport(const virDomainDef *def, + const virDomainDeviceInfo *info, + virQEMUCapsPtr qemuCaps, + const char *devicename); int qemuDomainPrepareDiskSourceData(virDomainDiskDefPtr disk, diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index d50744a952..c376f3f897 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -3226,7 +3226,7 @@ qemuDomainEnsureVirtioAddress(bool *releaseAddr, else if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VIRTIO_S390)) info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390; } else { - if (!qemuDomainCheckCCWS390AddressSupport(vm->def, *info, priv->qemuCaps, + if (!qemuDomainCheckCCWS390AddressSupport(vm->def, info, priv->qemuCaps, devicename)) return -1; } -- 2.20.1
On 1/30/19 12:40 PM, Daniel P. Berrangé wrote:
The virDomainDeviceInfo parameter is a large struct so it is preferrable to pass it by reference instead of by value.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/qemu/qemu_command.c | 4 ++-- src/qemu/qemu_domain.c | 10 +++++----- src/qemu/qemu_domain.h | 9 +++++---- src/qemu/qemu_domain_address.c | 2 +- 4 files changed, 13 insertions(+), 12 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/hyperv/hyperv_wmi_generator.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/hyperv/hyperv_wmi_generator.py b/src/hyperv/hyperv_wmi_generator.py index 518a55fd6d..fc1370955f 100755 --- a/src/hyperv/hyperv_wmi_generator.py +++ b/src/hyperv/hyperv_wmi_generator.py @@ -280,7 +280,6 @@ class WmiClass: # alter each version's property list so that common members are first # and in the same order as in the common dictionary - total = len(common) for cls in self.versions: index = 0 count = len(cls.properties) -- 2.20.1
On 1/30/19 12:40 PM, Daniel P. Berrangé wrote:
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/hyperv/hyperv_wmi_generator.py | 1 - 1 file changed, 1 deletion(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
PEP 8 says: "Comparisons to singletons like None should always be done with 'is' or 'is not', never the equality operators." There are potentially semantics differences, though in the case of this libvirt code its merely a style change: http://jaredgrubb.blogspot.com/2009/04/python-is-none-vs-none.html Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/hyperv/hyperv_wmi_generator.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/hyperv/hyperv_wmi_generator.py b/src/hyperv/hyperv_wmi_generator.py index fc1370955f..a9ece0ff00 100755 --- a/src/hyperv/hyperv_wmi_generator.py +++ b/src/hyperv/hyperv_wmi_generator.py @@ -65,7 +65,7 @@ class WmiClass: # because we'll generate "common" member and will be the "base" name if len(self.versions) > 1: first = self.versions[0] - if first.version == None: + if first.version is None: first.version = "v1" first.name = "%s_%s" % (first.name, first.version) -- 2.20.1
On 1/30/19 12:40 PM, Daniel P. Berrangé wrote:
PEP 8 says:
"Comparisons to singletons like None should always be done with 'is' or 'is not', never the equality operators."
There are potentially semantics differences, though in the case of this libvirt code its merely a style change:
http://jaredgrubb.blogspot.com/2009/04/python-is-none-vs-none.html
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/hyperv/hyperv_wmi_generator.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
participants (2)
-
Daniel P. Berrangé -
John Ferlan