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

[Jeff Cody]

On 04/12/2012 09:41 AM, Kevin Wolf wrote:
> 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.
> 

OK, we were talking past each other.  I thought you meant if g_strlcpy()
truncated, due to strlen(src_dir) being longer than strlen(src).


> Kevin