Re: [Qemu-devel] [PATCH V5 2/9] net/filter-mirror.c: Make filter mirror support vnet support.

[Zhang Chen]

On 05/25/2017 02:04 PM, Jason Wang wrote:
> On 2017年05月23日 22:20, Zhang Chen wrote:
> > We add the vnet_hdr option for filter-mirror, default is disable.
> > If you use virtio-net-pci net driver, please enable it.
> > You can use it for example:
> > -object 
> > filter-mirror,id=m0,netdev=hn0,queue=tx,outdev=mirror0,vnet_hdr=on
> >
> > If vnet_hdr=on we change the send packet format from
> > struct {int size; const uint8_t buf[];} to {int size; int 
> > vnet_hdr_len; const uint8_t buf[];}.
> > make other module(like colo-compare) know how to parse net packet 
> > correctly.
> >
> > Signed-off-by: Zhang Chen <address@hidden>
> > ---
> >   net/filter-mirror.c | 73 
> > +++++++++++++++++++++++++++++++++++++++++++++++++----
> >   qemu-options.hx     |  5 ++--
> >   2 files changed, 70 insertions(+), 8 deletions(-)
> >
> > diff --git a/net/filter-mirror.c b/net/filter-mirror.c
> > index 72fa7c2..8df0be6 100644
> > --- a/net/filter-mirror.c
> > +++ b/net/filter-mirror.c
> > @@ -38,15 +38,17 @@ typedef struct MirrorState {
> >       NetFilterState parent_obj;
> >       char *indev;
> >       char *outdev;
> > +    bool vnet_hdr;
> >       CharBackend chr_in;
> >       CharBackend chr_out;
> >       SocketReadState rs;
> >   } MirrorState;
> >   -static int filter_mirror_send(CharBackend *chr_out,
> > +static int filter_mirror_send(MirrorState *s,
> >                                 const struct iovec *iov,
> >                                 int iovcnt)
> >   {
> > +    NetFilterState *nf = NETFILTER(s);
> >       int ret = 0;
> >       ssize_t size = 0;
> >       uint32_t len = 0;
> > @@ -58,14 +60,42 @@ static int filter_mirror_send(CharBackend *chr_out,
> >       }
> >         len = htonl(size);
> > -    ret = qemu_chr_fe_write_all(chr_out, (uint8_t *)&len, sizeof(len));
> > +    ret = qemu_chr_fe_write_all(&s->chr_out, (uint8_t *)&len, 
> > sizeof(len));
> >       if (ret != sizeof(len)) {
> >           goto err;
> >       }
> >   +    if (s->vnet_hdr) {
> > +        /*
> > +         * If vnet_hdr = on, we send vnet header len to make other
> > +         * module(like colo-compare) know how to parse net
> > +         * packet correctly.
> > +         */
> > +        ssize_t vnet_hdr_len;
> > +
> > +        /*
> > +         * In anytime, nf->netdev and nf->netdev->peer both have a 
> > vnet_hdr_len,
> > +         * Here we just find out which is we need. When filter set 
> > RX or TX
> > +         * that the real vnet_hdr_len are different.
> > +         */
> > +        if (nf->netdev->using_vnet_hdr) {
> > +            vnet_hdr_len = nf->netdev->vnet_hdr_len;
> > +        } else if (nf->netdev->peer->using_vnet_hdr) {
> > +            vnet_hdr_len = nf->netdev->peer->vnet_hdr_len;
>
> Rethink about this, looks like what we really want is:
>
> - For tx direction, only check nf->netdev->vnet_hdr_len,
> - For rx direction, only check nf->netdev->peer->vnet_hdr_len

Agree, So do you means we should remove the using_vnet_hdr related codes 
then just
use the nf->direction to choice which we need check?

Thanks
Zhang Chen

> Thanks
>
> > +        } else {
> > +            return 0;
> > +        }
> > +
> > +        len = htonl(vnet_hdr_len);
> > +        ret = qemu_chr_fe_write_all(&s->chr_out, (uint8_t *)&len, 
> > sizeof(len));
> > +        if (ret != sizeof(len)) {
> > +            goto err;
> > +        }
> > +    }
> > +
> >       buf = g_malloc(size);
> >       iov_to_buf(iov, iovcnt, 0, buf, size);
> > -    ret = qemu_chr_fe_write_all(chr_out, (uint8_t *)buf, size);
> > +    ret = qemu_chr_fe_write_all(&s->chr_out, (uint8_t *)buf, size);
> >       g_free(buf);
> >       if (ret != size) {
> >           goto err;
> > @@ -141,7 +171,7 @@ static ssize_t 
> > filter_mirror_receive_iov(NetFilterState *nf,
> >       MirrorState *s = FILTER_MIRROR(nf);
> >       int ret;
> >   -    ret = filter_mirror_send(&s->chr_out, iov, iovcnt);
> > +    ret = filter_mirror_send(s, iov, iovcnt);
> >       if (ret) {
> >           error_report("filter_mirror_send failed(%s)", strerror(-ret));
> >       }
> > @@ -164,7 +194,7 @@ static ssize_t 
> > filter_redirector_receive_iov(NetFilterState *nf,
> >       int ret;
> >         if (qemu_chr_fe_get_driver(&s->chr_out)) {
> > -        ret = filter_mirror_send(&s->chr_out, iov, iovcnt);
> > +        ret = filter_mirror_send(s, iov, iovcnt);
> >           if (ret) {
> >               error_report("filter_mirror_send failed(%s)", 
> > strerror(-ret));
> >           }
> > @@ -308,6 +338,13 @@ static char *filter_mirror_get_outdev(Object 
> > *obj, Error **errp)
> >       return g_strdup(s->outdev);
> >   }
> >   +static char *filter_mirror_get_vnet_hdr(Object *obj, Error **errp)
> > +{
> > +    MirrorState *s = FILTER_MIRROR(obj);
> > +
> > +    return s->vnet_hdr ? g_strdup("on") : g_strdup("off");
> > +}
> > +
> >   static void
> >   filter_mirror_set_outdev(Object *obj, const char *value, Error **errp)
> >   {
> > @@ -322,6 +359,21 @@ filter_mirror_set_outdev(Object *obj, const char 
> > *value, Error **errp)
> >       }
> >   }
> >   +static void filter_mirror_set_vnet_hdr(Object *obj,
> > +                                       const char *value,
> > +                                       Error **errp)
> > +{
> > +    MirrorState *s = FILTER_MIRROR(obj);
> > +
> > +    if (strcmp(value, "on") && strcmp(value, "off")) {
> > +        error_setg(errp, "Invalid value for filter-mirror vnet_hdr, "
> > +                         "should be 'on' or 'off'");
> > +        return;
> > +    }
> > +
> > +    s->vnet_hdr = !strcmp(value, "on");
> > +}
> > +
> >   static char *filter_redirector_get_outdev(Object *obj, Error **errp)
> >   {
> >       MirrorState *s = FILTER_REDIRECTOR(obj);
> > @@ -340,8 +392,19 @@ filter_redirector_set_outdev(Object *obj, const 
> > char *value, Error **errp)
> >     static void filter_mirror_init(Object *obj)
> >   {
> > +    MirrorState *s = FILTER_MIRROR(obj);
> > +
> >       object_property_add_str(obj, "outdev", filter_mirror_get_outdev,
> >                               filter_mirror_set_outdev, NULL);
> > +
> > +    /*
> > +     * The vnet_hdr is disabled by default, if you want to enable
> > +     * this option, you must enable all the option on related modules
> > +     * (like other filter or colo-compare).
> > +     */
> > +    s->vnet_hdr = false;
> > +    object_property_add_str(obj, "vnet_hdr", 
> > filter_mirror_get_vnet_hdr,
> > +                             filter_mirror_set_vnet_hdr, NULL);
> >   }
> >     static void filter_redirector_init(Object *obj)
> > diff --git a/qemu-options.hx b/qemu-options.hx
> > index 70c0ded..81fb96b 100644
> > --- a/qemu-options.hx
> > +++ b/qemu-options.hx
> > @@ -4024,10 +4024,9 @@ queue @var{all|rx|tx} is an option that can be 
> > applied to any netfilter.
> >   @option{tx}: the filter is attached to the transmit queue of the 
> > netdev,
> >                where it will receive packets sent by the netdev.
> >   address@hidden -object 
> > filter-mirror,address@hidden,address@hidden,address@hidden,address@hidden|rx|tx}]
> > address@hidden -object 
> > filter-mirror,address@hidden,address@hidden,address@hidden,address@hidden|off}[,address@hidden|rx|tx}]
> >   -filter-mirror on netdev @var{netdevid},mirror net packet to chardev
> > address@hidden
> > +filter-mirror on netdev @var{netdevid},mirror net packet to 
> > address@hidden, if vnet_hdr = on, filter-mirror will mirror 
> > packet with vnet_hdr_len.
> >     @item -object 
> > filter-redirector,address@hidden,address@hidden,address@hidden,
> >   address@hidden,address@hidden|rx|tx}]
>
>
>
> .

--
Thanks
Zhang Chen