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

[Jason Wang <jasowang@redhat.com>]

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
> Li Zhijian