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

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

On 07/10/2014 07:15 AM, Ian Jackson wrote:
> Yang Hongyang writes ("[PATCH v15 3/7] remus netbuffer: implement remus network buffering for nic devices"):
>> diff --git a/tools/hotplug/Linux/remus-netbuf-setup b/tools/hotplug/Linux/remus-netbuf-setup
>> new file mode 100644
>> index 0000000..58c46f3
>> --- /dev/null
>> +++ b/tools/hotplug/Linux/remus-netbuf-setup
>> @@ -0,0 +1,203 @@
>> +#!/bin/bash
>> +#============================================================================
>> +# ${XEN_SCRIPT_DIR}/remus-netbuf-setup
> ...
>> +# IFB         ifb interface to be cleaned up (required). [for teardown op only]
>
> 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?

>
> I had some other comments about this script in v10, which we were
> having a conversation about.  I'm afraid I seem to have dropped the
> thread of that.  I will reply separately in that thread.

I have replied in that thread.

>
>> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
>> index bcbd02b..b7d62c1 100644
>> --- a/tools/libxl/libxl.c
>> +++ b/tools/libxl/libxl.c
>> @@ -788,6 +788,24 @@ int libxl_domain_remus_start(libxl_ctx *ctx, libxl_domain_remus_info *info,
> ...
>> +    /* Setup network buffering */
>
> This comment is misleading.  This code doesn't actually set up any
> network buffering.  It doesn't call the setup script.  All it does is
> some checks and computing the script path.
>
> It would probably be better to simply remove the comment.  Or you
> could replace it with something more accurate.

Will remove the comment, thanks.

>
>> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
>> index 8ef20a0..2fe36a6 100644
>> --- a/tools/libxl/libxl_internal.h
>> +++ b/tools/libxl/libxl_internal.h
>> @@ -310,6 +310,10 @@ struct libxl__gc {
>>       libxl_ctx *owner;
>>   };
>>
>> +/* remus device ops specific structures start */
>> +typedef struct libxl__remus_netbuf_state libxl__remus_netbuf_state;
>> +/* remus device ops specific structures end */
>
> I don't think these comments add anything.  You should move these
> struct typedefs alongside the other struct typedefs (libxl__ao et al).
> There is then no need to comment what they are.
>
>>   struct libxl__ctx {
>>       xentoollog_logger *lg;
>>       xc_interface *xch;
>> @@ -374,6 +378,9 @@ struct libxl__ctx {
>>       LIBXL_LIST_ENTRY(libxl_ctx) sigchld_users_entry;
>>
>>       libxl_version_info version_info;
>> +
>> +    /* remus device ops specific structures */
>> +    libxl__remus_netbuf_state *rns;
>
> Again, I think this comment adds nothing that isn't in the type name.
> There is no need to say in a comment anything that is plain from the
> code.
>
> But before you delete it (and other comments that I'm going to quibble
> about), please wait for a comment from the other tools maintainer, Ian
> Campbell, who may disagree.
>
>> @@ -2631,6 +2639,8 @@ struct libxl__remus_state {
>>       libxl__ao *ao;
>>       uint32_t domid;
>>       libxl__remus_callback *callback;
>> +    /* Script to setup/teardown network buffers */
>> +    const char *netbufscript;
>
> And again.
>
>> diff --git a/tools/libxl/libxl_netbuffer.c b/tools/libxl/libxl_netbuffer.c
>> index 52d593c..025ee89 100644
>> --- a/tools/libxl/libxl_netbuffer.c
>> +++ b/tools/libxl/libxl_netbuffer.c
>> @@ -17,11 +17,486 @@
>>
>>   #include "libxl_internal.h"
>>
>> +#include <netlink/cache.h>
>> +#include <netlink/socket.h>
>> +#include <netlink/attr.h>
>> +#include <netlink/route/link.h>
>> +#include <netlink/route/route.h>
>> +#include <netlink/route/qdisc.h>
>> +#include <netlink/route/qdisc/plug.h>
>> +
>> +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.

>
>> +/*----- 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.

>
>> +    ns->nlsock = nl_socket_alloc();
>> +    if (!ns->nlsock) {
>> +        LOG(ERROR, "cannot allocate nl socket");
>> +        goto out;
>> +    }
>
> 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.

>
>> +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.

>
>> +/*----- checkpointing APIs -----*/
>> +
>> +/* The buffer_op's value, not the value passed to kernel */
>
> I would say "The value of buffer_op, not the value passed to kernel".

Ok, thanks.

>
>> +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"?

>
> Also do not put anything other than a libxl error code in a variable
> called "rc".

Ok.

>
>> +/*----- main flow of control -----*/
>> +
>> +/* helper functions */
>> +
>> +/*
>> + * If the device has a vifname, then use that instead of
>> + * the vifX.Y format.
>> + * it must ONLY be used for remus because if driver domains
>> + * were in use it would constitute a security vulnerability.
>> + */
>> +static const char *get_vifname(libxl__remus_device *dev,
>> +                               const libxl_device_nic *nic)
>> +{
> ...
>> +    path = libxl__sprintf(gc, "%s/backend/vif/%d/%d/vifname",
>> +                          libxl__xs_get_dompath(gc, 0), domid, nic->devid);
>
> Please use GCSPRINTF where applicable.

Ok, thanks.

>
>> +    rc = libxl__xs_read_checked(gc, XBT_NULL, path, &vifname);
>> +    if (!rc && !vifname) {
>> +        /* use the default name */
>
> This comment adds nothing to the comment above.
>
>> +        vifname = libxl__device_nic_devname(gc, domid,
>> +                                            nic->devid,
>> +                                            nic->nictype);
>
>> +    rc = ERROR_FAIL;
>> +    ifindex = rtnl_link_get_ifindex(ifb);
>> +    if (!ifindex) {
>> +        LOG(ERROR, "interface %s has no index", remus_nic->ifb);
>> +        goto out;
>
> 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.

>
>> +    qdisc = rtnl_qdisc_get_by_parent(netbuf_state->qdisc_cache, ifindex,
>> +                                     TC_H_ROOT);
>> +
>> +    if (qdisc) {
>> +        const char *tc_kind = rtnl_tc_get_kind(TC_CAST(qdisc));
>> +        /* Sanity check: Ensure that the root qdisc is a plug qdisc. */
>> +        if (!tc_kind || strcmp(tc_kind, "plug")) {
>> +            nl_object_put((struct nl_object *)qdisc);
>
> This freeing of qdisc should be in the `out' section.
>
> You can do this either by arranging that the `out' section is not used
> at all on successful completion, or by saying something like
>      if (rc) {
>          if (qdisc) nl_object_put((struct nl_object *)qdisc);
>      }

Ok thanks.

>
>> +            LOG(ERROR, "plug qdisc is not installed on %s", remus_nic->ifb);
>> +            goto out;
>> +        }
>> +        remus_nic->qdisc = qdisc;
>> +        rc = 0;
>> +    } else {
>> +        LOG(ERROR, "Cannot get qdisc handle from ifb %s", remus_nic->ifb);
>
> This path out of the function seems not to set rc.
>
> ... In fact, I see that you initialise rc to ERROR_FAIL above.  IMO
> this is not a good programming paradigm.  rc should be set, obviously,
> on every exit path, at the point the error is generated.

Ok thanks.

>
>> +/*
>> + * In return, the script writes the name of IFB device (during setup) to be
>> + * used for output buffering into XENBUS_PATH/ifb
>> + */
>> +static void netbuf_setup_script_cb(libxl__egc *egc,
>> +                                   libxl__async_exec_state *aes,
>> +                                   int status)
>
> This callback is not in chronological order.
>
> I'm afraid I've run out of time now so I'm going to stop here.  I hope
> these comments have been helpful.

Thank you for patiently reviewing this.

>
> Thanks,
> Ian.
> .
>

-- 
Thanks,
Yang.