Re: [Qemu-devel] [PATCH] net/net: Add ReadState for reuse codes

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

On 05/12/2016 09:11 AM, Jason Wang wrote:
>
>
> On 2016年05月11日 19:20, Zhang Chen wrote:
>>
>>
>> On 05/11/2016 05:01 PM, Jason Wang wrote:
>>>
>>>
>>> On 2016年05月06日 18:56, Zhang Chen wrote:
>>>> Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
>>>> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
>>>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>>>
>>> Looks good, just few nits.
>>>
>>> It's better to have a commit log.
>>
>> OK, I will add this log:
>>
>> This function is from net/socket.c, move it to net.c and net.h.
>> Add SocketReadState to make others reuse net_fill_rstate().
>> suggestion from jason.
>
> Looks good. Thanks
>
>>
>>>
>>>> ---
>>>>   include/net/net.h   |  8 ++++++
>>>>   net/filter-mirror.c | 60 
>>>> ++++++++------------------------------------
>>>>   net/net.c           | 56 ++++++++++++++++++++++++++++++++++++++++++
>>>>   net/socket.c        | 71 
>>>> +++++++++++++----------------------------------------
>>>>   4 files changed, 91 insertions(+), 104 deletions(-)
>>>>
>>>> diff --git a/include/net/net.h b/include/net/net.h
>>>> index 73e4c46..1439cf9 100644
>>>> --- a/include/net/net.h
>>>> +++ b/include/net/net.h
>>>> @@ -102,6 +102,14 @@ typedef struct NICState {
>>>>       bool peer_deleted;
>>>>   } NICState;
>>>>   +typedef struct ReadState {
>>>> +    int state; /* 0 = getting length, 1 = getting data */
>>>> +    uint32_t index;
>>>> +    uint32_t packet_len;
>>>> +    uint8_t buf[NET_BUFSIZE];
>>>> +} ReadState;
>>>
>>> I think SocketReadState is better here.
>>>
>>
>> I will rename it in next version.
>>
>>>> +
>>>> +int net_fill_rstate(ReadState *rs, const uint8_t *buf, int size);
>>>>   char *qemu_mac_strdup_printf(const uint8_t *macaddr);
>>>>   NetClientState *qemu_find_netdev(const char *id);
>>>>   int qemu_find_net_clients_except(const char *id, NetClientState 
>>>> **ncs,
>>>> diff --git a/net/filter-mirror.c b/net/filter-mirror.c
>>>> index c0c4dc6..bcec509 100644
>>>> --- a/net/filter-mirror.c
>>>> +++ b/net/filter-mirror.c
>>>> @@ -40,10 +40,7 @@ typedef struct MirrorState {
>>>>       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];
>>>> +    ReadState rs;
>>>>   } MirrorState;
>>>>     static int filter_mirror_send(CharDriverState *chr_out,
>>>> @@ -108,51 +105,14 @@ 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) { /* 0 = getting length, 1 = getting 
>>>> data */
>>>> -        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 {
>>>> -                error_report("serious error: oversized packet 
>>>> received.");
>>>> -                s->index = s->state = 0;
>>>> -                qemu_chr_add_handlers(s->chr_in, NULL, NULL, NULL, 
>>>> NULL);
>>>> -                return;
>>>> -            }
>>>> -
>>>> -            s->index += l;
>>>> -            buf += l;
>>>> -            size -= l;
>>>> -            if (s->index >= s->packet_len) {
>>>> -                s->index = 0;
>>>> -                s->state = 0;
>>>> -                redirector_to_filter(nf, s->buf, s->packet_len);
>>>> -            }
>>>> -            break;
>>>> -        }
>>>> +    int ret;
>>>> +
>>>> +    ret = net_fill_rstate(&s->rs, buf, size);
>>>> +
>>>> +    if (ret == -1) {
>>>> +        qemu_chr_add_handlers(s->chr_in, NULL, NULL, NULL, NULL);
>>>> +    } else if (ret == 1) {
>>>> +        redirector_to_filter(nf, s->rs.buf, s->rs.packet_len);
>>>>       }
>>>>   }
>>>>   @@ -274,7 +234,7 @@ static void 
>>>> filter_redirector_setup(NetFilterState *nf, Error **errp)
>>>>           }
>>>>       }
>>>>   -    s->state = s->index = 0;
>>>> +    s->rs.state = s->rs.index = 0;
>>>>         if (s->indev) {
>>>>           s->chr_in = qemu_chr_find(s->indev);
>>>> diff --git a/net/net.c b/net/net.c
>>>> index 0ad6217..926457e 100644
>>>> --- a/net/net.c
>>>> +++ b/net/net.c
>>>> @@ -1573,3 +1573,59 @@ QemuOptsList qemu_net_opts = {
>>>>           { /* end of list */ }
>>>>       },
>>>>   };
>>>> +
>>>> +/*
>>>> + * Returns
>>>> + * 0: readstate is not ready
>>>> + * 1: readstate is ready
>>>> + * otherwise error occurs
>>>> + */
>>>> +int net_fill_rstate(ReadState *rs, const uint8_t *buf, int size)
>>>> +{
>>>> +    unsigned int l;
>>>> +    while (size > 0) {
>>>> +        /* reassemble a packet from the network */
>>>> +        switch (rs->state) { /* 0 = getting length, 1 = getting 
>>>> data */
>>>> +        case 0:
>>>> +            l = 4 - rs->index;
>>>> +            if (l > size) {
>>>> +                l = size;
>>>> +            }
>>>> +            memcpy(rs->buf + rs->index, buf, l);
>>>> +            buf += l;
>>>> +            size -= l;
>>>> +            rs->index += l;
>>>> +            if (rs->index == 4) {
>>>> +                /* got length */
>>>> +                rs->packet_len = ntohl(*(uint32_t *)rs->buf);
>>>> +                rs->index = 0;
>>>> +                rs->state = 1;
>>>> +            }
>>>> +            break;
>>>> +        case 1:
>>>> +            l = rs->packet_len - rs->index;
>>>> +            if (l > size) {
>>>> +                l = size;
>>>> +            }
>>>> +            if (rs->index + l <= sizeof(rs->buf)) {
>>>> +                memcpy(rs->buf + rs->index, buf, l);
>>>> +            } else {
>>>> +                fprintf(stderr, "serious error: oversized packet 
>>>> received,"
>>>> +                    "connection terminated.\n");
>>>> +                rs->index = rs->state = 0;
>>>> +                return -1;
>>>> +            }
>>>> +
>>>> +            rs->index += l;
>>>> +            buf += l;
>>>> +            size -= l;
>>>> +            if (rs->index >= rs->packet_len) {
>>>> +                rs->index = 0;
>>>> +                rs->state = 0;
>>>> +                return 1;
>>>> +            }
>>>> +            break;
>>>> +        }
>>>> +    }
>>>> +    return 0;
>>>> +}
>>>> diff --git a/net/socket.c b/net/socket.c
>>>> index 9fa2cd8..9ecdd3b 100644
>>>> --- a/net/socket.c
>>>> +++ b/net/socket.c
>>>> @@ -38,11 +38,8 @@ typedef struct NetSocketState {
>>>>       NetClientState nc;
>>>>       int listen_fd;
>>>>       int fd;
>>>> -    int state; /* 0 = getting length, 1 = getting data */
>>>> -    unsigned int index;
>>>> -    unsigned int packet_len;
>>>> +    ReadState rs;
>>>>       unsigned int send_index;      /* number of bytes sent (only 
>>>> SOCK_STREAM) */
>>>> -    uint8_t buf[NET_BUFSIZE];
>>>>       struct sockaddr_in dgram_dst; /* contains inet host and port 
>>>> destination iff connectionless (SOCK_DGRAM) */
>>>>       IOHandler *send_fn;           /* differs between 
>>>> SOCK_STREAM/SOCK_DGRAM */
>>>>       bool read_poll;               /* waiting to receive data? */
>>>> @@ -147,7 +144,7 @@ static void net_socket_send(void *opaque)
>>>>   {
>>>>       NetSocketState *s = opaque;
>>>>       int size;
>>>> -    unsigned l;
>>>> +    int ret;
>>>>       uint8_t buf1[NET_BUFSIZE];
>>>>       const uint8_t *buf;
>>>>   @@ -166,60 +163,26 @@ static void net_socket_send(void *opaque)
>>>>           closesocket(s->fd);
>>>>             s->fd = -1;
>>>> -        s->state = 0;
>>>> -        s->index = 0;
>>>> -        s->packet_len = 0;
>>>> +        s->rs.state = 0;
>>>> +        s->rs.index = 0;
>>>> +        s->rs.packet_len = 0;
>>>>           s->nc.link_down = true;
>>>> -        memset(s->buf, 0, sizeof(s->buf));
>>>> +        memset(s->rs.buf, 0, sizeof(s->rs.buf));
>>>
>>> How about introduce a helper to do the initialization?
>>
>> Do you mean
>>
>> remove
>> +        s->rs.state = 0;
>> +        s->rs.index = 0;
>> +        s->rs.packet_len = 0;
>> +        memset(s->rs.buf, 0, sizeof(s->rs.buf));
>>
>> add
>>            s->rs->cb = xxx_cb;
>>            s->rs->opxx = xxx;
>>
>> void xxx_cb(void *opaque)
>> {
>>     NetSocketState *s = opaque;
>>
>>     s->rs.state = 0;
>>     s->rs.index = 0;
>>     s->rs.packet_len = 0;
>>     memset(s->rs.buf, 0, sizeof(s->rs.buf));
>> }
>>
>
> Kind of, and looks like there's no need for opaque, you can just pass 
> pointer to SocketReadState. And another parameter of callbacks.

OK,will fix it.

>
>>
>>>
>>>>           memset(s->nc.info_str, 0, sizeof(s->nc.info_str));
>>>>             return;
>>>>       }
>>>>       buf = buf1;
>>>> -    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;
>>>> -                goto eoc;
>>>> -            }
>>>>   -            s->index += l;
>>>> -            buf += l;
>>>> -            size -= l;
>>>> -            if (s->index >= s->packet_len) {
>>>> -                s->index = 0;
>>>> -                s->state = 0;
>>>> -                if (qemu_send_packet_async(&s->nc, s->buf, 
>>>> s->packet_len,
>>>> - net_socket_send_completed) == 0) {
>>>> -                    net_socket_read_poll(s, false);
>>>> -                    break;
>>>> -                }
>>>> -            }
>>>> -            break;
>>>> +    ret = net_fill_rstate(&s->rs, buf, size);
>>>> +
>>>> +    if (ret == -1) {
>>>> +        goto eoc;
>>>> +    } else if (ret == 1) {
>>>> +        if (qemu_send_packet_async(&s->nc, s->rs.buf,
>>>> +                                   s->rs.packet_len,
>>>> + net_socket_send_completed) == 0) {
>>>> +            net_socket_read_poll(s, false);
>>>
>>> This looks not elegant, maybe we could use callback (which was 
>>> initialized by the helper I mention above) to do this. Any thoughts 
>>> on this?
>>
>> Do you mean:
>>
>> remove
>> +        if (qemu_send_packet_async(&s->nc, s->rs.buf,
>> +                                   s->rs.packet_len,
>> +                                   net_socket_send_completed) == 0) {
>> +            net_socket_read_poll(s, false);
>>
>> add
>>
>> s->rs->done
>>
>> void socket_fill_rsstate_done_cb(SocketReadState *srs, void *opaque)
>> {
>>     NetSocketState *s = opaque;
>>
>>     if (qemu_send_packet_async(&s->nc, srs->buf,
>>                                srs->packet_len,
>>                                net_socket_send_completed) == 0) {
>>         net_socket_read_poll(s, false);
>>     }
>> }
>
> Yes, but there's no need for opaque, we can infer the container by 
> container_of().
>

But in filter-mirror.c we need do this:


void  redirector_fill_rsstate_done_cb(SocketReadState *srs, void *opaque)
{
     NetFilterState *nf = opaque;

     redirector_to_filter(nf, srs->buf, srs->packet_len);
}

so,I think we have to use void *opaque.




>>
>>>
>>>>           }
>>>>       }
>>>>   }
>>>> @@ -229,7 +192,7 @@ static void net_socket_send_dgram(void *opaque)
>>>>       NetSocketState *s = opaque;
>>>>       int size;
>>>>   -    size = qemu_recv(s->fd, s->buf, sizeof(s->buf), 0);
>>>> +    size = qemu_recv(s->fd, s->rs.buf, sizeof(s->rs.buf), 0);
>>>>       if (size < 0)
>>>>           return;
>>>>       if (size == 0) {
>>>> @@ -238,7 +201,7 @@ static void net_socket_send_dgram(void *opaque)
>>>>           net_socket_write_poll(s, false);
>>>>           return;
>>>>       }
>>>> -    if (qemu_send_packet_async(&s->nc, s->buf, size,
>>>> +    if (qemu_send_packet_async(&s->nc, s->rs.buf, size,
>>>>                                  net_socket_send_completed) == 0) {
>>>>           net_socket_read_poll(s, false);
>>>>       }
>>>
>>>
>>>
>>> .
>>>
>>
>
>
>
> .
>

-- 
Thanks
zhangchen