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)

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)