Re: [Xen-devel] [PATCH v3] tools/xl: correctly shows split eventchannel for netfront

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Sun, 2014-01-26 at 18:23 +0800, Annie Li wrote:
> From: Annie Li <annie.li@xxxxxxxxxx>
> 
> After split event channel feature was supported by netback/netfront,
> "xl network-list" does not show event channel correctly. Add tx-/rx-evt-ch
> to show tx/rx event channel correctly.
> 
> V3: Remove conditional limitation of LIBXL_HAVE_NETWORK_SPLIT_EVENTCHANNEL
> 
> V2: keep evtch field without breaking the API

For future reference these intraversion changelogs should be placed
after the S-o-b and a "---" separator, such that they don't get included
in the final commit message.

> Signed-off-by: Annie Li <annie.li@xxxxxxxxxx>

> ---
>  tools/libxl/libxl.c         |    8 ++++++++
>  tools/libxl/libxl.h         |   10 ++++++++++
>  tools/libxl/libxl_types.idl |    1 +
>  tools/libxl/xl_cmdimpl.c    |   12 ++++++------
>  4 files changed, 25 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 2845ca4..4ce9efc 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -3142,6 +3142,14 @@ int libxl_device_nic_getinfo(libxl_ctx *ctx, uint32_t 
> domid,
>      nicinfo->state = val ? strtoul(val, NULL, 10) : -1;
>      val = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, 
> "%s/event-channel", nicpath));
>      nicinfo->evtch = val ? strtoul(val, NULL, 10) : -1;
> +    if(nicinfo->evtch > 0)
> +        nicinfo->evtch_rx = nicinfo->evtch;
> +    else {
> +        val = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, 
> "%s/event-channel-tx", nicpath));
> +        nicinfo->evtch = val ? strtoul(val, NULL, 10) : -1;
> +        val = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, 
> "%s/event-channel-rx", nicpath));
> +        nicinfo->evtch_rx = val ? strtoul(val, NULL, 10) : -1;
> +    }
>      val = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "%s/tx-ring-ref", 
> nicpath));
>      nicinfo->rref_tx = val ? strtoul(val, NULL, 10) : -1;
>      val = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "%s/rx-ring-ref", 
> nicpath));
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 12d6c31..87f1fa4 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -409,6 +409,16 @@
>   */
>  #define LIBXL_HAVE_DRIVER_DOMAIN_CREATION 1
>  
> +/*
> + * LIBXL_HAVE_NETWORK_SPLIT_EVENTCHANNEL
> + * If this is defined, libxl_nicinfo will contain an integer type field: 
> evtch_rx,
> + * containing the value of event channel for rx path of netback&netfront 
> which support
> + * split event channel. The original evtch field contains the value of event 
> channel
> + * for tx path in such case.

Some of these lines are rather long. Please keep things < 80 characters.
Also put spaces either side of "&". Lastly please say explicitly that
for non-split front and back both fields will be the same.

> + *
> + */
> +#define LIBXL_HAVE_NETWORK_SPLIT_EVENTCHANNEL 1
> +
>  /* Functions annotated with LIBXL_EXTERNAL_CALLERS_ONLY may not be
>   * called from within libxl itself. Callers outside libxl, who
>   * do not #include libxl_internal.h, are fine. */
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 649ce50..5642cf5 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -489,6 +489,7 @@ libxl_nicinfo = Struct("nicinfo", [
>      ("devid", libxl_devid),
>      ("state", integer),
>      ("evtch", integer),
> +    ("evtch_rx", integer),
>      ("rref_tx", integer),
>      ("rref_rx", integer),
>      ], dir=DIR_OUT)
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index c30f495..be1162e 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -5842,9 +5842,9 @@ int main_networklist(int argc, char **argv)
>          /* No options */
>      }
>  
> -    /*      Idx  BE   MAC   Hdl  Sta  evch txr/rxr  BE-path */
> -    printf("%-3s %-2s %-17s %-6s %-5s %-6s %5s/%-5s %-30s\n",
> -           "Idx", "BE", "Mac Addr.", "handle", "state", "evt-ch", "tx-", 
> "rx-ring-ref", "BE-path");
> +    /*      Idx  BE   MAC   Hdl  Sta  txev/rxev txr/rxr  BE-path */
> +    printf("%-3s %-2s %-17s %-6s %-5s %6s/%-6s %5s/%-5s %-30s\n",
> +           "Idx", "BE", "Mac Addr.", "handle", "state", "tx-", "rx-evt-ch", 
> "tx-", "rx-ring-ref", "BE-path");

I know this is already overly long but please rewrap to 80 columns as
you add more.

How wide is the resulting output now? Is it over 80?

>      for (argv += optind, argc -= optind; argc > 0; --argc, ++argv) {
>          uint32_t domid = find_domain(*argv);
>          nics = libxl_device_nic_list(ctx, domid, &nb);
> @@ -5857,9 +5857,9 @@ int main_networklist(int argc, char **argv)
>                  printf("%-3d %-2d ", nicinfo.devid, nicinfo.backend_id);
>                  /* MAC */
>                  printf(LIBXL_MAC_FMT, LIBXL_MAC_BYTES(nics[i].mac));
> -                /* Hdl  Sta  evch txr/rxr  BE-path */
> -                printf("%6d %5d %6d %5d/%-11d %-30s\n",
> -                       nicinfo.devid, nicinfo.state, nicinfo.evtch,
> +                /* Hdl  Sta  txev/rxev txr/rxr  BE-path */
> +                printf(" %6d %5d %6d/%-9d %5d/%-11d %-30s\n",

It odd that the width fields here differ from the header, but that seems
to be a prexisting issue. (e.g. %5s/%-5s for tx-/rx-ring-ref in the
header is %5d/%-11d here -- I guess because rx-ring-ref is actually more
than 5 wide so it actually takes 11).

I think you should at least use the same thing in the header and data
lines for the new fields you are adding, but if you felt inclined to
clean up the existing mismatches too that would be great.

> +                       nicinfo.devid, nicinfo.state, nicinfo.evtch, 
> nicinfo.evtch_rx,
>                         nicinfo.rref_tx, nicinfo.rref_rx, nicinfo.backend);
>                  libxl_nicinfo_dispose(&nicinfo);
>              }



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