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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).