| Message ID | 1482400387-18231-1-git-send-email-zhangchen.fnst@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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. > > > . >
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 > > >
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 >> >> >> > > . >
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.
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; }
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(-)