[libvirt] [PATCH] maint: Fix incorrect parenthesis placement causing true/false assignment
There were a few places in our code where the following pattern in 'if' condition occurred: if ((foo = bar() < 0)) do something; This patch adjusts the conditions to the expected format: if ((foo = bar()) < 0) do something; Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1488192 Signed-off-by: Erik Skultety <eskultet@redhat.com> --- This can be especially "fun" if you try to assign multiple RNG devices to a domain and then try to hot-unplug any of them but the first. Anyhow I used the regex below to find these, I haven't spent much time tuning in, so in case you can come up with a better one that yields more results like these I'm all ears :). [[:alnum:]_]+ = [[:print:]]+[^)]) < 0)) daemon/admin.c | 2 +- src/locking/lock_daemon.c | 2 +- src/locking/lock_driver_sanlock.c | 2 +- src/qemu/qemu_hotplug.c | 2 +- tests/virnettlshelpers.c | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/daemon/admin.c b/daemon/admin.c index c5678bb99..baf310c7b 100644 --- a/daemon/admin.c +++ b/daemon/admin.c @@ -446,7 +446,7 @@ adminDispatchConnectGetLoggingOutputs(virNetServerPtr server ATTRIBUTE_UNUSED, char *outputs = NULL; int noutputs = 0; - if ((noutputs = adminConnectGetLoggingOutputs(&outputs, args->flags) < 0)) { + if ((noutputs = adminConnectGetLoggingOutputs(&outputs, args->flags)) < 0) { virNetMessageSaveError(rerr); return -1; } diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c index 6fbbf4b3d..fe3eaf903 100644 --- a/src/locking/lock_daemon.c +++ b/src/locking/lock_daemon.c @@ -1310,7 +1310,7 @@ int main(int argc, char **argv) { } srv = virNetDaemonGetServer(lockDaemon->dmn, "virtlockd"); - if ((rv = virLockDaemonSetupNetworkingSystemD(srv) < 0)) { + if ((rv = virLockDaemonSetupNetworkingSystemD(srv)) < 0) { ret = VIR_LOCK_DAEMON_ERR_NETWORK; goto cleanup; } diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c index b5e69c472..7513df4d7 100644 --- a/src/locking/lock_driver_sanlock.c +++ b/src/locking/lock_driver_sanlock.c @@ -313,7 +313,7 @@ virLockManagerSanlockSetupLockspace(virLockManagerSanlockDriverPtr driver) goto error_unlink; } - if ((rv = virLockManagerSanlockInitLockspace(driver, &ls) < 0)) { + if ((rv = virLockManagerSanlockInitLockspace(driver, &ls)) < 0) { char *err = NULL; if (virLockManagerSanlockError(rv, &err)) { virReportError(VIR_ERR_INTERNAL_ERROR, diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 9611df517..b365078ec 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -5270,7 +5270,7 @@ qemuDomainDetachRNGDevice(virQEMUDriverPtr driver, int rc; int ret = -1; - if ((idx = virDomainRNGFind(vm->def, rng) < 0)) { + if ((idx = virDomainRNGFind(vm->def, rng)) < 0) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("device not present in domain configuration")); return -1; diff --git a/tests/virnettlshelpers.c b/tests/virnettlshelpers.c index b735c4e2f..f66205ef7 100644 --- a/tests/virnettlshelpers.c +++ b/tests/virnettlshelpers.c @@ -424,7 +424,7 @@ void testTLSWriteCertChain(const char *filename, for (i = 0; i < ncerts; i++) { size = sizeof(buffer); - if ((err = gnutls_x509_crt_export(certs[i], GNUTLS_X509_FMT_PEM, buffer, &size) < 0)) { + if ((err = gnutls_x509_crt_export(certs[i], GNUTLS_X509_FMT_PEM, buffer, &size)) < 0) { VIR_WARN("Failed to export certificate %s", gnutls_strerror(err)); unlink(filename); abort(); -- 2.13.3
On Tue, Sep 05, 2017 at 10:17:36AM +0200, Erik Skultety wrote:
There were a few places in our code where the following pattern in 'if' condition occurred:
if ((foo = bar() < 0)) do something;
This patch adjusts the conditions to the expected format:
if ((foo = bar()) < 0) do something;
I'd forbid this short ugly notation. Whoever is lazy to write: ... foo = bar(); if (foo < 0) ... is free to object until I send this e-mail.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1488192
Signed-off-by: Erik Skultety <eskultet@redhat.com> ---
This can be especially "fun" if you try to assign multiple RNG devices to a domain and then try to hot-unplug any of them but the first. Anyhow I used the regex below to find these, I haven't spent much time tuning in, so in case you can come up with a better one that yields more results like these I'm all ears :).
[[:alnum:]_]+ = [[:print:]]+[^)]) < 0))
I tried some other ones that would find '==' or '!=' or stuff split in multiple lines and did not find any. Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
On Tue, Sep 05, 2017 at 11:02:46AM +0200, Martin Kletzander wrote:
On Tue, Sep 05, 2017 at 10:17:36AM +0200, Erik Skultety wrote:
There were a few places in our code where the following pattern in 'if' condition occurred:
if ((foo = bar() < 0)) do something;
This patch adjusts the conditions to the expected format:
if ((foo = bar()) < 0) do something;
I'd forbid this short ugly notation. Whoever is lazy to write:
... foo = bar(); if (foo < 0) ...
is free to object until I send this e-mail.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1488192
Signed-off-by: Erik Skultety <eskultet@redhat.com> ---
This can be especially "fun" if you try to assign multiple RNG devices to a domain and then try to hot-unplug any of them but the first. Anyhow I used the regex below to find these, I haven't spent much time tuning in, so in case you can come up with a better one that yields more results like these I'm all ears :).
[[:alnum:]_]+ = [[:print:]]+[^)]) < 0))
I tried some other ones that would find '==' or '!=' or stuff split in multiple lines and did not find any.
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
Pushed, thanks. Erik
On Tue, Sep 05, 2017 at 11:02:46AM +0200, Martin Kletzander wrote:
On Tue, Sep 05, 2017 at 10:17:36AM +0200, Erik Skultety wrote:
There were a few places in our code where the following pattern in 'if' condition occurred:
if ((foo = bar() < 0)) do something;
This patch adjusts the conditions to the expected format:
if ((foo = bar()) < 0) do something;
I'd forbid this short ugly notation. Whoever is lazy to write:
... foo = bar(); if (foo < 0) ...
is free to object until I send this e-mail.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1488192
Signed-off-by: Erik Skultety <eskultet@redhat.com> ---
This can be especially "fun" if you try to assign multiple RNG devices to a domain and then try to hot-unplug any of them but the first. Anyhow I used the regex below to find these, I haven't spent much time tuning in, so in case you can come up with a better one that yields more results like these I'm all ears :).
[[:alnum:]_]+ = [[:print:]]+[^)]) < 0))
It would be maybe worth creating a syntax-check rule for it. Pavel
participants (3)
-
Erik Skultety -
Martin Kletzander -
Pavel Hrdina