Re: [Xen-devel] [PATCH RFC 4/6] COLO-Proxy: Add primary userspace colo proxy start args

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Thu, Jan 26, 2017 at 02:36:07PM +0800, Zhang Chen wrote:
> Qemu need this args to start userspace colo-proxy.
> 
> Signed-off-by: Zhang Chen <zhangchen.fnst@xxxxxxxxxxxxxx>

Since we have:

   # Note that the COLO configuration settings should be considered unstable.
   # They may change incompatibly in future versions of Xen.

I have no major objection with this patch.

But I do wonder if you can reduce the repetition somehow.

See below.

> ---
>  tools/libxl/libxl_dm.c      | 104 ++++++++++++++++++++
>  tools/libxl/libxl_nic.c     | 232 
> ++++++++++++++++++++++++++++++++++++++++++++
>  tools/libxl/libxl_types.idl |  31 +++++-
>  tools/libxl/xl_cmdimpl.c    |  58 +++++++++++
>  4 files changed, 424 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index 281058d..b3484df 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
>          }
> diff --git a/tools/libxl/libxl_nic.c b/tools/libxl/libxl_nic.c
> index 61b55ca..b7a3596 100644
> --- a/tools/libxl/libxl_nic.c
> +++ b/tools/libxl/libxl_nic.c
> @@ -196,6 +196,123 @@ static void libxl__device_nic_add(libxl__egc *egc, 
> uint32_t domid,
>          flexarray_append(back, nic->coloft_forwarddev);
>      }
>  
> +    if (nic->sock_mirror_id) {
> +        flexarray_append(back, "sock_mirror_id");
> +        flexarray_append(back, nic->sock_mirror_id);
> +    }
> +    if (nic->sock_mirror_ip) {
> +        flexarray_append(back, "sock_mirror_ip");
> +        flexarray_append(back, nic->sock_mirror_ip);
> +    }

Here, please use a macro:

#define MAYBE_ADD_COLO_ARGS(arg) ...

     MAYBE_ADD_COLO_ARGS(sock_mirror_id);
     MAYBE_ADD_COLO_ARGS(sock_mirror_ip);
     ...
#undef MAYBE_ADD_COLO_ARGS
> +
>      flexarray_append(back, "mac");
>      flexarray_append(back,GCSPRINTF(LIBXL_MAC_FMT, 
> LIBXL_MAC_BYTES(nic->mac)));
>      if (nic->ip) {
> @@ -348,6 +465,121 @@ static int libxl__device_nic_from_xenstore(libxl__gc 
> *gc,
>                                  GCSPRINTF("%s/forwarddev", libxl_path),
>                                  (const char **)(&nic->coloft_forwarddev));
>      if (rc) goto out;
> +    rc = libxl__xs_read_checked(NOGC, XBT_NULL,
> +                                GCSPRINTF("%s/sock_mirror_id", libxl_path),
> +                                (const char **)(&nic->sock_mirror_id));
> +    if (rc) goto out;

And another macro for this hunk.

> +    rc = libxl__xs_read_checked(NOGC, XBT_NULL,
> +                                GCSPRINTF("%s/sock_mirror_ip", libxl_path),
> +                                (const char **)(&nic->sock_mirror_ip));
> +    if (rc) goto out;
> +    rc = libxl__xs_read_checked(NOGC, XBT_NULL,
> +                                GCSPRINTF("%s/sock_mirror_port", libxl_path),
> +                                (const char **)(&nic->sock_mirror_port));
> +    if (rc) goto out;
> +    rc = libxl__xs_read_checked(NOGC, XBT_NULL,
[...]
>  
>      /* vif_ioemu nics use the same xenstore entries as vif interfaces */
>      rc = libxl__xs_read_checked(gc, XBT_NULL,
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 89c2c9d..1deb11b 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -629,7 +629,36 @@ libxl_device_nic = Struct("device_nic", [
>      ("gatewaydev", string),
>      # Note that the COLO configuration settings should be considered 
> unstable.
>      # They may change incompatibly in future versions of Xen.
> -    ("coloft_forwarddev", string)
> +    ("coloft_forwarddev", string),
> +    ("sock_mirror_id", string),
> +    ("sock_mirror_ip", string),
> +    ("sock_mirror_port", string),
> +    ("sock_compare_pri_in_id", string),
> +    ("sock_compare_pri_in_ip", string),
> +    ("sock_compare_pri_in_port", string),
> +    ("sock_compare_sec_in_id", string),
> +    ("sock_compare_sec_in_ip", string),
> +    ("sock_compare_sec_in_port", string),
> +    ("sock_redirector0_id", string),
> +    ("sock_redirector0_ip", string),
> +    ("sock_redirector0_port", string),
> +    ("sock_redirector1_id", string),
> +    ("sock_redirector1_ip", string),
> +    ("sock_redirector1_port", string),
> +    ("sock_redirector2_id", string),
> +    ("sock_redirector2_ip", string),
> +    ("sock_redirector2_port", string),
> +    ("filter_mirror_queue", string),
> +    ("filter_mirror_outdev", string),
> +    ("filter_redirector0_queue", string),
> +    ("filter_redirector0_indev", string),
> +    ("filter_redirector0_outdev", string),
> +    ("filter_redirector1_queue", string),
> +    ("filter_redirector1_indev", string),
> +    ("filter_redirector1_outdev", string),

I suggest you prefix all these fields with colo_.

Wei.

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