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

[Zhang Chen <zhangchen.fnst@xxxxxxxxxxxxxx>]

On 01/28/2017 01:05 AM, Wei Liu wrote:
> 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
Make sense, I will fix it in next version.

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

> > +    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_.
OK, will fix in next version.

Thanks
Zhang Chen

> Wei.
>
>
> .
--
Thanks
Zhang Chen




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