[Libguestfs] [RFC nbdkit PATCH 4/5] file: Utilize nbdkit_string_intern

[Richard W.M. Jones]

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 <eblake at redhat.com>
> ---
> 
> 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