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 <arm...@redhat.com> >> --- >> 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). > > 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. > > 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. > > 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. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
