Pino Toscano
2018-Feb-21 17:13 UTC
Re: [Libguestfs] [nbdkit PATCH v2] plugin: add and use nbdkit_realpath
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
Eric Blake
2018-Feb-21 18:52 UTC
Re: [Libguestfs] [nbdkit PATCH v2] plugin: add and use nbdkit_realpath
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
Eric Blake
2018-Apr-06 16:02 UTC
Re: [Libguestfs] [nbdkit PATCH v2] plugin: add and use nbdkit_realpath
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
Possibly Parallel Threads
- Re: [nbdkit PATCH v2] plugin: add and use nbdkit_realpath
- [nbdkit PATCH v2] plugin: add and use nbdkit_realpath
- Re: [nbdkit PATCH v2] plugin: add and use nbdkit_realpath
- [nbdkit PATCH] plugin: add and use nbdkit_realpath
- [nbdkit PATCH v3 0/2] Add nbdkit_realpath