On Mon, Aug 24, 2020 at 05:02:56PM -0500, Eric Blake wrote: > On 8/24/20 7:52 AM, Eric Blake wrote: > >I'm about to add an 'exportname' filter, and in the process, I > >noticed a few shortcomings in our API. Time to fix those before > >the 1.22 release locks our API in stone. First, .list_exports > >needs to know if it is pre- or post-TLS, as that may affect which > >names are exported. Next, overloading .list_exports to do both > >NBD_OPT_LIST and mapping "" to a canonical name is proving to be > >awkward; the canonical mapping is only needed during an > >NBD_INFO_NAME response to NBD_OPT_GO, and making .open try to > >grab the entire .list_exports list just to use only the first > >entry (even if the plugin optimized based on the bool to only > >provide one entry) is awkward, compared to just having a > >dedicated function. Finally, as long as we are going to support > >NBD_INFO_NAME, we can also support NBD_INFO_DESCRIPTION; but > >while we map "" to a canonical name prior to calling .open, > >getting the description makes more sense after the connection > >is established, alongside .get_size. > >--- > > > >I obviously need to finish the code to go with this, but here's where > >I would like to see the API before we finalize the 1.22 release. > > > > >+++ b/docs/nbdkit-plugin.pod > > >+=head2 C<.default_export> > >+ > >+ const char *default_export (int readonly, int is_tls); > > Oh fun. For some plugins (like ondemand), this is trivial: return a > compile-time constant string. But for others (like sh and eval), > there's a lifetime issue: this callback is used _before_ .open, ergo > there is no handle struct that it can be associated with. What's > more, this is called _after_ .preconnect, which means it is logical > to expect that the default export name might change over time > (consider a plugin that advertises the largest file in a directory > as its default, but where the directory can change _which_ file is > largest between when the first client connects and when the second > client connects). And the string returned by the sh script is in > malloc'd memory (by it's very nature of coming from the user script, > rather than being a compile-time constant). Without a handle to > store this string in, we would have a memory leak: there is no way > to associate this inside the handle's struct so that .close can > reclaim it, but storing it globally is not thread-safe to parallel > client connections.
I'm not sure there's an actual problem here, because there is still a connection object inside the server any time we have a TCP connection, so you can just store it there, unless I'm misunderstanding something. But anyway ... > So I'm thinking I need to add a helper > function: > > const char *nbdkit_string_intern (const char *str); > > Return a pointer to a copy of str, where nbdkit owns the lifetime of > the copy (allowing the caller to not have to worry about persisting > str indefinitely). If called when there is no client connection > (such as during .load), the copy will remain valid through .unload; > if called in the context of a client connection (any callback from > .preconnect through .close), the copy will remain valid through > .close. I'm a bit unclear why the plugin would have to call this (or this function be in the public API at all). Why can't string interning be done inside the server. Have a global struct where strings returned from the plugin are interned, and free it on server exit. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://people.redhat.com/~rjones/virt-df/ _______________________________________________ Libguestfs mailing list [email protected] https://www.redhat.com/mailman/listinfo/libguestfs
