[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [for-2.9 8/8] sheepdog: Fix blockdev-add
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-block] [Qemu-devel] [for-2.9 8/8] sheepdog: Fix blockdev-add |
Date: |
Thu, 30 Mar 2017 09:47:54 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux) |
Max Reitz <address@hidden> writes:
> 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.)
Correct. I messed it up.
>> 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.
The difference being that -drive actually worked %-}
>> 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.
I'm not sure I understand what you mean by "parsing @server". Care to
suggest a wording and a place for the FIXME comment?
I agree this code will need an update when we wean our internal
interfaces off SocketAddress.
>>> 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.
Well, it's a flat QDict, not a SocketAddressFlat. Is its "server.type"
member unset when nobody's looking?
Permit me to digress. The block layer's QAPI/QDict/QemuOpts ménage à
trois is disgusting. I understand how we got there, but that doesn't
make it less ugly.
Anyway, I'm willing to make bdrv_parse_filename() set "server.type".
Would you like me to require -drive server.type= when not using the
pseudo-filename, for consistency with other backends?
>> 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).
sd_open() rejects attempts to set both server.host and server.path.
>>> 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.
- [Qemu-block] [for-2.9 3/8] io vnc sockets: Clean up SocketAddressKind switches, (continued)
[Qemu-block] [for-2.9 2/8] char: Fix socket with "type": "vsock" address, Markus Armbruster, 2017/03/29
Re: [Qemu-block] [for-2.9 2/8] char: Fix socket with "type": "vsock" address, Stefan Hajnoczi, 2017/03/29
Re: [Qemu-block] [for-2.9 2/8] char: Fix socket with "type": "vsock" address, Max Reitz, 2017/03/29
[Qemu-block] [for-2.9 4/8] block: Document -drive problematic code and bugs, Markus Armbruster, 2017/03/29