[RFC nbdkit PATCH] protocol: Alter .list_exports, add .default_export
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. docs/nbdkit-filter.pod | 41 +++++++++++++++++++---- docs/nbdkit-plugin.pod | 74 ++++++++++++++++++++++++++++++++++++------ 2 files changed, 98 insertions(+), 17 deletions(-) diff --git a/docs/nbdkit-filter.pod b/docs/nbdkit-filter.pod index bf01db14..f15bdd96 100644 --- a/docs/nbdkit-filter.pod +++ b/docs/nbdkit-filter.pod @@ -339,12 +339,18 @@ an error message and return C<-1>. =head2 C<.list_exports> int (*list_exports) (nbdkit_next_list_exports *next, void *nxdata, - int readonly, int default_only, + int readonly, int is_tls, struct nbdkit_exports *exports); This intercepts the plugin C<.list_exports> method and can be used to filter which exports are advertised. +The C<readonly> parameter matches what is passed to <.preconnect> and +C<.open>, and may be changed by the filter when calling into the +plugin. The C<is_tls> parameter informs the filter whether TLS +negotiation has been completed by the client, but is not passed on to +C<next> because it cannot be altered. + It is possible for filters to transform the exports list received back from the layer below. Without error checking it would look like this: @@ -355,8 +361,8 @@ from the layer below. Without error checking it would look like this: struct nbdkit_export e; char *name, *desc; - exports2 = nbdkit_exports_new (default_only); - next_list_exports (nxdata, readonly, default_only, exports); + exports2 = nbdkit_exports_new (); + next_list_exports (nxdata, readonly, exports); for (i = 0; i < nbdkit_exports_count (exports2); ++i) { e = nbdkit_get_export (exports2, i); name = adjust (e.name); @@ -376,11 +382,9 @@ an error message and return C<-1>. Two functions are provided to filters only for allocating and freeing the list: - struct nbdkit_exports *nbdkit_exports_new (int default_only); + struct nbdkit_exports *nbdkit_exports_new (void); -Allocates and returns a new, empty exports list. The C<default_only> -parameter should match whether the list is intended to grab the -canonical name of the default export, or all exports. +Allocates and returns a new, empty exports list. On error this function can return C<NULL>. In this case it calls C<nbdkit_error> as required. C<errno> will be set to a suitable @@ -408,6 +412,20 @@ Returns the number of exports in the list. Returns a copy of the C<i>'th export. +=head2 C<.default_export> + + const char *default_export (nbdkit_next_default_export *next, void *nxdata, + int readonly, int is_tls) + +This intercepts the plugin C<.default_export> method and can be used to +alter the canonical export name used in place of the default C<"">. + +The C<readonly> parameter matches what is passed to <.preconnect> and +C<.open>, and may be changed by the filter when calling into the +plugin. The C<is_tls> parameter informs the filter whether TLS +negotiation has been completed by the client, but is not passed on to +C<next> because it cannot be altered. + =head2 C<.open> void * (*open) (nbdkit_next_open *next, void *nxdata, @@ -527,6 +545,15 @@ This function is only called once per connection and cached by nbdkit. Similarly, repeated calls to C<next_ops-E<gt>get_size> will return a cached value. +=head2 C<.export_description> + + const char *export_description (struct nbdkit_next_ops *next_ops, + void *nxdata, void *handle); + +This intercepts the plugin C<.export_description> method and can be +used to read or modify the export description that the NBD client +will see. + =head2 C<.can_write> =head2 C<.can_flush> diff --git a/docs/nbdkit-plugin.pod b/docs/nbdkit-plugin.pod index 5c641e83..ecb6bba8 100644 --- a/docs/nbdkit-plugin.pod +++ b/docs/nbdkit-plugin.pod @@ -245,6 +245,12 @@ Early in option negotiation the client may try to list the exports served by the plugin, and plugins can optionally implement this callback to answer the client. See L</EXPORT NAME> below. +=item C<.default_export> + +During option negotiation, if the client requests the default export +name (C<"">), this optional callback provides a canonical name to +use in its place prior to calling C<.open>. + =item C<.open> A new client has connected and finished the NBD handshake. TLS @@ -664,15 +670,16 @@ error message and return C<-1>. =head2 C<.list_exports> - int list_exports (int readonly, int default_only, + int list_exports (int readonly, int is_tls, struct nbdkit_exports *exports); This optional callback is called if the client tries to list the exports served by the plugin (using C<NBD_OPT_LIST>). If the plugin -does not supply this callback then a single export called C<""> is -returned. The NBD protocol defines C<""> as the default export, so -this is suitable for plugins which ignore the export name and always -serve the same content. See also L</EXPORT NAME> below. +does not supply this callback then the reply is determined by the +result of C<.default_export>, which in turn defaults to advertising +the name C<"">. The NBD protocol defines C<""> as the default export, +so this default is suitable for plugins which ignore the export name +and always serve the same content. See also L</EXPORT NAME> below. The C<readonly> flag informs the plugin that the server was started with the I<-r> flag on the command line, which is the same value @@ -681,11 +688,11 @@ not yet have a way to let the client advertise an intent to be read-only even when the server allows writes, so this parameter may not be as useful as it appears. -If the C<default_only> flag is set then the client is querying for the -name of the default export, and the plugin may optimize by adding only -a single export to the returned list (the default export name, usually -C<"">). The plugin can ignore this flag and return all exports if it -wants. +The C<is_tls> flag informs the plugin whether this listing was +requested after the client has completed TLS negotiation. When +running the server in a mode that permits but not requires TLS, +be careful that any exports listed when C<is_tls> is C<false> do +not leak unintended information. The C<exports> parameter is an opaque object for collecting the list of exports. Call C<nbdkit_add_export> to add a single export to the @@ -709,6 +716,37 @@ Returning C<0> will send the list of exports back to the client. If there is an error, C<.list_exports> should call C<nbdkit_error> with an error message and return C<-1>. +=head2 C<.default_export> + + const char *default_export (int readonly, int is_tls); + +This optional callback is called if the client tries to connect to +the default export C<"">. If the plugin does not supply this callback, +the connection continues with the empty name; if the plugin returns +a string, nbdkit returns that name to clients that support it (see +the C<NBD_INFO_NAME> response to C<NBD_OPT_GO>), and behaves as if +the client had passed that string instead of an empty name. Similarly, +if the plugin does not supply a C<.list_exports> callback, the result +of this callback determines what export name to advertise to a client +requesting an export list. + +The C<readonly> flag informs the plugin that the server was started +with the I<-r> flag on the command line, which is the same value +passed to C<.preconnect> and C<.open>. However, the NBD protocol does +not yet have a way to let the client advertise an intent to be +read-only even when the server allows writes, so this parameter may +not be as useful as it appears. + +The C<is_tls> flag informs the plugin whether the canonical name for +the default export is being requested after the client has completed +TLS negotiation. When running the server in a mode that permits but +not requires TLS, be careful that a default export name does not leak +unintended information. + +If the plugin returns C<NULL>, the client is not permitted to connect +to the default export. However, this is not an error in the protocol, +so it is not necessary to call C<nbdkit_error>. + =head2 C<.open> void *open (int readonly); @@ -769,6 +807,22 @@ to get the size (in bytes) of the block device being exported. The returned size must be E<ge> 0. If there is an error, C<.get_size> should call C<nbdkit_error> with an error message and return C<-1>. +=head2 C<.export_description> + + const char *export_description (void *handle); + +This is called during the option negotiation phase only if the +client specifically requested an export description (see the +C<NBD_INFO_DESCRIPTION> response to C<NBD_OPT_GO>). Any +description provided must be human-readable UTF-8, no longer +than 4096 bytes. Ideally, this description should match any +description set during C<.list_exports>, but that is not +enforced. + +If the plugin returns C<NULL>, or if this callback is omitted, no +description is offered to the client. As this is not an error in the +protocol, it is not necessary to call C<nbdkit_error>. + =head2 C<.can_write> int can_write (void *handle); -- 2.28.0
On Mon, Aug 24, 2020 at 07:52:33AM -0500, 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.
Yes, looks like a good change with a reasonable motive behind it. Rich.
docs/nbdkit-filter.pod | 41 +++++++++++++++++++---- docs/nbdkit-plugin.pod | 74 ++++++++++++++++++++++++++++++++++++------ 2 files changed, 98 insertions(+), 17 deletions(-)
diff --git a/docs/nbdkit-filter.pod b/docs/nbdkit-filter.pod index bf01db14..f15bdd96 100644 --- a/docs/nbdkit-filter.pod +++ b/docs/nbdkit-filter.pod @@ -339,12 +339,18 @@ an error message and return C<-1>. =head2 C<.list_exports>
int (*list_exports) (nbdkit_next_list_exports *next, void *nxdata, - int readonly, int default_only, + int readonly, int is_tls, struct nbdkit_exports *exports);
This intercepts the plugin C<.list_exports> method and can be used to filter which exports are advertised.
+The C<readonly> parameter matches what is passed to <.preconnect> and +C<.open>, and may be changed by the filter when calling into the +plugin. The C<is_tls> parameter informs the filter whether TLS +negotiation has been completed by the client, but is not passed on to +C<next> because it cannot be altered. + It is possible for filters to transform the exports list received back from the layer below. Without error checking it would look like this:
@@ -355,8 +361,8 @@ from the layer below. Without error checking it would look like this: struct nbdkit_export e; char *name, *desc;
- exports2 = nbdkit_exports_new (default_only); - next_list_exports (nxdata, readonly, default_only, exports); + exports2 = nbdkit_exports_new (); + next_list_exports (nxdata, readonly, exports); for (i = 0; i < nbdkit_exports_count (exports2); ++i) { e = nbdkit_get_export (exports2, i); name = adjust (e.name); @@ -376,11 +382,9 @@ an error message and return C<-1>. Two functions are provided to filters only for allocating and freeing the list:
- struct nbdkit_exports *nbdkit_exports_new (int default_only); + struct nbdkit_exports *nbdkit_exports_new (void);
-Allocates and returns a new, empty exports list. The C<default_only> -parameter should match whether the list is intended to grab the -canonical name of the default export, or all exports. +Allocates and returns a new, empty exports list.
On error this function can return C<NULL>. In this case it calls C<nbdkit_error> as required. C<errno> will be set to a suitable @@ -408,6 +412,20 @@ Returns the number of exports in the list.
Returns a copy of the C<i>'th export.
+=head2 C<.default_export> + + const char *default_export (nbdkit_next_default_export *next, void *nxdata, + int readonly, int is_tls) + +This intercepts the plugin C<.default_export> method and can be used to +alter the canonical export name used in place of the default C<"">. + +The C<readonly> parameter matches what is passed to <.preconnect> and +C<.open>, and may be changed by the filter when calling into the +plugin. The C<is_tls> parameter informs the filter whether TLS +negotiation has been completed by the client, but is not passed on to +C<next> because it cannot be altered. + =head2 C<.open>
void * (*open) (nbdkit_next_open *next, void *nxdata, @@ -527,6 +545,15 @@ This function is only called once per connection and cached by nbdkit. Similarly, repeated calls to C<next_ops-E<gt>get_size> will return a cached value.
+=head2 C<.export_description> + + const char *export_description (struct nbdkit_next_ops *next_ops, + void *nxdata, void *handle); + +This intercepts the plugin C<.export_description> method and can be +used to read or modify the export description that the NBD client +will see. + =head2 C<.can_write>
=head2 C<.can_flush> diff --git a/docs/nbdkit-plugin.pod b/docs/nbdkit-plugin.pod index 5c641e83..ecb6bba8 100644 --- a/docs/nbdkit-plugin.pod +++ b/docs/nbdkit-plugin.pod @@ -245,6 +245,12 @@ Early in option negotiation the client may try to list the exports served by the plugin, and plugins can optionally implement this callback to answer the client. See L</EXPORT NAME> below.
+=item C<.default_export> + +During option negotiation, if the client requests the default export +name (C<"">), this optional callback provides a canonical name to +use in its place prior to calling C<.open>. + =item C<.open>
A new client has connected and finished the NBD handshake. TLS @@ -664,15 +670,16 @@ error message and return C<-1>.
=head2 C<.list_exports>
- int list_exports (int readonly, int default_only, + int list_exports (int readonly, int is_tls, struct nbdkit_exports *exports);
This optional callback is called if the client tries to list the exports served by the plugin (using C<NBD_OPT_LIST>). If the plugin -does not supply this callback then a single export called C<""> is -returned. The NBD protocol defines C<""> as the default export, so -this is suitable for plugins which ignore the export name and always -serve the same content. See also L</EXPORT NAME> below. +does not supply this callback then the reply is determined by the +result of C<.default_export>, which in turn defaults to advertising +the name C<"">. The NBD protocol defines C<""> as the default export, +so this default is suitable for plugins which ignore the export name +and always serve the same content. See also L</EXPORT NAME> below.
The C<readonly> flag informs the plugin that the server was started with the I<-r> flag on the command line, which is the same value @@ -681,11 +688,11 @@ not yet have a way to let the client advertise an intent to be read-only even when the server allows writes, so this parameter may not be as useful as it appears.
-If the C<default_only> flag is set then the client is querying for the -name of the default export, and the plugin may optimize by adding only -a single export to the returned list (the default export name, usually -C<"">). The plugin can ignore this flag and return all exports if it -wants. +The C<is_tls> flag informs the plugin whether this listing was +requested after the client has completed TLS negotiation. When +running the server in a mode that permits but not requires TLS, +be careful that any exports listed when C<is_tls> is C<false> do +not leak unintended information.
The C<exports> parameter is an opaque object for collecting the list of exports. Call C<nbdkit_add_export> to add a single export to the @@ -709,6 +716,37 @@ Returning C<0> will send the list of exports back to the client. If there is an error, C<.list_exports> should call C<nbdkit_error> with an error message and return C<-1>.
+=head2 C<.default_export> + + const char *default_export (int readonly, int is_tls); + +This optional callback is called if the client tries to connect to +the default export C<"">. If the plugin does not supply this callback, +the connection continues with the empty name; if the plugin returns +a string, nbdkit returns that name to clients that support it (see +the C<NBD_INFO_NAME> response to C<NBD_OPT_GO>), and behaves as if +the client had passed that string instead of an empty name. Similarly, +if the plugin does not supply a C<.list_exports> callback, the result +of this callback determines what export name to advertise to a client +requesting an export list. + +The C<readonly> flag informs the plugin that the server was started +with the I<-r> flag on the command line, which is the same value +passed to C<.preconnect> and C<.open>. However, the NBD protocol does +not yet have a way to let the client advertise an intent to be +read-only even when the server allows writes, so this parameter may +not be as useful as it appears. + +The C<is_tls> flag informs the plugin whether the canonical name for +the default export is being requested after the client has completed +TLS negotiation. When running the server in a mode that permits but +not requires TLS, be careful that a default export name does not leak +unintended information. + +If the plugin returns C<NULL>, the client is not permitted to connect +to the default export. However, this is not an error in the protocol, +so it is not necessary to call C<nbdkit_error>. + =head2 C<.open>
void *open (int readonly); @@ -769,6 +807,22 @@ to get the size (in bytes) of the block device being exported. The returned size must be E<ge> 0. If there is an error, C<.get_size> should call C<nbdkit_error> with an error message and return C<-1>.
+=head2 C<.export_description> + + const char *export_description (void *handle); + +This is called during the option negotiation phase only if the +client specifically requested an export description (see the +C<NBD_INFO_DESCRIPTION> response to C<NBD_OPT_GO>). Any +description provided must be human-readable UTF-8, no longer +than 4096 bytes. Ideally, this description should match any +description set during C<.list_exports>, but that is not +enforced. + +If the plugin returns C<NULL>, or if this callback is omitted, no +description is offered to the client. As this is not an error in the +protocol, it is not necessary to call C<nbdkit_error>. + =head2 C<.can_write>
int can_write (void *handle); -- 2.28.0
_______________________________________________ Libguestfs mailing list Libguestfs@redhat.com https://www.redhat.com/mailman/listinfo/libguestfs
-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW
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. 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. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
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/
On 8/25/20 5:00 AM, Richard W.M. Jones wrote:
+=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.
Plugins cannot access the TCP connection except through public APIs ;) I've got the patch working (it passes 'make check-valgrind'), so I'll post it later this morning to demo why it is useful.
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.
The server IS doing the interning, and associating the intern'd strings with the connection or with overall server exit. But plugins need to use it when they need a longer lifetime but don't want to clean up themselves. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On Tue, Aug 25, 2020 at 06:16:17AM -0500, Eric Blake wrote:
On 8/25/20 5:00 AM, Richard W.M. Jones wrote:
+=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.
Plugins cannot access the TCP connection except through public APIs ;)
Sure, plugins can't, but there's some code in the server which you've written which could be doing: GET_CONN; const char *new_name; ... new_name = plugin->default_export (...); if (conn->default_exportname != NULL) free (conn->default_exportname); conn->default_exportname = strdup (new_name);
I've got the patch working (it passes 'make check-valgrind'), so I'll post it later this morning to demo why it is useful.
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.
The server IS doing the interning, and associating the intern'd strings with the connection or with overall server exit. But plugins need to use it when they need a longer lifetime but don't want to clean up themselves.
I must be missing something here ... 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 8/25/20 7:21 AM, Richard W.M. Jones wrote:
On Tue, Aug 25, 2020 at 06:16:17AM -0500, Eric Blake wrote:
On 8/25/20 5:00 AM, Richard W.M. Jones wrote:
+=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.
Plugins cannot access the TCP connection except through public APIs ;)
Sure, plugins can't, but there's some code in the server which you've written which could be doing:
GET_CONN; const char *new_name; ... new_name = plugin->default_export (...); if (conn->default_exportname != NULL) free (conn->default_exportname); conn->default_exportname = strdup (new_name);
So you're suggesting that the signature for .default_export should return 'char *' of malloc()d storage, and that nbdkit is then responsible for free()ing that string. It's an ABI coupling; it means we are forcing the plugin and server to agree on which allocation routines to use for any information passed across that boundary. And it forces plugins to malloc memory, even when their answer is a compile-time constant. True, we've already done that with nbdkit_realpath and nbdkit_absolute_path (nbdkit calls malloc(), so the plugin must call free()), but elsewhere we've tried avoiding it to reduce the ABI dependency and allow us (either the server or the plugin) the freedom to use an alternative allocator scheme (witness nbdkit_extents_new() - the client needs memory allocated, but that allocation is wrapped inside a public nbdkit function, so nbdkit itself is now responsible for both the allocation and freeing of the memory, and is no longer limited to malloc()/free() dependency).
The server IS doing the interning, and associating the intern'd strings with the connection or with overall server exit. But plugins need to use it when they need a longer lifetime but don't want to clean up themselves.
I must be missing something here ...
I'd better post the patch, then, as code speaks better than prose. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
I think I get where my misunderstanding is now. I thought the problem was that the server could not store the string, but your concern is that the plugin cannot free the string after returning it to the caller (server). I can't really think of a better way to do it than the interning option you presented in the initial email. Interning might also be useful in future cases where we want to send strings to the server from the plugin. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top
participants (2)
-
Eric Blake -
Richard W.M. Jones