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 <ebl...@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
