Libguestfs - Jul 2020 - Re: [PATCH nbdkit] server: Deprecate the -e/--exportname parameter.

On 7/22/20 4:02 AM, Richard W.M. Jones wrote:
>> The more I think about it, the more I disagree with disabling this.
>> Or, put another way, I think that the _only_ time -e makes sense is
>> _when_ you are using --run.  Consider:
>>
>> nbdkit -U - -e foo info --run 'nbdsh -u $uri -c
"print(h.pread(3, 0))"'
>> nbdkit -U - -e bar info --run 'nbdsh -u $uri -c
"print(h.pread(3, 0))"'
>>
>> Of course, you can accomplish the same with:
>>
>> nbdkit -U - info --run 'nbdsh -u
nbd+unix:///foo\?socket=$unixsocket \
>>    -c "print(h.pread(3, 0))"'
>>
>> but that's a lot more painful to write.
>>
>> We _don't_ need to advertise 'foo' or 'bar' over
NBD_OPT_INFO (at
>> least, not for plugins that don't implement the forthcoming
>> .export_list callback), but _do_ need a way for the captive
>> application (nbdsh in this case) to know _which_ export the captive
>> should connect to.
>>
>> And, if we reinstate _just_ that usage of -e,...
>>
>>>> (4) I have temporarily disabled tests/test-long-name.sh.  This
is
>>>>       still a valid test, but we will have to rewrite it to use
>>>>       (probably) sh or eval plugins once we have an
implementation of
>>>>       list_exports.
>>>> ---
>>
>> ...we may not have to disable this test after all.
> 
> I feel for the end user this is all pretty confusing and requires them
> to have some insight into what the plugin is expecting and even
> understanding of the protocol.
> 
> Perhaps we should have a rule something like this:
> 
>   - plugins should serve default content on the "" export
> 
>   - unless they implement .list_exports, in which case the
>     first export returned is the default export
That could be expensive, reading the entire list and throwing away all 
but the first. I'm now leaning towards two callbacks:

.list_exports: compute all exports to advertise
.default_export: compute the name to be returned for NBD_OPT_INFO("") 
and to use by default as $exportname in --run

Then, any plugin that wants to advertise a different default export name 
supplies .default_export, and if .default_export is missing, we supply 
"" on their behalf.  We could also translate a client request for
""
into .default_export before calling .open (v3) or populating 
nbdkit_export_name (v2) (I could see that mattering for the file plugin 
with a mode that serves all files in a directory, since stat("") fails
but providing a default filename as the largest file in the directory 
could be useful).
> 
> Then we could implement --run transparently by having it set
> $exportname/$uri to the first entry in .list_exports or "" if
> .list_exports is not implemented.
> 
> Here's another idea: Is it possible that if the --run script itself
> overrides exportname it could be used by $uri.  Not sure if there's
> some kind of delayed shell variable expansion that would make this
> possible:
> 
>    nbdkit ... --run 'exportname=foo; nbdsh -u $uri ...'
Not as written, but we _can_ do the following.  Right now, --run creates 
a shell fragment, which prepends this code in front of the user's:

uri=...
nbd=...
...
exportname=...

we could instead prepend:

set_export() {
   uri=...
   nbd=...
   ...
   exportname=$1
}
set_export ...

where that last line uses .default_export (defaulting to "" as usual).
The user can now run:

nbdkit ... --run 'set_export foo; nbdsh -u "$uri" ... '

Oh, and I just realized, because $uri can contain ?, we really should 
scrub all uses in the tree to always write it as "$uri" rather than 
unquoted, just to set a good example.  (It's unlikely that anyone would 
ever have a subdirectory named 'nbd+unix:' in their current working 
directory to the point that the ? would trigger unintended globbing, but 
better safe than sorry...)

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On Wed, Jul 22, 2020 at 06:12:30AM -0500, Eric Blake
wrote:
> On 7/22/20 4:02 AM, Richard W.M. Jones wrote:
> >Perhaps we should have a rule something like this:
> >
> >  - plugins should serve default content on the "" export
> >
> >  - unless they implement .list_exports, in which case the
> >    first export returned is the default export
> 
> That could be expensive, reading the entire list and throwing away
> all but the first. I'm now leaning towards two callbacks:
> 
> .list_exports: compute all exports to advertise
> .default_export: compute the name to be returned for
> NBD_OPT_INFO("") and to use by default as $exportname in --run
Maybe I don't understand this, but isn't NBD_OPT_INFO("")
explicitly
asking for info about the "" export name?  In what situation would
.default_export ever return anything other than ""?
> Then, any plugin that wants to advertise a different default export
> name supplies .default_export, and if .default_export is missing, we
> supply "" on their behalf.  We could also translate a client
request
> for "" into .default_export before calling .open (v3) or
populating
> nbdkit_export_name (v2) (I could see that mattering for the file
> plugin with a mode that serves all files in a directory, since
> stat("") fails but providing a default filename as the largest
file
> in the directory could be useful).
> 
> >Then we could implement --run transparently by having it set
> >$exportname/$uri to the first entry in .list_exports or "" if
> >.list_exports is not implemented.
> >
> >Here's another idea: Is it possible that if the --run script itself
> >overrides exportname it could be used by $uri.  Not sure if there's
> >some kind of delayed shell variable expansion that would make this
> >possible:
> >
> >   nbdkit ... --run 'exportname=foo; nbdsh -u $uri ...'
> 
> Not as written, but we _can_ do the following.  Right now, --run
> creates a shell fragment, which prepends this code in front of the
> user's:
> 
> uri=...
> nbd=...
> ...
> exportname=...
> 
> we could instead prepend:
> 
> set_export() {
>   uri=...
>   nbd=...
>   ...
>   exportname=$1
> }
> set_export ...
> 
> where that last line uses .default_export (defaulting to "" as
> usual). The user can now run:
> 
> nbdkit ... --run 'set_export foo; nbdsh -u "$uri" ... '
Cure is beginning to sound worse than the disease.  We can bring back
-e but only for the narrow use case of specifying an export name for
--run and with lots of documentation about why it's needed.
> Oh, and I just realized, because $uri can contain ?, we really
> should scrub all uses in the tree to always write it as "$uri"
> rather than unquoted, just to set a good example.  (It's unlikely
> that anyone would ever have a subdirectory named 'nbd+unix:' in
> their current working directory to the point that the ? would
> trigger unintended globbing, but better safe than sorry...)
Yes indeed.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

On 7/22/20 6:25 AM, Richard W.M. Jones wrote:
> On Wed, Jul 22, 2020 at 06:12:30AM -0500, Eric Blake wrote:
>> On 7/22/20 4:02 AM, Richard W.M. Jones wrote:
>>> Perhaps we should have a rule something like this:
>>>
>>>   - plugins should serve default content on the "" export
>>>
>>>   - unless they implement .list_exports, in which case the
>>>     first export returned is the default export
>>
>> That could be expensive, reading the entire list and throwing away
>> all but the first. I'm now leaning towards two callbacks:
>>
>> .list_exports: compute all exports to advertise
>> .default_export: compute the name to be returned for
>> NBD_OPT_INFO("") and to use by default as $exportname in
--run
> 
> Maybe I don't understand this, but isn't NBD_OPT_INFO("")
explicitly
> asking for info about the "" export name?  In what situation
would
> .default_export ever return anything other than ""?
The NBD protocol permits it in replies to NBD_OPT_INFO:

     * `NBD_INFO_NAME` (1)

       Represents the server's canonical name of the export. The name
       MAY differ from the name presented in the client's option
       request, and the information item MAY be omitted if the client
       option request already used the canonical name.  This
       information type represents the same name that would appear in
       the name portion of an `NBD_REP_SERVER` in response to
       `NBD_OPT_LIST`.

That is, when I added NBD_OPT_INFO to the NBD protocol, I specifically 
envisioned a server where if the client asks "I want the default export, 
what am I going to be served?" the server can then reply with a 
canonical name rather than "".
>> we could instead prepend:
>>
>> set_export() {
>>    uri=...
>>    nbd=...
>>    ...
>>    exportname=$1
>> }
>> set_export ...
>>
>> where that last line uses .default_export (defaulting to ""
as
>> usual). The user can now run:
>>
>> nbdkit ... --run 'set_export foo; nbdsh -u "$uri" ...
'
> 
> Cure is beginning to sound worse than the disease.  We can bring back
> -e but only for the narrow use case of specifying an export name for
> --run and with lots of documentation about why it's needed.
Yep, now you see why I was starting to complain that we were premature 
on crippling -e with --run.
> 
>> Oh, and I just realized, because $uri can contain ?, we really
>> should scrub all uses in the tree to always write it as
"$uri"
>> rather than unquoted, just to set a good example.  (It's unlikely
>> that anyone would ever have a subdirectory named 'nbd+unix:' in
>> their current working directory to the point that the ? would
>> trigger unintended globbing, but better safe than sorry...)
> 
> Yes indeed.
As that's an easy scrub, I'll probably complete it momentarily.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org