[Libguestfs] [nbdkit PATCH v2] plugin: add and use nbdkit_realpath

[Eric Blake]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20180406/96edde47/attachment.sig>