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 <eblake@redhat.com>
---
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 *@match 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 *@match 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 */
--
2.17.2
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 <eblake@redhat.com>
> ---
> 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 *@match 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 *@match 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 <rjones@redhat.com>
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
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 <eblake@redhat.com>
> ---
> 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 *@match 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 *@match 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/!@want 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
On 12/6/18 8:18 AM, Vladimir Sementsov-Ogievskiy wrote:
> 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 <eblake@redhat.com>
>> ---
>> nbd/client.c | 66 +++++++++++++++++++++++++++++++++++++---------------
>> 1 file changed, 47 insertions(+), 19 deletions(-)
>> @@ -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.
Why? We already validated that the overall option is not too large, and
even if the resulting name from the server is larger than
NBD_MAX_NAME_SIZE, it's still nice to report that name to the client for
'qemu-nbd --list'. And these days, we only call nbd_receive_list if a
server lacked NBD_OPT_GO, which is getting rarer and rarer, so
micro-optimizing a rare case to avoid a large malloc isn't going to make
a noticeable difference.
>
>> }
>>
>> - 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
The description is not bound by NBD_MAX_NAME_SIZE. The NBD protocol
DOES document a maximum string size (4k), but we are (so far) not
actually honoring that limit (our choice for NBD_MAX_NAME_SIZE is 256,
which is smaller than the NBD protocol limit).
>
>> + *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.
Okay, I'll try and be more careful in v2 about not altering the callers
pointers on failure.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
06.12.2018 19:31, Eric Blake wrote:
> On 12/6/18 8:18 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 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 <eblake@redhat.com>
>>> ---
>>> nbd/client.c | 66 +++++++++++++++++++++++++++++++++++++---------------
>>> 1 file changed, 47 insertions(+), 19 deletions(-)
>
>>> @@ -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.
>
> Why? We already validated
Ah, than OK, the worst case is ~ NBD_MAX_BUFFER_SIZE (32M), not 4G. Missed that :(
that the overall option is not too large, and even if the resulting name from the server is larger than NBD_MAX_NAME_SIZE, it's still nice to report that name to the client for 'qemu-nbd --list'. And these days, we only call nbd_receive_list if a server lacked NBD_OPT_GO, which is getting rarer and rarer, so micro-optimizing a rare case to avoid a large malloc isn't going to make a noticeable difference.
>
>>
>>> }
>>>
>>> - 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
>
> The description is not bound by NBD_MAX_NAME_SIZE. The NBD protocol DOES document a maximum string size (4k), but we are (so far) not actually honoring that limit (our choice for NBD_MAX_NAME_SIZE is 256, which is smaller than the NBD protocol limit).
>
Ohm, yes, you are right :(. Too much reviewing, I should stop and make some patches :)
>>
>>> + *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.
>
> Okay, I'll try and be more careful in v2 about not altering the callers pointers on failure.
>
--
Best regards,
Vladimir
© 2016 - 2025 Red Hat, Inc.