Re: [Qemu-devel] [PATCH V2 1/3] net/filter-traffic: add filter-traffic.h

[Zhang Chen <zhangchen.fnst@cn.fujitsu.com>]

On 03/01/2016 11:02 AM, Li Zhijian wrote:
>
>
> On 02/29/2016 08:23 PM, Zhang Chen wrote:
>> We can reuse filter-traffic by filter-mirror,
>> filter-redirector and so on.
>>
> Is the main purpose to export filter_traffic_send() ?
> IMO, define it as a public API would be better.
>
> Using MirrorState and RedirectorState are clearer rather than 
> TrafficState.
>
>

OK, I will fix it in next version.

>> Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>> ---
>>   net/filter-mirror.c  | 70 
>> +++++++--------------------------------------------
>>   net/filter-traffic.h | 71 
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 80 insertions(+), 61 deletions(-)
>>   create mode 100644 net/filter-traffic.h
>>
>> diff --git a/net/filter-mirror.c b/net/filter-mirror.c
>> index ee13d94..d9698f2 100644
>> --- a/net/filter-mirror.c
>> +++ b/net/filter-mirror.c
>> @@ -9,65 +9,13 @@
>>    * later.  See the COPYING file in the top-level directory.
>>    */
>>
>> -#include "net/filter.h"
>> -#include "net/net.h"
>> -#include "qemu-common.h"
>> -#include "qapi/qmp/qerror.h"
>> -#include "qapi-visit.h"
>> -#include "qom/object.h"
>> -#include "qemu/main-loop.h"
>> -#include "qemu/error-report.h"
>> -#include "trace.h"
>> -#include "sysemu/char.h"
>> -#include "qemu/iov.h"
>> -#include "qemu/sockets.h"
>> +#include "net/filter-traffic.h"
>>
>>   #define FILTER_MIRROR(obj) \
>> -    OBJECT_CHECK(MirrorState, (obj), TYPE_FILTER_MIRROR)
>> +    OBJECT_CHECK(TrafficState, (obj), TYPE_FILTER_MIRROR)
>>
>>   #define TYPE_FILTER_MIRROR "filter-mirror"
>>
>> -typedef struct MirrorState {
>> -    NetFilterState parent_obj;
>> -    char *outdev;
>> -    CharDriverState *chr_out;
>> -} MirrorState;
>> -
>> -static int filter_mirror_send(NetFilterState *nf,
>> -                                   const struct iovec *iov,
>> -                                   int iovcnt)
>> -{
>> -    MirrorState *s = FILTER_MIRROR(nf);
>> -    int ret = 0;
>> -    ssize_t size = 0;
>> -    uint32_t len =  0;
>> -    char *buf;
>> -
>> -    size = iov_size(iov, iovcnt);
>> -    if (!size) {
>> -        return 0;
>> -    }
>> -
>> -    len = htonl(size);
>> -    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(s->chr_out, (uint8_t *)buf, size);
>> -    g_free(buf);
>> -    if (ret != size) {
>> -        goto err;
>> -    }
>> -
>> -    return 0;
>> -
>> -err:
>> -    return ret < 0 ? ret : -EIO;
>> -}
>> -
>>   static ssize_t filter_mirror_receive_iov(NetFilterState *nf,
>>                                            NetClientState *sender,
>>                                            unsigned flags,
>> @@ -77,7 +25,7 @@ static ssize_t 
>> filter_mirror_receive_iov(NetFilterState *nf,
>>   {
>>       int ret;
>>
>> -    ret = filter_mirror_send(nf, iov, iovcnt);
>> +    ret = filter_traffic_send(nf, iov, iovcnt);
>>       if (ret) {
>>           error_report("filter_mirror_send failed(%s)", strerror(-ret));
>>       }
>> @@ -91,7 +39,7 @@ static ssize_t 
>> filter_mirror_receive_iov(NetFilterState *nf,
>>
>>   static void filter_mirror_cleanup(NetFilterState *nf)
>>   {
>> -    MirrorState *s = FILTER_MIRROR(nf);
>> +    TrafficState *s = FILTER_MIRROR(nf);
>>
>>       if (s->chr_out) {
>>           qemu_chr_fe_release(s->chr_out);
>> @@ -100,7 +48,7 @@ static void filter_mirror_cleanup(NetFilterState *nf)
>>
>>   static void filter_mirror_setup(NetFilterState *nf, Error **errp)
>>   {
>> -    MirrorState *s = FILTER_MIRROR(nf);
>> +    TrafficState *s = FILTER_MIRROR(nf);
>>
>>       if (!s->outdev) {
>>           error_setg(errp, "filter filter mirror needs 'outdev' "
>> @@ -132,7 +80,7 @@ static void filter_mirror_class_init(ObjectClass 
>> *oc, void *data)
>>
>>   static char *filter_mirror_get_outdev(Object *obj, Error **errp)
>>   {
>> -    MirrorState *s = FILTER_MIRROR(obj);
>> +    TrafficState *s = FILTER_MIRROR(obj);
>>
>>       return g_strdup(s->outdev);
>>   }
>> @@ -140,7 +88,7 @@ static char *filter_mirror_get_outdev(Object *obj, 
>> Error **errp)
>>   static void
>>   filter_mirror_set_outdev(Object *obj, const char *value, Error **errp)
>>   {
>> -    MirrorState *s = FILTER_MIRROR(obj);
>> +    TrafficState *s = FILTER_MIRROR(obj);
>>
>>       g_free(s->outdev);
>>       s->outdev = g_strdup(value);
>> @@ -159,7 +107,7 @@ static void filter_mirror_init(Object *obj)
>>
>>   static void filter_mirror_fini(Object *obj)
>>   {
>> -    MirrorState *s = FILTER_MIRROR(obj);
>> +    TrafficState *s = FILTER_MIRROR(obj);
>>
>>       g_free(s->outdev);
>>   }
>> @@ -170,7 +118,7 @@ static const TypeInfo filter_mirror_info = {
>>       .class_init = filter_mirror_class_init,
>>       .instance_init = filter_mirror_init,
>>       .instance_finalize = filter_mirror_fini,
>> -    .instance_size = sizeof(MirrorState),
>> +    .instance_size = sizeof(TrafficState),
>>   };
>>
>>   static void register_types(void)
>> diff --git a/net/filter-traffic.h b/net/filter-traffic.h
>> new file mode 100644
>> index 0000000..24cb5c3
>> --- /dev/null
>> +++ b/net/filter-traffic.h
>> @@ -0,0 +1,71 @@
>> +/*
>> + * Copyright (c) 2016 HUAWEI TECHNOLOGIES CO., LTD.
>> + * Copyright (c) 2016 FUJITSU LIMITED
>> + * Copyright (c) 2016 Intel Corporation
>> + *
>> + * Author: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or
>> + * later.  See the COPYING file in the top-level directory.
>> + */
>> +
>> +#include "net/filter.h"
>> +#include "net/net.h"
>> +#include "qemu-common.h"
>> +#include "qapi/qmp/qerror.h"
>> +#include "qapi-visit.h"
>> +#include "qom/object.h"
>> +#include "qemu/main-loop.h"
>> +#include "qemu/error-report.h"
>> +#include "trace.h"
>> +#include "sysemu/char.h"
>> +#include "qemu/iov.h"
>> +#include "qemu/sockets.h"
>> +
>> +#ifndef QEMU_FILTER_MIRROR_H
>> +#define QEMU_FILTER_MIRROR_H
> I prefer to see this is the first line of a C header code.
> s/MIRROR/TRAFFIC/
>
>

Thanks, will fix it.

>> +
>> +typedef struct TrafficState {
>> +    NetFilterState parent_obj;
>> +    NetQueue *incoming_queue;/* guest normal net queue */
>> +    char *indev;
>> +    char *outdev;
>> +    CharDriverState *chr_in;
>> +    CharDriverState *chr_out;
>> +} TrafficState;
>> +
>> +static int filter_traffic_send(NetFilterState *nf,
>> +                                   const struct iovec *iov,
>> +                                   int iovcnt)
>> +{
>> +    TrafficState *s = (TrafficState *)nf;
>
> that's dangerous.
>

will fix.

>
>> +    int ret = 0;
>> +    ssize_t size = 0;
>> +    uint32_t len =  0;
>> +    char *buf;
>> +    size = iov_size(iov, iovcnt);
>> +    if (!size) {
>> +        return 0;
>> +    }
>> +
>> +    len = htonl(size);
>> +    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(s->chr_out, (uint8_t *)buf, size);
>> +    g_free(buf);
>> +    if (ret != size) {
>> +        goto err;
>> +    }
>> +
>> +    return 0;
>> +
>> +err:
>> +    return ret < 0 ? ret : -EIO;
>> +}
>> +
>> +#endif /* QEMU_FILTER_MIRROR_H */
>>
> s/MIRROR/TRAFFIC/
>

will fix

Thanks
zhangchen

> Thanks
> Li Zhijian
> .
>

-- 
Thanks
zhangchen