From: Jason Wang <jasowang@redhat.com>
To: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>,
qemu devel <qemu-devel@nongnu.org>
Cc: "eddie . dong" <eddie.dong@intel.com>,
Li Zhijian <lizhijian@cn.fujitsu.com>,
"Dr . David Alan Gilbert" <dgilbert@redhat.com>
Subject: Re: [Qemu-devel] [PATCH] net/net: Add ReadState for reuse codes
Date: Thu, 12 May 2016 09:11:46 +0800 [thread overview]
Message-ID: <5733D852.9080301@redhat.com> (raw)
In-Reply-To: <57331577.2090206@cn.fujitsu.com>
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.
>
>>
>>> 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().
>
>>
>>> }
>>> }
>>> }
>>> @@ -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);
>>> }
>>
>>
>>
>> .
>>
>
next prev parent reply other threads:[~2016-05-12 1:11 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-05-06 10:56 [Qemu-devel] [PATCH] net/net: Add ReadState for reuse codes Zhang Chen
2016-05-10 2:57 ` Li Zhijian
2016-05-10 5:33 ` Zhang Chen
2016-05-11 7:01 ` Zhang Chen
2016-05-11 9:01 ` Jason Wang
2016-05-11 11:20 ` Zhang Chen
2016-05-12 1:11 ` Jason Wang [this message]
2016-05-12 6:33 ` Zhang Chen
2016-05-12 8:07 ` Jason Wang
2016-05-12 8:23 ` Zhang Chen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5733D852.9080301@redhat.com \
--to=jasowang@redhat.com \
--cc=dgilbert@redhat.com \
--cc=eddie.dong@intel.com \
--cc=lizhijian@cn.fujitsu.com \
--cc=qemu-devel@nongnu.org \
--cc=zhangchen.fnst@cn.fujitsu.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.