* [PATCH] libxl/libxl_qmp.c: Fix code style in qmp_next()
@ 2016-12-22 9:53 Zhang Chen
2016-12-22 10:47 ` Wei Liu
0 siblings, 1 reply; 6+ messages in thread
From: Zhang Chen @ 2016-12-22 9:53 UTC (permalink / raw)
To: Xen devel, Ian Jackson, Wei Liu; +Cc: Zhang Chen
Make select loop more readable.
Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
---
tools/libxl/libxl_qmp.c | 123 ++++++++++++++++++++++++------------------------
1 file changed, 61 insertions(+), 62 deletions(-)
diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index ad22ad4..123a6bf 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -427,79 +427,78 @@ static int qmp_next(libxl__gc *gc, libxl__qmp_handler *qmp)
size_t incomplete_size = 0;
int rc = 0;
- do {
- fd_set rfds;
- int ret = 0;
- struct timeval timeout = {
- .tv_sec = qmp->timeout,
- .tv_usec = 0,
- };
+ fd_set rfds;
+ int ret = 0;
+ struct timeval timeout = {
+ .tv_sec = qmp->timeout,
+ .tv_usec = 0,
+ };
- FD_ZERO(&rfds);
- FD_SET(qmp->qmp_fd, &rfds);
+ FD_ZERO(&rfds);
+ FD_SET(qmp->qmp_fd, &rfds);
+ do {
ret = select(qmp->qmp_fd + 1, &rfds, NULL, NULL, &timeout);
- if (ret == 0) {
- LOGD(ERROR, qmp->domid, "timeout");
- return -1;
- } else if (ret < 0) {
- if (errno == EINTR)
- continue;
- LOGED(ERROR, qmp->domid, "Select error");
- return -1;
- }
+ } while (ret == -1 && errno == EINTR);
- rd = read(qmp->qmp_fd, qmp->buffer, QMP_RECEIVE_BUFFER_SIZE);
- if (rd == 0) {
- LOGD(ERROR, qmp->domid, "Unexpected end of socket");
- return -1;
- } else if (rd < 0) {
- LOGED(ERROR, qmp->domid, "Socket read error");
- return rd;
- }
- qmp->buffer[rd] = '\0';
-
- DEBUG_REPORT_RECEIVED(qmp->domid, qmp->buffer, rd);
-
- do {
- char *end = NULL;
- if (incomplete) {
- size_t current_pos = s - incomplete;
- incomplete = libxl__realloc(gc, incomplete,
- incomplete_size + rd + 1);
- strncat(incomplete + incomplete_size, qmp->buffer, rd);
- s = incomplete + current_pos;
- incomplete_size += rd;
- s_end = incomplete + incomplete_size;
- } else {
- incomplete = libxl__strndup(gc, qmp->buffer, rd);
- incomplete_size = rd;
- s = incomplete;
- s_end = s + rd;
- rd = 0;
- }
+ if (ret == 0) {
+ LOGD(ERROR, qmp->domid, "timeout");
+ return -1;
+ } else if (ret < 0) {
+ LOGED(ERROR, qmp->domid, "Select error");
+ return -1;
+ }
- end = strstr(s, "\r\n");
- if (end) {
- libxl__json_object *o = NULL;
+ rd = read(qmp->qmp_fd, qmp->buffer, QMP_RECEIVE_BUFFER_SIZE);
+ if (rd == 0) {
+ LOGD(ERROR, qmp->domid, "Unexpected end of socket");
+ return -1;
+ } else if (rd < 0) {
+ LOGED(ERROR, qmp->domid, "Socket read error");
+ return rd;
+ }
+ qmp->buffer[rd] = '\0';
+
+ DEBUG_REPORT_RECEIVED(qmp->domid, qmp->buffer, rd);
- *end = '\0';
+ do {
+ char *end = NULL;
+ if (incomplete) {
+ size_t current_pos = s - incomplete;
+ incomplete = libxl__realloc(gc, incomplete,
+ incomplete_size + rd + 1);
+ strncat(incomplete + incomplete_size, qmp->buffer, rd);
+ s = incomplete + current_pos;
+ incomplete_size += rd;
+ s_end = incomplete + incomplete_size;
+ } else {
+ incomplete = libxl__strndup(gc, qmp->buffer, rd);
+ incomplete_size = rd;
+ s = incomplete;
+ s_end = s + rd;
+ rd = 0;
+ }
- o = libxl__json_parse(gc, s);
+ end = strstr(s, "\r\n");
+ if (end) {
+ libxl__json_object *o = NULL;
- if (o) {
- rc = qmp_handle_response(gc, qmp, o);
- } else {
- LOGD(ERROR, qmp->domid, "Parse error of : %s", s);
- return -1;
- }
+ *end = '\0';
- s = end + 2;
+ o = libxl__json_parse(gc, s);
+
+ if (o) {
+ rc = qmp_handle_response(gc, qmp, o);
} else {
- break;
+ LOGD(ERROR, qmp->domid, "Parse error of : %s", s);
+ return -1;
}
- } while (s < s_end);
- } while (s < s_end);
+
+ s = end + 2;
+ } else {
+ break;
+ }
+ } while (s < s_end);
return rc;
}
--
2.7.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] libxl/libxl_qmp.c: Fix code style in qmp_next()
2016-12-22 9:53 [PATCH] libxl/libxl_qmp.c: Fix code style in qmp_next() Zhang Chen
@ 2016-12-22 10:47 ` Wei Liu
2016-12-23 3:16 ` Zhang Chen
2017-01-04 17:56 ` Anthony PERARD
0 siblings, 2 replies; 6+ messages in thread
From: Wei Liu @ 2016-12-22 10:47 UTC (permalink / raw)
To: Zhang Chen; +Cc: Anthony PERARD, Xen devel, Ian Jackson, Wei Liu
Also CC Anthony, who wrote the original code.
On Thu, Dec 22, 2016 at 05:53:07PM +0800, Zhang Chen wrote:
> Make select loop more readable.
The behaviour of this function is changed. The changes are not
necessarily wrong, but we need to have clear commit message for why the
change of behaviour is desirable.
Basically you break a big loop into two disjoint ones. I think the
original code handles short-read correctly while this patch doesn't.
See below.
>
> Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
> ---
> tools/libxl/libxl_qmp.c | 123 ++++++++++++++++++++++++------------------------
> 1 file changed, 61 insertions(+), 62 deletions(-)
>
> diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
> index ad22ad4..123a6bf 100644
> --- a/tools/libxl/libxl_qmp.c
> +++ b/tools/libxl/libxl_qmp.c
> @@ -427,79 +427,78 @@ static int qmp_next(libxl__gc *gc, libxl__qmp_handler *qmp)
> size_t incomplete_size = 0;
> int rc = 0;
>
> - do {
> - fd_set rfds;
> - int ret = 0;
> - struct timeval timeout = {
> - .tv_sec = qmp->timeout,
> - .tv_usec = 0,
> - };
> + fd_set rfds;
> + int ret = 0;
> + struct timeval timeout = {
> + .tv_sec = qmp->timeout,
> + .tv_usec = 0,
> + };
>
> - FD_ZERO(&rfds);
> - FD_SET(qmp->qmp_fd, &rfds);
> + FD_ZERO(&rfds);
> + FD_SET(qmp->qmp_fd, &rfds);
>
> + do {
> ret = select(qmp->qmp_fd + 1, &rfds, NULL, NULL, &timeout);
> - if (ret == 0) {
> - LOGD(ERROR, qmp->domid, "timeout");
> - return -1;
> - } else if (ret < 0) {
> - if (errno == EINTR)
> - continue;
> - LOGED(ERROR, qmp->domid, "Select error");
> - return -1;
> - }
> + } while (ret == -1 && errno == EINTR);
>
A side note.
Select may modify timeout, so I think we need to reset timeout at the
beginning of the loop.
> - rd = read(qmp->qmp_fd, qmp->buffer, QMP_RECEIVE_BUFFER_SIZE);
> - if (rd == 0) {
> - LOGD(ERROR, qmp->domid, "Unexpected end of socket");
> - return -1;
> - } else if (rd < 0) {
> - LOGED(ERROR, qmp->domid, "Socket read error");
> - return rd;
> - }
> - qmp->buffer[rd] = '\0';
> -
> - DEBUG_REPORT_RECEIVED(qmp->domid, qmp->buffer, rd);
> -
> - do {
> - char *end = NULL;
> - if (incomplete) {
> - size_t current_pos = s - incomplete;
> - incomplete = libxl__realloc(gc, incomplete,
> - incomplete_size + rd + 1);
> - strncat(incomplete + incomplete_size, qmp->buffer, rd);
> - s = incomplete + current_pos;
> - incomplete_size += rd;
> - s_end = incomplete + incomplete_size;
> - } else {
> - incomplete = libxl__strndup(gc, qmp->buffer, rd);
> - incomplete_size = rd;
> - s = incomplete;
> - s_end = s + rd;
> - rd = 0;
> - }
> + if (ret == 0) {
> + LOGD(ERROR, qmp->domid, "timeout");
> + return -1;
> + } else if (ret < 0) {
> + LOGED(ERROR, qmp->domid, "Select error");
> + return -1;
> + }
>
> - end = strstr(s, "\r\n");
> - if (end) {
> - libxl__json_object *o = NULL;
> + rd = read(qmp->qmp_fd, qmp->buffer, QMP_RECEIVE_BUFFER_SIZE);
> + if (rd == 0) {
> + LOGD(ERROR, qmp->domid, "Unexpected end of socket");
> + return -1;
> + } else if (rd < 0) {
> + LOGED(ERROR, qmp->domid, "Socket read error");
> + return rd;
> + }
> + qmp->buffer[rd] = '\0';
> +
Here, as I understand it, read can return incomplete message. For
example, when the buffer is not big enough.
And the inner loop in original code handles that by checking if there is
"\r\n". If not, it will read from the socket again.
So I'm afraid this patch is not correct. Please point out if there is
anything I missed.
Wei.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] libxl/libxl_qmp.c: Fix code style in qmp_next()
2016-12-22 10:47 ` Wei Liu
@ 2016-12-23 3:16 ` Zhang Chen
2016-12-23 11:44 ` Wei Liu
2017-01-04 17:56 ` Anthony PERARD
1 sibling, 1 reply; 6+ messages in thread
From: Zhang Chen @ 2016-12-23 3:16 UTC (permalink / raw)
To: Wei Liu; +Cc: Anthony PERARD, Xen devel, Ian Jackson
On 12/22/2016 06:47 PM, Wei Liu wrote:
> Also CC Anthony, who wrote the original code.
>
> On Thu, Dec 22, 2016 at 05:53:07PM +0800, Zhang Chen wrote:
>> Make select loop more readable.
> The behaviour of this function is changed. The changes are not
> necessarily wrong, but we need to have clear commit message for why the
> change of behaviour is desirable.
>
> Basically you break a big loop into two disjoint ones. I think the
> original code handles short-read correctly while this patch doesn't.
>
> See below.
>> Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
>> ---
>> tools/libxl/libxl_qmp.c | 123 ++++++++++++++++++++++++------------------------
>> 1 file changed, 61 insertions(+), 62 deletions(-)
>>
>> diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
>> index ad22ad4..123a6bf 100644
>> --- a/tools/libxl/libxl_qmp.c
>> +++ b/tools/libxl/libxl_qmp.c
>> @@ -427,79 +427,78 @@ static int qmp_next(libxl__gc *gc, libxl__qmp_handler *qmp)
>> size_t incomplete_size = 0;
>> int rc = 0;
>>
>> - do {
>> - fd_set rfds;
>> - int ret = 0;
>> - struct timeval timeout = {
>> - .tv_sec = qmp->timeout,
>> - .tv_usec = 0,
>> - };
>> + fd_set rfds;
>> + int ret = 0;
>> + struct timeval timeout = {
>> + .tv_sec = qmp->timeout,
>> + .tv_usec = 0,
>> + };
>>
>> - FD_ZERO(&rfds);
>> - FD_SET(qmp->qmp_fd, &rfds);
>> + FD_ZERO(&rfds);
>> + FD_SET(qmp->qmp_fd, &rfds);
>>
>> + do {
>> ret = select(qmp->qmp_fd + 1, &rfds, NULL, NULL, &timeout);
>> - if (ret == 0) {
>> - LOGD(ERROR, qmp->domid, "timeout");
>> - return -1;
>> - } else if (ret < 0) {
>> - if (errno == EINTR)
>> - continue;
>> - LOGED(ERROR, qmp->domid, "Select error");
>> - return -1;
>> - }
>> + } while (ret == -1 && errno == EINTR);
>>
> A side note.
>
> Select may modify timeout, so I think we need to reset timeout at the
> beginning of the loop.
OK, I will fix this in next version.
>
>> - rd = read(qmp->qmp_fd, qmp->buffer, QMP_RECEIVE_BUFFER_SIZE);
>> - if (rd == 0) {
>> - LOGD(ERROR, qmp->domid, "Unexpected end of socket");
>> - return -1;
>> - } else if (rd < 0) {
>> - LOGED(ERROR, qmp->domid, "Socket read error");
>> - return rd;
>> - }
>> - qmp->buffer[rd] = '\0';
>> -
>> - DEBUG_REPORT_RECEIVED(qmp->domid, qmp->buffer, rd);
>> -
>> - do {
>> - char *end = NULL;
>> - if (incomplete) {
>> - size_t current_pos = s - incomplete;
>> - incomplete = libxl__realloc(gc, incomplete,
>> - incomplete_size + rd + 1);
>> - strncat(incomplete + incomplete_size, qmp->buffer, rd);
>> - s = incomplete + current_pos;
>> - incomplete_size += rd;
>> - s_end = incomplete + incomplete_size;
>> - } else {
>> - incomplete = libxl__strndup(gc, qmp->buffer, rd);
>> - incomplete_size = rd;
>> - s = incomplete;
>> - s_end = s + rd;
>> - rd = 0;
>> - }
>> + if (ret == 0) {
>> + LOGD(ERROR, qmp->domid, "timeout");
>> + return -1;
>> + } else if (ret < 0) {
>> + LOGED(ERROR, qmp->domid, "Select error");
>> + return -1;
>> + }
>>
>> - end = strstr(s, "\r\n");
>> - if (end) {
>> - libxl__json_object *o = NULL;
>> + rd = read(qmp->qmp_fd, qmp->buffer, QMP_RECEIVE_BUFFER_SIZE);
>> + if (rd == 0) {
>> + LOGD(ERROR, qmp->domid, "Unexpected end of socket");
>> + return -1;
>> + } else if (rd < 0) {
>> + LOGED(ERROR, qmp->domid, "Socket read error");
>> + return rd;
>> + }
>> + qmp->buffer[rd] = '\0';
>> +
> Here, as I understand it, read can return incomplete message. For
> example, when the buffer is not big enough.
>
> And the inner loop in original code handles that by checking if there is
> "\r\n". If not, it will read from the socket again.
>
> So I'm afraid this patch is not correct. Please point out if there is
> anything I missed.
Yes, this patch have some logic error, but I think the code
looks odd like that:
"
} while (s < s_end);
} while (s < s_end);
"
The original code use "break" and "continue" to control the double loop
that make people hard to understand.
So, Can I change the code without "break" and "continue" like this?
"
} while (end);
} while (s < s_end);
"
If yes, I will fix this in next version.
Thanks
Zhang Chen
> Wei.
>
>
> .
>
--
Thanks
Zhang Chen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] libxl/libxl_qmp.c: Fix code style in qmp_next()
2016-12-23 3:16 ` Zhang Chen
@ 2016-12-23 11:44 ` Wei Liu
2016-12-26 6:48 ` Zhang Chen
0 siblings, 1 reply; 6+ messages in thread
From: Wei Liu @ 2016-12-23 11:44 UTC (permalink / raw)
To: Zhang Chen; +Cc: Anthony PERARD, Xen devel, Wei Liu, Ian Jackson
On Fri, Dec 23, 2016 at 11:16:58AM +0800, Zhang Chen wrote:
[...]
> >>+
> >Here, as I understand it, read can return incomplete message. For
> >example, when the buffer is not big enough.
> >
> >And the inner loop in original code handles that by checking if there is
> >"\r\n". If not, it will read from the socket again.
> >
> >So I'm afraid this patch is not correct. Please point out if there is
> >anything I missed.
>
> Yes, this patch have some logic error, but I think the code
> looks odd like that:
> "
> } while (s < s_end);
> } while (s < s_end);
> "
>
> The original code use "break" and "continue" to control the double loop
> that make people hard to understand.
>
> So, Can I change the code without "break" and "continue" like this?
I'm not sure I follow how you would like to change the code.
There is one continue for outer loop and one break for inner loop.
I don't think you can eliminate the continue for outer loop, otherwise
how could you restart the loop?
> "
> } while (end);
> } while (s < s_end);
> "
As for this, I think it is buggy, too. It will cause the inner loop to
loop indefinitely.
Wei.
>
> If yes, I will fix this in next version.
>
> Thanks
> Zhang Chen
>
> >Wei.
> >
> >
> >.
> >
>
> --
> Thanks
> Zhang Chen
>
>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] libxl/libxl_qmp.c: Fix code style in qmp_next()
2016-12-23 11:44 ` Wei Liu
@ 2016-12-26 6:48 ` Zhang Chen
0 siblings, 0 replies; 6+ messages in thread
From: Zhang Chen @ 2016-12-26 6:48 UTC (permalink / raw)
To: Wei Liu; +Cc: Anthony PERARD, Xen devel, Ian Jackson
On 12/23/2016 07:44 PM, Wei Liu wrote:
> On Fri, Dec 23, 2016 at 11:16:58AM +0800, Zhang Chen wrote:
> [...]
>>>> +
>>> Here, as I understand it, read can return incomplete message. For
>>> example, when the buffer is not big enough.
>>>
>>> And the inner loop in original code handles that by checking if there is
>>> "\r\n". If not, it will read from the socket again.
>>>
>>> So I'm afraid this patch is not correct. Please point out if there is
>>> anything I missed.
>> Yes, this patch have some logic error, but I think the code
>> looks odd like that:
>> "
>> } while (s < s_end);
>> } while (s < s_end);
>> "
>>
>> The original code use "break" and "continue" to control the double loop
>> that make people hard to understand.
>>
>> So, Can I change the code without "break" and "continue" like this?
> I'm not sure I follow how you would like to change the code.
>
> There is one continue for outer loop and one break for inner loop.
>
> I don't think you can eliminate the continue for outer loop, otherwise
> how could you restart the loop?
>
>> "
>> } while (end);
>> } while (s < s_end);
>> "
> As for this, I think it is buggy, too. It will cause the inner loop to
> loop indefinitely.
OK, I got your points, I will send next version just about text-indent.
Thanks
Zhang Chen
> Wei.
>
>> If yes, I will fix this in next version.
>>
>> Thanks
>> Zhang Chen
>>
>>> Wei.
>>>
>>>
>>> .
>>>
>> --
>> Thanks
>> Zhang Chen
>>
>>
>>
>
> .
>
--
Thanks
Zhang Chen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] libxl/libxl_qmp.c: Fix code style in qmp_next()
2016-12-22 10:47 ` Wei Liu
2016-12-23 3:16 ` Zhang Chen
@ 2017-01-04 17:56 ` Anthony PERARD
1 sibling, 0 replies; 6+ messages in thread
From: Anthony PERARD @ 2017-01-04 17:56 UTC (permalink / raw)
To: Wei Liu; +Cc: Xen devel, Ian Jackson, Zhang Chen
On Thu, Dec 22, 2016 at 10:47:35AM +0000, Wei Liu wrote:
> On Thu, Dec 22, 2016 at 05:53:07PM +0800, Zhang Chen wrote:
> > diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
> > index ad22ad4..123a6bf 100644
> > --- a/tools/libxl/libxl_qmp.c
> > +++ b/tools/libxl/libxl_qmp.c
> > @@ -427,79 +427,78 @@ static int qmp_next(libxl__gc *gc, libxl__qmp_handler *qmp)
> > size_t incomplete_size = 0;
> > int rc = 0;
> >
> > - do {
> > - fd_set rfds;
> > - int ret = 0;
> > - struct timeval timeout = {
> > - .tv_sec = qmp->timeout,
> > - .tv_usec = 0,
> > - };
> > + fd_set rfds;
> > + int ret = 0;
> > + struct timeval timeout = {
> > + .tv_sec = qmp->timeout,
> > + .tv_usec = 0,
> > + };
> >
> > - FD_ZERO(&rfds);
> > - FD_SET(qmp->qmp_fd, &rfds);
> > + FD_ZERO(&rfds);
> > + FD_SET(qmp->qmp_fd, &rfds);
> >
> > + do {
> > ret = select(qmp->qmp_fd + 1, &rfds, NULL, NULL, &timeout);
> > - if (ret == 0) {
> > - LOGD(ERROR, qmp->domid, "timeout");
> > - return -1;
> > - } else if (ret < 0) {
> > - if (errno == EINTR)
> > - continue;
> > - LOGED(ERROR, qmp->domid, "Select error");
> > - return -1;
> > - }
> > + } while (ret == -1 && errno == EINTR);
> >
>
> A side note.
>
> Select may modify timeout, so I think we need to reset timeout at the
> beginning of the loop.
>
rfds is also modified by select() and needs to be reset before calling
select again.
--
Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-01-04 17:56 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-12-22 9:53 [PATCH] libxl/libxl_qmp.c: Fix code style in qmp_next() Zhang Chen
2016-12-22 10:47 ` Wei Liu
2016-12-23 3:16 ` Zhang Chen
2016-12-23 11:44 ` Wei Liu
2016-12-26 6:48 ` Zhang Chen
2017-01-04 17:56 ` Anthony PERARD
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).