Re: [RFC PATCH 06/18] qemu-storage-daemon: Add --nbd-server option

[Markus Armbruster]

Max Reitz <address@hidden> writes:

> On 17.10.19 15:01, Kevin Wolf wrote:
>> Add a --nbd-server option to qemu-storage-daemon to start the built-in
>> NBD server right away. It maps the arguments for nbd-server-start to the
>> command line.
>
> Well, it doesn’t quite, because nbd-server-start takes a
> SocketAddressLegacy, and this takes a SocketAddress.
>
> On one hand I can understand why you would do it differently (especially
> for command-line options), but on the other I find it a bit problematic
> to have --nbd-server be slightly different from nbd-server-start when
> both are intended to be the same.
>
> My biggest problem though lies in the duplication in the QAPI schema.
> If NbdServerOptions.addr were a SocketAddressLegacy, we could let
> nbd-server-start’s options just be of type NbdServerOptions and thus get
> rid of the duplication.
>
> I suspect in practice it’s all not that big of a problem.  I can’t call
> it bad if --nbd-server is just nicer to use.  And the biggest problem
> with duplication in the QAPI schema is that nbd-server-start and
> --nbd-server might get out of sync.  But realistically, I don’t see that
> happen, because if nbd-server-start changes, nbd_server_start() will
> change, too, so we’ll get compile errors in nbd_server_start_options().
>
> *shrug*

Two good reasons for making new --nbd-server differ from existing
nbd-server-start:

1. The nesting sucks.  The CLI's dotted key syntax makes it suck harder.

2. New interfaces should not use SocketAddressLegacy (or any other
   "simple" union) if we can help it.

The duplication is the price we pay for getting it right om the second
try.

> But I do think the commit message should explain why we can’t just use
> NbdServerOptions for nbd-server-start.

Yes, and throw in a comment.