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

On 7/21/20 12:46 PM, Eric Blake wrote:
>> (2) The --run option no longer sets $exportname (to -e) nor puts the
>>      export name into the $uri.  However this was always the wrong
>>      thing to do since export names are per connection, not per server.
>>      Existing --run scripts will see $exportname expand to ""
which is
>>      most likely what they saw before and overwhelmingly more likely to
>>      be correct than if the -e option had been used.
>>
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.

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

On Tue, Jul 21, 2020 at 07:51:29PM -0500, Eric Blake
wrote:
> On 7/21/20 12:46 PM, Eric Blake wrote:
> 
> >>(2) The --run option no longer sets $exportname (to -e) nor puts
the
> >>     export name into the $uri.  However this was always the wrong
> >>     thing to do since export names are per connection, not per
server.
> >>     Existing --run scripts will see $exportname expand to
"" which is
> >>     most likely what they saw before and overwhelmingly more
likely to
> >>     be correct than if the -e option had been used.
> >>
> 
> 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

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

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

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