[Libguestfs] [PATCH nbdkit] server: Pass the export name through filter .open calls.

[Eric Blake]

On 7/22/20 3:53 AM, Richard W.M. Jones wrote:
> On Tue, Jul 21, 2020 at 07:41:21PM -0500, Eric Blake wrote:
>> On 7/21/20 3:47 PM, Richard W.M. Jones wrote:
>>> +++ b/server/plugins.c
>>> @@ -278,12 +278,30 @@ plugin_preconnect (struct backend *b, int readonly)
>>>   }
>>>   static void *
>>> -plugin_open (struct backend *b, int readonly)
>>> +plugin_open (struct backend *b, int readonly, const char *exportname)
>>>   {
>>> +  GET_CONN;
>>>     struct backend_plugin *p = container_of (b, struct backend_plugin, backend);
>>>     assert (p->plugin.open != NULL);
>>> +  /* Save the exportname since the lifetime of the string passed in
>>> +   * here is likely to be brief.  In addition this provides a place
>>> +   * for nbdkit_export_name to retrieve it if called from the plugin.
>>> +   *
>>> +   * In API V3 we propose to pass the exportname as an extra parameter
>>> +   * to the (new) plugin.open and deprecate nbdkit_export_name for V3
>>> +   * users.  Even then we will still need to save it in the handle
>>> +   * because of the lifetime issue.
>>> +   */
>>> +  if (conn->exportname == NULL) {
>>
>> Can't we assert(!conn->exportname) at this point?  After all, we
>> only ever call .open at most once per connection.
> 
> I don't think so - backend_reopen will call plugin_open a second time.
> As a test I added assert (conn->exportname == NULL) before this line
> and it crashed in tests/test-retry.sh.

Fair enough.


>>> +  if (conn->exportname_from_set_meta_context &&
>>> +      strcmp (conn->exportname_from_set_meta_context, exportname) != 0) {
>>> +    debug ("newstyle negotiation: NBD_OPT_SET_META_CONTEXT export name \"%s\" ≠ final client exportname \"%s\", so discarding the previous context",
>>
>> Long line, and I don't know if we use UTF-8 in other debug messages
>> or should stick to straight ascii.  Compilation may have a glitch if
>> compiled under a different locale than the end binary runs in, but
>> these days, it's uncommon to find someone running in a single-byte
>> locale instead of UTF-8.
> 
> I thought we might be using ‘’ quotes (as we do in libguestfs) but I
> see that we don't.  I think we should use Unicode characters more
> where they are appropriate, but I'll break up this long line.  I also
> added a comment.

I see you used Unicode quoting when refactoring the nbd plugin recently, 
but yeah, I agree we haven't been using it much yet.  I'm not opposed to 
Unicode in C strings, but just pointing out that it can reduce the set 
of platforms where things work out of the box (although these days, 
since we require working gcc or clang for __attribute__((cleanup)), not 
using Unicode is unlikely).  Switching existing code to use nicer 
quoting would be a separate cleanup series.

> 
>>> @@ -148,7 +166,7 @@ ext2_prepare (struct nbdkit_next_ops *next_ops, void *nxdata, void *handle,
>>>     struct ext2_inode inode;
>>>     int64_t r;
>>>     CLEANUP_FREE char *name = NULL;
>>> -  const char *fname = file ?: nbdkit_export_name ();
>>> +  const char *fname = file ?: h->exportname;
>>
>> Hmm - we already pass the same 'readonly' state to filter's
>> .prepare, but not to backend_prepare(), which has to reconstruct it.
>> Would it be easier to also change the signature of backend_prepare()
>> to take both the original readonly and exportname passed to
>> backend_open(), rather than making the filter have to save it off in
>> the filter?  It looks like protocol-handshake.c is the only caller,
>> and still has everything in scope at the time.
> 
> Possibly, although this would be a seperate change.

As I replied in a later mail, no, it would not be easier.

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