[Gluster-devel] doubts in posix_handle_path and posix_handle_pump

[Anand Avati]

On 6/3/14, 3:49 AM, Xavier Hernandez wrote:
> On Tuesday 03 June 2014 15:42:19 Pranith Kumar Karampuri wrote:
>> On 06/03/2014 02:42 PM, Xavier Hernandez wrote:
>>> The possible problem I see is that in the comments it says that this
>>> function returns a path to an IA_IFDIR (it will return IA_IFDIR on an
>>> lstat), however if one of the symlinks is missing or anything else fails,
>>> it won't return an error *but* it will return a path to an existing file.
>>> An lstat on this path will return IA_IFLNK instead of IA_IFDIR. I don't
>>> know if this can be a problem in some places.

I don't see how it could return a path which returns IA_IFLNK, no matter 
what. The very first path generated would be a path to a symlink. If it 
is missing, the very first pump iteration would fail, and we would be 
left with a path to a non-existent file. After that, at any poing we 
would either be left with a path which ELOOP's or points to a dir. If an 
intermediate symlink is missing, the entire path itself will give ENOENT 
on all operations. In any case we should never end up with a path 
pointing to IA_IFLNK.

>>
>> This is exactly what I was referring to, I don't see an easy way to find
>> out if there is any failure in the function. One needs to do extra lstat
>> or a 'path' based syscall like getxattr etc on the returned path to
>> check if it returned a good path. So do you think the best thing is to
>> ignore the return value of the function call but depend on an lstat or a
>> path based syscall of the path?

Directly calling getxattr() instead of lgetxattr() is a dangerous thing 
to do in posix. We should never, even accidentally, follow a user's 
symlink. Always use the lXXXX() variant syscall unless you really really 
know what you are doing.

>>
> The only point to consider is for gfid's representing directories. Other types
> of file do not have any problem (the returned path can be considered valid
> even if lstat() fails). For directories there are 3 places where things can
> fail:
>
> At line 360: I think this is not a problem. If lstat() fails (basically
> because it does not exist), the returned path can be considered valid.
>
> At line 367: If posix_handle_pump() fails, it could mean:
>
> * The symlink is not a valid directory symlink:
>    * it's a corrupted one: any operation on this file should be denied
>    * it's a normal symlink that has lost one of the hard-links: though it's bad
> to have damaged gfid's, the returned path can be considered valid.
>
> * readlink() failed: this woulb be very weird. Access to the file should be
> denied.
>
> At line 374: If lstat() fails, probably it means that the symlink of one of
> the parents of the directory is missing. The returned path won't fail on
> lstat(), but it should because it will return symlink information instead of
> directory information.
>
> I think that it's very hard to determine if something went wrong only by
> inspecting the returned path. I think the best approach would be that
> posix_handle_path() return -1 if posix_handle_pump() or lstat() at line 374
> fail, and each caller decide what to do in case of failure.
>
> However I don't know all the details of the posix xlator, so maybe I'm wrong
> and this is not necessary. Let's see if there is someone else with more
> experience on it to see what he thinks.
>


We could add the above check and return -1. But note that the point of 
the function is to create a path for a certain operation. But the 
operation itself will anyways fail with the desired errno if the path 
could not be "pumped up" (for whatever reason) or if an intermediate 
symlink is missing.

Thanks,
Avati