On Tue, Aug 25, 2020 at 10:46:58AM -0500, Eric Blake wrote: > Instead of needing to provide .unload, we can let nbdkit manage the > lifetime for us. However, if we like this approach, it may be wiser > to introduce new variants of nbdkit_realpath/absolute_path which > return const char * already intern'd. > > Signed-off-by: Eric Blake <[email protected]> > --- > > See the cover letter: if we like this, there are several other plugins > that could also reduce their .unload, or maybe we want to add > convenience functions to make the combo > 'nbdkit_realpath/nbdkit_string_intern' simpler. > > plugins/file/file.c | 20 ++++++-------------- > 1 file changed, 6 insertions(+), 14 deletions(-) > > diff --git a/plugins/file/file.c b/plugins/file/file.c > index 08418194..6a0aad93 100644 > --- a/plugins/file/file.c > +++ b/plugins/file/file.c > @@ -64,8 +64,8 @@ > #define fdatasync fsync > #endif > > -static char *filename = NULL; > -static char *directory = NULL; > +static const char *filename = NULL; > +static const char *directory = NULL; > > /* posix_fadvise mode: -1 = don't set it, or POSIX_FADV_*. */ > static int fadvise_mode = > @@ -91,13 +91,6 @@ is_enotsup (int err) > return err == ENOTSUP || err == EOPNOTSUPP; > } > > -static void > -file_unload (void) > -{ > - free (filename); > - free (directory); > -} > - > /* Called for each key=value passed on the command line. This plugin > * only accepts file=<filename> and dir=<dirname>, where exactly > * one is required. > @@ -111,15 +104,15 @@ file_config (const char *key, const char *value) > * existence checks to the last possible moment. > */ > if (strcmp (key, "file") == 0) { > - free (filename); > - filename = nbdkit_realpath (value); > + CLEANUP_FREE char *tmp = nbdkit_realpath (value); > + filename = nbdkit_string_intern (tmp); > if (!filename) > return -1; > } > else if (strcmp (key, "directory") == 0 || > strcmp (key, "dir") == 0) { > - free (directory); > - directory = nbdkit_realpath (value); > + CLEANUP_FREE char *tmp = nbdkit_realpath (value); > + directory = nbdkit_string_intern (value); > if (!directory) > return -1; > } > @@ -812,7 +805,6 @@ static struct nbdkit_plugin plugin = { > .name = "file", > .longname = "nbdkit file plugin", > .version = PACKAGE_VERSION, > - .unload = file_unload, > .config = file_config, > .config_complete = file_config_complete, > .config_help = file_config_help, > --
While this is kind of nice, it also IMHO demonstrates why having the intern function do two different things implicitly has the potential to cause confusion for users. A developer might believe they could copy the pattern > + CLEANUP_FREE char *tmp = nbdkit_realpath (value); > + directory = nbdkit_string_intern (value); into a connected function, but would find that the string gets freed at close rather than unload. 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 _______________________________________________ Libguestfs mailing list [email protected] https://www.redhat.com/mailman/listinfo/libguestfs
