On 17.08.20 17:27, Kevin Wolf wrote:
> Am 17.08.2020 um 17:13 hat Max Reitz geschrieben:
>> On 13.08.20 18:29, Kevin Wolf wrote:
>>> Every block export needs a block node to export, so move the 'device'
>>> option from BlockExportOptionsNbd to BlockExportOptions.
>>>
>>> To maintain compatibility in nbd-server-add, BlockExportOptionsNbd needs
>>> to be wrapped by a new type NbdServerAddOptions that adds 'device' back
>>> because nbd-server-add doesn't use the BlockExportOptions base type.
>>>
>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>> ---
>>>  qapi/block-export.json         | 27 +++++++++++++++++++++------
>>>  block/export/export.c          | 26 ++++++++++++++++++++------
>>>  block/monitor/block-hmp-cmds.c |  6 +++---
>>>  blockdev-nbd.c                 |  4 ++--
>>>  qemu-nbd.c                     |  2 +-
>>>  5 files changed, 47 insertions(+), 18 deletions(-)
>>
>> (Code diff looks good, just a question on the interface:)
>>
>>> diff --git a/qapi/block-export.json b/qapi/block-export.json
>>> index 4ce163411f..d68f3bf87e 100644
>>> --- a/qapi/block-export.json
>>> +++ b/qapi/block-export.json
>>
>> [...]
>>
>>> @@ -167,6 +179,8 @@
>>>  # Describes a block export, i.e. how single node should be exported on an
>>>  # external interface.
>>>  #
>>> +# @device: The device name or node name of the node to be exported
>>> +#
>>
>> Wouldn’t it be better to restrict ourselves to a node name here?
>> (Bluntly ignoring the fact that doing so would make this patch an
>> incompatible change, which would require some reordering in this series,
>> unless we decide to just ignore that intra-series incompatibility.)
> 
> We already have intra-series incompatibility, so I wouldn't mind that.
> 
>> OTOH...  What does “better” mean.  It won’t hurt anyone to keep this as
>> @device.  It’s just that I feel like if we had no legacy burden and did
>> this all from scratch, we would just make it @node-name.
>>
>> Did you deliberately decide against @node-name?
> 
> At first I thought I could still share code between nbd-server-add and
> block-export-add, but that's not the case. Then I guess the only other
> reason I have is consistency with other QMP commands. I won't pretend
> it's a strong one.

If “using @node-name would be more natural” doesn’t resonate with you,
then I suppose we should just leave it as @device.  It isn’t harder to
handle for qemu, and maybe it makes transitioning easier for some users
(although I can’t quite imagine how).

Max