From: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
To: Jason Wang <jasowang@redhat.com>, qemu devel <qemu-devel@nongnu.org>
Cc: Li Zhijian <lizhijian@cn.fujitsu.com>,
Wen Congyang <wency@cn.fujitsu.com>,
"eddie . dong" <eddie.dong@intel.com>,
"Dr . David Alan Gilbert" <dgilbert@redhat.com>
Subject: Re: [Qemu-devel] [PATCH] net/net: Add ReadState for reuse codes
Date: Wed, 11 May 2016 19:20:23 +0800 [thread overview]
Message-ID: <57331577.2090206@cn.fujitsu.com> (raw)
In-Reply-To: <5732F4FF.5060206@redhat.com>
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.
>
>> ---
>> 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));
}
>
>> 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);
}
}
>
>> }
>> }
>> }
>> @@ -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
next prev parent reply other threads:[~2016-05-11 11:20 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 [this message]
2016-05-12 1:11 ` Jason Wang
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=57331577.2090206@cn.fujitsu.com \
--to=zhangchen.fnst@cn.fujitsu.com \
--cc=dgilbert@redhat.com \
--cc=eddie.dong@intel.com \
--cc=jasowang@redhat.com \
--cc=lizhijian@cn.fujitsu.com \
--cc=qemu-devel@nongnu.org \
--cc=wency@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).