Re: [Qemu-devel] [PATCH 08/14] nbd/client: Refactor nbd_receive_list()

[Vladimir Sementsov-Ogievskiy]

01.12.2018 1:03, Eric Blake wrote:
> Add some parameters to make this function reusable in upcoming
> export listing, where we will want to capture the name and
> description rather than compare against a user-supplied name.
> No change in semantics to the existing caller.
> 
> Signed-off-by: Eric Blake <address@hidden>
> ---
>   nbd/client.c | 66 +++++++++++++++++++++++++++++++++++++---------------
>   1 file changed, 47 insertions(+), 19 deletions(-)
> 
> diff --git a/nbd/client.c b/nbd/client.c
> index 1dc8f83e19a..27785c55d0a 100644
> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -232,18 +232,21 @@ static int nbd_handle_reply_err(QIOChannel *ioc, 
> NBDOptionReply *reply,
>       return result;
>   }
> 
> -/* Process another portion of the NBD_OPT_LIST reply.  Set address@hidden if
> - * the current reply matches @want or if the server does not support
> - * NBD_OPT_LIST, otherwise leave @match alone.  Return 0 if iteration
> - * is complete, positive if more replies are expected, or negative
> - * with @errp set if an unrecoverable error occurred. */
> +/* Process another portion of the NBD_OPT_LIST reply.  If @want, then
> + * set address@hidden if the current reply matches @want or if the server
> + * does not support NBD_OPT_LIST, otherwise leave @match alone.
> + * Otherwise, @nameout and @description are malloc'd to contain

what this "otherwise" referred to?

upd: aha, after rereading, I understood that it relates to the very first
If, and the whole thing became clear. Hmm, I'm okay with this now, but it
may be still worth some simplifying.

> + * NUL-terminated copies of the reply.

You didn't say anything about @match in "Otherwise" branch

   Return 0 if iteration is

however, "iterations is complete" may differ for @want/address@hidden cases

> + * complete, positive if more replies are expected, or negative with
> + * @errp set if an unrecoverable error occurred. */
>   static int nbd_receive_list(QIOChannel *ioc, const char *want, bool *match,
> -                            Error **errp)
> +                            char **nameout, char **description, Error **errp)
>   {
>       NBDOptionReply reply;
>       uint32_t len;
>       uint32_t namelen;
> -    char name[NBD_MAX_NAME_SIZE + 1];
> +    char array[NBD_MAX_NAME_SIZE + 1];
> +    char *name = array;
>       int error;
> 
>       if (nbd_receive_option_reply(ioc, NBD_OPT_LIST, &reply, errp) < 0) {
> @@ -253,7 +256,12 @@ static int nbd_receive_list(QIOChannel *ioc, const char 
> *want, bool *match,
>       if (error <= 0) {
>           /* The server did not support NBD_OPT_LIST, so set *match on
>            * the assumption that any name will be accepted.  */
> -        *match = true;
> +        if (want) {
> +            *match = true;
> +        } else if (!error) {
> +            error_setg(errp, "Server does not support export lists");
> +            error = -1;
> +        }
>           return error;
>       }
>       len = reply.length;
> @@ -290,30 +298,49 @@ static int nbd_receive_list(QIOChannel *ioc, const char 
> *want, bool *match,
>           nbd_send_opt_abort(ioc);
>           return -1;
>       }
> -    if (namelen != strlen(want)) {
> -        if (nbd_drop(ioc, len, errp) < 0) {
> -            error_prepend(errp,
> -                          "failed to skip export name with wrong length: ");
> -            nbd_send_opt_abort(ioc);
> -            return -1;
> +    if (want) {
> +        if (namelen != strlen(want)) {
> +            if (nbd_drop(ioc, len, errp) < 0) {
> +                error_prepend(errp,
> +                              "failed to skip export name with wrong length: 
> ");
> +                nbd_send_opt_abort(ioc);
> +                return -1;
> +            }
> +            return 1;
>           }
> -        return 1;
> +        assert(namelen < sizeof(array));
> +    } else {
> +        assert(nameout);

this assert looks a bit redundant, if nameout is 0, next line will abort not 
less clearly

> +        *nameout = name = g_new(char, namelen + 1);

We should check namelen <= NBD_MAX_NAME_SIZE before it, I think.

>       }
> 
> -    assert(namelen < sizeof(name));
>       if (nbd_read(ioc, name, namelen, errp) < 0) {
>           error_prepend(errp, "failed to read export name: ");
>           nbd_send_opt_abort(ioc);
> +        if (!want) {
> +            free(name);

g_free

> +        }
>           return -1;
>       }
>       name[namelen] = '\0';
>       len -= namelen;
> -    if (nbd_drop(ioc, len, errp) < 0) {
> +    if (!want) {
> +        assert(description);

NBD_MAX_NAME_SIZE

> +        *description = g_new(char, len + 1);
> +        if (nbd_read(ioc, *description, len, errp) < 0) {
> +            error_prepend(errp, "failed to read export description: ");
> +            nbd_send_opt_abort(ioc);
> +            free(name);
> +            free(*description);

g_free

> +            return -1;
> +        }
> +        (*description)[len] = '\0';
> +    } else if (nbd_drop(ioc, len, errp) < 0) {
>           error_prepend(errp, "failed to read export description: ");
>           nbd_send_opt_abort(ioc);
>           return -1;
>       }
> -    if (!strcmp(name, want)) {
> +    if (want && !strcmp(name, want)) {
>           *match = true;
>       }
>       return 1;

one more thing: on fail path, you finally fill output name and description
with freed pointers. I'd prefer to keep them unchanged in this case, however,
it's a matter of taste.


> @@ -498,7 +525,8 @@ static int nbd_receive_query_exports(QIOChannel *ioc,
>       }
> 
>       while (1) {
> -        int ret = nbd_receive_list(ioc, wantname, &foundExport, errp);
> +        int ret = nbd_receive_list(ioc, wantname, &foundExport,
> +                                   NULL, NULL, errp);
> 
>           if (ret < 0) {
>               /* Server gave unexpected reply */
> 


-- 
Best regards,
Vladimir