Libguestfs - Feb 2018 - Re: [nbdkit PATCH v2] plugin: add and use nbdkit_realpath

Introduce a new helper function to resolve a path name, calling
nbdkit_error on failure: other than doing what nbdkit_absolute_path
does, it also checks that the file exists (and thus avoids errors later
on).  To help distinguish it from nbdkit_absolute_path, improve the
documentation of the latter.

Apply it where an existing path is required, both in nbdkit itself and
in plugins.

Related to: https://bugzilla.redhat.com/show_bug.cgi?id=1527334
---
 docs/nbdkit-plugin.pod      | 18 +++++++++++++++++-
 include/nbdkit-common.h     |  1 +
 plugins/example2/example2.c |  2 +-
 plugins/file/file.c         |  6 +-----
 plugins/gzip/gzip.c         |  2 +-
 plugins/split/split.c       |  2 +-
 plugins/vddk/vddk.c         |  2 +-
 plugins/xz/xz.c             |  2 +-
 src/plugins.c               |  2 +-
 src/utils.c                 | 19 +++++++++++++++++++
 10 files changed, 44 insertions(+), 12 deletions(-)

diff --git a/docs/nbdkit-plugin.pod b/docs/nbdkit-plugin.pod
index 44822fc..5faba1d 100644
--- a/docs/nbdkit-plugin.pod
+++ b/docs/nbdkit-plugin.pod
@@ -200,13 +200,29 @@ descriptor.
  char *nbdkit_absolute_path (const char *filename);
 
 The utility function C<nbdkit_absolute_path> converts any path to an
-absolute path.
+absolute path: if it is relative, then all this function does is
+prepending the current working directory to the path, with no extra
+checks.
 
 If conversion was not possible, this calls C<nbdkit_error> and returns
 C<NULL>.  Note that this function does not check that the file exists.
 
 The returned string must be freed by the caller.
 
+=head2 C<nbdkit_realpath>
+
+ char *nbdkit_realpath (const char *filename);
+
+The utility function C<nbdkit_realpath> converts any path to an
+absolute path, resolving symlinks.  Under the hood it uses the
+C<realpath> function, and thus it fails if the path does not exist,
+or it is not possible to access to any of the components of the path.
+
+If the path resolution was not possible, this calls C<nbdkit_error>
+and returns C<NULL>.
+
+The returned string must be freed by the caller.
+
 =head1 CALLBACKS
 
 =head2 C<.name>
diff --git a/include/nbdkit-common.h b/include/nbdkit-common.h
index 5e69579..693213f 100644
--- a/include/nbdkit-common.h
+++ b/include/nbdkit-common.h
@@ -60,6 +60,7 @@ extern void nbdkit_vdebug (const char *msg, va_list args);
 extern char *nbdkit_absolute_path (const char *path);
 extern int64_t nbdkit_parse_size (const char *str);
 extern int nbdkit_read_password (const char *value, char **password);
+extern char *nbdkit_realpath (const char *path);
 
 #ifdef __cplusplus
 }
diff --git a/plugins/example2/example2.c b/plugins/example2/example2.c
index 5bc4f94..a2d6fca 100644
--- a/plugins/example2/example2.c
+++ b/plugins/example2/example2.c
@@ -78,7 +78,7 @@ example2_config (const char *key, const char *value)
 {
   if (strcmp (key, "file") == 0) {
     /* See FILENAMES AND PATHS in nbdkit-plugin(3). */
-    filename = nbdkit_absolute_path (value);
+    filename = nbdkit_realpath (value);
     if (!filename)
       return -1;
   }
diff --git a/plugins/file/file.c b/plugins/file/file.c
index f8cb3d3..b6e33de 100644
--- a/plugins/file/file.c
+++ b/plugins/file/file.c
@@ -65,7 +65,7 @@ file_config (const char *key, const char *value)
   if (strcmp (key, "file") == 0) {
     /* See FILENAMES AND PATHS in nbdkit-plugin(3). */
     free (filename);
-    filename = nbdkit_absolute_path (value);
+    filename = nbdkit_realpath (value);
     if (!filename)
       return -1;
   }
@@ -90,10 +90,6 @@ file_config_complete (void)
     nbdkit_error ("you must supply the file=<FILENAME> parameter
after the plugin name on the command line");
     return -1;
   }
-  if (access (filename, F_OK) < 0) {
-    nbdkit_error ("access '%s': %m", filename);
-    return -1;
-  }
 
   return 0;
 }
diff --git a/plugins/gzip/gzip.c b/plugins/gzip/gzip.c
index e9dbfdb..09dd629 100644
--- a/plugins/gzip/gzip.c
+++ b/plugins/gzip/gzip.c
@@ -62,7 +62,7 @@ gzip_config (const char *key, const char *value)
 {
   if (strcmp (key, "file") == 0) {
     /* See FILENAMES AND PATHS in nbdkit-plugin(3). */
-    filename = nbdkit_absolute_path (value);
+    filename = nbdkit_realpath (value);
     if (!filename)
       return -1;
   }
diff --git a/plugins/split/split.c b/plugins/split/split.c
index 47c366d..bdcdcf7 100644
--- a/plugins/split/split.c
+++ b/plugins/split/split.c
@@ -76,7 +76,7 @@ split_config (const char *key, const char *value)
       return -1;
     }
     filenames = new_filenames;
-    filenames[nr_files] = nbdkit_absolute_path (value);
+    filenames[nr_files] = nbdkit_realpath (value);
     if (filenames[nr_files] == NULL)
       return -1;
     nr_files++;
diff --git a/plugins/vddk/vddk.c b/plugins/vddk/vddk.c
index 1c15127..8bc1517 100644
--- a/plugins/vddk/vddk.c
+++ b/plugins/vddk/vddk.c
@@ -153,7 +153,7 @@ vddk_config (const char *key, const char *value)
   if (strcmp (key, "config") == 0) {
     /* See FILENAMES AND PATHS in nbdkit-plugin(3). */
     free (config);
-    config = nbdkit_absolute_path (value);
+    config = nbdkit_realpath (value);
     if (!config)
       return -1;
   }
diff --git a/plugins/xz/xz.c b/plugins/xz/xz.c
index 437f798..f45e489 100644
--- a/plugins/xz/xz.c
+++ b/plugins/xz/xz.c
@@ -67,7 +67,7 @@ xz_config (const char *key, const char *value)
 {
   if (strcmp (key, "file") == 0) {
     /* See FILENAMES AND PATHS in nbdkit-plugin(3). */
-    filename = nbdkit_absolute_path (value);
+    filename = nbdkit_realpath (value);
     if (!filename)
       return -1;
   }
diff --git a/src/plugins.c b/src/plugins.c
index dba3e24..595b632 100644
--- a/src/plugins.c
+++ b/src/plugins.c
@@ -134,7 +134,7 @@ plugin_dump_fields (struct backend *b)
   struct backend_plugin *p = container_of (b, struct backend_plugin, backend);
   char *path;
 
-  path = nbdkit_absolute_path (p->filename);
+  path = nbdkit_realpath (p->filename);
   printf ("path=%s\n", path);
   free (path);
 
diff --git a/src/utils.c b/src/utils.c
index 0083370..c6c8003 100644
--- a/src/utils.c
+++ b/src/utils.c
@@ -228,3 +228,22 @@ nbdkit_read_password (const char *value, char **password)
 
   return 0;
 }
+
+char *
+nbdkit_realpath (const char *path)
+{
+  char *ret;
+
+  if (path == NULL || *path == '\0') {
+    nbdkit_error ("cannot resolve a null or empty path");
+    return NULL;
+  }
+
+  ret = realpath (path, NULL);
+  if (ret == NULL) {
+    nbdkit_error ("realpath(%s): %m", path);
+    return NULL;
+  }
+
+  return ret;
+}
-- 
2.14.3

On 02/14/2018 10:53 AM, Pino Toscano wrote:
> Introduce a new helper function to resolve a path name, calling
> nbdkit_error on failure: other than doing what nbdkit_absolute_path
> does, it also checks that the file exists (and thus avoids errors later
> on).  To help distinguish it from nbdkit_absolute_path, improve the
> documentation of the latter.
> 
> Apply it where an existing path is required, both in nbdkit itself and
> in plugins.
> 
> Related to: https://bugzilla.redhat.com/show_bug.cgi?id=1527334
> ---
> +++ b/src/utils.c
> @@ -228,3 +228,22 @@ nbdkit_read_password (const char *value, char
**password)
>   
>     return 0;
>   }
> +
> +char *
> +nbdkit_realpath (const char *path)
> +{
> +  char *ret;
> +
> +  if (path == NULL || *path == '\0') {
> +    nbdkit_error ("cannot resolve a null or empty path");
> +    return NULL;
> +  }
> +
> +  ret = realpath (path, NULL);
Wait. Does this even work?  Remember, in nbdkit_absolute_path(), we are 
prepending get_current_dir_name() (why the GNU spelling, instead of 
getcwd(NULL, 0), since either way is a GNU extension?).  But in main.c, 
we call an unconditional chdir("/") as part of fork_into_background, 
which has the annoying result that when using 'nbdkit -f' you honor your
current $PWD, but when running in the background, relative filenames are 
now parsed against /.  Shouldn't we fix it so that nbdkit saves off 
getcwd() up front, prior to fork_into_background, and then both 
nbdkit_absolute_path() and nbdkit_realpath() convert any relative names 
into something relative to the ORIGINAL working directory, regardless of 
foreground or background operation?

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

On Wednesday, 14 February 2018 18:06:10 CET Eric Blake
wrote:
> On 02/14/2018 10:53 AM, Pino Toscano wrote:
> > Introduce a new helper function to resolve a path name, calling
> > nbdkit_error on failure: other than doing what nbdkit_absolute_path
> > does, it also checks that the file exists (and thus avoids errors
later
> > on).  To help distinguish it from nbdkit_absolute_path, improve the
> > documentation of the latter.
> > 
> > Apply it where an existing path is required, both in nbdkit itself and
> > in plugins.
> > 
> > Related to: https://bugzilla.redhat.com/show_bug.cgi?id=1527334
> > ---
> 
> > +++ b/src/utils.c
> > @@ -228,3 +228,22 @@ nbdkit_read_password (const char *value, char
**password)
> >   
> >     return 0;
> >   }
> > +
> > +char *
> > +nbdkit_realpath (const char *path)
> > +{
> > +  char *ret;
> > +
> > +  if (path == NULL || *path == '\0') {
> > +    nbdkit_error ("cannot resolve a null or empty path");
> > +    return NULL;
> > +  }
> > +
> > +  ret = realpath (path, NULL);
> 
> Wait. Does this even work?
It works in the same way as nbdkit_absolute_path() did: when calling it
on a relative path, nbdkit_absolute_path() will prepend $PWD to it,
while the new nbdkit_realpath() will do something similar.

At least the usages that I changed are called before start_serving()
(and thus before fork_into_background()), so I think there should be no
issue wrt paths.

Did I miss anything?

-- 
Pino Toscano