Re: [Qemu-devel] [PATCH V3 2/3] net/filter-mirror:Add filter-redirector func

[Li Zhijian <lizhijian@cn.fujitsu.com>]

On 03/07/2016 05:09 PM, Jason Wang wrote:
>
>
> On 03/07/2016 04:26 PM, Li Zhijian wrote:
>>
>>
>> On 03/07/2016 03:56 PM, Jason Wang wrote:
>>>
>>>
>>> On 03/04/2016 08:01 PM, Zhang Chen wrote:
>>>> Filter-redirector is a netfilter plugin.
>>>> It gives qemu the ability to redirect net packet.
>>>> redirector can redirect filter's net packet to outdev.
>>>> and redirect indev's packet to filter.
>>>>
>>>>                         filter
>>>>                           +
>>>>                           |
>>>>                           |
>>>>               redirector  |
>>>>                  +--------------+
>>>>                  |        |     |
>>>>                  |        |     |
>>>>                  |        |     |
>>>>     indev +-----------+   +---------->  outdev
>>>>                  |    |         |
>>>>                  |    |         |
>>>>                  |    |         |
>>>>                  +--------------+
>>>>                       |
>>>>                       |
>>>>                       v
>>>>                     filter
>>>>
>>>> usage:
>>>>
>>>> -netdev user,id=hn0
>>>> -chardev socket,id=s0,host=ip_primary,port=X,server,nowait
>>>> -chardev socket,id=s1,host=ip_primary,port=Y,server,nowait
>>>> -filter-redirector,id=r0,netdev=hn0,queue=tx/rx/all,indev=s0,outdev=s1
>>>>
>>>> Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
>>>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>>>> ---
>>>>    net/filter-mirror.c | 211
>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>    qemu-options.hx     |   8 ++
>>>>    vl.c                |   3 +-
>>>>    3 files changed, 221 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/net/filter-mirror.c b/net/filter-mirror.c
>>>> index 4ff7619..d137168 100644
>>>> --- a/net/filter-mirror.c
>>>> +++ b/net/filter-mirror.c
>>>> @@ -25,11 +25,19 @@
>>>>    #define FILTER_MIRROR(obj) \
>>>>        OBJECT_CHECK(MirrorState, (obj), TYPE_FILTER_MIRROR)
>>>>
>>>> +#define FILTER_REDIRECTOR(obj) \
>>>> +    OBJECT_CHECK(MirrorState, (obj), TYPE_FILTER_REDIRECTOR)
>>>> +
>>>>    #define TYPE_FILTER_MIRROR "filter-mirror"
>>>> +#define TYPE_FILTER_REDIRECTOR "filter-redirector"
>>>> +#define REDIRECT_HEADER_LEN sizeof(uint32_t)
>>>>
>>>>    typedef struct MirrorState {
>>>>        NetFilterState parent_obj;
>>>> +    NetQueue *incoming_queue;
>>>> +    char *indev;
>>>>        char *outdev;
>>>> +    CharDriverState *chr_in;
>>>>        CharDriverState *chr_out;
>>>>    } MirrorState;
>>>>
>>>> @@ -67,6 +75,68 @@ err:
>>>>        return ret < 0 ? ret : -EIO;
>>>>    }
>>>>
>>>> +static int redirector_chr_can_read(void *opaque)
>>>> +{
>>>> +    return REDIRECT_HEADER_LEN;
>>>> +}
>>>> +
>>>> +static void redirector_chr_read(void *opaque, const uint8_t *buf,
>>>> int size)
>>>> +{
>>>> +    NetFilterState *nf = opaque;
>>>> +    MirrorState *s = FILTER_REDIRECTOR(nf);
>>>> +    uint32_t len;
>>>> +    int ret = 0;
>>>> +    uint8_t *recv_buf;
>>>> +
>>>> +    memcpy(&len, buf, size);
>>>
>>> stack overflow if size > sizeof(len)?
>> IIUC, it seems never happend because the 'size' will never greater
>> than the return value of
>> redirector_chr_can_read() which will always return REDIRECT_HEADER_LEN ?
>
> Right, so it's safe.
>
>>
>>
>>>
>>>> +    if (size < REDIRECT_HEADER_LEN) {
>>>> +        ret = qemu_chr_fe_read_all(s->chr_in, ((uint8_t *)&len) +
>>>> size,
>>>> +                REDIRECT_HEADER_LEN - size);
>>>
>>> There maybe some misunderstanding for my previous reply. You can have a
>>> look at net_socket_send() for reference. You could
>>>
>>> - use a buffer for storing len
>>> - each time when you receive partial len, store them in the buffer and
>>> advance the pointer until you receive at least sizeof(len) bytes.
>> qemu_chr_fe_read_all() seem have done this work.
>
> Not the same. qemu_chr_fe_read_all() will do loop reading and usleep
> which is suboptimal. My proposal does not have this issue. It will make
> redirector_chr_read() can handle arbitrary length of data and won't do
> any busy reading.
>
>> Do you mean that
>> we implement a similar code to do that instead of qemu_chr_fe_read_all()
>
> Nope, if you have a look at net_socket_send() it won't do any usleep and
> loop reading, it will return immediately when it does not get sufficient
> data. But it's really your call, not a must but worth to be optimized on
> top in the future.
>
Thanks for you explain.
Is it just like bellow code(not completed) ?

#define REDIRECTOR_MAX_LEN NET_BUFSIZE

typedef struct MirrorState {
      NetFilterState parent_obj;
      char *indev;
      char *outdev;
      CharDriverState *chr_in;
      CharDriverState *chr_out;
+    int state; /* 0 = getting length, 1 = getting data */
+    unsigned int index;
+    unsigned int packet_len;
+    uint8_t buf[REDIRECTOR_MAX_LEN];
  } MirrorState;


static int redirector_chr_can_read(void *opaque)
{
     return REDIRECTOR_MAX_LEN;
}

static void redirector_chr_read(void *opaque, const uint8_t *buf, int size)
{
     NetFilterState *nf = opaque;
     MirrorState *s = FILTER_REDIRECTOR(nf);
     unsigned int l;

     while (size > 0) {
         /* reassemble a packet from the network */
         switch (s->state) {
         case 0:
             l = 4 - s->index;
             if (l > size) {
                 l = size;
             }
             memcpy(s->buf + s->index, buf, l);
             buf += l;
             size -= l;
             s->index += l;
             if (s->index == 4) {
                 /* got length */
                 s->packet_len = ntohl(*(uint32_t *)s->buf);
                 s->index = 0;
                 s->state = 1;
             }
             break;
         case 1:
             l = s->packet_len - s->index;
             if (l > size) {
                 l = size;
             }
             if (s->index + l <= sizeof(s->buf)) {
                 memcpy(s->buf + s->index, buf, l);
             } else {
                 fprintf(stderr, "serious error: oversized packet received,"
                     "connection terminated.\n");
                 s->state = 0;
                 /* TODO: do something
                  */
             }

             s->index += l;
             buf += l;
             size -= l;
             if (s->index >= s->packet_len) {
                 s->index = 0;
                 s->state = 0;
                 /*
                  * TODO: pass packet to next filter
                   */
             }
             break;
         }
     }
}

Thanks
Li Zhijian