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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

Attachment: signature.asc
Description: OpenPGP digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]