2:30 p.m.
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 <source> can be repeated while <target> 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
2:30 p.m.
New subject: [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 <jferlan(a)redhat.com>
---
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
2:30 p.m.
New subject: [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 <jferlan(a)redhat.com>
---
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
2:30 p.m.
New subject: [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 <jferlan(a)redhat.com>
---
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
3:35 a.m.
On 12/17/19 9:30 PM, John Ferlan wrote:
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.
Well, obviously we pass NULL and at least in the apparmor case it is
valid (we don't need to format private data for devices), so I guess we
can remove the attribute, but I will let Dan chime in.
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 <source> can be repeated while <target> 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(-)
Reviewed-by: Michal Privoznik <mprivozn(a)redhat.com>
Michal
1801
days inactive
1802
days old
4 comments
2 participants
participants (2)
-
John Ferlan -
Michal Prívozník