Re: [Qemu-devel] [PATCH] nbd/client: add x-block-status hack for testing server

[Eric Blake <eblake@redhat.com>]

On 06/29/2018 05:01 AM, Vladimir Sementsov-Ogievskiy wrote:
> Sorry for being late, here are some thoughts. Anyway, idea is good, 
> we've planned to do something like this, but you were the first)

Actually, I'm now leaning towards a bit more extensive improvement that 
would let 'qemu-img map --output=json' show BOTH normal block status AND 
the NBD dirty bitmap status at once (that is, let the client query two 
status fields at once when passing an NBD option, rather than this 
hack's approach of replacing normal block status with just dirty bitmap 
status).  We'll see if I can come up with something before 3.0 
softfreeze (at any rate, I have my work cut out for me today).


>> +++ b/qapi/block-core.json
>> @@ -3484,12 +3484,22 @@
>>   #
>>   # @tls-creds:   TLS credentials ID
>>   #
>> +# @x-block-status: A string containing a semicolon-separated list of
>> +#                  block status context names to query and then
>> +#                  request
> 
> and then request only the first one

Ideally, I'd like to report dirty bitmap in addition to normal block 
status (and actually, we really only need to support exactly one string, 
none of this semicolon-separated stuff). We'll see where v2 gets us; but 
yes, this v1 hack uses only the first string.

My v1 hack served two purposes: stress-test the server (where the 
semicolon separation DID let me slam multiple requests, with multiple 
different spellings, to double-check that I was happy with the server's 
response in all cases), and hack a single reader for bitmap status; only 
the latter one is important in the long run.

>> @@ -438,8 +443,8 @@ static int nbd_open(BlockDriverState *bs, QDict 
>> *options, int flags,
>>       }
>>
>>       /* NBD handshake */
>> -    ret = nbd_client_init(bs, sioc, s->export,
>> -                          tlscreds, hostname, errp);
>> +    ret = nbd_client_init(bs, sioc, s->export, tlscreds, hostname,
>> +                          qemu_opt_get(opts, "x-block-status"), errp);
> 
> hm, so, after this nbd_open finish, info.x_block_status will become 
> invalid pointer..
> It's not used in other places, but looks like bad idea anyway. If we 
> don't want to allocate string,
> we can pass it as a separate const char* paramter to nbd_receive_negotiate.

As a hack, I just plumbed in the bare minimum to get from command line 
to the wire.  Yes, a more formal patch would have to strdup() something 
to last for the proper lifetime, and maybe pass it through via a struct 
rather than having to add yet more parameters for future additions.

>> +/* nbd_negotiate_list_meta_context:
>> + * Hack for testing meta context negotiation. Subdivide list on 
>> semicolons,
>> + * then pass that many queries for info and trace the results.
>> + * return 0 for successful negotiation
>> + *        -1 with errp set for any other error
>> + */
>> +static int nbd_negotiate_list_meta_context(QIOChannel *ioc,
>> +                                           const char *export,
>> +                                           const char *context,
>> +                                           Error **errp)
>> +{
> 
> hm, I'd prefer to refactor this, to not duplicate code.. this function 
> (may be a bit improved) may be called from
> nbd_negotiate_simple_meta_context(), and we will test normal code path 
> when debugging with the hack.
> However, I'm ok with this duplication for now, as a debugging hack.

Yes, very true. I did a LOT of copy-and-paste which would be nicer as 
parameterized reusable code.

>> +    while (reply.type == NBD_REP_META_CONTEXT) {
>> +        char *name;
>> +
> 
> don't you want to check reply.length before read?

Yes, in a formal patch, we must not trust that the server is 
non-malicious.  It might be worth refactoring cleanup along the lines of 
nbd_opt_read() in server.c that guarantees that a subsequent read fits 
within a predetermined remaining length, for less code at each point in 
the sequence that has to do such length validations.

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