[libvirt PATCH 0/4] Coverity fixes
Ján Tomko (4): tools: virt-host-validate: fix memory leak vbox: fix vboxCapsInit ch: fix logic in virCHMonitorBuildPtyJson tests: pcivpdtest: check return value of virCreateAnonymousFile src/ch/ch_monitor.c | 4 +--- src/vbox/vbox_common.c | 1 - tests/virpcivpdtest.c | 20 ++++++++++++++++++++ tools/virt-host-validate-ch.c | 2 +- 4 files changed, 22 insertions(+), 5 deletions(-) -- 2.31.1
virHostValidateGetCPUFlags returns an allocated virBitmap and it needs to be freed. Fixes: a0ec7165e3bbc416478740f6d2e8fef2ece18602 Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tools/virt-host-validate-ch.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/virt-host-validate-ch.c b/tools/virt-host-validate-ch.c index b00fdd0e90..b26f82738d 100644 --- a/tools/virt-host-validate-ch.c +++ b/tools/virt-host-validate-ch.c @@ -28,7 +28,7 @@ int virHostValidateCh(void) { int ret = 0; - virBitmap *flags; + g_autoptr(virBitmap) flags = NULL; bool hasHwVirt = false; bool hasVirtFlag = false; virArch arch = virArchFromHost(); -- 2.31.1
There is a stray mis-indented 'return NULL' left after a recent refactor. Fixes: c18d9e23fafabcfbb80481e0705931036b8e7331 Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/vbox/vbox_common.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 62c8e22cb5..72f1b9c466 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -102,7 +102,6 @@ vboxCapsInit(void) guest = virCapabilitiesAddGuest(caps, VIR_DOMAIN_OSTYPE_HVM, caps->host.arch, NULL, NULL, 0, NULL); - return NULL; virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_VBOX, NULL, NULL, 0, NULL); -- 2.31.1
There is a leftover 'ptys' variable, which we only assign to and one assignment to 'content', where we add an empty 'pty' object. Remove 'ptys'. Fixes: 93accefd9eca1bc3d7e923a979ab2d1b8a312ff7 Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/ch/ch_monitor.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c index 691d1ce64b..12c10da874 100644 --- a/src/ch/ch_monitor.c +++ b/src/ch/ch_monitor.c @@ -88,8 +88,6 @@ virCHMonitorBuildCPUJson(virJSONValue *content, virDomainDef *vmdef) static int virCHMonitorBuildPTYJson(virJSONValue *content, virDomainDef *vmdef) { - virJSONValue *ptys = virJSONValueNewObject(); - if (vmdef->nconsoles) { g_autoptr(virJSONValue) pty = virJSONValueNewObject(); if (virJSONValueObjectAppendString(pty, "mode", "Pty") < 0) @@ -100,7 +98,7 @@ virCHMonitorBuildPTYJson(virJSONValue *content, virDomainDef *vmdef) if (vmdef->nserials) { g_autoptr(virJSONValue) pty = virJSONValueNewObject(); - if (virJSONValueObjectAppendString(ptys, "mode", "Pty") < 0) + if (virJSONValueObjectAppendString(pty, "mode", "Pty") < 0) return -1; if (virJSONValueObjectAppend(content, "serial", &pty) < 0) return -1; -- 2.31.1
Fixes: 59c1bc3a0e25e6d725db41990f11e0b53137115d Fixes: 43820e4b8037680ec451761216750c6b139db67a Fixes: 600f580d623ae4077ddeb6c7cb24f8a315a7c73b Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tests/virpcivpdtest.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/tests/virpcivpdtest.c b/tests/virpcivpdtest.c index 284350fe29..62c51cdeb9 100644 --- a/tests/virpcivpdtest.c +++ b/tests/virpcivpdtest.c @@ -446,6 +446,8 @@ testVirPCIVPDReadVPDBytes(const void *opaque G_GNUC_UNUSED) buf = g_malloc0(dataLen); fd = virCreateAnonymousFile(fullVPDExample, dataLen); + if (fd < 0) + return -1; readBytes = virPCIVPDReadVPDBytes(fd, buf, dataLen, 0, &csum); @@ -482,6 +484,9 @@ testVirPCIVPDParseVPDStringResource(const void *opaque G_GNUC_UNUSED) dataLen = G_N_ELEMENTS(stringResExample); fd = virCreateAnonymousFile(stringResExample, dataLen); + if (fd < 0) + return -1; + result = virPCIVPDParseVPDLargeResourceString(fd, 0, dataLen, &csum, res); VIR_FORCE_CLOSE(fd); @@ -552,6 +557,9 @@ testVirPCIVPDParseFullVPD(const void *opaque G_GNUC_UNUSED) dataLen = G_N_ELEMENTS(fullVPDExample); fd = virCreateAnonymousFile(fullVPDExample, dataLen); + if (fd < 0) + return -1; + res = virPCIVPDParse(fd); VIR_FORCE_CLOSE(fd); @@ -620,6 +628,9 @@ testVirPCIVPDParseZeroLengthRW(const void *opaque G_GNUC_UNUSED) dataLen = G_N_ELEMENTS(fullVPDExample); fd = virCreateAnonymousFile(fullVPDExample, dataLen); + if (fd < 0) + return -1; + res = virPCIVPDParse(fd); VIR_FORCE_CLOSE(fd); @@ -670,6 +681,9 @@ testVirPCIVPDParseNoRW(const void *opaque G_GNUC_UNUSED) dataLen = G_N_ELEMENTS(fullVPDExample); fd = virCreateAnonymousFile(fullVPDExample, dataLen); + if (fd < 0) + return -1; + res = virPCIVPDParse(fd); VIR_FORCE_CLOSE(fd); @@ -723,6 +737,9 @@ testVirPCIVPDParseFullVPDSkipInvalidKeywords(const void *opaque G_GNUC_UNUSED) dataLen = G_N_ELEMENTS(fullVPDExample); fd = virCreateAnonymousFile(fullVPDExample, dataLen); + if (fd < 0) + return -1; + res = virPCIVPDParse(fd); VIR_FORCE_CLOSE(fd); @@ -776,6 +793,9 @@ testVirPCIVPDParseFullVPDSkipInvalidValues(const void *opaque G_GNUC_UNUSED) dataLen = G_N_ELEMENTS(fullVPDExample); fd = virCreateAnonymousFile(fullVPDExample, dataLen); + if (fd < 0) + return -1; + res = virPCIVPDParse(fd); VIR_FORCE_CLOSE(fd); -- 2.31.1
On Tue, Nov 23, 2021 at 3:20 PM Ján Tomko <jtomko@redhat.com> wrote:
Fixes: 59c1bc3a0e25e6d725db41990f11e0b53137115d Fixes: 43820e4b8037680ec451761216750c6b139db67a Fixes: 600f580d623ae4077ddeb6c7cb24f8a315a7c73b Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tests/virpcivpdtest.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)
diff --git a/tests/virpcivpdtest.c b/tests/virpcivpdtest.c index 284350fe29..62c51cdeb9 100644 --- a/tests/virpcivpdtest.c +++ b/tests/virpcivpdtest.c @@ -446,6 +446,8 @@ testVirPCIVPDReadVPDBytes(const void *opaque G_GNUC_UNUSED) buf = g_malloc0(dataLen);
fd = virCreateAnonymousFile(fullVPDExample, dataLen); + if (fd < 0) + return -1;
I would prefer if you rewrote this before merging as: if ((fd = virCreateAnonymousFile(fullVPDExample, dataLen)) < 0) return -1; (The whole patch.) I think it looks cleaner, but that's just my preference. readBytes = virPCIVPDReadVPDBytes(fd, buf, dataLen, 0, &csum);
@@ -482,6 +484,9 @@ testVirPCIVPDParseVPDStringResource(const void *opaque G_GNUC_UNUSED)
dataLen = G_N_ELEMENTS(stringResExample); fd = virCreateAnonymousFile(stringResExample, dataLen); + if (fd < 0) + return -1; + result = virPCIVPDParseVPDLargeResourceString(fd, 0, dataLen, &csum, res); VIR_FORCE_CLOSE(fd);
@@ -552,6 +557,9 @@ testVirPCIVPDParseFullVPD(const void *opaque G_GNUC_UNUSED)
dataLen = G_N_ELEMENTS(fullVPDExample); fd = virCreateAnonymousFile(fullVPDExample, dataLen); + if (fd < 0) + return -1; + res = virPCIVPDParse(fd); VIR_FORCE_CLOSE(fd);
@@ -620,6 +628,9 @@ testVirPCIVPDParseZeroLengthRW(const void *opaque G_GNUC_UNUSED)
dataLen = G_N_ELEMENTS(fullVPDExample); fd = virCreateAnonymousFile(fullVPDExample, dataLen); + if (fd < 0) + return -1; + res = virPCIVPDParse(fd); VIR_FORCE_CLOSE(fd);
@@ -670,6 +681,9 @@ testVirPCIVPDParseNoRW(const void *opaque G_GNUC_UNUSED)
dataLen = G_N_ELEMENTS(fullVPDExample); fd = virCreateAnonymousFile(fullVPDExample, dataLen); + if (fd < 0) + return -1; + res = virPCIVPDParse(fd); VIR_FORCE_CLOSE(fd);
@@ -723,6 +737,9 @@ testVirPCIVPDParseFullVPDSkipInvalidKeywords(const void *opaque G_GNUC_UNUSED)
dataLen = G_N_ELEMENTS(fullVPDExample); fd = virCreateAnonymousFile(fullVPDExample, dataLen); + if (fd < 0) + return -1; + res = virPCIVPDParse(fd); VIR_FORCE_CLOSE(fd);
@@ -776,6 +793,9 @@ testVirPCIVPDParseFullVPDSkipInvalidValues(const void *opaque G_GNUC_UNUSED)
dataLen = G_N_ELEMENTS(fullVPDExample); fd = virCreateAnonymousFile(fullVPDExample, dataLen); + if (fd < 0) + return -1; + res = virPCIVPDParse(fd); VIR_FORCE_CLOSE(fd);
-- 2.31.1
Kristína
On Tue, Nov 23, 2021 at 03:40:46PM +0100, Kristina Hanicova wrote:
On Tue, Nov 23, 2021 at 3:20 PM Ján Tomko <jtomko@redhat.com> wrote:
Fixes: 59c1bc3a0e25e6d725db41990f11e0b53137115d Fixes: 43820e4b8037680ec451761216750c6b139db67a Fixes: 600f580d623ae4077ddeb6c7cb24f8a315a7c73b Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tests/virpcivpdtest.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)
diff --git a/tests/virpcivpdtest.c b/tests/virpcivpdtest.c index 284350fe29..62c51cdeb9 100644 --- a/tests/virpcivpdtest.c +++ b/tests/virpcivpdtest.c @@ -446,6 +446,8 @@ testVirPCIVPDReadVPDBytes(const void *opaque G_GNUC_UNUSED) buf = g_malloc0(dataLen);
fd = virCreateAnonymousFile(fullVPDExample, dataLen); + if (fd < 0) + return -1;
I would prefer if you rewrote this before merging as:
if ((fd = virCreateAnonymousFile(fullVPDExample, dataLen)) < 0) return -1;
(The whole patch.) I think it looks cleaner, but that's just my preference.
It might look cleaner for some until you get to the point of having more parentheses there and we already had some issues with that. I, for one, prefer an extra line or even an extra variable in some cases, but similarly to you it is only my preference and we do not have a coding style for this, unfortunately. We even have your preferred style used in the libvirt coding style page [0]. And guess what, we have that wrong! Look: while ((rc = waitpid(pid, &st, 0) == -1) && errno == EINTR); // ok while ((rc = waitpid(pid, &st, 0) == -1) && errno == EINTR) { // Better /* nothing */ } I would hope that proves my point, but because we had this discussion a couple of times already I know there are lot of libvirt developers (probably in the majority) who prefer cramming as much into that one line, so I guess we won't achieve a consensus on this one ;) Just my €.02 ;) Have a nice day, Martin
On a Tuesday in 2021, Martin Kletzander wrote:
On Tue, Nov 23, 2021 at 03:40:46PM +0100, Kristina Hanicova wrote:
On Tue, Nov 23, 2021 at 3:20 PM Ján Tomko <jtomko@redhat.com> wrote:
Fixes: 59c1bc3a0e25e6d725db41990f11e0b53137115d Fixes: 43820e4b8037680ec451761216750c6b139db67a Fixes: 600f580d623ae4077ddeb6c7cb24f8a315a7c73b Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tests/virpcivpdtest.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)
diff --git a/tests/virpcivpdtest.c b/tests/virpcivpdtest.c index 284350fe29..62c51cdeb9 100644 --- a/tests/virpcivpdtest.c +++ b/tests/virpcivpdtest.c @@ -446,6 +446,8 @@ testVirPCIVPDReadVPDBytes(const void *opaque G_GNUC_UNUSED) buf = g_malloc0(dataLen);
fd = virCreateAnonymousFile(fullVPDExample, dataLen); + if (fd < 0) + return -1;
I would prefer if you rewrote this before merging as:
if ((fd = virCreateAnonymousFile(fullVPDExample, dataLen)) < 0) return -1;
(The whole patch.) I think it looks cleaner, but that's just my preference.
It might look cleaner for some until you get to the point of having more parentheses there and we already had some issues with that. I, for one, prefer an extra line or even an extra variable in some cases, but similarly to you it is only my preference and we do not have a coding style for this, unfortunately. We even have your preferred style used in the libvirt coding style page [0]. And guess what, we have that
I got a SIGSEGV trying to access [0], so I'll go with the other suggestion.
wrong! Look:
while ((rc = waitpid(pid, &st, 0) == -1) && errno == EINTR); // ok while ((rc = waitpid(pid, &st, 0) == -1) && errno == EINTR) { // Better /* nothing */ }
I would hope that proves my point, but because we had this discussion a couple of times already I know there are lot of libvirt developers (probably in the majority) who prefer cramming as much into that one line, so I guess we won't achieve a consensus on this one ;)
Just my €.02 ;)
Thank you for your 0,51 Kč ;) Jano
Have a nice day, Martin
On Tue, Nov 23, 2021 at 04:49:52PM +0100, Ján Tomko wrote:
On a Tuesday in 2021, Martin Kletzander wrote:
On Tue, Nov 23, 2021 at 03:40:46PM +0100, Kristina Hanicova wrote:
On Tue, Nov 23, 2021 at 3:20 PM Ján Tomko <jtomko@redhat.com> wrote:
Fixes: 59c1bc3a0e25e6d725db41990f11e0b53137115d Fixes: 43820e4b8037680ec451761216750c6b139db67a Fixes: 600f580d623ae4077ddeb6c7cb24f8a315a7c73b Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tests/virpcivpdtest.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)
diff --git a/tests/virpcivpdtest.c b/tests/virpcivpdtest.c index 284350fe29..62c51cdeb9 100644 --- a/tests/virpcivpdtest.c +++ b/tests/virpcivpdtest.c @@ -446,6 +446,8 @@ testVirPCIVPDReadVPDBytes(const void *opaque G_GNUC_UNUSED) buf = g_malloc0(dataLen);
fd = virCreateAnonymousFile(fullVPDExample, dataLen); + if (fd < 0) + return -1;
I would prefer if you rewrote this before merging as:
if ((fd = virCreateAnonymousFile(fullVPDExample, dataLen)) < 0) return -1;
(The whole patch.) I think it looks cleaner, but that's just my preference.
It might look cleaner for some until you get to the point of having more parentheses there and we already had some issues with that. I, for one, prefer an extra line or even an extra variable in some cases, but similarly to you it is only my preference and we do not have a coding style for this, unfortunately. We even have your preferred style used in the libvirt coding style page [0]. And guess what, we have that
I got a SIGSEGV trying to access [0], so I'll go with the other suggestion.
lol, it was a lazy reference, only gets resolved when someone wants to access it, I guess you do not have userfaultd support (although one might argue that I am the living proof of userfault-duh in this case). [0] https://libvirt.org/coding-style.html#semicolons
wrong! Look:
while ((rc = waitpid(pid, &st, 0) == -1) && errno == EINTR); // ok while ((rc = waitpid(pid, &st, 0) == -1) && errno == EINTR) { // Better /* nothing */ }
I would hope that proves my point, but because we had this discussion a couple of times already I know there are lot of libvirt developers (probably in the majority) who prefer cramming as much into that one line, so I guess we won't achieve a consensus on this one ;)
Just my €.02 ;)
Thank you for your 0,51 Kč ;)
Jano
Have a nice day, Martin
On Tue, Nov 23, 2021 at 3:21 PM Ján Tomko <jtomko@redhat.com> wrote:
Ján Tomko (4): tools: virt-host-validate: fix memory leak vbox: fix vboxCapsInit ch: fix logic in virCHMonitorBuildPtyJson tests: pcivpdtest: check return value of virCreateAnonymousFile
src/ch/ch_monitor.c | 4 +--- src/vbox/vbox_common.c | 1 - tests/virpcivpdtest.c | 20 ++++++++++++++++++++ tools/virt-host-validate-ch.c | 2 +- 4 files changed, 22 insertions(+), 5 deletions(-)
-- 2.31.1
Reviewed-by: Kristína Hanicová <khanicov@redhat.com>
participants (3)
-
Ján Tomko -
Kristina Hanicova -
Martin Kletzander