Re: [libvirt] [PATCH] conf: Resolve Coverity FORWARD_NULL

On 08/04/2015 11:42 AM, Michal Privoznik wrote: > On 04.08.2015 13:11, John Ferlan wrote: >> The recent changes to perform SCSI device address checks during the >> post parse callbacks ran afoul of the Coverity checker since the changes >> assumed that the 'xmlopt' parameter to virDomainDeviceDefPostParse >> would be non NULL (commit id 'ca2cf74e87'); however, what was missed >> is there was an "if (xmlopt &&" check being made, so Coverity believed >> that it could be possible for a NULL 'xmlopt'. >> >> Checking the various calling paths seemingly disproves that. If called >> from virDomainDeviceDefParse, there were two other possible calls that >> would end up dereffing, so that path could not be NULL. If called via >> virDomainDefPostParseDeviceIterator via virDomainDefPostParse there >> are two callers (virDomainDefParseXML and qemuParseCommandLine) >> which deref xmlopt either directly or through another call. >> >> So I'm removing the check for non-NULL xmlopt. >> >> Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; >> --- >> src/conf/domain_conf.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c >> index 77a50c3..dd5ebd7 100644 >> --- a/src/conf/domain_conf.c >> +++ b/src/conf/domain_conf.c >> @@ -4145,7 +4145,7 @@ virDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, >> { >> int ret; >> >> - if (xmlopt && xmlopt->config.devicesPostParseCallback) { >> + if (xmlopt->config.devicesPostParseCallback) { >> ret = xmlopt->config.devicesPostParseCallback(dev, def, caps, >> xmlopt->config.priv); >> if (ret < 0) >> > > ACK > > Although with the virDomainDefPostParse() it should be the same story. > @xmlopt can't be NULL there too. But that can be saved for a follow up > patch if you want. > > Michal > Myopia strikes again... I need a larger workspace! John Perhaps it'd be better to squash in the following rather than have a separate patch since it's same issue even though Coverity only complained about the initial one. Although if it's preferred to have a separate patch that's fine by me too: diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 7d3a24d..5eaeb21 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4190,7 +4190,7 @@ virDomainDefPostParse(virDomainDefPtr def, }; /* call the domain config callback */ - if (xmlopt && xmlopt->config.domainPostParseCallback) { + if (xmlopt->config.domainPostParseCallback) { ret = xmlopt->config.domainPostParseCallback(def, caps, xmlopt->config.priv); if (ret < 0)