Re: [Xen-devel] [PATCH V9 08/12] remus: implement the API to buffer/release packages

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Yang Hongyang writes ("[PATCH V9 08/12] remus: implement the API to 
buffer/release packages"):
> This patch implements two APIs:
> 1. netbuf_start_new_epoch()
>    It marks a new epoch. The packages before this epoch will
>    be flushed, and the packages after this epoch will be buffered.
>    It will be called after the guest is suspended.
> 2. netbuf_release_prev_epoch()
>    It flushes the buffered packages to client, and it will be
>    called when a checkpoint finishes.

Thanks.

(BTW "packages" should be "packets" throughout.)

> diff --git a/tools/libxl/libxl_netbuffer.c b/tools/libxl/libxl_netbuffer.c
> index a5f2b9a..4b4bc9d 100644
> --- a/tools/libxl/libxl_netbuffer.c
> +++ b/tools/libxl/libxl_netbuffer.c
> @@ -416,9 +416,66 @@ static int nic_teardown(libxl__remus_device *remus_dev,
>      return REMUS_INPROGRESS;
>  }
>  
> +/* The buffer_op's value, not the value passed to kernel */
> +enum {
> +    tc_buffer_start,
> +    tc_buffer_release
> +};

The comment refers to a "buffer_op" variable which is not nearby.  I
suggest replacing the comment with
    /* Internal value for libxl, not the value passed to kernel */

Are these names "tc_" not possibly clashing with some future kernel
things ?

> +static int remus_netbuf_op(libxl__remus_device_nic *remus_nic,
> +                           libxl__remus_netbuf_state *netbuf_state,
> +                           int buffer_op)
> +{
> +    int ret;
> +
> +    STATE_AO_GC(netbuf_state->ao);
> +
> +    if (buffer_op == tc_buffer_start)
> +        ret = rtnl_qdisc_plug_buffer(remus_nic->qdisc);
> +    else
> +        ret = rtnl_qdisc_plug_release_one(remus_nic->qdisc);

If you're going to have an enum here, I think you should use
"switch", and explicitly abort() on unknown values.  Otherwise when a
new op is invented this code will silently do the wrong thing.

> +    if (!ret) {

This is an opaque error handling style.  Please use libxl's standard
"goto out" style.

> +        ret = rtnl_qdisc_add(netbuf_state->nlsock,
> +                             remus_nic->qdisc,
> +                             NLM_F_REQUEST);
> +        if (ret)
> +            goto out;
> +    }
> +
> +    return REMUS_OK;

Please don't invent your own error numbers.  Please use the existing
libxl error code type.  Feel free to add new values if you need them.

> +out:
> +    LOG(ERROR, "Remus: cannot do netbuf op %s on %s:%s",
> +        ((buffer_op == tc_buffer_start) ?
> +        "start_new_epoch" : "release_prev_epoch"),

Perhaps there should be a static const array of these strings.  At the
very least you should make sure this code handles the _release op
explicitly and crashes if the value is not recognised.

>  libxl__remus_device_type remus_device_nic = {
>      .init = nic_init,
>      .destroy = nic_destroy,
> +    .postsuspend = netbuf_start_new_epoch,
> +    .commit = netbuf_release_prev_epoch,

Presumably, this is the patch where network buffering actually starts
to happen.  So should this come with a documentation change removing
any warnings about remus and networking ?

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel