btrfs-progs: Use btrfs_open_dir to avoid show error of ioctl or tree search

PATCH In review Zhao Lei 2 participants 13 messages Oct 12, 2015 – Oct 13, 2015 Btrfs file system

Share & tools

Zhao Lei

author

2015-10-12 13:24 UTC #0 lore raw

cover btrfs-progs: Use btrfs_open_dir to avoid show error of ioctl or tree search

Use btrfs_open_dir() instead of open_file_or_dir(), to show error before real action(in ioctl or tree search), to make the error message exact and unified.

It can also make code simple:
85 insertions(+), 185 deletions(-)

Also include some small bug fix.

Before patch:

  # grep open_file_or_dir *.c
  btrfs-fragments.c:      fd = open_file_or_dir(path, &dirstream);
  cmds-balance.c: fd = open_file_or_dir(path, &dirstream);
  cmds-balance.c: fd = open_file_or_dir(path, &dirstream);
  cmds-balance.c: fd = open_file_or_dir(path, &dirstream);
  cmds-balance.c: fd = open_file_or_dir(path, &dirstream);
  cmds-balance.c: fd = open_file_or_dir(path, &dirstream);
  cmds-filesystem.c:      fd = open_file_or_dir(path, &dirstream);
  cmds-filesystem.c:      fd = open_file_or_dir(path, &dirstream);
  cmds-filesystem.c:              fd = open_file_or_dir(argv[i], &dirstream);
  cmds-filesystem.c:      fd = open_file_or_dir(path, &dirstream);
  cmds-fi-usage.c:                fd = open_file_or_dir(argv[i], &dirstream);
  cmds-inspect.c: fd = open_file_or_dir(argv[optind+1], &dirstream);
  cmds-inspect.c: fd = open_file_or_dir(argv[optind+1], &dirstream);
  cmds-inspect.c:                         path_fd = open_file_or_dir(full_path, &dirs);
  cmds-inspect.c: fd = open_file_or_dir(argv[2], &dirstream);
  cmds-inspect.c: fd = open_file_or_dir(argv[1], &dirstream);
  cmds-inspect.c: fd = open_file_or_dir(argv[optind], &dirstream);
  cmds-qgroup.c:  fd = open_file_or_dir(path, &dirstream);
  cmds-qgroup.c:  fd = open_file_or_dir(path, &dirstream);
  cmds-qgroup.c:  fd = open_file_or_dir(path, &dirstream);
  cmds-qgroup.c:  fd = open_file_or_dir(path, &dirstream);
  cmds-quota.c:   fd = open_file_or_dir(path, &dirstream);
  cmds-quota.c:   fd = open_file_or_dir(path, &dirstream);
  cmds-replace.c: fd = open_file_or_dir(path, &dirstream);
  cmds-replace.c: fd = open_file_or_dir(path, &dirstream);
  cmds-subvolume.c:       fddst = open_file_or_dir(dstdir, &dirstream);
  cmds-subvolume.c:       fd = open_file_or_dir(dname, &dirstream);
  cmds-subvolume.c:       fd = open_file_or_dir(subvol, &dirstream);
  cmds-subvolume.c:       fddst = open_file_or_dir(dstdir, &dirstream1);
  cmds-subvolume.c:       fd = open_file_or_dir(subvol, &dirstream2);
  cmds-subvolume.c:       fd = open_file_or_dir(subvol, &dirstream);
  cmds-subvolume.c:       fd = open_file_or_dir(path, &dirstream);
  cmds-subvolume.c:       fd = open_file_or_dir(subvol, &dirstream);
  cmds-subvolume.c:       fd = open_file_or_dir(fullpath, &dirstream1);
  cmds-subvolume.c:       mntfd = open_file_or_dir(mnt, &dirstream2);
  cmds-subvolume.c:       fd = open_file_or_dir(argv[optind], &dirstream);
  props.c:        fd = open_file_or_dir3(object, &dirstream, open_flags);
  utils.c:                fdmnt = open_file_or_dir(mp, dirstream);
  utils.c:                fdmnt = open_file_or_dir(path, dirstream);
  utils.c: * Do the following checks before calling open_file_or_dir():
  utils.c:        ret = open_file_or_dir(path, dirstream);
  utils.c:int open_file_or_dir3(const char *fname, DIR **dirstream, int open_flags)
  utils.c:int open_file_or_dir(const char *fname, DIR **dirstream)
  utils.c:        return open_file_or_dir3(fname, dirstream, O_RDWR);
  utils.c:        fd = open_file_or_dir(path, &dirstream);
  #

After patch:

  # grep open_file_or_dir *.c
  cmds-filesystem.c:              fd = open_file_or_dir(argv[i], &dirstream); *1
  props.c:        fd = open_file_or_dir3(object, &dirstream, open_flags);
  utils.c:                ret = open_file_or_dir(mp, dirstream);
  utils.c: * Do the following checks before calling open_file_or_dir():
  utils.c:        ret = open_file_or_dir(path, dirstream);
  utils.c:int open_file_or_dir3(const char *fname, DIR **dirstream, int open_flags)
  utils.c:int open_file_or_dir(const char *fname, DIR **dirstream)
  utils.c:        return open_file_or_dir3(fname, dirstream, O_RDWR);
  utils.c:        fd = open_file_or_dir(path, &dirstream);
  #
  *1: It is used to open dir or file, can not use btrfs_open_dir()
      instead.

Zhao Lei (11):
btrfs-progs: subvolume: use btrfs_open_dir for btrfs subvolume command
btrfs-progs: filesystem: use btrfs_open_dir for btrfs filesystem
command
btrfs-progs: balance: use btrfs_open_dir for btrfs balance command
btrfs-progs: inspect: Bypass unnecessary clean function in open_error
btrfs-progs: inspect: set return value of error case
btrfs-progs: inspect: use btrfs_open_dir for btrfs inspect command
btrfs-progs: qgroup: use btrfs_open_dir for btrfs qgroup command
btrfs-progs: quota: use btrfs_open_dir for btrfs quota command
btrfs-progs: use btrfs_open_dir in open_path_or_dev_mnt
btrfs-progs: replace: use btrfs_open_dir for btrfs replace command
btrfs-progs: fragments: use btrfs_open_dir for btrfs-fragments command

 btrfs-fragments.c |  6 ++----
 cmds-balance.c    | 30 ++++++++++-------------------
 cmds-device.c     | 13 ++-----------
 cmds-fi-usage.c   |  4 +---
 cmds-filesystem.c | 19 +++++++------------
 cmds-inspect.c    | 26 +++++++++-----------------
 cmds-qgroup.c     | 24 ++++++++----------------
 cmds-quota.c      | 12 ++++--------
 cmds-replace.c    | 29 ++++++----------------------
 cmds-scrub.c      | 28 +++++-----------------------
 cmds-subvolume.c  | 56 +++++++++++++++++++------------------------------------
 utils.c           | 21 +++++++++++----------
 utils.h           |  2 +-
 13 files changed, 85 insertions(+), 185 deletions(-)

Signature · 1 line

1.8.5.1

Zhao Lei

author

2015-10-12 13:24 UTC #1 lore raw

PATCH 1/11 btrfs-progs: subvolume: use btrfs_open_dir for btrfs subvolume command

We can use btrfs_open_dir() to check whether target dir is in btrfs's mount point before open, instead of checking it in kernel space of ioctl, and return fuzzy error message.

Before patch:
# (/mnt/tmp is not btrfs mountpoint)
#

  # btrfs subvolume create /mnt/tmp/123
  Create subvolume '/mnt/tmp/123'
  ERROR: cannot create subvolume - Inappropriate ioctl for device
  #

After patch:

  # btrfs subvolume create /mnt/tmp/123
  ERROR: not btrfs filesystem: /mnt/tmp
  #

Signed-off-by Zhao Lei <[email protected]>

---
 cmds-subvolume.c | 56 +++++++++++++++++++-------------------------------------
 1 file changed, 19 insertions(+), 37 deletions(-)

cmds-subvolume.c +19 ~~-37~~

index c40330a..be1a54a 100644
@@ -181,11 +181,9 @@ static int cmd_subvol_create(int argc, char **argv)
 		goto out;
 	}
 
-	fddst = open_file_or_dir(dstdir, &dirstream);
-	if (fddst < 0) {
-		fprintf(stderr, "ERROR: can't access '%s'\n", dstdir);
+	fddst = btrfs_open_dir(dstdir, &dirstream, 1);
+	if (fddst < 0)
 		goto out;
-	}
 
 	printf("Create subvolume '%s/%s'\n", dstdir, newname);
 	if (inherit) {

@@ -348,9 +346,8 @@ again:
 	vname = basename(dupvname);
 	free(cpath);
 
-	fd = open_file_or_dir(dname, &dirstream);
+	fd = btrfs_open_dir(dname, &dirstream, 1);
 	if (fd < 0) {
-		fprintf(stderr, "ERROR: can't access '%s'\n", dname);
 		ret = 1;
 		goto out;
 	}

@@ -564,7 +561,7 @@ static int cmd_subvol_list(int argc, char **argv)
 	}
 
 	subvol = argv[optind];
-	fd = open_file_or_dir(subvol, &dirstream);
+	fd = btrfs_open_dir(subvol, &dirstream, 1);
 	if (fd < 0) {
 		ret = -1;
 		fprintf(stderr, "ERROR: can't access '%s'\n", subvol);

@@ -723,17 +720,13 @@ static int cmd_subvol_snapshot(int argc, char **argv)
 		goto out;
 	}
 
-	fddst = open_file_or_dir(dstdir, &dirstream1);
-	if (fddst < 0) {
-		fprintf(stderr, "ERROR: can't access '%s'\n", dstdir);
+	fddst = btrfs_open_dir(dstdir, &dirstream1, 1);
+	if (fddst < 0)
 		goto out;
-	}
 
-	fd = open_file_or_dir(subvol, &dirstream2);
-	if (fd < 0) {
-		fprintf(stderr, "ERROR: can't access '%s'\n", dstdir);
+	fd = btrfs_open_dir(subvol, &dirstream2, 1);
+	if (fd < 0)
 		goto out;
-	}
 
 	if (readonly) {
 		args.flags |= BTRFS_SUBVOL_RDONLY;

@@ -791,11 +784,9 @@ static int cmd_subvol_get_default(int argc, char **argv)
 		usage(cmd_subvol_get_default_usage);
 
 	subvol = argv[1];
-	fd = open_file_or_dir(subvol, &dirstream);
-	if (fd < 0) {
-		fprintf(stderr, "ERROR: can't access '%s'\n", subvol);
+	fd = btrfs_open_dir(subvol, &dirstream, 1);
+	if (fd < 0)
 		return 1;
-	}
 
 	ret = btrfs_list_get_default_subvolume(fd, &default_id);
 	if (ret) {

@@ -859,11 +850,9 @@ static int cmd_subvol_set_default(int argc, char **argv)
 
 	objectid = arg_strtou64(subvolid);
 
-	fd = open_file_or_dir(path, &dirstream);
-	if (fd < 0) {
-		fprintf(stderr, "ERROR: can't access '%s'\n", path);
+	fd = btrfs_open_dir(path, &dirstream, 1);
+	if (fd < 0)
 		return 1;
-	}
 
 	ret = ioctl(fd, BTRFS_IOC_DEFAULT_SUBVOL, &objectid);
 	e = errno;

@@ -906,11 +895,9 @@ static int cmd_subvol_find_new(int argc, char **argv)
 		return 1;
 	}
 
-	fd = open_file_or_dir(subvol, &dirstream);
-	if (fd < 0) {
-		fprintf(stderr, "ERROR: can't access '%s'\n", subvol);
+	fd = btrfs_open_dir(subvol, &dirstream, 1);
+	if (fd < 0)
 		return 1;
-	}
 
 	ret = ioctl(fd, BTRFS_IOC_SYNC);
 	if (ret < 0) {

@@ -980,11 +967,9 @@ static int cmd_subvol_show(int argc, char **argv)
 	ret = 1;
 	svpath = get_subvol_name(mnt, fullpath);
 
-	fd = open_file_or_dir(fullpath, &dirstream1);
-	if (fd < 0) {
-		fprintf(stderr, "ERROR: can't access '%s'\n", fullpath);
+	fd = btrfs_open_dir(fullpath, &dirstream1, 1);
+	if (fd < 0)
 		goto out;
-	}
 
 	ret = btrfs_list_get_path_rootid(fd, &sv_id);
 	if (ret) {

@@ -993,11 +978,9 @@ static int cmd_subvol_show(int argc, char **argv)
 		goto out;
 	}
 
-	mntfd = open_file_or_dir(mnt, &dirstream2);
-	if (mntfd < 0) {
-		fprintf(stderr, "ERROR: can't access '%s'\n", mnt);
+	mntfd = btrfs_open_dir(mnt, &dirstream2, 1);
+	if (mntfd < 0)
 		goto out;
-	}
 
 	if (sv_id == BTRFS_FS_TREE_OBJECTID) {
 		printf("%s is btrfs root\n", fullpath);

@@ -1271,9 +1254,8 @@ static int cmd_subvol_sync(int argc, char **argv)
 	if (check_argc_min(argc - optind, 1))
 		usage(cmd_subvol_sync_usage);
 
-	fd = open_file_or_dir(argv[optind], &dirstream);
+	fd = btrfs_open_dir(argv[optind], &dirstream, 1);
 	if (fd < 0) {
-		fprintf(stderr, "ERROR: can't access '%s'\n", argv[optind]);
 		ret = 1;
 		goto out;
 	}

Signature · 1 line

1.8.5.1

Zhao Lei

author

2015-10-12 13:24 UTC #2 lore raw

PATCH 2/11 btrfs-progs: filesystem: use btrfs_open_dir for btrfs filesystem command

In reply to Zhao Lei

We can use btrfs_open_dir() to check whether target dir is in btrfs's mount point before open, instead of checking it in kernel space of ioctl, and return fuzzy error message.

Before patch:
# (/mnt/tmp is not btrfs mountpoint)
#

  # btrfs filesystem df /mnt/tmp
  ERROR: couldn't get space info - Inappropriate ioctl for device
  ERROR: get_df failed Inappropriate ioctl for device
  #

After patch:

  # ./btrfs filesystem df /mnt/tmp
  ERROR: not btrfs filesystem: /mnt/tmp
  #

Signed-off-by Zhao Lei <[email protected]>

---
 cmds-fi-usage.c   |  4 +---
 cmds-filesystem.c | 19 +++++++------------
 2 files changed, 8 insertions(+), 15 deletions(-)

cmds-fi-usage.c +1 -3

index 50d6333..5fefed4 100644
@@ -901,10 +901,8 @@ int cmd_filesystem_usage(int argc, char **argv)
 		int chunkcount = 0;
 		int devcount = 0;
 
-		fd = open_file_or_dir(argv[i], &dirstream);
+		fd = btrfs_open_dir(argv[i], &dirstream, 1);
 		if (fd < 0) {
-			fprintf(stderr, "ERROR: can't access '%s'\n",
-				argv[i]);
 			ret = 1;
 			goto out;
 		}

cmds-filesystem.c +7 ~~-12~~

index 3663734..91bf1fa 100644
@@ -205,11 +205,10 @@ static int cmd_filesystem_df(int argc, char **argv)
 
 	path = argv[1];
 
-	fd = open_file_or_dir(path, &dirstream);
-	if (fd < 0) {
-		fprintf(stderr, "ERROR: can't access '%s'\n", path);
+	fd = btrfs_open_dir(path, &dirstream, 1);
+	if (fd < 0)
 		return 1;
-	}
+
 	ret = get_df(fd, &sargs);
 
 	if (ret == 0) {

@@ -939,11 +938,9 @@ static int cmd_filesystem_sync(int argc, char **argv)
 
 	path = argv[1];
 
-	fd = open_file_or_dir(path, &dirstream);
-	if (fd < 0) {
-		fprintf(stderr, "ERROR: can't access '%s'\n", path);
+	fd = btrfs_open_dir(path, &dirstream, 1);
+	if (fd < 0)
 		return 1;
-	}
 
 	printf("FSSync '%s'\n", path);
 	res = ioctl(fd, BTRFS_IOC_SYNC);

@@ -1229,11 +1226,9 @@ static int cmd_filesystem_resize(int argc, char **argv)
 		return 1;
 	}
 
-	fd = open_file_or_dir(path, &dirstream);
-	if (fd < 0) {
-		fprintf(stderr, "ERROR: can't access '%s'\n", path);
+	fd = btrfs_open_dir(path, &dirstream, 1);
+	if (fd < 0)
 		return 1;
-	}
 
 	printf("Resize '%s' of '%s'\n", path, amount);
 	memset(&args, 0, sizeof(args));

Signature · 1 line

1.8.5.1

Zhao Lei

author

2015-10-12 13:24 UTC #3 lore raw

PATCH 3/11 btrfs-progs: balance: use btrfs_open_dir for btrfs balance command

In reply to Zhao Lei

We can use btrfs_open_dir() to check whether target dir is in btrfs's mount point before open, instead of checking it in kernel space of ioctl, and return fuzzy error message.

Before patch:

  # btrfs balance start /mnt/tmp
  ERROR: error during balancing '/mnt/tmp' - Inappropriate ioctl for device
  There may be more info in syslog - try dmesg | tail
  #

After patch:

  # btrfs balance start /mnt/tmp
  ERROR: not btrfs filesystem: /mnt/tmp
  #

Signed-off-by Zhao Lei <[email protected]>

---
 cmds-balance.c | 30 ++++++++++--------------------
 1 file changed, 10 insertions(+), 20 deletions(-)

cmds-balance.c +10 ~~-20~~

index 9af218b..b02e40d 100644
@@ -306,11 +306,9 @@ static int do_balance(const char *path, struct btrfs_ioctl_balance_args *args,
 	int e;
 	DIR *dirstream = NULL;
 
-	fd = open_file_or_dir(path, &dirstream);
-	if (fd < 0) {
-		fprintf(stderr, "ERROR: can't access '%s'\n", path);
+	fd = btrfs_open_dir(path, &dirstream, 1);
+	if (fd < 0)
 		return 1;
-	}
 
 	ret = ioctl(fd, BTRFS_IOC_BALANCE_V2, args);
 	e = errno;

@@ -503,11 +501,9 @@ static int cmd_balance_pause(int argc, char **argv)
 
 	path = argv[1];
 
-	fd = open_file_or_dir(path, &dirstream);
-	if (fd < 0) {
-		fprintf(stderr, "ERROR: can't access '%s'\n", path);
+	fd = btrfs_open_dir(path, &dirstream, 1);
+	if (fd < 0)
 		return 1;
-	}
 
 	ret = ioctl(fd, BTRFS_IOC_BALANCE_CTL, BTRFS_BALANCE_CTL_PAUSE);
 	e = errno;

@@ -544,11 +540,9 @@ static int cmd_balance_cancel(int argc, char **argv)
 
 	path = argv[1];
 
-	fd = open_file_or_dir(path, &dirstream);
-	if (fd < 0) {
-		fprintf(stderr, "ERROR: can't access '%s'\n", path);
+	fd = btrfs_open_dir(path, &dirstream, 1);
+	if (fd < 0)
 		return 1;
-	}
 
 	ret = ioctl(fd, BTRFS_IOC_BALANCE_CTL, BTRFS_BALANCE_CTL_CANCEL);
 	e = errno;

@@ -586,11 +580,9 @@ static int cmd_balance_resume(int argc, char **argv)
 
 	path = argv[1];
 
-	fd = open_file_or_dir(path, &dirstream);
-	if (fd < 0) {
-		fprintf(stderr, "ERROR: can't access '%s'\n", path);
+	fd = btrfs_open_dir(path, &dirstream, 1);
+	if (fd < 0)
 		return 1;
-	}
 
 	memset(&args, 0, sizeof(args));
 	args.flags |= BTRFS_BALANCE_RESUME;

@@ -679,11 +671,9 @@ static int cmd_balance_status(int argc, char **argv)
 
 	path = argv[optind];
 
-	fd = open_file_or_dir(path, &dirstream);
-	if (fd < 0) {
-		fprintf(stderr, "ERROR: can't access '%s'\n", path);
+	fd = btrfs_open_dir(path, &dirstream, 1);
+	if (fd < 0)
 		return 2;
-	}
 
 	ret = ioctl(fd, BTRFS_IOC_BALANCE_PROGRESS, &args);
 	e = errno;

Signature · 1 line

1.8.5.1

Zhao Lei

author

2015-10-12 13:24 UTC #4 lore raw

PATCH 4/11 btrfs-progs: inspect: Bypass unnecessary clean function in open_error

In reply to Zhao Lei

No need to cleanup fd in open_fail case, because it is not opened.

Signed-off-by Zhao Lei <[email protected]>

---
 cmds-inspect.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

cmds-inspect.c +1 -2

index fc3db99..879fd43 100644
@@ -626,9 +626,8 @@ static int cmd_inspect_min_dev_size(int argc, char **argv)
 	}
 
 	ret = print_min_dev_size(fd, devid);
-out:
 	close_file_or_dir(fd, dirstream);
-
+out:
 	return !!ret;
 }

Signature · 1 line

1.8.5.1

Zhao Lei

author

2015-10-12 13:24 UTC #5 lore raw

PATCH 5/11 btrfs-progs: inspect: set return value of error case

In reply to Zhao Lei

In case of open_file_or_dir() failed, ret is not set to right value, and the function will return unwanted value(ret of sprintf).

Signed-off-by Zhao Lei <[email protected]>

---
 cmds-inspect.c | 1 +
 1 file changed, 1 insertion(+)

cmds-inspect.c +1 -0

index 879fd43..a13a170 100644
@@ -243,6 +243,7 @@ static int cmd_inspect_logical_resolve(int argc, char **argv)
 				if (path_fd < 0) {
 					fprintf(stderr, "ERROR: can't access "
 						"'%s'\n", full_path);
+					ret = -ENOENT;
 					goto out;
 				}
 			}

Signature · 1 line

1.8.5.1

Zhao Lei

author

2015-10-12 13:24 UTC #6 lore raw

PATCH 6/11 btrfs-progs: inspect: use btrfs_open_dir for btrfs inspect command

In reply to Zhao Lei

We can use btrfs_open_dir() to check whether target dir is in btrfs's mount point before open, instead of checking it in kernel space of ioctl, and return fuzzy error message.

Before patch:

  # ./btrfs inspect-internal rootid /mnt/tmp1
  ERROR: Failed to lookup root id - Inappropriate ioctl for device
  btrfs inspect-internal rootid: rootid failed with ret=-1
  # ./btrfs inspect-internal inode-resolve 256 /mnt/tmp1
  ioctl ret=-1, error: Inappropriate ioctl for device
  # ./btrfs inspect-internal min-dev-size /mnt/tmp1
  Error invoking tree search ioctl: Inappropriate ioctl for device

After patch:

  # ./btrfs inspect-internal rootid /mnt/tmp1
  ERROR: not a btrfs filesystem: /mnt/tmp1
  # ./btrfs inspect-internal inode-resolve 256 /mnt/tmp1
  ERROR: not a btrfs filesystem: /mnt/tmp1
  # ./btrfs inspect-internal min-dev-size /mnt/tmp1
  ERROR: not a btrfs filesystem: /mnt/tmp1

Signed-off-by Zhao Lei <[email protected]>

---
 cmds-inspect.c | 22 +++++++---------------
 1 file changed, 7 insertions(+), 15 deletions(-)

cmds-inspect.c +7 ~~-15~~

index a13a170..40ab49b 100644
@@ -116,11 +116,9 @@ static int cmd_inspect_inode_resolve(int argc, char **argv)
 	if (check_argc_exact(argc - optind, 2))
 		usage(cmd_inspect_inode_resolve_usage);
 
-	fd = open_file_or_dir(argv[optind+1], &dirstream);
-	if (fd < 0) {
-		fprintf(stderr, "ERROR: can't access '%s'\n", argv[optind+1]);
+	fd = btrfs_open_dir(argv[optind + 1], &dirstream, 1);
+	if (fd < 0)
 		return 1;
-	}
 
 	ret = __ino_to_path_fd(arg_strtou64(argv[optind]), fd, verbose,
 			       argv[optind+1]);

@@ -189,9 +187,8 @@ static int cmd_inspect_logical_resolve(int argc, char **argv)
 	loi.size = size;
 	loi.inodes = ptr_to_u64(inodes);
 
-	fd = open_file_or_dir(argv[optind+1], &dirstream);
+	fd = btrfs_open_dir(argv[optind + 1], &dirstream, 1);
 	if (fd < 0) {
-		fprintf(stderr, "ERROR: can't access '%s'\n", argv[optind+1]);
 		ret = 12;
 		goto out;
 	}

@@ -239,10 +236,8 @@ static int cmd_inspect_logical_resolve(int argc, char **argv)
 						name);
 				BUG_ON(ret >= bytes_left);
 				free(name);
-				path_fd = open_file_or_dir(full_path, &dirs);
+				path_fd = btrfs_open_dir(full_path, &dirs, 1);
 				if (path_fd < 0) {
-					fprintf(stderr, "ERROR: can't access "
-						"'%s'\n", full_path);
 					ret = -ENOENT;
 					goto out;
 				}

@@ -279,9 +274,8 @@ static int cmd_inspect_subvolid_resolve(int argc, char **argv)
 	if (check_argc_exact(argc, 3))
 		usage(cmd_inspect_subvolid_resolve_usage);
 
-	fd = open_file_or_dir(argv[2], &dirstream);
+	fd = btrfs_open_dir(argv[2], &dirstream, 1);
 	if (fd < 0) {
-		fprintf(stderr, "ERROR: can't access '%s'\n", argv[2]);
 		ret = -ENOENT;
 		goto out;
 	}

@@ -320,9 +314,8 @@ static int cmd_inspect_rootid(int argc, char **argv)
 	if (check_argc_exact(argc, 2))
 		usage(cmd_inspect_rootid_usage);
 
-	fd = open_file_or_dir(argv[1], &dirstream);
+	fd = btrfs_open_dir(argv[1], &dirstream, 1);
 	if (fd < 0) {
-		fprintf(stderr, "ERROR: can't access '%s'\n", argv[1]);
 		ret = -ENOENT;
 		goto out;
 	}

@@ -619,9 +612,8 @@ static int cmd_inspect_min_dev_size(int argc, char **argv)
 	if (check_argc_exact(argc - optind, 1))
 		usage(cmd_inspect_min_dev_size_usage);
 
-	fd = open_file_or_dir(argv[optind], &dirstream);
+	fd = btrfs_open_dir(argv[optind], &dirstream, 1);
 	if (fd < 0) {
-		fprintf(stderr, "ERROR: can't access '%s'\n", argv[optind]);
 		ret = -ENOENT;
 		goto out;
 	}

Signature · 1 line

1.8.5.1

Zhao Lei

author

2015-10-12 13:24 UTC #7 lore raw

PATCH 7/11 btrfs-progs: qgroup: use btrfs_open_dir for btrfs qgroup command

In reply to Zhao Lei

We can use btrfs_open_dir() to check whether target dir is in btrfs's mount point before open, instead of checking it in kernel space of ioctl, and return fuzzy error message.

Before patch:

  # ./btrfs qgroup create 1/5  /mnt/tmp1
  ERROR: unable to create quota group: Inappropriate ioctl for device
  #
  # ./btrfs qgroup assign 1/5 2/5 /mnt/tmp1
  ERROR: unable to assign quota group: Inappropriate ioctl for device
  #
  # ./btrfs qgroup show /mnt/tmp1
  ERROR: can't perform the search - Inappropriate ioctl for device
  ERROR: can't list qgroups: Inappropriate ioctl for device
  #
  # ./btrfs qgroup limit 1G 1/5  /mnt/tmp1
  ERROR: unable to limit requested quota group: Inappropriate ioctl for device

After patch:

  # ./btrfs qgroup create 1/5 /mnt/tmp1
  ERROR: not a btrfs filesystem: /mnt/tmp1
  # ./btrfs qgroup assign 1/5 2/5 /mnt/tmp1
  ERROR: not a btrfs filesystem: /mnt/tmp1
  # ./btrfs qgroup show /mnt/tmp1
  ERROR: not a btrfs filesystem: /mnt/tmp1
  # ./btrfs qgroup limit 1G 1/5 /mnt/tmp1
  ERROR: not a btrfs filesystem: /mnt/tmp1

Signed-off-by Zhao Lei <[email protected]>

---
 cmds-qgroup.c | 24 ++++++++----------------
 1 file changed, 8 insertions(+), 16 deletions(-)

cmds-qgroup.c +8 ~~-16~~

index 48c1733..0ad99f4 100644
@@ -79,11 +79,9 @@ static int qgroup_assign(int assign, int argc, char **argv)
 		fprintf(stderr, "ERROR: bad relation requested '%s'\n", path);
 		return 1;
 	}
-	fd = open_file_or_dir(path, &dirstream);
-	if (fd < 0) {
-		fprintf(stderr, "ERROR: can't access '%s'\n", path);
+	fd = btrfs_open_dir(path, &dirstream, 1);
+	if (fd < 0)
 		return 1;
-	}
 
 	ret = ioctl(fd, BTRFS_IOC_QGROUP_ASSIGN, &args);
 	e = errno;

@@ -137,11 +135,9 @@ static int qgroup_create(int create, int argc, char **argv)
 	args.create = create;
 	args.qgroupid = parse_qgroupid(argv[1]);
 
-	fd = open_file_or_dir(path, &dirstream);
-	if (fd < 0) {
-		fprintf(stderr, "ERROR: can't access '%s'\n", path);
+	fd = btrfs_open_dir(path, &dirstream, 1);
+	if (fd < 0)
 		return 1;
-	}
 
 	ret = ioctl(fd, BTRFS_IOC_QGROUP_CREATE, &args);
 	e = errno;

@@ -351,11 +347,9 @@ static int cmd_qgroup_show(int argc, char **argv)
 		usage(cmd_qgroup_show_usage);
 
 	path = argv[optind];
-	fd = open_file_or_dir(path, &dirstream);
-	if (fd < 0) {
-		fprintf(stderr, "ERROR: can't access '%s'\n", path);
+	fd = btrfs_open_dir(path, &dirstream, 1);
+	if (fd < 0)
 		return 1;
-	}
 
 	if (filter_flag) {
 		qgroupid = btrfs_get_path_rootid(fd);

@@ -460,11 +454,9 @@ static int cmd_qgroup_limit(int argc, char **argv)
 	} else
 		usage(cmd_qgroup_limit_usage);
 
-	fd = open_file_or_dir(path, &dirstream);
-	if (fd < 0) {
-		fprintf(stderr, "ERROR: can't access '%s'\n", path);
+	fd = btrfs_open_dir(path, &dirstream, 1);
+	if (fd < 0)
 		return 1;
-	}
 
 	ret = ioctl(fd, BTRFS_IOC_QGROUP_LIMIT, &args);
 	e = errno;

Signature · 1 line

1.8.5.1

Zhao Lei

author

2015-10-12 13:24 UTC #8 lore raw

PATCH 8/11 btrfs-progs: quota: use btrfs_open_dir for btrfs quota command

In reply to Zhao Lei

We can use btrfs_open_dir() to check whether target dir is in btrfs's mount point before open, instead of checking it in kernel space of ioctl, and return fuzzy error message.

Before patch:

  # ./btrfs quota enable /mnt/tmp1
  ERROR: quota command failed: Inappropriate ioctl for device
  # ./btrfs quota disable /mnt/tmp1
  ERROR: quota command failed: Inappropriate ioctl for device
  # ./btrfs quota rescan /mnt/tmp1
  ERROR: quota rescan failed: Inappropriate ioctl for device
  #

After patch:

  # ./btrfs quota enable /mnt/tmp1
  ERROR: not a btrfs filesystem: /mnt/tmp1
  # ./btrfs quota disable /mnt/tmp1
  ERROR: not a btrfs filesystem: /mnt/tmp1
  # ./btrfs quota rescan /mnt/tmp1
  ERROR: not a btrfs filesystem: /mnt/tmp1
  #

Signed-off-by Zhao Lei <[email protected]>

---
 cmds-quota.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

cmds-quota.c +4 -8

index 8adc1bf..efbc3ef 100644
@@ -45,11 +45,9 @@ static int quota_ctl(int cmd, int argc, char **argv)
 	memset(&args, 0, sizeof(args));
 	args.cmd = cmd;
 
-	fd = open_file_or_dir(path, &dirstream);
-	if (fd < 0) {
-		fprintf(stderr, "ERROR: can't access '%s'\n", path);
+	fd = btrfs_open_dir(path, &dirstream, 1);
+	if (fd < 0)
 		return 1;
-	}
 
 	ret = ioctl(fd, BTRFS_IOC_QUOTA_CTL, &args);
 	e = errno;

@@ -141,11 +139,9 @@ static int cmd_quota_rescan(int argc, char **argv)
 	memset(&args, 0, sizeof(args));
 
 	path = argv[optind];
-	fd = open_file_or_dir(path, &dirstream);
-	if (fd < 0) {
-		fprintf(stderr, "ERROR: can't access '%s'\n", path);
+	fd = btrfs_open_dir(path, &dirstream, 1);
+	if (fd < 0)
 		return 1;
-	}
 
 	ret = ioctl(fd, ioctlnum, &args);
 	e = errno;

Signature · 1 line

1.8.5.1

Zhao Lei

author

2015-10-12 13:24 UTC #9 lore raw

PATCH 9/11 btrfs-progs: use btrfs_open_dir in open_path_or_dev_mnt

In reply to Zhao Lei

Use btrfs_open_dir() in open_path_or_dev_mnt() to make the function return error when target is neither block device nor btrfs mount point.

Also add "verbose" argument to let function output common error message instead of putting duplicated lines in caller.

Before patch:

  # ./btrfs device stats /mnt/tmp1
  ERROR: getting dev info for devstats failed: Inappropriate ioctl for device
  # ./btrfs replace start /dev/vdd /dev/vde /mnt/tmp1
  ERROR: ioctl(DEV_REPLACE_STATUS) failed on "/mnt/tmp1": Inappropriate ioctl for device

After patch:

  # ./btrfs device stats /mnt/tmp1
  ERROR: not a btrfs filesystem: /mnt/tmp1
  # ./btrfs replace start /dev/vdd /dev/vde /mnt/tmp1
  ERROR: not a btrfs filesystem: /mnt/tmp1

Signed-off-by Zhao Lei <[email protected]>

---
 cmds-device.c  | 13 ++-----------
 cmds-replace.c | 13 ++-----------
 cmds-scrub.c   | 28 +++++-----------------------
 utils.c        | 21 +++++++++++----------
 utils.h        |  2 +-
 5 files changed, 21 insertions(+), 56 deletions(-)

cmds-device.c +2 ~~-11~~

index 5f2b952..a9354f5 100644
@@ -385,18 +385,9 @@ static int cmd_device_stats(int argc, char **argv)
 
 	dev_path = argv[optind];
 
-	fdmnt = open_path_or_dev_mnt(dev_path, &dirstream);
-
-	if (fdmnt < 0) {
-		if (errno == EINVAL)
-			fprintf(stderr,
-				"ERROR: '%s' is not a mounted btrfs device\n",
-				dev_path);
-		else
-			fprintf(stderr, "ERROR: can't access '%s': %s\n",
-				dev_path, strerror(errno));
+	fdmnt = open_path_or_dev_mnt(dev_path, &dirstream, 1);
+	if (fdmnt < 0)
 		return 1;
-	}
 
 	ret = get_fs_info(dev_path, &fi_args, &di_args);
 	if (ret) {

cmds-replace.c +2 ~~-11~~

index 9ab8438..385b764 100644
@@ -170,18 +170,9 @@ static int cmd_replace_start(int argc, char **argv)
 		usage(cmd_replace_start_usage);
 	path = argv[optind + 2];
 
-	fdmnt = open_path_or_dev_mnt(path, &dirstream);
-
-	if (fdmnt < 0) {
-		if (errno == EINVAL)
-			fprintf(stderr,
-				"ERROR: '%s' is not a mounted btrfs device\n",
-				path);
-		else
-			fprintf(stderr, "ERROR: can't access '%s': %s\n",
-				path, strerror(errno));
+	fdmnt = open_path_or_dev_mnt(path, &dirstream, 1);
+	if (fdmnt < 0)
 		goto leave_with_error;
-	}
 
 	/* check for possible errors before backgrounding */
 	status_args.cmd = BTRFS_IOCTL_DEV_REPLACE_CMD_STATUS;

cmds-scrub.c +5 ~~-23~~

index ea6ffc9..da614f2 100644
@@ -1198,17 +1198,9 @@ static int scrub_start(int argc, char **argv, int resume)
 
 	path = argv[optind];
 
-	fdmnt = open_path_or_dev_mnt(path, &dirstream);
-
-	if (fdmnt < 0) {
-		if (errno == EINVAL)
-			error_on(!do_quiet, "'%s' is not a mounted btrfs device",
-				path);
-		else
-			error_on(!do_quiet, "can't access '%s': %s",
-				path, strerror(errno));
+	fdmnt = open_path_or_dev_mnt(path, &dirstream, !do_quiet);
+	if (fdmnt < 0)
 		return 1;
-	}
 
 	ret = get_fs_info(path, &fi_args, &di_args);
 	if (ret) {

@@ -1604,12 +1596,8 @@ static int cmd_scrub_cancel(int argc, char **argv)
 
 	path = argv[1];
 
-	fdmnt = open_path_or_dev_mnt(path, &dirstream);
+	fdmnt = open_path_or_dev_mnt(path, &dirstream, 1);
 	if (fdmnt < 0) {
-		if (errno == EINVAL)
-			error("'%s' is not a mounted btrfs device", path);
-		else
-			error("can't access '%s': %s", path, strerror(errno));
 		ret = 1;
 		goto out;
 	}

@@ -1705,15 +1693,9 @@ static int cmd_scrub_status(int argc, char **argv)
 
 	path = argv[optind];
 
-	fdmnt = open_path_or_dev_mnt(path, &dirstream);
-
-	if (fdmnt < 0) {
-		if (errno == EINVAL)
-			error("'%s' is not a mounted btrfs device", path);
-		else
-			error("can't access '%s': %s", path, strerror(errno));
+	fdmnt = open_path_or_dev_mnt(path, &dirstream, 1);
+	if (fdmnt < 0)
 		return 1;
-	}
 
 	ret = get_fs_info(path, &fi_args, &di_args);
 	if (ret) {

utils.c +11 ~~-10~~

index f1e3248..6f5df23 100644
@@ -1081,27 +1081,28 @@ out:
  *
  * On error, return -1, errno should be set.
  */
-int open_path_or_dev_mnt(const char *path, DIR **dirstream)
+int open_path_or_dev_mnt(const char *path, DIR **dirstream, int verbose)
 {
 	char mp[PATH_MAX];
-	int fdmnt;
-
-	fdmnt = is_block_device(path);
-	if (fdmnt == 1) {
-		int ret;
+	int ret;
 
+	if (is_block_device(path)) {
 		ret = get_btrfs_mount(path, mp, sizeof(mp));
 		if (ret < 0) {
 			/* not a mounted btrfs dev */
+			error_on(verbose, "'%s' is not a mounted btrfs device",
+				 path);
 			errno = EINVAL;
 			return -1;
 		}
-		fdmnt = open_file_or_dir(mp, dirstream);
-	} else if (fdmnt == 0) {
-		fdmnt = open_file_or_dir(path, dirstream);
+		ret = open_file_or_dir(mp, dirstream);
+		error_on(verbose && ret < 0, "can't access '%s': %s",
+			 path, strerror(errno));
+	} else {
+		ret = btrfs_open_dir(path, dirstream, 1);
 	}
 
-	return fdmnt;
+	return ret;
 }
 
 /*

utils.h +1 -1

index 044ea15..1dc12ec 100644
@@ -158,7 +158,7 @@ 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 check_arg_type(const char *input);
-int open_path_or_dev_mnt(const char *path, DIR **dirstream);
+int open_path_or_dev_mnt(const char *path, DIR **dirstream, int verbose);
 int btrfs_open_dir(const char *path, DIR **dirstream, int verbose);
 u64 btrfs_device_size(int fd, struct stat *st);
 /* Helper to always get proper size of the destination string */

Signature · 1 line

1.8.5.1

Zhao Lei

author

2015-10-12 13:24 UTC #10 lore raw

PATCH 10/11 btrfs-progs: replace: use btrfs_open_dir for btrfs replace command

In reply to Zhao Lei

We can use btrfs_open_dir() to check whether target dir is in btrfs's mount point before open, instead of checking it in kernel space of ioctl, and return fuzzy error message.

Before patch:

  # ./btrfs replace cancel /mnt/tmp1
  ERROR: ioctl(DEV_REPLACE_CANCEL) failed on "/mnt/tmp1": Inappropriate ioctl for device
  # ./btrfs replace status /mnt/tmp1
  ERROR: ioctl(DEV_REPLACE_STATUS) failed on "/mnt/tmp1": Inappropriate ioctl for device

After patch:

  # ./btrfs replace cancel /mnt/tmp1
  ERROR: not a btrfs filesystem: /mnt/tmp1
  # ./btrfs replace status /mnt/tmp1
  ERROR: not a btrfs filesystem: /mnt/tmp1

Signed-off-by Zhao Lei <[email protected]>

---
 cmds-replace.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

cmds-replace.c +4 ~~-12~~

index 385b764..9596f2a 100644
@@ -348,7 +348,6 @@ static const char *const cmd_replace_status_usage[] = {
 static int cmd_replace_status(int argc, char **argv)
 {
 	int fd;
-	int e;
 	int c;
 	char *path;
 	int once = 0;

@@ -370,13 +369,9 @@ static int cmd_replace_status(int argc, char **argv)
 		usage(cmd_replace_status_usage);
 
 	path = argv[optind];
-	fd = open_file_or_dir(path, &dirstream);
-	e = errno;
-	if (fd < 0) {
-		fprintf(stderr, "ERROR: can't access \"%s\": %s\n",
-			path, strerror(e));
+	fd = btrfs_open_dir(path, &dirstream, 1);
+	if (fd < 0)
 		return 1;
-	}
 
 	ret = print_replace_status(fd, path, once);
 	close_file_or_dir(fd, dirstream);

@@ -541,12 +536,9 @@ static int cmd_replace_cancel(int argc, char **argv)
 		usage(cmd_replace_cancel_usage);
 
 	path = argv[optind];
-	fd = open_file_or_dir(path, &dirstream);
-	if (fd < 0) {
-		fprintf(stderr, "ERROR: can't access \"%s\": %s\n",
-			path, strerror(errno));
+	fd = btrfs_open_dir(path, &dirstream, 1);
+	if (fd < 0)
 		return 1;
-	}
 
 	args.cmd = BTRFS_IOCTL_DEV_REPLACE_CMD_CANCEL;
 	args.result = BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_RESULT;

Signature · 1 line

1.8.5.1

Zhao Lei

author

2015-10-12 13:24 UTC #11 lore raw

PATCH 11/11 btrfs-progs: fragments: use btrfs_open_dir for btrfs-fragments command

In reply to Zhao Lei

We can use btrfs_open_dir() to check whether target dir is in btrfs's mount point before open, instead of checking it in deeper code, and return fuzzy error message.

Before patch:
./btrfs-fragments -o 123 /mnt/tmp1
ERROR: can't perform the search

After patch:

  # ./btrfs-fragments -o 123 /mnt/tmp1
  ERROR: not a btrfs filesystem: /mnt/tmp1

Signed-off-by Zhao Lei <[email protected]>

---
 btrfs-fragments.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

btrfs-fragments.c +2 -4

index d742f60..17768c3 100644
@@ -436,11 +436,9 @@ int main(int argc, char **argv)
 
 	path = argv[optind++];
 
-	fd = open_file_or_dir(path, &dirstream);
-	if (fd < 0) {
-		fprintf(stderr, "ERROR: can't access '%s'\n", path);
+	fd = btrfs_open_dir(path, &dirstream, 1);
+	if (fd < 0)
 		exit(1);
-	}
 
 	if (flags == 0)
 		flags = BTRFS_BLOCK_GROUP_DATA | BTRFS_BLOCK_GROUP_METADATA;

Signature · 1 line

1.8.5.1

David Sterba

2015-10-13 15:59 UTC #12 lore raw

In reply to Zhao Lei

Quoted 4 lines · Zhao Lei

Use btrfs_open_dir() instead of open_file_or_dir(), to show error before real action(in ioctl or tree search), to make the error message exact and unified.

Quoted 13 lines

Zhao Lei (11):
btrfs-progs: subvolume: use btrfs_open_dir for btrfs subvolume command
btrfs-progs: filesystem: use btrfs_open_dir for btrfs filesystem
command
btrfs-progs: balance: use btrfs_open_dir for btrfs balance command
btrfs-progs: inspect: Bypass unnecessary clean function in open_error
btrfs-progs: inspect: set return value of error case
btrfs-progs: inspect: use btrfs_open_dir for btrfs inspect command
btrfs-progs: qgroup: use btrfs_open_dir for btrfs qgroup command
btrfs-progs: quota: use btrfs_open_dir for btrfs quota command
btrfs-progs: use btrfs_open_dir in open_path_or_dev_mnt
btrfs-progs: replace: use btrfs_open_dir for btrfs replace command
btrfs-progs: fragments: use btrfs_open_dir for btrfs-fragments command

All merged, thanks! I appreciate you took the time to test all the changes and the patch separation made review easy.