Re: [libvirt] [PREPOST 05/17] src/xenxs:Refactor PCI parsing code

On 07/11/2014 11:09 AM, Jim Fehlig wrote: > David Kiarie wrote: >> + >> + /* pci=['0000:00:1b.0','0000:00:13.0'] */ >> + if (!(key = list->str)) >> + goto skippci; >> + if (!(nextkey = strchr(key, ':'))) >> + goto skippci; >> + >> + if (virStrncpy(domain, key, (nextkey - key), sizeof(domain)) == NULL) { >> + virReportError(VIR_ERR_INTERNAL_ERROR, >> + _("Domain %s too big for destination"), key); >> > > Pre-existing, but while we are touching the code, I wonder if the use of > virReportError here is correct? The device is skipped if there is a > problem parsing it. I think these errors should be logged via VIR_WARN, > but would like confirmation from another libvirt dev before asking you > to change them. At the very least, the error should be changed to > VIR_ERR_CONF_SYNTAX. How easy is it to trigger this error path via user input? If the string that we are splitting is normally provided from a sane source, using VIR_ERR_INTERNAL_ERROR is okay; if the string we are splitting can come from a user that can easily try to fuzz things to confuse us, then VIR_ERR_CONF_SYNTAX is indeed nicer. As for whether to skip a device we can't parse, or outright fail, I'm not sure which is better. If you are just skipping the device, then using VIR_WARN instead of virReportError will avoid the odd case of returning success to the user while still having an error set. Don't know if I helped or just made it more confusing. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org