[PATCH 1/2] btrfs-progs: make get_btrfs_mount callable

[PATCH 1/2] btrfs-progs: make get_btrfs_mount callable

[Anand Jain]

get_btrfs_mount is reusable function but it is printing
errors, this removes it. Here the parent function of
open_path_or_dev_mnt does print error msg on error.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 utils.c |    6 ++----
 utils.h |    1 +
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/utils.c b/utils.c
index a7441c1..cb69b2b 100644
--- a/utils.c
+++ b/utils.c
@@ -692,7 +692,8 @@ int is_block_device(const char *path) {
  * On failure, returns -errno (not mounted yields -EINVAL)
  * Is noisy on failures, expects to be given a mounted device.
  */
-static int get_btrfs_mount(const char *dev, char *mp, size_t mp_size) {
+int get_btrfs_mount(const char *dev, char *mp, size_t mp_size)
+{
 	int ret;
 	int fd = -1;
 
@@ -717,7 +718,6 @@ static int get_btrfs_mount(const char *dev, char *mp, size_t mp_size) {
 
 	ret = check_mounted_where(fd, dev, mp, mp_size, NULL);
 	if (!ret) {
-		fprintf(stderr, "%s is not a mounted btrfs device\n", dev);
 		ret = -EINVAL;
 	} else { /* mounted, all good */
 		ret = 0;
@@ -725,8 +725,6 @@ static int get_btrfs_mount(const char *dev, char *mp, size_t mp_size) {
 out:
 	if (fd != -1)
 		close(fd);
-	if (ret)
-		fprintf(stderr, "Could not get mountpoint for %s\n", dev);
 	return ret;
 }
 
diff --git a/utils.h b/utils.h
index 251ef8e..8370686 100644
--- a/utils.h
+++ b/utils.h
@@ -91,5 +91,6 @@ int csum_tree_block(struct btrfs_root *root, struct extent_buffer *buf,
 int ask_user(char *question);
 int lookup_ino_rootid(int fd, u64 *rootid);
 int btrfs_scan_lblkid(int update_kernel);
+int get_btrfs_mount(const char *dev, char *mp, size_t mp_size);
 
 #endif
-- 
1.7.1

[PATCH 2/2] btrfs-progs: filesystem show of specified mounted disk should work

[Anand Jain]

Originally, thinking was user will use mount point if the disk
is mounted. But thats not really true, actually user don't
(or shouldn't) care to check if disk mounted, so whether disk
is mounted/unmounted when disk path is specified it should work.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 cmds-filesystem.c |   34 +++++++++++++++++++++++++++-------
 utils.c           |   27 ++++++++++++++++++++++++++-
 utils.h           |    4 +++-
 3 files changed, 56 insertions(+), 9 deletions(-)

diff --git a/cmds-filesystem.c b/cmds-filesystem.c
index b737ec9..e539b01 100644
--- a/cmds-filesystem.c
+++ b/cmds-filesystem.c
@@ -321,8 +321,15 @@ static int check_arg_type(char *input)
 	if (!input)
 		return BTRFS_ARG_UNKNOWN;
 
-	if (realpath(input, path))
-		return BTRFS_ARG_PATH;
+	if (realpath(input, path)) {
+		if (is_block_device(input) == 1)
+			return BTRFS_ARG_BLKDEV;
+
+		if (is_mount_point(input) == 1)
+			return BTRFS_ARG_MNTPOINT;
+
+		return BTRFS_ARG_UNKNOWN;
+	}
 
 	if (!uuid_parse(input, out))
 		return BTRFS_ARG_UUID;
@@ -346,6 +353,8 @@ static int btrfs_scan_kernel(void *search)
 		return 1;
 
 	type = check_arg_type(search);
+	if (type == BTRFS_ARG_BLKDEV)
+		return 1;
 
 	while ((mnt = getmntent(f)) != NULL) {
 		if (strcmp(mnt->mnt_type, "btrfs"))
@@ -363,11 +372,11 @@ static int btrfs_scan_kernel(void *search)
 			if (uuid_compare(fs_info_arg.fsid, uuid))
 				continue;
 			break;
-		case BTRFS_ARG_PATH:
+		case BTRFS_ARG_MNTPOINT:
 			if (strcmp(search, mnt->mnt_dir))
 				continue;
 			break;
-		default:
+		case BTRFS_ARG_UNKNOWN:
 			break;
 		}
 
@@ -386,7 +395,7 @@ static int btrfs_scan_kernel(void *search)
 }
 
 static const char * const cmd_show_usage[] = {
-	"btrfs filesystem show [options] [<path>|<uuid>]",
+	"btrfs filesystem show [options|<path>|<uuid>]",
 	"Show the structure of a filesystem",
 	"-d|--all-devices   show only disks under /dev containing btrfs filesystem",
 	"-m|--mounted       show only mounted btrfs",
@@ -403,6 +412,7 @@ static int cmd_show(int argc, char **argv)
 	int ret;
 	int where = BTRFS_SCAN_LBLKID;
 	int type = 0;
+	char mp[BTRFS_PATH_NAME_MAX + 1];
 
 	while (1) {
 		int long_index;
@@ -427,8 +437,13 @@ static int cmd_show(int argc, char **argv)
 		}
 	}
 
-	if (check_argc_max(argc, optind + 1))
-		usage(cmd_show_usage);
+	if (where == BTRFS_SCAN_LBLKID) {
+		if (check_argc_max(argc, optind + 1))
+			usage(cmd_show_usage);
+	} else {
+		if (check_argc_max(argc, optind))
+			usage(cmd_show_usage);
+	}
 	if (argc > optind) {
 		search = argv[optind];
 		type = check_arg_type(search);
@@ -436,6 +451,11 @@ static int cmd_show(int argc, char **argv)
 			fprintf(stderr, "ERROR: arg type unknown\n");
 			usage(cmd_show_usage);
 		}
+		if (type == BTRFS_ARG_BLKDEV) {
+			ret = get_btrfs_mount(search, mp, sizeof(mp));
+			if (ret == 0)
+				search = mp;
+		}
 	}
 
 	if (where == BTRFS_SCAN_DEV)
diff --git a/utils.c b/utils.c
index cb69b2b..d241044 100644
--- a/utils.c
+++ b/utils.c
@@ -677,7 +677,8 @@ error:
  * Returns negative errno on failure, otherwise
  * returns 1 for blockdev, 0 for not-blockdev
  */
-int is_block_device(const char *path) {
+int is_block_device(const char *path)
+{
 	struct stat statbuf;
 
 	if (stat(path, &statbuf) < 0)
@@ -687,6 +688,30 @@ int is_block_device(const char *path) {
 }
 
 /*
+ * check if given path is a mount point
+ * return 1 if yes. 0 if no. -1 for error
+ */
+int is_mount_point(const char *path)
+{
+	FILE *f;
+	struct mntent *mnt;
+	int ret = 0;
+
+	f = setmntent("/proc/self/mounts", "r");
+	if (f == NULL)
+		return -1;
+
+	while ((mnt = getmntent(f)) != NULL) {
+		if (strcmp(mnt->mnt_dir, path))
+			continue;
+		ret = 1;
+		break;
+	}
+	endmntent(f);
+	return ret;
+}
+
+/*
  * Find the mount point for a mounted device.
  * On success, returns 0 with mountpoint in *mp.
  * On failure, returns -errno (not mounted yields -EINVAL)
diff --git a/utils.h b/utils.h
index 8370686..4d0db1d 100644
--- a/utils.h
+++ b/utils.h
@@ -33,8 +33,9 @@
 #define BTRFS_UPDATE_KERNEL	1
 
 #define BTRFS_ARG_UNKNOWN	0
-#define BTRFS_ARG_PATH		1
+#define BTRFS_ARG_MNTPOINT	1
 #define BTRFS_ARG_UUID		2
+#define BTRFS_ARG_BLKDEV	3
 
 int make_btrfs(int fd, const char *device, const char *label,
 	       u64 blocks[6], u64 num_bytes, u32 nodesize,
@@ -76,6 +77,7 @@ int set_label(const char *btrfs_dev, const char *label);
 
 char *__strncpy__null(char *dest, const char *src, size_t n);
 int is_block_device(const char *file);
+int is_mount_point(const char *file);
 int open_path_or_dev_mnt(const char *path, DIR **dirstream);
 u64 btrfs_device_size(int fd, struct stat *st);
 /* Helper to always get proper size of the destination string */
-- 
1.7.1

Re: [PATCH 2/2] btrfs-progs: filesystem show of specified mounted disk should work

[David Sterba]

On Tue, Oct 22, 2013 at 01:53:22PM +0800, Anand Jain wrote:
> @@ -386,7 +395,7 @@ static int btrfs_scan_kernel(void *search)
>  static const char * const cmd_show_usage[] = {
> -	"btrfs filesystem show [options] [<path>|<uuid>]",
> +	"btrfs filesystem show [options|<path>|<uuid>]",

Options should stay separate from the path/uuid, you're extending the
syntax to accept a device:

	"btrfs filesystem show [options] [<path>|<uuid>|<device>]",

I'm fixing it locally, let me know if this doesn't match what you've
intended.

david

Re: [PATCH 1/2] btrfs-progs: make get_btrfs_mount callable

[David Sterba]

On Tue, Oct 22, 2013 at 01:53:21PM +0800, Anand Jain wrote:
> get_btrfs_mount is reusable function but it is printing
> errors, this removes it. Here the parent function of
> open_path_or_dev_mnt does print error msg on error.

Yeah, but then each caller should duplicate the is_block_device check to
keep get_btrfs_mount silent. There should be 2 functions, one that does
all the error checking and prints error messages and one that is silent
and usable eg. from a library. I'll keep the patch as-is for now because
there are no other users than open_path_or_dev_mnt and the function is
not exported to library.

david

Re: [PATCH 2/2] btrfs-progs: filesystem show of specified mounted disk should work

[David Sterba]

Somewhere among the updates 'btrfs fi show LABEL' stopped working and
xfstests/btrfs/006 fails. I did not know that this functionality exists so I
haven't paid attention to it during reviews.

btrfs/006 5s ...        [18:10:35] [18:10:40] - output mismatch (see /root/xfstests/results//btrfs/006.out.bad)
    --- tests/btrfs/006.out     2013-08-29 11:58:37.000000000 +0200
    +++ /root/xfstests/results//btrfs/006.out.bad       2013-10-22 18:10:40.000000000 +0200
    @@ -4,15 +4,44 @@
     TestLabel.006
     == Mount.
     == Show filesystem by label
    -Label: 'TestLabel.006'  uuid: <UUID>
    -   Total devices <EXACTNUM> FS bytes used <SIZE>
    -   devid <DEVID> size <SIZE> used <SIZE> path SCRATCH_DEV
    +ERROR: arg type unknown
    +usage: btrfs filesystem show [options|<path>|<uuid>]
    +
    +    Show the structure of a filesystem
    +
    +    -d|--all-devices   show only disks under /dev containing btrfs filesystem
    +    -m|--mounted       show only mounted btrfs
    +    If no argument is given, structure of all present filesystems is shown.
    +
    +ERROR: arg type unknown
    +usage: btrfs filesystem show [options|<path>|<uuid>]
    +
    +    Show the structure of a filesystem
    +
    +    -d|--all-devices   show only disks under /dev containing btrfs filesystem
    +    -m|--mounted       show only mounted btrfs
    +    If no argument is given, structure of all present filesystems is shown.

     == Show filesystem by UUID
    -Label: 'TestLabel.006'  uuid: <EXACTUUID>
    +Label: none  uuid: <UUID>
    +   Total devices 4 FS bytes used <SIZE>
    +   devid <DEVID> size <SIZE> used <SIZE> path /dev/sda5
    +   devid <DEVID> size <SIZE> used <SIZE> path /dev/sda6
    +   devid <DEVID> size <SIZE> used <SIZE> path /dev/sda7
    +   devid <DEVID> size <SIZE> used <SIZE> path /dev/sda8
    +
    +Label: TestLabel.006  uuid: <UUID>
        Total devices <EXACTNUM> FS bytes used <SIZE>
        devid <DEVID> size <SIZE> used <SIZE> path SCRATCH_DEV

    +Label: none  uuid: <UUID>
    +   Total devices 1 FS bytes used <SIZE>
    +   devid <DEVID> size <SIZE> used <SIZE> path /dev/sdb
    +
    +Label: none  uuid: <UUID>
    +   Total devices 1 FS bytes used <SIZE>
    +   devid <DEVID> size <SIZE> used <SIZE> path /dev/sda15
    +
     == Sync filesystem
     FSSync 'SCRATCH_MNT'
     == Show device stats by mountpoint

I did a quick tested with some older integration branch with
"btrfs-progs: btrfs_list_find_updated_files: Fix memory leak" on top and it
worked.

david

Re: [PATCH 2/2] btrfs-progs: filesystem show of specified mounted disk should work

[Anand Jain]

As I remember, there was no code to handle the look up by label.
it was a dummy option which did nothing.

So the below commit removed the option.

------
commit 50eaae45f2b47643f9a4c43ce72f7d6e06d4e323
Author: Anand Jain <anand.jain@oracle.com>
Date:   Mon Jul 15 13:30:48 2013 +0800

     btrfs-progs: label option in btrfs filesystem show is not coded

     Signed-off-by: Anand Jain <anand.jain@oracle.com>
     Signed-off-by: David Sterba <dsterba@suse.cz>
-------


On 10/23/13 12:39 AM, David Sterba wrote:
> Somewhere among the updates 'btrfs fi show LABEL' stopped working and
> xfstests/btrfs/006 fails. I did not know that this functionality exists so I
> haven't paid attention to it during reviews.

> I did a quick tested with some older integration branch with
> "btrfs-progs: btrfs_list_find_updated_files: Fix memory leak" on top and it
> worked.

  let me try. If there was any code which managed
  the label lookup option.


Thanks, Anand

Re: [PATCH 2/2] btrfs-progs: filesystem show of specified mounted disk should work

[Anand Jain]

On 10/22/13 10:33 PM, David Sterba wrote:
> On Tue, Oct 22, 2013 at 01:53:22PM +0800, Anand Jain wrote:
>> @@ -386,7 +395,7 @@ static int btrfs_scan_kernel(void *search)
>>   static const char * const cmd_show_usage[] = {
>> -	"btrfs filesystem show [options] [<path>|<uuid>]",
>> +	"btrfs filesystem show [options|<path>|<uuid>]",
>
> Options should stay separate from the path/uuid, you're extending the
> syntax to accept a device:
>
> 	"btrfs filesystem show [options] [<path>|<uuid>|<device>]",
>
> I'm fixing it locally, let me know if this doesn't match what you've
> intended.

  I am confused, on how the options should be represented,
  but the internal design is as below.
  When either uuid/path/dev is provide the option
  -d/-m (which implies to read-disks or kernel) will not apply,
  since internally we would determine that based on whether the
  given disk/uuid is mounted/unmounted.
  so option -d/-m don't apply when last argument is provided.
  so the right commands are

  btrfs fi show <-- probe mount-points and then disks for unmounted btrfs
  btrfs fi show -d <-- probe all disks even they are mounted (for legacy 
purpose, as in the original design)
  btrfs fi show /dev/sda <-- use mount point if disk is mounted OR disk 
path if unmounted
  btrfs fi show -m <-- show only mounted btrfs.


Thanks, Anand

Re: [PATCH 1/2] btrfs-progs: make get_btrfs_mount callable

[Anand Jain]

On 10/22/13 10:43 PM, David Sterba wrote:
> On Tue, Oct 22, 2013 at 01:53:21PM +0800, Anand Jain wrote:
>> get_btrfs_mount is reusable function but it is printing
>> errors, this removes it. Here the parent function of
>> open_path_or_dev_mnt does print error msg on error.
>
> Yeah, but then each caller should duplicate the is_block_device check to
> keep get_btrfs_mount silent. There should be 2 functions, one that does
> all the error checking and prints error messages and one that is silent
> and usable eg. from a library. I'll keep the patch as-is for now because
> there are no other users than open_path_or_dev_mnt and the function is
> not exported to library.

  yeah. sounds good. certainly we do have opportunities
  when we are on the lib functions.

Thanks, Anand

Re: [PATCH 2/2] btrfs-progs: filesystem show of specified mounted disk should work

[David Sterba]

On Wed, Oct 23, 2013 at 10:08:16AM +0800, Anand Jain wrote:
> On 10/22/13 10:33 PM, David Sterba wrote:
> >On Tue, Oct 22, 2013 at 01:53:22PM +0800, Anand Jain wrote:
> >>@@ -386,7 +395,7 @@ static int btrfs_scan_kernel(void *search)
> >>  static const char * const cmd_show_usage[] = {
> >>-	"btrfs filesystem show [options] [<path>|<uuid>]",
> >>+	"btrfs filesystem show [options|<path>|<uuid>]",
> >
> >Options should stay separate from the path/uuid, you're extending the
> >syntax to accept a device:
> >
> >	"btrfs filesystem show [options] [<path>|<uuid>|<device>]",
> >
> >I'm fixing it locally, let me know if this doesn't match what you've
> >intended.
> 
>  I am confused, on how the options should be represented,
>  but the internal design is as below.

Hm right, it is a bit confusing, I think because of the syntax that
allows either options or the path/uuid/device specifier, not both, which
is not so common.

I still prefer to keep them separate, because it's something that can be
clarified in the help text or documentation.

Besides, that we may want to add more options that affect
path/uuid/device output, the argument description looks consistent with
other commands and if some combination is not allowed, then an error
message will say why. I really don't expect an average user to think too
hard about it.

david

Re: [PATCH 2/2] btrfs-progs: filesystem show of specified mounted disk should work

[Hugo Mills]

[-- Attachment #1: Type: text/plain, Size: 2284 bytes --]

On Thu, Oct 24, 2013 at 04:51:00PM +0200, David Sterba wrote:
> On Wed, Oct 23, 2013 at 10:08:16AM +0800, Anand Jain wrote:
> > On 10/22/13 10:33 PM, David Sterba wrote:
> > >On Tue, Oct 22, 2013 at 01:53:22PM +0800, Anand Jain wrote:
> > >>@@ -386,7 +395,7 @@ static int btrfs_scan_kernel(void *search)
> > >>  static const char * const cmd_show_usage[] = {
> > >>-	"btrfs filesystem show [options] [<path>|<uuid>]",
> > >>+	"btrfs filesystem show [options|<path>|<uuid>]",
> > >
> > >Options should stay separate from the path/uuid, you're extending the
> > >syntax to accept a device:
> > >
> > >	"btrfs filesystem show [options] [<path>|<uuid>|<device>]",
> > >
> > >I'm fixing it locally, let me know if this doesn't match what you've
> > >intended.
> > 
> >  I am confused, on how the options should be represented,
> >  but the internal design is as below.
> 
> Hm right, it is a bit confusing, I think because of the syntax that
> allows either options or the path/uuid/device specifier, not both, which
> is not so common.

   Typically, that ends up written as something like:

btrfs filesystem show [options]
btrfs filesystem show [<path>|<uuid>|<device>]

or just as

btrfs filesystem show [options] [<path>|<uuid>|<device>]

with a comment in the man page that the second parameter can't be
supplied with any of the options (if that's the case). Of the two, I
prefer the former, but with the acknowledgement that when the command
grows some options that can be used with the p/u/d, you'll end up
having to change the help text.

   Hugo.

> I still prefer to keep them separate, because it's something that can be
> clarified in the help text or documentation.
> 
> Besides, that we may want to add more options that affect
> path/uuid/device output, the argument description looks consistent with
> other commands and if some combination is not allowed, then an error
> message will say why. I really don't expect an average user to think too
> hard about it.
> 
> david

-- 
=== Hugo Mills: hugo@... carfax.org.uk | darksatanic.net | lug.org.uk ===
  PGP key: 65E74AC0 from wwwkeys.eu.pgp.net or http://www.carfax.org.uk
           --- I believe that it's closely correlated with ---           
                       the aeroswine coefficient.                        

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 2/2] btrfs-progs: filesystem show of specified mounted disk should work

[Anand Jain]

Thanks for the comments.
I took care of it in the recent patch
  ([PATCH] btrfs-progs: make filesystem show by label work)
with this -d/-m option may accept the argument as well.
except for -m with the argument is of unmounted disk.

Found a new bug.. more than one subvol of a fsid might
have been mounted, then we print as many mount points in
the system - thats not end user desirable. I have drafted
a fix patch, I will be sending it later after a few more tests.

Thanks, Anand