Re: [Xen-devel] [RFC v2 4/9] libxl_qmp: Move the buffer realloc to the same scope level as read
On Mon, Apr 23, 2018 at 03:50:27PM +0100, Anthony PERARD wrote: > On Mon, Apr 23, 2018 at 10:03:39AM +0100, Wei Liu wrote: > > On Mon, Apr 16, 2018 at 06:32:22PM +0100, Anthony PERARD wrote: > > > In qmp_next(), the inner loop should only try to parse messages from > > > QMP, if there is more than one. > > > > > > The handling of the receive buffer ('incomplete'), should be done at the > > > same scope level as read(). It doesn't need to be handle more that once > > > after a read. > > > > > > > In general I agree this is a better idea than the current code. > > > > > Before this patch, when on message what handled, the inner loop would > > > > Sorry, I failed to parse "when on message what handled". > > I probably wanted to write: "when one message was handled". Is this > better? Yes. > > > > restart by adding the 'buffer' into 'incomplete' (after reallocation). > > > Since 'rd' was not reset, the buffer would be strcat a second time. > > > After that, the stream from the QMP server would have syntax error, and > > > the parsor would throw errors. > > > > > > This is unlikely to happen as the receive buffer is very large. And > > > receiving two messages in a row is unlikely. In the current case, this > > > could be an event and a response to a command. > > > > > > Signed-off-by: Anthony PERARD > > > --- > > > tools/libxl/libxl_qmp.c | 31 --- > > > 1 file changed, 16 insertions(+), 15 deletions(-) > > > > > > diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c > > > index 4d207c3842..a25f445fb6 100644 > > > --- a/tools/libxl/libxl_qmp.c > > > +++ b/tools/libxl/libxl_qmp.c > > > @@ -524,23 +524,24 @@ static int qmp_next(libxl__gc *gc, > > > libxl__qmp_handler *qmp) > > > > > > DEBUG_REPORT_RECEIVED(qmp->domid, qmp->buffer, (int)rd); > > > > > > +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; > > > > This can be dropped, because s is not changed. It is just the reversal > > of what is a few lines above. > > No, realloc may change the location of the allocated memory. So, between > "current_pos = s - incomplete" and "s = incomplete + current_pos", the > value of 'incomplete' may have changed. Indeed, I missed that somehow. Wei. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [RFC v2 4/9] libxl_qmp: Move the buffer realloc to the same scope level as read
On Mon, Apr 23, 2018 at 10:03:39AM +0100, Wei Liu wrote: > On Mon, Apr 16, 2018 at 06:32:22PM +0100, Anthony PERARD wrote: > > In qmp_next(), the inner loop should only try to parse messages from > > QMP, if there is more than one. > > > > The handling of the receive buffer ('incomplete'), should be done at the > > same scope level as read(). It doesn't need to be handle more that once > > after a read. > > > > In general I agree this is a better idea than the current code. > > > Before this patch, when on message what handled, the inner loop would > > Sorry, I failed to parse "when on message what handled". I probably wanted to write: "when one message was handled". Is this better? > > restart by adding the 'buffer' into 'incomplete' (after reallocation). > > Since 'rd' was not reset, the buffer would be strcat a second time. > > After that, the stream from the QMP server would have syntax error, and > > the parsor would throw errors. > > > > This is unlikely to happen as the receive buffer is very large. And > > receiving two messages in a row is unlikely. In the current case, this > > could be an event and a response to a command. > > > > Signed-off-by: Anthony PERARD > > --- > > tools/libxl/libxl_qmp.c | 31 --- > > 1 file changed, 16 insertions(+), 15 deletions(-) > > > > diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c > > index 4d207c3842..a25f445fb6 100644 > > --- a/tools/libxl/libxl_qmp.c > > +++ b/tools/libxl/libxl_qmp.c > > @@ -524,23 +524,24 @@ static int qmp_next(libxl__gc *gc, libxl__qmp_handler > > *qmp) > > > > DEBUG_REPORT_RECEIVED(qmp->domid, qmp->buffer, (int)rd); > > > > +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; > > This can be dropped, because s is not changed. It is just the reversal > of what is a few lines above. No, realloc may change the location of the allocated memory. So, between "current_pos = s - incomplete" and "s = incomplete + current_pos", the value of 'incomplete' may have changed. > > +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; > > This can be dropped. > > And I think we should take this change to change "incomplete" to > something more meaningful, like "qmp_msg_buf". I think that can be done. -- Anthony PERARD ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [RFC v2 4/9] libxl_qmp: Move the buffer realloc to the same scope level as read
On Mon, Apr 16, 2018 at 06:32:22PM +0100, Anthony PERARD wrote: > In qmp_next(), the inner loop should only try to parse messages from > QMP, if there is more than one. > > The handling of the receive buffer ('incomplete'), should be done at the > same scope level as read(). It doesn't need to be handle more that once > after a read. > In general I agree this is a better idea than the current code. > Before this patch, when on message what handled, the inner loop would Sorry, I failed to parse "when on message what handled". > restart by adding the 'buffer' into 'incomplete' (after reallocation). > Since 'rd' was not reset, the buffer would be strcat a second time. > After that, the stream from the QMP server would have syntax error, and > the parsor would throw errors. > > This is unlikely to happen as the receive buffer is very large. And > receiving two messages in a row is unlikely. In the current case, this > could be an event and a response to a command. > > Signed-off-by: Anthony PERARD > --- > tools/libxl/libxl_qmp.c | 31 --- > 1 file changed, 16 insertions(+), 15 deletions(-) > > diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c > index 4d207c3842..a25f445fb6 100644 > --- a/tools/libxl/libxl_qmp.c > +++ b/tools/libxl/libxl_qmp.c > @@ -524,23 +524,24 @@ static int qmp_next(libxl__gc *gc, libxl__qmp_handler > *qmp) > > DEBUG_REPORT_RECEIVED(qmp->domid, qmp->buffer, (int)rd); > > +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; This can be dropped, because s is not changed. It is just the reversal of what is a few lines above. > +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; This can be dropped. And I think we should take this change to change "incomplete" to something more meaningful, like "qmp_msg_buf". > +} > + > 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; > -} > > end = strstr(s, "\r\n"); > if (end) { > -- > Anthony PERARD > ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [RFC v2 4/9] libxl_qmp: Move the buffer realloc to the same scope level as read
In qmp_next(), the inner loop should only try to parse messages from QMP, if there is more than one. The handling of the receive buffer ('incomplete'), should be done at the same scope level as read(). It doesn't need to be handle more that once after a read. Before this patch, when on message what handled, the inner loop would restart by adding the 'buffer' into 'incomplete' (after reallocation). Since 'rd' was not reset, the buffer would be strcat a second time. After that, the stream from the QMP server would have syntax error, and the parsor would throw errors. This is unlikely to happen as the receive buffer is very large. And receiving two messages in a row is unlikely. In the current case, this could be an event and a response to a command. Signed-off-by: Anthony PERARD --- tools/libxl/libxl_qmp.c | 31 --- 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c index 4d207c3842..a25f445fb6 100644 --- a/tools/libxl/libxl_qmp.c +++ b/tools/libxl/libxl_qmp.c @@ -524,23 +524,24 @@ static int qmp_next(libxl__gc *gc, libxl__qmp_handler *qmp) DEBUG_REPORT_RECEIVED(qmp->domid, qmp->buffer, (int)rd); +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; +} + 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; -} end = strstr(s, "\r\n"); if (end) { -- Anthony PERARD ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel