Re: [Xen-devel] [PATCH] libxl/libxl_qmp.c: Fix code style in qmp_next()

[Wei Liu <wei.liu2@xxxxxxxxxx>]

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@xxxxxxxxxxxxxx>
> ---
>  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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel