Re: [Qemu-devel] [snabb-devel] Re: [PATCH v10 15/18] Add the vhost-user netdev backend to the command line

[Eric Blake]

On 06/09/2014 03:19 PM, Nikolay Nikolaev wrote:

>>> +static CharDriverState *net_vhost_parse_chardev(
>>> +        const NetdevVhostUserOptions *opts)
>>
>> Unusual indentation, but I see your dilemma of fitting 80 columns.
>>
> What woudl be the right approach here, is leaving it 84 colums acceptable:

checkpatch.pl is for guidance; it is okay to have a patch that doesn't
pass. At any rate, I won't reject a patch for long lines, or for unusual
style, where it is obvious that there is no way to fit things into 80
columns while still complying with all other style issues.


>>> +        error_report("vhost-user requires frontend driver
>> virtio-net-*");
>>
>> How many of these error_report() should be using Error **errp logistics
>> instead?
>>
>> I don't see this 'errp' logic applied here. These are static functions
> called in the init function to check for compatible command line parameters.
> This init fucntion is part of 'net_client_init_fun[]" which does not
> provide 'Error **errp' argument. The way I see it, there is no way to
> propagate the 'errp' up. Or am I getting this wrong?

It was more of a 'food for thought' question - we may eventually want to
enhance the code base to be able to propagate errp up; but since that is
a more invasive patch, and your series isn't making the situation worse,
it does not necessarily mean you have to do that additional work.  On
the other hand, if the conversion is worth doing, then getting it done
sooner makes it easier to take advantage of the improved error handling
for other new code in the same area.


>>> +
>>> +    /* verify net frontend */
>>> +    if (qemu_opts_foreach(qemu_find_opts("device"), net_vhost_check_net,
>>> +                          (void *)name, true) == -1) {
>>
>> This is C; why do you need a cast to (void*)?
>>
> Because of the constness in 'const char *name'. I am casting to 'char *
> now', is it better?

Okay, I see.  Sometimes, I like to put in a comment /* cast away const
*/ to make it obvious why I'm doing things like that.  It also serves as
a spur to investigate a couple of things: 1) should the callback
signature should have allowed a const in the first place, rather than
making callers have to cast away const, 2) if the callback can modify
the pointer but I'm casting away const, am I going to cause data
corruption and/or a crash

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature