[libvirt] [PATCH 0/7] Some stylistic cleanups
Ján Tomko (7): maint: use parentheses after if nwfilter: remove pointless assignment openvz: pass sizeof to snprintf virsh-edit: remove unreachable break remote: remove unused label virsh: use logical or for boolean values Simplify some conditions src/conf/domain_conf.c | 2 +- src/nwfilter/nwfilter_gentech_driver.c | 1 - src/openvz/openvz_driver.c | 3 +-- src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_migration.c | 2 +- src/remote/remote_driver.c | 3 +-- tools/virsh-domain.c | 2 +- tools/virsh-edit.c | 1 - 8 files changed, 6 insertions(+), 10 deletions(-) -- 2.16.1
Some instances of ARCH_IS_PPC64 did not use them. Introduced by commits da636d8 and ef08a54 Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/conf/domain_conf.c | 2 +- src/qemu/qemu_domain.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a248d73de..115550985 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14738,7 +14738,7 @@ virDomainVideoDefaultType(const virDomainDef *def) if (def->os.type == VIR_DOMAIN_OSTYPE_XEN || def->os.type == VIR_DOMAIN_OSTYPE_LINUX) return VIR_DOMAIN_VIDEO_TYPE_XEN; - else if ARCH_IS_PPC64(def->os.arch) + else if (ARCH_IS_PPC64(def->os.arch)) return VIR_DOMAIN_VIDEO_TYPE_VGA; else return VIR_DOMAIN_VIDEO_TYPE_CIRRUS; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 8b4efc82d..687a2d6b5 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -5154,7 +5154,7 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, if (dev->type == VIR_DOMAIN_DEVICE_VIDEO) { if (dev->data.video->type == VIR_DOMAIN_VIDEO_TYPE_DEFAULT) { - if ARCH_IS_PPC64(def->os.arch) + if (ARCH_IS_PPC64(def->os.arch)) dev->data.video->type = VIR_DOMAIN_VIDEO_TYPE_VGA; else if (qemuDomainIsVirt(def)) dev->data.video->type = VIR_DOMAIN_VIDEO_TYPE_VIRTIO; -- 2.16.1
On Tue, Mar 06, 2018 at 15:01:33 +0100, Ján Tomko wrote:
Some instances of ARCH_IS_PPC64 did not use them.
Introduced by commits da636d8 and ef08a54
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/conf/domain_conf.c | 2 +- src/qemu/qemu_domain.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
ACK
Changing a parameter passed by value has no effect. Introduced by <commit 3f74b2eb>. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/nwfilter/nwfilter_gentech_driver.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c index 840d419bb..5ef26b6af 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -360,7 +360,6 @@ virNWFilterRuleDefToRuleInst(virNWFilterDefPtr def, inst->nrules, ruleinst) < 0) goto cleanup; - inst = NULL; ret = 0; cleanup: -- 2.16.1
On Tue, Mar 06, 2018 at 15:01:34 +0100, Ján Tomko wrote:
Changing a parameter passed by value has no effect.
Introduced by <commit 3f74b2eb>.
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/nwfilter/nwfilter_gentech_driver.c | 1 - 1 file changed, 1 deletion(-)
ACK
The size argument accounts for the nul-byte to terminate the string. Use sizeof and remove the pointless assignment. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/openvz/openvz_driver.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index 9bd73d85c..ebdc3890e 100644 --- a/src/openvz/openvz_driver.c +++ b/src/openvz/openvz_driver.c @@ -1355,8 +1355,7 @@ static int openvzDomainSetVcpusInternal(virDomainObjPtr vm, if (pcpus > 0 && pcpus < nvcpus) nvcpus = pcpus; - snprintf(str_vcpus, 31, "%d", nvcpus); - str_vcpus[31] = '\0'; + snprintf(str_vcpus, sizeof(str_vcpus), "%d", nvcpus); openvzSetProgramSentinal(prog, vm->def->name); if (virRun(prog, NULL) < 0) -- 2.16.1
On Tue, Mar 06, 2018 at 15:01:35 +0100, Ján Tomko wrote:
The size argument accounts for the nul-byte to terminate the string. Use sizeof and remove the pointless assignment.
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/openvz/openvz_driver.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
ACK
Introduced by <commit 1bb1de8>. Signed-off-by: Ján Tomko <jtomko@redhat.com> Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tools/virsh-edit.c | 1 - 1 file changed, 1 deletion(-) diff --git a/tools/virsh-edit.c b/tools/virsh-edit.c index 92a00b7f9..d97794fed 100644 --- a/tools/virsh-edit.c +++ b/tools/virsh-edit.c @@ -138,7 +138,6 @@ do { EDIT_RELAX; relax_avail = false; goto redefine; - break; } ATTRIBUTE_FALLTHROUGH; #endif -- 2.16.1
On Tue, Mar 06, 2018 at 15:01:36 +0100, Ján Tomko wrote:
Introduced by <commit 1bb1de8>.
Signed-off-by: Ján Tomko <jtomko@redhat.com> Signed-off-by: Ján Tomko <jtomko@redhat.com>
You don't need to overdo it. The commit hook looks for just one.
--- tools/virsh-edit.c | 1 - 1 file changed, 1 deletion(-)
ACK
On 03/06/2018 09:21 AM, Peter Krempa wrote:
On Tue, Mar 06, 2018 at 15:01:36 +0100, Ján Tomko wrote:
Introduced by <commit 1bb1de8>.
Signed-off-by: Ján Tomko <jtomko@redhat.com> Signed-off-by: Ján Tomko <jtomko@redhat.com> You don't need to overdo it. The commit hook looks for just one.
He repeatedly pushes the "Close Door" button on elevators too. At least when I'm running to catch it, anyway...
Leftover from <commit 84371303>. It seems nobody builds WITH_POLKIT0 and -Wunused-label. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/remote/remote_driver.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 9ea726dc4..2ec69166f 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -4341,7 +4341,6 @@ remoteAuthPolkit0(virConnectPtr conn, struct private_data *priv, return -1; /* virError already set by call */ } - out: VIR_DEBUG("PolicyKit-0 authentication complete"); return 0; } -- 2.16.1
On Tue, Mar 06, 2018 at 15:01:37 +0100, Ján Tomko wrote:
Leftover from <commit 84371303>. It seems nobody builds WITH_POLKIT0 and -Wunused-label.
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/remote/remote_driver.c | 1 - 1 file changed, 1 deletion(-)
You could also fixing it by removing the whole function :) ACK
Bitwise or just looks wrong here. Introduced by <commit 69e0cd3>. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tools/virsh-domain.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 29bc8e6db..c78cf7f21 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -7213,7 +7213,7 @@ cmdGuestvcpus(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptStringReq(ctl, cmd, "cpulist", &cpulist)) return false; - if (cpulist && !(enable | disable)) { + if (cpulist && !(enable || disable)) { vshError(ctl, _("One of options --enable or --disable is required by " "option --cpulist")); return false; -- 2.16.1
In qemuMigrationSrcRun, we already checked for non-NULL mig and then dereferenced it. It's only possible for mig to be NULL in the error section. In remoteConnectOpen, conn->uri cannot be NULL in the second part of the OR expression due to short-circuit evaluation. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/qemu/qemu_migration.c | 2 +- src/remote/remote_driver.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index bf89e184e..e5231555d 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -3992,7 +3992,7 @@ qemuMigrationSrcRun(virQEMUDriverPtr driver, goto error; } - if (mig && mig->nbd && + if (mig->nbd && qemuMigrationSrcCancelDriveMirror(driver, vm, true, QEMU_ASYNC_JOB_MIGRATION_OUT, dconn) < 0) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 2ec69166f..9390bead6 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -1310,7 +1310,7 @@ remoteConnectOpen(virConnectPtr conn, int ret, rflags = 0; const char *autostart = virGetEnvBlockSUID("LIBVIRT_AUTOSTART"); - if (inside_daemon && (!conn->uri || (conn->uri && !conn->uri->server))) + if (inside_daemon && (!conn->uri || !conn->uri->server)) return VIR_DRV_OPEN_DECLINED; if (!(priv = remoteAllocPrivateData())) -- 2.16.1
On Tue, Mar 06, 2018 at 15:01:39 +0100, Ján Tomko wrote:
In qemuMigrationSrcRun, we already checked for non-NULL mig and then dereferenced it. It's only possible for mig to be NULL in the error section.
In remoteConnectOpen, conn->uri cannot be NULL in the second part of the OR expression due to short-circuit evaluation.
Signed-off-by: Ján Tomko <jtomko@redhat.com> ---
I'd commit those as 2 commits. ACK if you split it.
participants (3)
-
Ján Tomko -
Laine Stump -
Peter Krempa