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

[David kiarie]

Looking at both of your comments, I go with VIR_WARN

On Wed, Jul 16, 2014 at 12:26 AM, Eric Blake <eblake at redhat.com> wrote:
> 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
>