[libvirt] [PATCH] conf: Resolve Coverity FORWARD_NULL
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@redhat.com> --- 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) -- 2.1.0
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@redhat.com> --- 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
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@redhat.com> --- 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)
On 04.08.2015 17:54, John Ferlan wrote:
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@redhat.com> --- 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)
I'd prefer to have it in a single patch. ACK with this squashed in then. Michal
participants (2)
-
John Ferlan -
Michal Privoznik