[PATCH 1/2] mountpoint: account for error from in mnt_fs_get_target

[PATCH 1/2] mountpoint: account for error from in mnt_fs_get_target

[Dave Reisner]

commit 04f087ec didn't take into consideration that mnt_fs_get_target()
could return an error, and would therefore show false positives, such
as:

$ mkdir foo; mountpoint foo
foo is a mountpoint

Signed-off-by: Dave Reisner <dreisner@archlinux.org>
---
 sys-utils/mountpoint.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/sys-utils/mountpoint.c b/sys-utils/mountpoint.c
index bd92667..a45cabd 100644
--- a/sys-utils/mountpoint.c
+++ b/sys-utils/mountpoint.c
@@ -45,6 +45,7 @@ static int dir_to_device(const char *spec, dev_t *dev)
 	struct libmnt_table *tb = mnt_new_table_from_file("/proc/self/mountinfo");
 	struct libmnt_fs *fs;
 	struct libmnt_cache *cache;
+	int rc = -1;
 
 	if (!tb) {
 		/*
@@ -82,12 +83,14 @@ static int dir_to_device(const char *spec, dev_t *dev)
 	mnt_table_set_cache(tb, cache);
 
 	fs = mnt_table_find_target(tb, spec, MNT_ITER_BACKWARD);
-	if (fs && mnt_fs_get_target(fs))
+	if (fs && mnt_fs_get_target(fs)) {
 		*dev = mnt_fs_get_devno(fs);
+		rc = 0;
+	}
 
 	mnt_free_table(tb);
 	mnt_free_cache(cache);
-	return 0;
+	return rc;
 }
 
 static int print_devno(const char *devname, struct stat *st)
-- 
1.7.9.2

[PATCH 2/2] libmount: don't treat "none" differently

[Dave Reisner]

This causes more problems than it solves. In the latest edition:

  # mount -t proc none foo
  mount: foo: mount failed: Invalid argument

A check for source and target fails in mnt_context_apply_fstab()
because, even though they were indeed specified on the cmdline,
__mnt_fs_set_source_ptr() altered this and NULL'd out the source.

If you're able to mount this device via other means, other tools start
reporting oddities, such as mount's output:

  (null) on /foo type proc (rw,relatime)

or findmnt:

  TARGET      SOURCE FSTYPE OPTIONS
  /foo               proc   rw,relatime

Simply treat "none" like any other source when passed in. We still keep
conventions to allow NULL as a valid source and replace it with "none".

Signed-off-by: Dave Reisner <dreisner@archlinux.org>
---
There may be some history here I'm not aware of that makes this all a silly
idea...

 libmount/src/fs.c        |   14 ++++----------
 libmount/src/tab.c       |    7 +++----
 libmount/src/tab_parse.c |    7 +------
 3 files changed, 8 insertions(+), 20 deletions(-)

diff --git a/libmount/src/fs.c b/libmount/src/fs.c
index a28e66c..048f0e2 100644
--- a/libmount/src/fs.c
+++ b/libmount/src/fs.c
@@ -304,11 +304,7 @@ int __mnt_fs_set_source_ptr(struct libmnt_fs *fs, char *source)
 
 	assert(fs);
 
-	if (source && !strcmp(source, "none")) {
-		free(source);
-		source = NULL;
-
-	} else if (source && strchr(source, '=')) {
+	if (source && strchr(source, '=')) {
 		if (blkid_parse_tag_string(source, &t, &v) != 0)
 			return -1;
 	}
@@ -341,11 +337,9 @@ int mnt_fs_set_source(struct libmnt_fs *fs, const char *source)
 
 	if (!fs)
 		return -EINVAL;
-	if (source) {
-		p = strdup(source);
-		if (!p)
-			return -ENOMEM;
-	}
+	p = strdup(source ? source : "none");
+	if (!p)
+		return -ENOMEM;
 
 	rc = __mnt_fs_set_source_ptr(fs, p);
 	if (rc)
diff --git a/libmount/src/tab.c b/libmount/src/tab.c
index 21b05c7..7dd9654 100644
--- a/libmount/src/tab.c
+++ b/libmount/src/tab.c
@@ -480,9 +480,8 @@ struct libmnt_fs *mnt_table_find_target(struct libmnt_table *tb, const char *pat
  * The 2nd, 3rd and 4th iterations are not performed when @tb cache is not
  * set (see mnt_table_set_cache()).
  *
- * Note that valid source path is NULL; the libmount uses NULL instead of
- * "none".  The "none" is used in /proc/{mounts,self/mountninfo} for pseudo
- * filesystems.
+ * Note that NULL is a valid source path; it will be replaced with "none". The
+ * "none" is used in /proc/{mounts,self/mountinfo} for pseudo filesystems.
  *
  * Returns: a tab entry or NULL.
  */
@@ -505,7 +504,7 @@ struct libmnt_fs *mnt_table_find_srcpath(struct libmnt_table *tb, const char *pa
 
 		p = mnt_fs_get_srcpath(fs);
 
-		if (path == NULL && src == NULL)
+		if (path == NULL && (src == NULL || !strcmp(src, "none")))
 			return fs;			/* source is "none" */
 		if (path && p && streq_except_trailing_slash(p, path))
 			return fs;
diff --git a/libmount/src/tab_parse.c b/libmount/src/tab_parse.c
index 0f618bb..5bc55ae 100644
--- a/libmount/src/tab_parse.c
+++ b/libmount/src/tab_parse.c
@@ -180,12 +180,7 @@ static int mnt_parse_mountinfo_line(struct libmnt_fs *fs, char *s)
 		unmangle_string(fs->vfs_optstr);
 		unmangle_string(fstype);
 		unmangle_string(src);
-
-		if (!strcmp(fs->fs_optstr, "none")) {
-			free(fs->fs_optstr);
-			fs->fs_optstr = NULL;
-		} else
-			unmangle_string(fs->fs_optstr);
+		unmangle_string(fs->fs_optstr);
 
 		rc = __mnt_fs_set_fstype_ptr(fs, fstype);
 		if (!rc) {
-- 
1.7.9.2

Re: [PATCH 1/2] mountpoint: account for error from in mnt_fs_get_target

[Karel Zak]

On Thu, Mar 01, 2012 at 10:46:59PM -0500, Dave Reisner wrote:
>  sys-utils/mountpoint.c |    7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)

 Applied, thanks.

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH 2/2] libmount: don't treat "none" differently

[Karel Zak]

On Thu, Mar 01, 2012 at 10:47:00PM -0500, Dave Reisner wrote:
> This causes more problems than it solves. In the latest edition:

 Yes, I agree. You're right that exceptions suck.

> Simply treat "none" like any other source when passed in.

 Yes,

> We still keep conventions to allow NULL as a valid source and
> replace it with "none".

 but I don't agree with this idea. The old code converted "none" to
 NULL, you want to convert NULL to "none". I think both is wrong.

 It would be better to keep the difference between "none" and NULL. 
 The NULL means that the source is not set, "none" means that the source
 is unnecessary. As you said 'treat "none" like any other source'.

> @@ -341,11 +337,9 @@ int mnt_fs_set_source(struct libmnt_fs *fs, const char *source)
>  
>  	if (!fs)
>  		return -EINVAL;
> -	if (source) {
> -		p = strdup(source);
> -		if (!p)
> -			return -ENOMEM;
> -	}
> +	p = strdup(source ? source : "none");
> +	if (!p)
> +		return -ENOMEM;

 IMHO better is:

    if (source) {
        p = strdup(source);
        if (!p)
            return -ENOMEM;
    }

 Dave, we have some regression test for libmount, try

   $ make -C libmount/src tests
   
   # cd tests
   # ./run.sh libmount
   # ./run.sh mount

 maybe we need more tests for the "none" stuff.

     Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com