[PATCH 1/2] add lpath_to_handle to libhandle

[PATCH 1/2] add lpath_to_handle to libhandle

[Bill Kendall]

path_to_handle() is not reliable when called on a path which
is a symlink. If the symlink is dangling, or if its points
to a non-XFS filesystem then path_to_handle() will fail. The
reason is that path_to_handle() must open the path in order
to obtain an fd for the xfsctl call.

It's common during xfsrestore to have dangling symlinks since
the target of the link may not be restored before the symlink.

This patch adds a new function to libhandle, lpath_to_handle.
It is just like path_to_handle, except it takes a filesystem
path in addition to the path which you want convert to a
handle.

Alex Elder is going to take care of bumping the libhandle
minor number, and adjusting the xfsdump/xfsprogs version numbers
and dependencies to ensure a compatible libhandle is installed
for xfsdump.

Signed-off-by: Bill Kendall <wkendall@sgi.com>

diff --git a/include/handle.h b/include/handle.h
index b211a2f..3f1a137 100644
--- a/include/handle.h
+++ b/include/handle.h
@@ -27,6 +27,8 @@ struct attrlist_cursor;
  struct parent;

  extern int  path_to_handle (char *__path, void **__hanp, size_t *__hlen);
+extern int  lpath_to_handle (char *__fspath, char *__path,
+			     void **__hanp, size_t *__hlen);
  extern int  path_to_fshandle (char *__path, void **__fshanp, size_t *__fshlen);
  extern int  handle_to_fshandle (void *__hanp, size_t __hlen, void **__fshanp,
  				size_t *__fshlen);
diff --git a/libhandle/handle.c b/libhandle/handle.c
index 6276797..6c9380d 100644
--- a/libhandle/handle.c
+++ b/libhandle/handle.c
@@ -111,16 +111,29 @@ path_to_handle(
  	void		**hanp,		/* output, pointer to data */
  	size_t		*hlen)		/* output, size of returned data */
  {
+	return lpath_to_handle(path, path, hanp, hlen);
+}
+
+/* Like path_to_handle, but reliable for paths which are either dangling
+ * symlinks or symlinks whose targets are not in XFS filesystems.
+ */
+int
+lpath_to_handle(
+	char		*fspath,	/* input,  path in filesystem */
+	char		*path,		/* input,  path to convert */
+	void		**hanp,		/* output, pointer to data */
+	size_t		*hlen)		/* output, size of returned data */
+{
  	int		fd;
  	int		result;
  	comarg_t	obj;

-	fd = open(path, O_RDONLY);
+	fd = open(fspath, O_RDONLY);
  	if (fd < 0)
  		return -1;

  	obj.path = path;
-	result = obj_to_handle(path, fd, XFS_IOC_PATH_TO_HANDLE,
+	result = obj_to_handle(fspath, fd, XFS_IOC_PATH_TO_HANDLE,
  				obj, hanp, hlen);
  	close(fd);
  	return result;

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

RE: [PATCH 1/2] add lpath_to_handle to libhandle

[Alex Elder]

Bill Kendall wrote:
> path_to_handle() is not reliable when called on a path which
> is a symlink. If the symlink is dangling, or if its points
> to a non-XFS filesystem then path_to_handle() will fail. The
> reason is that path_to_handle() must open the path in order
> to obtain an fd for the xfsctl call.
> 
> It's common during xfsrestore to have dangling symlinks since
> the target of the link may not be restored before the symlink.
> 
> This patch adds a new function to libhandle, lpath_to_handle.
> It is just like path_to_handle, except it takes a filesystem
> path in addition to the path which you want convert to a
> handle.
> 
> Alex Elder is going to take care of bumping the libhandle
> minor number, and adjusting the xfsdump/xfsprogs version numbers
> and dependencies to ensure a compatible libhandle is installed
> for xfsdump.

Looks good.  I will up the version number after get this pulled in.
I will publish this and get the version number/dependency stuff
straightened out before I take the change for xfsrestore.

					-Alex

> Signed-off-by: Bill Kendall <wkendall@sgi.com>

Reviewed-by: Alex Elder <aelder@sgi.com>

> diff --git a/include/handle.h b/include/handle.h
> index b211a2f..3f1a137 100644
> --- a/include/handle.h
> +++ b/include/handle.h
> @@ -27,6 +27,8 @@ struct attrlist_cursor;
>   struct parent;
> 
>   extern int  path_to_handle (char *__path, void **__hanp, size_t *__hlen);
> +extern int  lpath_to_handle (char *__fspath, char *__path,
> +			     void **__hanp, size_t *__hlen);
>   extern int  path_to_fshandle (char *__path, void **__fshanp, size_t *__fshlen);
>   extern int  handle_to_fshandle (void *__hanp, size_t __hlen, void **__fshanp,
>   				size_t *__fshlen);
> diff --git a/libhandle/handle.c b/libhandle/handle.c
> index 6276797..6c9380d 100644
> --- a/libhandle/handle.c
> +++ b/libhandle/handle.c
> @@ -111,16 +111,29 @@ path_to_handle(
>   	void		**hanp,		/* output, pointer to data */
>   	size_t		*hlen)		/* output, size of returned data */
>   {
> +	return lpath_to_handle(path, path, hanp, hlen);
> +}
> +
> +/* Like path_to_handle, but reliable for paths which are either dangling
> + * symlinks or symlinks whose targets are not in XFS filesystems.
> + */
> +int
> +lpath_to_handle(
> +	char		*fspath,	/* input,  path in filesystem */
> +	char		*path,		/* input,  path to convert */
> +	void		**hanp,		/* output, pointer to data */
> +	size_t		*hlen)		/* output, size of returned data */
> +{
>   	int		fd;
>   	int		result;
>   	comarg_t	obj;
> 
> -	fd = open(path, O_RDONLY);
> +	fd = open(fspath, O_RDONLY);
>   	if (fd < 0)
>   		return -1;
> 
>   	obj.path = path;
> -	result = obj_to_handle(path, fd, XFS_IOC_PATH_TO_HANDLE,
> +	result = obj_to_handle(fspath, fd, XFS_IOC_PATH_TO_HANDLE,
>   				obj, hanp, hlen);
>   	close(fd);
>   	return result;
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 1/2] add lpath_to_handle to libhandle

[Christoph Hellwig]

On Fri, Oct 23, 2009 at 01:08:37PM -0500, Alex Elder wrote:
> Looks good.  I will up the version number after get this pulled in.
> I will publish this and get the version number/dependency stuff
> straightened out before I take the change for xfsrestore.

Please hold back for a while with making a release.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 1/2] add lpath_to_handle to libhandle

[Christoph Hellwig]

On Thu, Oct 22, 2009 at 11:52:23AM -0500, Bill Kendall wrote:
> path_to_handle() is not reliable when called on a path which
> is a symlink. If the symlink is dangling, or if its points
> to a non-XFS filesystem then path_to_handle() will fail. The
> reason is that path_to_handle() must open the path in order
> to obtain an fd for the xfsctl call.
>
> It's common during xfsrestore to have dangling symlinks since
> the target of the link may not be restored before the symlink.
>
> This patch adds a new function to libhandle, lpath_to_handle.
> It is just like path_to_handle, except it takes a filesystem
> path in addition to the path which you want convert to a
> handle.

I'm not sure this is a good API.  We can derive a useful path for
the ioctl by using basename on the filename if it is a link without
needing to expose the details to the caller.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 1/2] add lpath_to_handle to libhandle

[Christoph Hellwig]

On Sat, Oct 24, 2009 at 09:37:29AM -0400, Christoph Hellwig wrote:
> On Fri, Oct 23, 2009 at 01:08:37PM -0500, Alex Elder wrote:
> > Looks good.  I will up the version number after get this pulled in.
> > I will publish this and get the version number/dependency stuff
> > straightened out before I take the change for xfsrestore.
> 
> Please hold back for a while with making a release.

So you made a relase without any communication to the developers and no
time to respon.  If this is the way SGI thinks they should work I think
it's time to make the kernel.org trees an official new xfsprogs-ng tree
and just ignore SGI.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 1/2] add lpath_to_handle to libhandle

[Christoph Hellwig]

This also needs documentation in the path_to_handle manpage, including
a good way to find the fspath argumeent if you stick with that, and
the failure mode if it points to a wrong filesystem.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 1/2] add lpath_to_handle to libhandle

[Bill Kendall]

On 10/24/2009 08:39 AM, Christoph Hellwig wrote:
> On Thu, Oct 22, 2009 at 11:52:23AM -0500, Bill Kendall wrote:
>> path_to_handle() is not reliable when called on a path which
>> is a symlink. If the symlink is dangling, or if its points
>> to a non-XFS filesystem then path_to_handle() will fail. The
>> reason is that path_to_handle() must open the path in order
>> to obtain an fd for the xfsctl call.
>>
>> It's common during xfsrestore to have dangling symlinks since
>> the target of the link may not be restored before the symlink.
>>
>> This patch adds a new function to libhandle, lpath_to_handle.
>> It is just like path_to_handle, except it takes a filesystem
>> path in addition to the path which you want convert to a
>> handle.
> 
> I'm not sure this is a good API.  We can derive a useful path for
> the ioctl by using basename on the filename if it is a link without
> needing to expose the details to the caller.

Based on Christoph's suggestion here's a rework of the patch
(that I've been sitting on for a while). This requires no change
to the libhandle API and no changes in xfsdump (and hence just
this one patch. The previously posted patch 2/2 is dropped).


Signed-off-by: Bill Kendall <wkendall@sgi.com>

Index: xfsprogs-kernel.org/libhandle/handle.c
===================================================================
--- xfsprogs-kernel.org.orig/libhandle/handle.c
+++ xfsprogs-kernel.org/libhandle/handle.c
@@ -16,6 +16,7 @@
   * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
   */

+#include <libgen.h>
  #include <xfs/xfs.h>
  #include <xfs/handle.h>
  #include <xfs/parent.h>
@@ -40,6 +41,7 @@ typedef union {

  static int obj_to_handle(char *, int, unsigned int, comarg_t, void**, size_t*);
  static int handle_to_fsfd(void *, char **);
+static char *path_to_fspath(char *path);


  /*
@@ -70,13 +72,18 @@ path_to_fshandle(
  	comarg_t	obj;
  	struct fdhash	*fdhp;
  	char		*tmppath;
+	char		*fspath;

-	fd = open(path, O_RDONLY);
+	fspath = path_to_fspath(path);
+	if (fspath == NULL)
+		return -1;
+
+	fd = open(fspath, O_RDONLY);
  	if (fd < 0)
  		return -1;

  	obj.path = path;
-	result = obj_to_handle(path, fd, XFS_IOC_PATH_TO_FSHANDLE,
+	result = obj_to_handle(fspath, fd, XFS_IOC_PATH_TO_FSHANDLE,
  				obj, fshanp, fshlen);
  	if (result < 0) {
  		close(fd);
@@ -95,7 +102,7 @@ path_to_fshandle(
  		}

  		fdhp->fsfd = fd;
-		strncpy(fdhp->fspath, path, sizeof(fdhp->fspath));
+		strncpy(fdhp->fspath, fspath, sizeof(fdhp->fspath));
  		memcpy(fdhp->fsh, *fshanp, FSIDSIZE);

  		fdhp->fnxt = fdhash_head;
@@ -114,18 +121,45 @@ path_to_handle(
  	int		fd;
  	int		result;
  	comarg_t	obj;
+	char		*fspath;
+
+	fspath = path_to_fspath(path);
+	if (fspath == NULL)
+		return -1;

-	fd = open(path, O_RDONLY);
+	fd = open(fspath, O_RDONLY);
  	if (fd < 0)
  		return -1;

  	obj.path = path;
-	result = obj_to_handle(path, fd, XFS_IOC_PATH_TO_HANDLE,
+	result = obj_to_handle(fspath, fd, XFS_IOC_PATH_TO_HANDLE,
  				obj, hanp, hlen);
  	close(fd);
  	return result;
  }

+/* Given a path, return a suitable "fspath" for use in obtaining
+ * an fd for xfsctl calls. In most circumstances the input path is
+ * sufficient. However, if the input path is a sym link the
+ * parent directory is returned so as to avoid issues with
+ * dangling links and links pointing into other filesystems.
+ */
+static char *
+path_to_fspath(char *path)
+{
+	static char dirpath[MAXPATHLEN];
+	struct stat statbuf;
+
+	if (lstat(path, &statbuf) != 0)
+		return NULL;
+
+	if (S_ISLNK(statbuf.st_mode)) {
+		strcpy(dirpath, path);
+		return dirname(dirpath);
+	}
+
+	return path;
+}

  int
  fd_to_handle (

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 1/2] add lpath_to_handle to libhandle

[Christoph Hellwig]

> Based on Christoph's suggestion here's a rework of the patch
> (that I've been sitting on for a while). This requires no change
> to the libhandle API and no changes in xfsdump (and hence just
> this one patch. The previously posted patch 2/2 is dropped).

The patch looks good for me from review, but fails to apply probably due
to whitespace damage in the mailer.

One thing that could be changes is to also do the fspath conversion for
block and chacater special files.  While we can open those they will not
end up in the xfs file operations and thus not provide the nessecary
ioctl.


Reviewed-by: Christoph Hellwig <hch@lst.de>


Btw, it would be nice if you could write a testcase for xfstests that
fails with the old version on links but works with the new one.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 1/2] add lpath_to_handle to libhandle

[Bill Kendall]

On Wed, Dec 23, 2009 at 08:15:35AM -0500, Christoph Hellwig wrote:
> One thing that could be changes is to also do the fspath conversion for
> block and chacater special files.  While we can open those they will not
> end up in the xfs file operations and thus not provide the nessecary
> ioctl.

Parent dir is now used for all types except regular files and directories.
Special file conversions still fail, but at least it's XFS making that
decision. Also this prevents calling open on named pipes, which could
block.

> Btw, it would be nice if you could write a testcase for xfstests that
> fails with the old version on links but works with the new one.

I'll take a look at this and post a separate patch.


Signed-off-by: Bill Kendall <wkendall@sgi.com>

Index: xfsprogs-kernel.org/libhandle/handle.c
===================================================================
--- xfsprogs-kernel.org.orig/libhandle/handle.c
+++ xfsprogs-kernel.org/libhandle/handle.c
@@ -16,6 +16,7 @@
  * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
  */
 
+#include <libgen.h>
 #include <xfs/xfs.h>
 #include <xfs/handle.h>
 #include <xfs/parent.h>
@@ -40,6 +41,7 @@ typedef union {
 
 static int obj_to_handle(char *, int, unsigned int, comarg_t, void**, size_t*);
 static int handle_to_fsfd(void *, char **);
+static char *path_to_fspath(char *path);
 
 
 /*
@@ -70,13 +72,18 @@ path_to_fshandle(
 	comarg_t	obj;
 	struct fdhash	*fdhp;
 	char		*tmppath;
+	char		*fspath;
 
-	fd = open(path, O_RDONLY);
+	fspath = path_to_fspath(path);
+	if (fspath == NULL)
+		return -1;
+
+	fd = open(fspath, O_RDONLY);
 	if (fd < 0)
 		return -1;
 
 	obj.path = path;
-	result = obj_to_handle(path, fd, XFS_IOC_PATH_TO_FSHANDLE,
+	result = obj_to_handle(fspath, fd, XFS_IOC_PATH_TO_FSHANDLE,
 				obj, fshanp, fshlen);
 	if (result < 0) {
 		close(fd);
@@ -95,7 +102,7 @@ path_to_fshandle(
 		}
 
 		fdhp->fsfd = fd;
-		strncpy(fdhp->fspath, path, sizeof(fdhp->fspath));
+		strncpy(fdhp->fspath, fspath, sizeof(fdhp->fspath));
 		memcpy(fdhp->fsh, *fshanp, FSIDSIZE);
 
 		fdhp->fnxt = fdhash_head;
@@ -114,18 +121,46 @@ path_to_handle(
 	int		fd;
 	int		result;
 	comarg_t	obj;
+	char		*fspath;
 
-	fd = open(path, O_RDONLY);
+	fspath = path_to_fspath(path);
+	if (fspath == NULL)
+		return -1;
+
+	fd = open(fspath, O_RDONLY);
 	if (fd < 0)
 		return -1;
 
 	obj.path = path;
-	result = obj_to_handle(path, fd, XFS_IOC_PATH_TO_HANDLE,
+	result = obj_to_handle(fspath, fd, XFS_IOC_PATH_TO_HANDLE,
 				obj, hanp, hlen);
 	close(fd);
 	return result;
 }
 
+/* Given a path, return a suitable "fspath" for use in obtaining
+ * an fd for xfsctl calls. For regular files and directories the
+ * input path is sufficient. For other types the parent directory
+ * is used to avoid issues with opening dangling symlinks and
+ * potentially blocking in an open on a named pipe. Also
+ * symlinks to files on other filesystems would be a problem,
+ * since an fd would be obtained for the wrong fs.
+ */
+static char *
+path_to_fspath(char *path)
+{
+	static char dirpath[MAXPATHLEN];
+	struct stat statbuf;
+
+	if (lstat(path, &statbuf) != 0)
+		return NULL;
+
+	if (S_ISREG(statbuf.st_mode) || S_ISDIR(statbuf.st_mode))
+		return path;
+
+	strcpy(dirpath, path);
+	return dirname(dirpath);
+}
 
 int
 fd_to_handle (

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 1/2] add lpath_to_handle to libhandle

[Christoph Hellwig]

After talking to Alex I applied this now.  I also wrote a short
description for the changes.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs