Re: [PATCH v15 3/7] remus netbuffer: implement remus network buffering for nic devices

[Hongyang Yang <yanghy@cn.fujitsu.com>]

On 07/11/2014 01:43 AM, Ian Jackson wrote:
> Hongyang Yang writes ("Re: [PATCH v15 3/7] remus netbuffer: implement remus network buffering for nic devices"):
>> On 07/10/2014 07:15 AM, Ian Jackson wrote:
>>> It occurs to me to ask: Is it wise to use such a bare name for the
>>> variable IFB ?  I would suggest that it might be better to use a
>>> variable with XEN or REMUS or something in it.  Otherwise perhaps
>>> someone somewhere might think IFB stood for something else and would
>>> set it to something irrelevant (or your setting of it might break
>>> their usage...)
>>
>> Maybe REMUS_IFB?
>
> Sounds good to me.
>
>>>> +struct libxl__remus_netbuf_state {
>>>> +    libxl__ao *ao;
>>>> +    uint32_t domid;
>>>> +    const char *netbufscript;
>>>
>>> Why is there a copy of netbufscript here ?
>>
>> We use netbuf state in device concrete layer, so we made a copy
>> of this.
>
> What I mean is, why does the concrete layer not have access to the
> remus_state ?  Is there something wrong with passing it the
> remus_state ?

I intend to make less connections between layers, but I will rethink
about this.

>
>>>> +/*----- init() and destroy() -----*/
>>>> +
>>>> +static int nic_init(const libxl__remus_device_ops *self,
>>>> +                    libxl__remus_state *rs)
>>>> +{
>>>
>>> This function seems to leak the things it allocates, on error.
>>> The `out' section should probably call destroy.
>>
>> If this failed, we will call teardown, and in teardown, the
>> nic_destroy will be called.
>
> OIC.  I think this needs to be documented in a comment in
> libxl__remus_device_ops.  Normally one wouldn't expect to have to call
> destroy() if init() fails.
>
>>> Last time I commented on the error handling in this patch.  I see that
>>> you have added an nl_geterror in some places.
>>>
>>> When I make a comment like that, you need to apply it whereever
>>> relevant in the code, and to the whole patch series.
>>
>> Yes, actually I've checked the whole patch about this and
>> corrected them. for this special case, nl_socket_alloc() doesn't
>> return an error number, so we can't use nl_geterror, it returns
>> NULL if alloc failed, so I think this error message should be ok.
>
> Err, OK.
>
>>>> +static void nic_destroy(const libxl__remus_device_ops *self,
>>>> +                        libxl__remus_state *rs)
>>> ...
>>>> +    /* free qdisc cache */
>>>> +    if (ns->qdisc_cache) {
>>>> +        nl_cache_clear(ns->qdisc_cache);
>>>> +        nl_cache_free(ns->qdisc_cache);
>>>
>>> Can these functions fail ?  If so, what does it mean and what should
>>> the program do ?
>>
>> No, they both return void.
>
> Oh, good.
>
>>>> +static int remus_netbuf_op(libxl__remus_device_nic *remus_nic,
>>>> +                           libxl__remus_netbuf_state *netbuf_state,
>>>> +                           int buffer_op)
>>>> +{
>>>> +    int rc;
>>>> +
>>>> +    STATE_AO_GC(netbuf_state->ao);
>>>> +
>>>> +    if (buffer_op == tc_buffer_start)
>>>> +        rc = rtnl_qdisc_plug_buffer(remus_nic->qdisc);
>>>> +    else
>>>> +        rc = rtnl_qdisc_plug_release_one(remus_nic->qdisc);
>>>
>>> Error handling again.
>>
>> I can't quite follow you, what's wrong with the error handling in this
>> function except the name of the "rc"?
>
> In fact reading it again I was confused by the fact that this function
> doesn't actually return rc.  Sorry about that - but I guess this shows
> why using the conventional names etc. is important.
>
>>> How does rtnl_link_get_ifindex report its error code ?  Or can it fail
>>> only one way ?
>>
>> It Returns Interface index or 0 if not set. No error code will be returned.
>
> Oh.  The error handling in this rtnl facility you're using does seem
> rather poor - but of course this isn't your fault.  Thanks for
> clarifying.
>
> Thanks for your comprehensive reply.
>
> Ian.
> .
>

-- 
Thanks,
Yang.