Re: [libvirt] [PATCH 2/3] vmx: The virVMXParseDisk deviceType can be NULL, add some missing checks

On 05.09.2015 16:29, Matthias Bolte wrote: > 2015-09-02 13:00 GMT+02:00 Michal Privoznik <mprivozn(a)redhat.com&gt;: >> On 01.09.2015 16:52, Matthias Bolte wrote: >>> --- >>> src/vmx/vmx.c | 22 +++++++++++----------- >>> 1 file changed, 11 insertions(+), 11 deletions(-) >>> >>> diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c >>> index ba4d046..9d1574f 100644 >>> --- a/src/vmx/vmx.c >>> +++ b/src/vmx/vmx.c >>> @@ -2178,8 +2178,9 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virConfPtr con >>> (*def)->transient = STRCASEEQ(mode, >>> "independent-nonpersistent"); >>> } else if (virFileHasSuffix(fileName, ".iso") || >>> - STRCASEEQ(deviceType, "atapi-cdrom") || >>> - STRCASEEQ(deviceType, "cdrom-raw")) { >>> + (deviceType && >>> + (STRCASEEQ(deviceType, "atapi-cdrom") || >>> + STRCASEEQ(deviceType, "cdrom-raw")))) { >>> /* >>> * This function was called in order to parse a harddisk device, >>> * but .iso files, 'atapi-cdrom', and 'cdrom-raw' devices are for >>> @@ -2199,13 +2200,12 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virConfPtr con >>> if (virFileHasSuffix(fileName, ".iso")) { >>> char *tmp; >>> >>> - if (deviceType != NULL) { >>> - if (STRCASENEQ(deviceType, "cdrom-image")) { >>> - virReportError(VIR_ERR_INTERNAL_ERROR, >>> - _("Expecting VMX entry '%s' to be 'cdrom-image' " >>> - "but found '%s'"), deviceType_name, deviceType); >>> - goto cleanup; >>> - } >>> + if (deviceType && STRCASENEQ(deviceType, "cdrom-image")) { >>> + virReportError(VIR_ERR_INTERNAL_ERROR, >>> + _("Expecting VMX entry '%s' to be 'cdrom-image' " >>> + "but found '%s'"), deviceType_name, >>> + deviceType ? deviceType : "unknown"); >> >> The control can get here iff deviceType != NULL. >> > > The check for NULL was preexisting here, I just merged two nested if > blocks here. Also I don't see how you get to the conclusion that the > control can only get here if deviceType != NULL. There is no code > before or around this block that would do that. > Maybe I should have been more specific. I meant the virReportError(). There's no need for (preexisting) ternary operator since the if() right above checks for deviceType.