Re: [Qemu-devel] [for-2.9 8/8] sheepdog: Fix blockdev-add

[Max Reitz]

On 29.03.2017 22:32, Eric Blake wrote:
> On 03/29/2017 02:59 PM, Max Reitz wrote:
>> On 29.03.2017 18:45, Markus Armbruster wrote:
>>> Commit 831acdc "sheepdog: Implement bdrv_parse_filename()" and commit
>>> d282f34 "sheepdog: Support blockdev-add" have different ideas on how
>>> the QemuOpts parameters for the server address are named.  Fix that.
>>> While there, rename BlockdevOptionsSheepdog member addr to server, for
>>> consistency with BlockdevOptionsSsh, BlockdevOptionsGluster,
>>> BlockdevOptionsNbd.
>>>
>>> Commit 831acdc's example becomes
>>>
>>>     --drive driver=sheepdog,server.host=fido,vdi=dolly
>>>
>>> instead of
>>>
>>>     --drive driver=sheepdog,host=fido,vdi=dolly
>>>
>>> Signed-off-by: Markus Armbruster <address@hidden>
>>> ---
>>>  block/sheepdog.c     | 18 +++++++++---------
>>>  qapi/block-core.json |  4 ++--
>>>  2 files changed, 11 insertions(+), 11 deletions(-)
>>
>> OK, so let me get this straight: Before this patch, @addr was advertised
>> as a runtime parameter that actually wasn't supported at all? (Because I
>> can't see any place in block/sheepdog.c where it would be parsed.)
> 
> Both commits mentioned in the commit message are new to 2.9, so we
> aren't breaking any back-compat, but are avoiding cementing in a mistake.
> 
> Before this patch, @addr was the QMP spelling (inconsistent with all the
> others, where ssh, gluster, and nbd spelled it @server); and because it
> was NOT mentioned in the sheepdog.c QemuOpts code, it was impossible to
> parse it through -drive.  Which means we have a needless difference
> between -drive and -blockdev-add.
> 
> After this patch, it is spelled @server for both -drive and
> -blockdev-add, and QMP is consistent (both with the QemuOpts code, and
> with the other network storage drivers).

Yes, right, which is why I agree that in principle this patch is
necessary for 2.9.

>> This patch now renames @addr to @server and instead of actually parsing
>> the option, it just removes the so-far convenience[1] options @host,
>> @port, and @path and just fetches the from @server?
> 
> But the convenience options have not been released, so fixing them now
> is the right way to go.

Right. My main point was that @server still isn't actually parsed.

>> If that is true, I can't say I like it the least. I guess it works for
>> 2.9, but there should be a FIXME somewhere in this patch.
> 
> A fixme to what? Unlike patch 7/8 which has to deal with legacy -drive
> code, here, we have nothing released to break.

A FIXME for parsing @server.

>> Also, if you set any of server.* in bdrv_parse_filename(), you also have
>> to set server.type. It's a SocketAddressFlat, .type is a required field.
>> I know it's just for internal use, but that doesn't change the fact that
>> it's an invalid SocketAddressFlat object without.
> 
> That part's true. Also, you can't set server.host and server.path at the
> same time (compare to nbd_process_legacy_socket_options taking pains to
> make sure that a valid QAPI object is constructed).
> 
>>
>> Max
>>
>>
>> [1] Originally there were apparently introduced for
>> bdrv_parse_filename() -- but of course you can just use them in -drive
>> directly...
> 
> There's a difference between the psuedo-file '-drive sheepdog:...'
> (where you can use them directly, and that doesn't change in this
> patch), and the structured '-drive driver=sheepdog,...' (which didn't
> exist before 831acdc, and which we want to match to our QMP).
> 
> I'm in favor of this patch, but think you found a real issue with not
> constructing a valid qapi object.

I agree that this patch is pretty much OK (and definitely necessary) for
2.9, what I'm asking for is some form of acknowledgment that we want to
actually make @server a valid SocketAddressFlat object and parse it as
such in 2.10.

Max

signature.asc
Description: OpenPGP digital signature