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
_______________________________________________
Gluster-devel mailing list
Gluster-devel@xxxxxxxxxxx
http://supercolony.gluster.org/mailman/listinfo/gluster-devel
