Re: [Qemu-devel] [RFC PATCH] block: for a streaming job, fix relative base name arguments

[Kevin Wolf]

Am 12.04.2012 15:17, schrieb Jeff Cody:
>>> + *
>>> + * Returns NULL if no relative or absolute path can be found.
>>> + */
>>> +static char *path_find_relative(char *current, char *backing)
>>> +{
>>> +    char *src;
>>> +    char *dest;
>>> +    char *src_tmp;
>>> +    char *src_dir;
>>> +    char *rel_backing = NULL;
>>> +    char relpath[PATH_MAX] = {0};
>>> +    int offset = 0;
>>> +
>>> +
>>> +    src = realpath(current, NULL);
>>
>> My POSIX manpage says:
>>
>> "If resolved_name is a null pointer, the behavior of realpath() is
>> implementation-defined."
>>
>> It seems to have been standardised by now, but I'm not sure if it is a
>> good idea to rely on a quite new feature on some OSes.
>>
> 
> As the comments pointed out by Eric and Paolo indicate, the issue is
> worse on some platforms. It would be nice to be able to rely on
> canonicalize_file_name(), so I like Daniel's suggestion of pulling in
> Paolo's proposed glib patch into QEMU.  Any objections to bundling
> Paolo's patch with my next (v1) submission?

I haven't looked at that patch yet, but if it's licensed appropriately
(IIUC, Paolo is not the only copyright owner), I think we can pull it in.

>>> +    }
>>> +
>>> +    src_tmp = g_strdup(src);
>>> +
>>> +    if (!strcmp(backing, dest)) {
>>> +        rel_backing = g_strdup(backing);
>>> +        goto free_and_exit;
>>> +    }
>>
>> This is a weaker form of path_is_absolute(), right? It only returns
>> backing if the path is absolute and canonical. I don't think we really
>> need the latter condition.
>>
> 
> Agree, and I would go further and say we don't need to return if it is
> absolute.  Just let the function do as its name implies, and always
> return a relative path (except on error, as suggested below).

Yes, I think that makes sense.

>>> +            break;
>>> +        } else if (strlen(src_tmp) <= 1) {
>>> +            break;
>>> +        }
>>> +        src_dir = dirname(src_tmp);
>>> +        g_strlcpy(src_tmp, src_dir, strlen(src));
>>
>> Same as above.
>>
> 
> OK.  Although, if src_tmp starts out as canonical (src), dirname should
> never return something longer than src.

Hm, unexpected answer... Just to make sure we're not talking past each
other: I meant the undefined behaviour because src_dir and src_tmp can
overlap.

Kevin