| Message ID | 20181130220344.3350618-9-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | nbd: add qemu-nbd --list | expand |
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.
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 */ >
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.
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. >
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 */
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(-)