Re: [PATCH 1/4] mdadm/util:integrate stat operations into one utility

Liu Zhilong <zlliu@xxxxxxxx> · Fri, 14 Apr 2017 18:14:27 +0800

On 04/05/2017 11:42 PM, jes.sorensen@xxxxxxxxx wrote:
Zhilong Liu <zlliu@xxxxxxxx> writes:
mdadm/util: there are dupilicate codes about stat checking the
block device, move the operations into one utility function and
make it concise.

Signed-off-by: Zhilong Liu <zlliu@xxxxxxxx>
---
  Assemble.c    |  5 ++---
  Build.c       | 21 +++------------------
  Incremental.c | 12 +-----------
  Manage.c      | 10 ++--------
  mdadm.h       |  1 +
  super-intel.c |  4 +---
  util.c        | 15 +++++++++++++++
  7 files changed, 25 insertions(+), 43 deletions(-)

diff --git a/Assemble.c b/Assemble.c
index 672cd12..c6571e6 100644
--- a/Assemble.c
+++ b/Assemble.c
@@ -522,9 +522,8 @@ static int select_devices(struct mddev_dev *devlist,
  		struct stat stb;
  		if (tmpdev->used != 3)
  			continue;
-		if (stat(tmpdev->devname, &stb)< 0) {
-			pr_err("fstat failed for %s: %s\n",
-			       tmpdev->devname, strerror(errno));
+		if (check_blkdev_via_stat(tmpdev->devname) &&
+		    stat(tmpdev->devname, &stb)) {
  			tmpdev->used = 2;
  		} else {
  			struct dev_policy *pol = devid_policy(stb.st_rdev);
Sorry, but this change makes no sense. First calling a function using
stat() to check, and then call stat() one more time to get the stat data
anyway.

In fact, looking through this patch, you are doing it all over the
place.

This makes things worse, not better!

diff --git a/Build.c b/Build.c
index 691dd6f..b500ad7 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_blkdev_via_stat(dv->devname))
  			return 1;
-		}
  	}
  
  	if (s->raiddisks != subdevs) {
@@ -171,16 +163,9 @@ int Build(char *mddev, struct mddev_dev *devlist,
  		int fd;
  		if (strcmp("missing", dv->devname) == 0)
  			continue;
-		if (stat(dv->devname, &stb)) {
-			pr_err("Weird: %s has disappeared.\n",
-				dv->devname);
+		if (check_blkdev_via_stat(dv->devname))
  			goto abort;
-		}
-		if ((stb.st_mode & S_IFMT)!= S_IFBLK) {
-			pr_err("Weird: %s is no longer a block device.\n",
-				dv->devname);
-			goto abort;
-		}
+		stat(dv->devname, &stb);
  		fd = open(dv->devname, O_RDONLY|O_EXCL);
  		if (fd < 0) {
  			pr_err("Cannot open %s: %s\n",
diff --git a/Incremental.c b/Incremental.c
index 28f1f77..78adf18 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_blkdev_via_stat(devname) && stat(devname, &stb))
  		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)
diff --git a/Manage.c b/Manage.c
index 618c98b..6aec65d 100644
--- a/Manage.c
+++ b/Manage.c
@@ -1544,17 +1544,11 @@ 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_blkdev_via_stat(dv->devname) &&
+				    stat(dv->devname, &stb)) {
  					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 612bd86..0aea822 100644
--- a/mdadm.h
+++ b/mdadm.h
@@ -1422,6 +1422,7 @@ extern int check_raid(int fd, char *name);
  extern int check_partitions(int fd, char *dname,
  			    unsigned long long freesize,
  			    unsigned long long size);
+extern int check_blkdev_via_stat(char *dev);
  
  extern int get_mdp_major(void);
  extern int get_maj_min(char *dev, int *major, int *minor);
diff --git a/super-intel.c b/super-intel.c
index 84dfe2b..924ad8a 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -6953,9 +6953,7 @@ static int validate_geometry_imsm_volume(struct supertype *st, int level,
  	}
  
  	/* This device must be a member of the set */
-	if (stat(dev, &stb) < 0)
-		return 0;
-	if ((S_IFMT & stb.st_mode) != S_IFBLK)
+	if (check_blkdev_via_stat(dev) && stat(dev, &stb))
  		return 0;
  	for (dl = super->disks ; dl ; dl = dl->next) {
  		if (dl->major == (int)major(stb.st_rdev) &&
diff --git a/util.c b/util.c
index 9fc7ba0..03d4256 100644
--- a/util.c
+++ b/util.c
@@ -774,6 +774,21 @@ int ask(char *mesg)
  }
  #endif /* MDASSEMBLE */
  
+int check_blkdev_via_stat(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;
+}
+
For a function like this, lets name it better md_is_blkdev()

How about return the devid after checking? Because always need the 
stb.st_rdev to parse
the major and minor number. Although "util.c" has "devnm2devid" to 
gather the devid via
to devnm, but it's convenient to return devid when check the blkdev with 
absolute path.
would you mind the function like this?

// returns dev-id when success, return 0 when failure
dev_t stat_md_is_blkdev(char *dev)
{
	struct stat stb;

	if (stat(dev, &stb) != 0) {
		pr_err("stat failed for %s: %s\n", dev, strerror(errno));
		return 0;
	}
	if ((S_IFMT & stb.st_mode) != S_IFBLK) {
		pr_err("%s is not a block device.\n", dev);
		return 0;
	}
	return stb.st_rdev;
}


Thanks,
-Zhilong

Jes


--
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






