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

[Max Reitz]

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.)

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?

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.

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.

Max


[1] Originally there were apparently introduced for
bdrv_parse_filename() -- but of course you can just use them in -drive
directly...

signature.asc
Description: OpenPGP digital signature