[libvirt] [PATCH 0/3] Some coverity adjustments

[Michal Prívozník]

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 at redhat.com>

Michal