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

[Richard W.M. Jones]

On Fri, Nov 30, 2018 at 04:03:37PM -0600, 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
> + * NUL-terminated copies of the reply.  Return 0 if iteration is
> + * 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);
> +        *nameout = name = g_new(char, namelen + 1);
>      }
> 
> -    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);
> +        }
>          return -1;
>      }
>      name[namelen] = '\0';
>      len -= namelen;
> -    if (nbd_drop(ioc, len, errp) < 0) {
> +    if (!want) {
> +        assert(description);
> +        *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);
> +            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;
> @@ -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 */

I found this patch a lot easier to review once I started to use the
‘git show -w’ option.  The changes look like a reasonable adaptation
of the function, and the only consumer of the function (at this point)
is unaffected, so:

Reviewed-by: Richard W.M. Jones <address@hidden>

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW