[PATCH 0/4] integrated stat and fstat into utility functions

[PATCH 0/4] integrated stat and fstat into utility functions

[Zhilong Liu]

hi, Jes;

This patches focus on integrating stat and fstat operations, move the
duplicated codes into one utility function, and also fixed one bug in
waitclean(), it forgot to check the block device.

Thanks,
Zhilong

Zhilong Liu (4):
  mdadm/util:integrate stat operations into one utility
  mdadm/util:integrate fstat operations into one utility function
  mdadm/Create:declaring an existing struct within same function
  mdadm/Monitor:check the block device when use waitclean parameter

 Assemble.c    | 15 ++++-----------
 Build.c       | 21 +++------------------
 Create.c      | 17 +++++++----------
 Grow.c        |  4 +---
 Incremental.c | 25 ++-----------------------
 Manage.c      | 10 ++--------
 Monitor.c     |  2 ++
 bitmap.c      | 12 ++++--------
 mdadm.h       |  2 ++
 super-intel.c | 14 +++-----------
 util.c        | 30 ++++++++++++++++++++++++++++++
 11 files changed, 60 insertions(+), 92 deletions(-)

-- 
2.10.2

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

[Zhilong Liu]

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@suse.com>
---
 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);
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;
+}
+
 int is_standard(char *dev, int *nump)
 {
 	/* tests if dev is a "standard" md dev name.
-- 
2.10.2

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

[jes.sorensen]

Zhilong Liu <zlliu@suse.com> 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@suse.com>
> ---
>  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()

Jes

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

[Liu Zhilong]

On 04/05/2017 11:42 PM, jes.sorensen@gmail.com wrote:
> Zhilong Liu <zlliu@suse.com> 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@suse.com>
>> ---
>>   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
>

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

[Jes Sorensen]

On 04/14/2017 06:14 AM, Liu Zhilong wrote:
>
>
> On 04/05/2017 11:42 PM, jes.sorensen@gmail.com wrote:
>> Zhilong Liu <zlliu@suse.com> writes:
[snip]
>> 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;
> }

I am generally wary of too many smart handlers, but I think it makes 
some sense here to de-duplicate the repeated code.

That said, your function would ditch the error information, and the 
caller wouldn't know why it failed. If you made it more like this and 
return the error code, plus stick the major/minor number into rdev, if 
one is provided:

int stat_md_is_blkdev(char *devname, *dev_t rdev)
{
}

Jes

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

[Liu Zhilong]

On 04/14/2017 11:20 PM, Jes Sorensen wrote:
> On 04/14/2017 06:14 AM, Liu Zhilong wrote:
>>
>>
>> On 04/05/2017 11:42 PM, jes.sorensen@gmail.com wrote:
>>> Zhilong Liu <zlliu@suse.com> writes:
> [snip]
>>> 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;
>> }
>
> I am generally wary of too many smart handlers, but I think it makes 
> some sense here to de-duplicate the repeated code.
>
> That said, your function would ditch the error information, and the 
> caller wouldn't know why it failed. If you made it more like this and 
> return the error code, plus stick the major/minor number into rdev, if 
> one is provided:
>
> int stat_md_is_blkdev(char *devname, *dev_t rdev)
> {
> }
>

I want to integrate two situations into one function.
1. only check the 'dev'(such as /dev/loop2) whether or not is block device.
2. optionally return the rdev-id after checking the block device.
this sample "int stat_md_is_blkdev(char *devname, *dev_t rdev)" is smart 
for the situation 2. but for situation 1, the second parameter is a 
little waste.

how about like this?
dev_t stat_md_is_blkdev(char *devname, bool require_rdev)
{
     struct stat stb;

        if (stat(devname, &stb) != 0) {
                printf("stat failed for %s.\n", devname);
                return 1;
        }
        if ((S_IFMT & stb.st_mode) != S_IFBLK) {
                printf("%s is not a block device.\n", devname);
                return 1;
        }
        if (require_rdev)
           return stb.st_rdev;
     return 0;
}

the caller would be like this,
situation 1:
char dev[20] = "/dev/loop2";
if (stat_md_is_blkdev(dev, false))
   return 1;

situation 2:
dev_t st_rdev;
char dev[20] = "/dev/loop2";
st_rdev = stat_md_is_blkdev(dev, true);
if (st_rdev == 1)
   return 1;


Thanks very much,
-Zhilong

> Jes
>
>

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

[Zhilong Liu]

On 04/17/2017 03:08 PM, Liu Zhilong wrote:
>
>
> On 04/14/2017 11:20 PM, Jes Sorensen wrote:
>> On 04/14/2017 06:14 AM, Liu Zhilong wrote:
>>>
>>>
>>> On 04/05/2017 11:42 PM, jes.sorensen@gmail.com wrote:
>>>> Zhilong Liu <zlliu@suse.com> writes:
>> [snip]
>>>> 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;
>>> }
>>
>> I am generally wary of too many smart handlers, but I think it makes 
>> some sense here to de-duplicate the repeated code.
>>
>> That said, your function would ditch the error information, and the 
>> caller wouldn't know why it failed. If you made it more like this and 
>> return the error code, plus stick the major/minor number into rdev, 
>> if one is provided:
>>
>> int stat_md_is_blkdev(char *devname, *dev_t rdev)
>> {
>> }
>>
>
> I want to integrate two situations into one function.
> 1. only check the 'dev'(such as /dev/loop2) whether or not is block 
> device.
> 2. optionally return the rdev-id after checking the block device.
> this sample "int stat_md_is_blkdev(char *devname, *dev_t rdev)" is 
> smart for the situation 2. but for situation 1, the second parameter 
> is a little waste.
>
> how about like this?
> dev_t stat_md_is_blkdev(char *devname, bool require_rdev)
> {
>     struct stat stb;
>
>        if (stat(devname, &stb) != 0) {
>                printf("stat failed for %s.\n", devname);

in addition:  the errno wouldn't be missed in the final patch.
pr_err("stat failed for %s: %s\n", dev, strerror(errno));

Thanks,
-Zhilong
>                return 1;
>        }
>        if ((S_IFMT & stb.st_mode) != S_IFBLK) {
>                printf("%s is not a block device.\n", devname);
>                return 1;
>        }
>        if (require_rdev)
>           return stb.st_rdev;
>     return 0;
> }
>
> the caller would be like this,
> situation 1:
> char dev[20] = "/dev/loop2";
> if (stat_md_is_blkdev(dev, false))
>   return 1;
>
> situation 2:
> dev_t st_rdev;
> char dev[20] = "/dev/loop2";
> st_rdev = stat_md_is_blkdev(dev, true);
> if (st_rdev == 1)
>   return 1;
>
>
> Thanks very much,
> -Zhilong
>
>> Jes
>>
>>
>
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

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

[Jes Sorensen]

On 04/17/2017 03:08 AM, Liu Zhilong wrote:
>
>
> On 04/14/2017 11:20 PM, Jes Sorensen wrote:
>> On 04/14/2017 06:14 AM, Liu Zhilong wrote:
>>>
>>>
>>> On 04/05/2017 11:42 PM, jes.sorensen@gmail.com wrote:
>>>> Zhilong Liu <zlliu@suse.com> writes:
>> [snip]
>>>> 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;
>>> }
>>
>> I am generally wary of too many smart handlers, but I think it makes
>> some sense here to de-duplicate the repeated code.
>>
>> That said, your function would ditch the error information, and the
>> caller wouldn't know why it failed. If you made it more like this and
>> return the error code, plus stick the major/minor number into rdev, if
>> one is provided:
>>
>> int stat_md_is_blkdev(char *devname, *dev_t rdev)
>> {
>> }
>>
>
> I want to integrate two situations into one function.
> 1. only check the 'dev'(such as /dev/loop2) whether or not is block device.
> 2. optionally return the rdev-id after checking the block device.
> this sample "int stat_md_is_blkdev(char *devname, *dev_t rdev)" is smart
> for the situation 2. but for situation 1, the second parameter is a
> little waste.
>
> how about like this?
> dev_t stat_md_is_blkdev(char *devname, bool require_rdev)
> {
>     struct stat stb;
>
>        if (stat(devname, &stb) != 0) {
>                printf("stat failed for %s.\n", devname);
>                return 1;
>        }
>        if ((S_IFMT & stb.st_mode) != S_IFBLK) {
>                printf("%s is not a block device.\n", devname);
>                return 1;
>        }
>        if (require_rdev)
>           return stb.st_rdev;
>     return 0;
> }
>
> the caller would be like this,
> situation 1:
> char dev[20] = "/dev/loop2";
> if (stat_md_is_blkdev(dev, false))
>   return 1;
>
> situation 2:
> dev_t st_rdev;
> char dev[20] = "/dev/loop2";
> st_rdev = stat_md_is_blkdev(dev, true);
> if (st_rdev == 1)
>   return 1;
>

No, this is really ugly - if you have a function that exhibits two 
different behaviors based on a boolean flag, you should have two 
versions. Parse a pointer for an optional return of dev_t and return the 
error value.

Jes

[PATCH 2/4] mdadm/util:integrate fstat operations into one utility function

[Zhilong Liu]

mdadm/util: there are dupilicate codes about fstat checking the
block device, move the operations into one utility function and
make it concise.

Signed-off-by: Zhilong Liu <zlliu@suse.com>
---
 Assemble.c    | 10 ++--------
 Create.c      |  5 +----
 Grow.c        |  4 +---
 Incremental.c | 13 +------------
 bitmap.c      | 12 ++++--------
 mdadm.h       |  1 +
 super-intel.c | 10 ++--------
 util.c        | 15 +++++++++++++++
 8 files changed, 27 insertions(+), 43 deletions(-)

diff --git a/Assemble.c b/Assemble.c
index c6571e6..47c6685 100644
--- a/Assemble.c
+++ b/Assemble.c
@@ -204,14 +204,8 @@ 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_blkdev_via_fstat(dfd, devname) &&
+			   fstat(dfd, &stb)) {
 			tmpdev->used = 2;
 		} else if (must_be_container(dfd)) {
 			if (st) {
diff --git a/Create.c b/Create.c
index 32987af..e43c1d6 100644
--- a/Create.c
+++ b/Create.c
@@ -326,11 +326,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_blkdev_via_fstat(dfd, dname)) {
 			close(dfd);
-			pr_err("%s is not a block device\n",
-				dname);
 			exit(2);
 		}
 		close(dfd);
diff --git a/Grow.c b/Grow.c
index 78a3474..0085f91 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_blkdev_via_fstat(nfd, newdev) && fstat(nfd, &stb)) {
 		close(nfd);
 		free(st);
 		return 1;
diff --git a/Incremental.c b/Incremental.c
index 78adf18..19af045 100644
--- a/Incremental.c
+++ b/Incremental.c
@@ -164,19 +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));
-		goto out;
-	}
-	if ((stb.st_mode & S_IFMT) != S_IFBLK) {
-		if (c->verbose >= 0)
-			pr_err("%s is not a block device.\n",
-				devname);
+	if (check_blkdev_via_fstat(dfd, devname) && fstat(dfd, &stb))
 		goto out;
-	}
 
 	dinfo.disk.major = major(stb.st_rdev);
 	dinfo.disk.minor = minor(stb.st_rdev);
diff --git a/bitmap.c b/bitmap.c
index ccedfd3..69ce640 100644
--- a/bitmap.c
+++ b/bitmap.c
@@ -193,14 +193,7 @@ bitmap_file_open(char *filename, struct supertype **stp, int node_num)
 		return -1;
 	}
 
-	if (fstat(fd, &stb) < 0) {
-		pr_err("failed to determine bitmap file/device type: %s\n",
-			strerror(errno));
-		close(fd);
-		return -1;
-	}
-
-	if ((stb.st_mode & S_IFMT) == S_IFBLK) {
+	if (check_blkdev_via_fstat(fd, filename)) {
 		/* block device, so we are probably after an internal bitmap */
 		if (!st)
 			st = guess_super(fd);
@@ -221,6 +214,9 @@ bitmap_file_open(char *filename, struct supertype **stp, int node_num)
 		}
 
 		*stp = st;
+	} else {
+		close(fd);
+		return -1;
 	}
 
 	return fd;
diff --git a/mdadm.h b/mdadm.h
index 0aea822..e021149 100644
--- a/mdadm.h
+++ b/mdadm.h
@@ -1423,6 +1423,7 @@ 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 check_blkdev_via_fstat(int fd, 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 924ad8a..014616c 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -6604,14 +6604,8 @@ 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_blkdev_via_fstat(dfd, devname) &&
+			   fstat(dfd, &stb)) {
 			tmpdev->used = 2;
 		} else if (must_be_container(dfd)) {
 			struct supertype *cst;
diff --git a/util.c b/util.c
index 03d4256..09750a0 100644
--- a/util.c
+++ b/util.c
@@ -789,6 +789,21 @@ int check_blkdev_via_stat(char *dev)
 	return 0;
 }
 
+int check_blkdev_via_fstat(int fd, char *dev)
+{
+	struct stat stb;
+
+	if (fstat(fd, &stb) != 0) {
+		pr_err("fstat 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.
-- 
2.10.2

Re: [PATCH 2/4] mdadm/util:integrate fstat operations into one utility function

[jes.sorensen]

Zhilong Liu <zlliu@suse.com> writes:
> mdadm/util: there are dupilicate codes about fstat checking the
> block device, move the operations into one utility function and
> make it concise.
>
> Signed-off-by: Zhilong Liu <zlliu@suse.com>
> ---
>  Assemble.c    | 10 ++--------
>  Create.c      |  5 +----
>  Grow.c        |  4 +---
>  Incremental.c | 13 +------------
>  bitmap.c      | 12 ++++--------
>  mdadm.h       |  1 +
>  super-intel.c | 10 ++--------
>  util.c        | 15 +++++++++++++++
>  8 files changed, 27 insertions(+), 43 deletions(-)
>
> diff --git a/Assemble.c b/Assemble.c
> index c6571e6..47c6685 100644
> --- a/Assemble.c
> +++ b/Assemble.c
> @@ -204,14 +204,8 @@ 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_blkdev_via_fstat(dfd, devname) &&
> +			   fstat(dfd, &stb)) {
>  			tmpdev->used = 2;
>  		} else if (must_be_container(dfd)) {
>  			if (st) {

Same complaint as prior patch, do not run fstat() twice!!!!

Jes

[PATCH 3/4] mdadm/Create:declaring an existing struct within same function

[Zhilong Liu]

Create:declaring 'struct stat stb' twice within the same
function, rename stb as stb2 when declares 'struct stat'
at the second time.

Signed-off-by: Zhilong Liu <zlliu@suse.com>
---
 Create.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/Create.c b/Create.c
index e43c1d6..619e68a 100644
--- a/Create.c
+++ b/Create.c
@@ -865,7 +865,7 @@ int Create(struct supertype *st, char *mddev,
 		for (dnum=0, raid_disk_num=0, dv = devlist ; dv ;
 		     dv=(dv->next)?(dv->next):moved_disk, dnum++) {
 			int fd;
-			struct stat stb;
+			struct stat stb2;
 			struct mdinfo *inf = &infos[dnum];
 
 			if (dnum >= total_slots)
@@ -921,9 +921,9 @@ int Create(struct supertype *st, char *mddev,
 							dv->devname);
 						goto abort_locked;
 					}
-					fstat(fd, &stb);
-					inf->disk.major = major(stb.st_rdev);
-					inf->disk.minor = minor(stb.st_rdev);
+					fstat(fd, &stb2);
+					inf->disk.major = major(stb2.st_rdev);
+					inf->disk.minor = minor(stb2.st_rdev);
 				}
 				if (fd >= 0)
 					remove_partitions(fd);
@@ -944,8 +944,8 @@ int Create(struct supertype *st, char *mddev,
 
 				if (!have_container) {
 					/* getinfo_super might have lost these ... */
-					inf->disk.major = major(stb.st_rdev);
-					inf->disk.minor = minor(stb.st_rdev);
+					inf->disk.major = major(stb2.st_rdev);
+					inf->disk.minor = minor(stb2.st_rdev);
 				}
 				break;
 			case 2:
-- 
2.10.2

Re: [PATCH 3/4] mdadm/Create:declaring an existing struct within same function

[jes.sorensen]

Zhilong Liu <zlliu@suse.com> writes:
> Create:declaring 'struct stat stb' twice within the same
> function, rename stb as stb2 when declares 'struct stat'
> at the second time.
>
> Signed-off-by: Zhilong Liu <zlliu@suse.com>
> ---
>  Create.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)

Applied!

Thanks,
Jes

[PATCH 4/4] mdadm/Monitor:check the block device when use waitclean parameter

[Zhilong Liu]

WaitClean(): stat2devnm() returns NULL for non block devices.
Check the pointer is valid derefencing it. This would happen
when using --waitclean, such as the 'f' and 'd' file type,
causing a core dump.

Signed-off-by: Zhilong Liu <zlliu@suse.com>
---
 Monitor.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Monitor.c b/Monitor.c
index 036a561..101b765 100644
--- a/Monitor.c
+++ b/Monitor.c
@@ -1076,6 +1076,8 @@ int WaitClean(char *dev, int sock, int verbose)
 			pr_err("Couldn't open %s: %s\n", dev, strerror(errno));
 		return 1;
 	}
+	if (check_blkdev_via_fstat(fd, dev))
+		return 2;
 
 	strcpy(devnm, fd2devnm(fd));
 	mdi = sysfs_read(fd, devnm, GET_VERSION|GET_LEVEL|GET_SAFEMODE);
-- 
2.10.2

Re: [PATCH 0/4] integrated stat and fstat into utility functions

[Paul Menzel]

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

Dear Zhilong,


Am Samstag, den 01.04.2017, 20:51 +0800 schrieb Zhilong Liu:

[…]

> Zhilong Liu (4):
>   mdadm/util:integrate stat operations into one utility
>   mdadm/util:integrate fstat operations into one utility function
>   mdadm/Create:declaring an existing struct within same function
>   mdadm/Monitor:check the block device when use waitclean parameter

Just a small wish, could you please add exactly one space between the
“subsystem prefix” and the summary?

Like *mdadm/util: Integrate stat …*. That’d be great.

[…]


Thanks,

Paul

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 232 bytes --]

Re: [PATCH 0/4] integrated stat and fstat into utility functions

[Zhilong Liu]

On 04/14/2017 08:31 PM, Paul Menzel wrote:
> Dear Zhilong,
>
>
> Am Samstag, den 01.04.2017, 20:51 +0800 schrieb Zhilong Liu:
>
> […]
>
>> Zhilong Liu (4):
>>    mdadm/util:integrate stat operations into one utility
>>    mdadm/util:integrate fstat operations into one utility function
>>    mdadm/Create:declaring an existing struct within same function
>>    mdadm/Monitor:check the block device when use waitclean parameter
> Just a small wish, could you please add exactly one space between the
> “subsystem prefix” and the summary?
>
> Like *mdadm/util: Integrate stat …*. That’d be great.
>
> […]

Thanks very much for your suggestion. :-).

Best regards,
Zhilong
>
> Thanks,
>
> Paul