Re: [PATCH] mdadm/util:integrate fstat and stat into a utility function

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 03/30/2017 12:46 AM, Zhilong Liu wrote:
mdadm/util: lots of dupilicate codes about stat and fstat
check the block device, move them into a utility function
to make it concise.

Signed-off-by: Zhilong Liu <zlliu@xxxxxxxx>
---
 Assemble.c    |  9 +--------
 Build.c       | 10 +---------
 Create.c      |  5 +----
 Dump.c        |  3 +--
 Grow.c        |  4 +---
 Incremental.c | 27 +++------------------------
 Manage.c      |  9 +--------
 mdadm.h       |  1 +
 super-intel.c |  9 +--------
 util.c        | 15 +++++++++++++++
 10 files changed, 26 insertions(+), 66 deletions(-)

I'm a little mixed on this one. Defaulting to stat() rather than fstat() seems like a sledgehammer approach for the cases where we already have the fd open. I would prefer to default to fstat or have two versions I'd say.

Cheers,
Jes


diff --git a/Assemble.c b/Assemble.c
index 6a6a56b..d3bfaa2 100644
--- a/Assemble.c
+++ b/Assemble.c
@@ -204,14 +204,7 @@ static int select_devices(struct mddev_dev *devlist,
 				pr_err("cannot open device %s: %s\n",
 				       devname, strerror(errno));
 			tmpdev->used = 2;
-		} else if (fstat(dfd, &stb)< 0) {
-			/* Impossible! */
-			pr_err("fstat failed for %s: %s\n",
-			       devname, strerror(errno));
-			tmpdev->used = 2;
-		} else if ((stb.st_mode & S_IFMT) != S_IFBLK) {
-			pr_err("%s is not a block device.\n",
-			       devname);
+		} else if (check_block_dev(devname)) {
 			tmpdev->used = 2;
 		} else if (must_be_container(dfd)) {
 			if (st) {
diff --git a/Build.c b/Build.c
index a5fcc06..4fa606c 100644
--- a/Build.c
+++ b/Build.c
@@ -67,16 +67,8 @@ int Build(char *mddev, struct mddev_dev *devlist,
 			missing_disks++;
 			continue;
 		}
-		if (stat(dv->devname, &stb)) {
-			pr_err("Cannot find %s: %s\n",
-				dv->devname, strerror(errno));
-			return 1;
-		}
-		if ((stb.st_mode & S_IFMT) != S_IFBLK) {
-			pr_err("%s is not a block device.\n",
-				dv->devname);
+		if (check_block_dev(dv->devname))
 			return 1;
-		}
 	}

 	if (s->raiddisks != subdevs) {
diff --git a/Create.c b/Create.c
index 10e7d10..dbd9ce3 100644
--- a/Create.c
+++ b/Create.c
@@ -327,11 +327,8 @@ int Create(struct supertype *st, char *mddev,
 				dname, strerror(errno));
 			exit(2);
 		}
-		if (fstat(dfd, &stb) != 0 ||
-		    (stb.st_mode & S_IFMT) != S_IFBLK) {
+		if (check_block_dev(dname)) {
 			close(dfd);
-			pr_err("%s is not a block device\n",
-				dname);
 			exit(2);
 		}
 		close(dfd);
diff --git a/Dump.c b/Dump.c
index 7bdbf6f..29fc02a 100644
--- a/Dump.c
+++ b/Dump.c
@@ -131,8 +131,7 @@ int Dump_metadata(char *dev, char *dir, struct context *c,
 		if (de->d_name[0] == '.')
 			continue;
 		xasprintf(&p, "/dev/disk/by-id/%s", de->d_name);
-		if (stat(p, &stb) != 0 ||
-		    (stb.st_mode & S_IFMT) != S_IFBLK ||
+		if (check_block_dev(p) != 0 ||
 		    stb.st_rdev != dstb.st_rdev) {
 			/* Not this one */
 			free(p);
diff --git a/Grow.c b/Grow.c
index b86b53e..063996d 100755
--- a/Grow.c
+++ b/Grow.c
@@ -145,9 +145,7 @@ int Grow_Add_device(char *devname, int fd, char *newdev)
 		free(st);
 		return 1;
 	}
-	fstat(nfd, &stb);
-	if ((stb.st_mode & S_IFMT) != S_IFBLK) {
-		pr_err("%s is not a block device!\n", newdev);
+	if (check_block_dev(newdev)) {
 		close(nfd);
 		free(st);
 		return 1;
diff --git a/Incremental.c b/Incremental.c
index 81afc7e..b2fad39 100644
--- a/Incremental.c
+++ b/Incremental.c
@@ -108,18 +108,8 @@ int Incremental(struct mddev_dev *devlist, struct context *c,

 	struct createinfo *ci = conf_get_create_info();

-	if (stat(devname, &stb) < 0) {
-		if (c->verbose >= 0)
-			pr_err("stat failed for %s: %s.\n",
-				devname, strerror(errno));
+	if (check_block_dev(devname))
 		return rv;
-	}
-	if ((stb.st_mode & S_IFMT) != S_IFBLK) {
-		if (c->verbose >= 0)
-			pr_err("%s is not a block device.\n",
-				devname);
-		return rv;
-	}
 	dfd = dev_open(devname, O_RDONLY);
 	if (dfd < 0) {
 		if (c->verbose >= 0)
@@ -159,8 +149,7 @@ int Incremental(struct mddev_dev *devlist, struct context *c,
 		devlist = conf_get_devs();
 		for (;devlist; devlist = devlist->next) {
 			struct stat st2;
-			if (stat(devlist->devname, &st2) == 0 &&
-			    (st2.st_mode & S_IFMT) == S_IFBLK &&
+			if (check_block_dev(devlist->devname) == 0 &&
 			    st2.st_rdev == stb.st_rdev)
 				break;
 		}
@@ -175,18 +164,8 @@ int Incremental(struct mddev_dev *devlist, struct context *c,
 	/* 2/ Find metadata, reject if none appropriate (check
 	 *            version/name from args) */

-	if (fstat(dfd, &stb) < 0) {
-		if (c->verbose >= 0)
-			pr_err("fstat failed for %s: %s.\n",
-				devname, strerror(errno));
+	if (check_block_dev(devname))
 		goto out;
-	}
-	if ((stb.st_mode & S_IFMT) != S_IFBLK) {
-		if (c->verbose >= 0)
-			pr_err("%s is not a block device.\n",
-				devname);
-		goto out;
-	}

 	dinfo.disk.major = major(stb.st_rdev);
 	dinfo.disk.minor = minor(stb.st_rdev);
diff --git a/Manage.c b/Manage.c
index 55218d9..8687084 100644
--- a/Manage.c
+++ b/Manage.c
@@ -1543,17 +1543,10 @@ int Manage_subdevs(char *devname, int fd,
 				close(tfd);
 			} else {
 				int open_err = errno;
-				if (stat(dv->devname, &stb) != 0) {
-					pr_err("Cannot find %s: %s\n",
-					       dv->devname, strerror(errno));
-					goto abort;
-				}
-				if ((stb.st_mode & S_IFMT) != S_IFBLK) {
+				if (check_block_dev(dv->devname) != 0) {
 					if (dv->disposition == 'M')
 						/* non-fatal. Also improbable */
 						continue;
-					pr_err("%s is not a block device.\n",
-					       dv->devname);
 					goto abort;
 				}
 				if (dv->disposition == 'r')
diff --git a/mdadm.h b/mdadm.h
index dbf1f92..ad43a35 100644
--- a/mdadm.h
+++ b/mdadm.h
@@ -1416,6 +1416,7 @@ extern int parse_cluster_confirm_arg(char *inp, char **devname, int *slot);
 extern int check_ext2(int fd, char *name);
 extern int check_reiser(int fd, char *name);
 extern int check_raid(int fd, char *name);
+extern int check_block_dev(char *dev);
 extern int check_partitions(int fd, char *dname,
 			    unsigned long long freesize,
 			    unsigned long long size);
diff --git a/super-intel.c b/super-intel.c
index 785488a..21b5887 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -6584,14 +6584,7 @@ count_volumes_list(struct md_list *devlist, char *homehost,
 			dprintf("cannot open device %s: %s\n",
 				devname, strerror(errno));
 			tmpdev->used = 2;
-		} else if (fstat(dfd, &stb)< 0) {
-			/* Impossible! */
-			dprintf("fstat failed for %s: %s\n",
-				devname, strerror(errno));
-			tmpdev->used = 2;
-		} else if ((stb.st_mode & S_IFMT) != S_IFBLK) {
-			dprintf("%s is not a block device.\n",
-				devname);
+		} else if (check_block_dev(devname)) {
 			tmpdev->used = 2;
 		} else if (must_be_container(dfd)) {
 			struct supertype *cst;
diff --git a/util.c b/util.c
index 683c869..e7aeb54 100644
--- a/util.c
+++ b/util.c
@@ -750,6 +750,21 @@ int ask(char *mesg)
 }
 #endif /* MDASSEMBLE */

+int check_block_dev(char *dev)
+{
+	struct stat stb;
+
+	if (stat(dev, &stb) != 0) {
+		pr_err("stat failed for %s: %s\n", dev, strerror(errno));
+		return -1;
+	}
+	if ((S_IFMT & stb.st_mode) != S_IFBLK) {
+		pr_err("%s is not a block device.\n", dev);
+		return -1;
+	}
+	return 0;
+}
+
 int is_standard(char *dev, int *nump)
 {
 	/* tests if dev is a "standard" md dev name.


--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux RAID Wiki]     [ATA RAID]     [Linux SCSI Target Infrastructure]     [Linux Block]     [Linux IDE]     [Linux SCSI]     [Linux Hams]     [Device Mapper]     [Device Mapper Cryptographics]     [Kernel]     [Linux Admin]     [Linux Net]     [GFS]     [RPM]     [git]     [Yosemite Forum]


  Powered by Linux