[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
On 02/21/2018 11:13 AM, Pino Toscano wrote:
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 ---
+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.
My question is whether the $PWD it is prepending is guaranteed to be correct. If we call this too late, then it is not; it would be trivial to rewrite it so that we manually prepend something cached from when the program started, so that this is then conceptually correct no matter when we change directories later.
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.
Oh. Hmm. Interesting observation, that .config and .config_complete finish PRIOR to start_serving() (and makes sense, too, as if we're going to diagnose any errors, printing to stderr is a LOT easier at the point where we haven't forked and redirected stderr). But maybe that means that we should document that these two functions are available for plugins/filters, but ONLY for use during .config/.config_complete; that any time later they are unsafe. OR, we can fix them to ALWAYS work, even if they are invoked somewhere outside of .config.
Did I miss anything?
No, but I missed that .config runs before forking. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Reviving this thread... On 02/21/2018 12:52 PM, Eric Blake wrote:
On 02/21/2018 11:13 AM, Pino Toscano wrote:
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 ---
+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.
My question is whether the $PWD it is prepending is guaranteed to be correct. If we call this too late, then it is not; it would be trivial to rewrite it so that we manually prepend something cached from when the program started, so that this is then conceptually correct no matter when we change directories later.
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.
Oh. Hmm. Interesting observation, that .config and .config_complete finish PRIOR to start_serving() (and makes sense, too, as if we're going to diagnose any errors, printing to stderr is a LOT easier at the point where we haven't forked and redirected stderr). But maybe that means that we should document that these two functions are available for plugins/filters, but ONLY for use during .config/.config_complete; that any time later they are unsafe. OR, we can fix them to ALWAYS work, even if they are invoked somewhere outside of .config.
Did I miss anything?
No, but I missed that .config runs before forking.
Other utility functions (nbdkit_parse_size, nbdkit_read_password) are also mainly useful only during .config, in that they print error messages if called there; but appear to work correctly even if called during .pread for whatever reason (the error message is no longer printed to stderr, but at least they aren't called relative to theh wrong directory). So, is this something where we merely document the issue (the utility functions are only useful during .config, and may not work as intended if called during .pread and friends)? Or do we go for the robustness angle, and rewrite both the existing nbdkit_absolute_path() and your new function (which DOES add the nice benefit of checking for ENOENT) to cache the original working directory so they can work even across daemonizing and chdir("/"), even though the error message portion only works during .config? -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
participants (2)
-
Eric Blake -
Pino Toscano