Libguestfs - Sep 2019 - Re: [nbdkit PATCH v2 5/7] server: Allow longer NBD_OPT

On Fri, Sep 27, 2019 at 11:48:47PM -0500, Eric Blake
wrote:
> Fixes the fact that clients could not request the maximum string
> length except with NBD_OPT_EXPORT_LEN.  Updates the testsuite to
> match.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  server/protocol-handshake-newstyle.c | 12 +++++++-----
>  tests/test-long-name.sh              | 10 ++++------
>  2 files changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/server/protocol-handshake-newstyle.c
b/server/protocol-handshake-newstyle.c
> index 34958360..3b5d144e 100644
> --- a/server/protocol-handshake-newstyle.c
> +++ b/server/protocol-handshake-newstyle.c
> @@ -48,7 +48,7 @@
>  #define MAX_NR_OPTIONS 32
> 
>  /* Maximum length of any option data (bytes). */
> -#define MAX_OPTION_LENGTH 4096
> +#define MAX_OPTION_LENGTH (NBD_MAX_STRING * 4)
I may have missed it - why was * 4 chosen?

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

On 9/28/19 3:07 PM, Richard W.M. Jones wrote:
> On Fri, Sep 27, 2019 at 11:48:47PM -0500, Eric Blake wrote:
>> Fixes the fact that clients could not request the maximum string
>> length except with NBD_OPT_EXPORT_LEN.  Updates the testsuite to
>> match.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---
>>   server/protocol-handshake-newstyle.c | 12 +++++++-----
>>   tests/test-long-name.sh              | 10 ++++------
>>   2 files changed, 11 insertions(+), 11 deletions(-)
>>
>> diff --git a/server/protocol-handshake-newstyle.c
b/server/protocol-handshake-newstyle.c
>> index 34958360..3b5d144e 100644
>> --- a/server/protocol-handshake-newstyle.c
>> +++ b/server/protocol-handshake-newstyle.c
>> @@ -48,7 +48,7 @@
>>   #define MAX_NR_OPTIONS 32
>>
>>   /* Maximum length of any option data (bytes). */
>> -#define MAX_OPTION_LENGTH 4096
>> +#define MAX_OPTION_LENGTH (NBD_MAX_STRING * 4)
> 
> I may have missed it - why was * 4 chosen?
NBD_OPT_SET_META_CONTEXT allows two strings plus a few glue bytes, so 
more than 8k of data from a compliant client.  16k is the next power of 
2.  We can bump it larger if we want, especially since 16k pales in 
comparison to our 32M limit on NBD_CMD_WRITE, but for now, there is 
nothing in the NBD protocol larger than NBD_OPT_SET_META_CONTEXT.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On 9/30/19 11:30 AM, Eric Blake wrote:
>>>   /* Maximum length of any option data (bytes). */
>>> -#define MAX_OPTION_LENGTH 4096
>>> +#define MAX_OPTION_LENGTH (NBD_MAX_STRING * 4)
>>
>> I may have missed it - why was * 4 chosen?
> 
> NBD_OPT_SET_META_CONTEXT allows two strings plus a few glue bytes, so 
> more than 8k of data from a compliant client.  16k is the next power of 
> 2.  We can bump it larger if we want, especially since 16k pales in 
> comparison to our 32M limit on NBD_CMD_WRITE, but for now, there is 
> nothing in the NBD protocol larger than NBD_OPT_SET_META_CONTEXT.
Actually, having just written that, I now realize that 
NBD_OPT_SET_META_CONTEXT allows you to request more than one context at 
a time.  It's very easy to provoke a request larger than 16k by 
requesting 3 contexts at once (nbdsh can do so), even if we only ever 
respond to a single recognized context.  So I should probably just go 
whole-hog and cap this at the same limit as NBD_CMD_WRITE, rather than 
having two independent limits.  And I probably ought to beef up the 
testsuite to actually demonstrate nbdsh provoking that large of an option.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org