[PATCH 0/4] mdadm:checking level once mode has been set

[PATCH 0/4] mdadm:checking level once mode has been set

[Zhilong Liu]

mdadm: it would be better to check --level ealier,
because it would fall to different prompt if user
forgets to specify the --level. such as:
./mdadm -CR /dev/md0 -b internal -n2 -x1 /dev/loop[0-2]

Signed-off-by: Zhilong Liu <zlliu@suse.com>

diff --git a/Create.c b/Create.c
index 9a951b0..beec29f 100644
--- a/Create.c
+++ b/Create.c
@@ -125,10 +125,6 @@ int Create(struct supertype *st, char *mddev,
 	memset(&info, 0, sizeof(info));
 	if (s->level == UnSet && st && st->ss->default_geometry)
 		st->ss->default_geometry(st, &s->level, NULL, NULL);
-	if (s->level == UnSet) {
-		pr_err("a RAID level is needed to create an array.\n");
-		return 1;
-	}
 	if (s->raiddisks < 4 && s->level == 6) {
 		pr_err("at least 4 raid-devices needed for level 6\n");
 		return 1;
diff --git a/mdadm.c b/mdadm.c
index 19a06db..ad24bdf 100644
--- a/mdadm.c
+++ b/mdadm.c
@@ -350,6 +350,12 @@ int main(int argc, char *argv[])
 				pr_err("Must give -a/--add for devices to add: %s\n", optarg);
 				exit(2);
 			}
+			if (devs_found > 0 && s.level == UnSet && !devmode) {
+				if (mode == CREATE || mode == BUILD) {
+					pr_err("a RAID level is needed to create or build an array.\n");
+					exit(2);
+				}
+			}
 			dv = xmalloc(sizeof(*dv));
 			dv->devname = optarg;
 			dv->disposition = devmode;
-- 
2.10.2

[PATCH 1/4] mdadm:bitmap cannot be set twice

[Zhilong Liu]

mdadm:it doesn't make sense to set bitmap twice.

Signed-off-by: Zhilong Liu <zlliu@suse.com>

diff --git a/mdadm.c b/mdadm.c
index b5ac061..eb9a4e9 100644
--- a/mdadm.c
+++ b/mdadm.c
@@ -1137,6 +1137,10 @@ int main(int argc, char *argv[])
 		case O(CREATE,Bitmap): /* here we create the bitmap */
 		case O(GROW,'b'):
 		case O(GROW,Bitmap):
+			if (s.bitmap_file) {
+				pr_err("bitmap cannot be set twice. Second value: %s.\n", optarg);
+				exit(2);
+			}
 			if (strcmp(optarg, "internal") == 0 ||
 			    strcmp(optarg, "none") == 0 ||
 			    strchr(optarg, '/') != NULL) {
-- 
2.10.2

[PATCH 5/5] mdadm:checking level once mode has been set

[Zhilong Liu]

mdadm: it would be better to check --level ealier,
because it would fall to different prompt if user
forgets to specify the --level parameter. such as:
./mdadm -CR /dev/md0 -b internal -n2 -x1 /dev/loop[0-2]

Signed-off-by: Zhilong Liu <zlliu@suse.com>

diff --git a/Create.c b/Create.c
index 2721884..50ec85e 100644
--- a/Create.c
+++ b/Create.c
@@ -125,10 +125,6 @@ int Create(struct supertype *st, char *mddev,
 	memset(&info, 0, sizeof(info));
 	if (s->level == UnSet && st && st->ss->default_geometry)
 		st->ss->default_geometry(st, &s->level, NULL, NULL);
-	if (s->level == UnSet) {
-		pr_err("a RAID level is needed to create an array.\n");
-		return 1;
-	}
 	if (s->raiddisks < 4 && s->level == 6) {
 		pr_err("at least 4 raid-devices needed for level 6\n");
 		return 1;
diff --git a/mdadm.c b/mdadm.c
index d6ad8dc..fcb33d1 100644
--- a/mdadm.c
+++ b/mdadm.c
@@ -349,6 +349,12 @@ int main(int argc, char *argv[])
 				pr_err("Must give -a/--add for devices to add: %s\n", optarg);
 				exit(2);
 			}
+			if (devs_found > 0 && s.level == UnSet && !devmode) {
+				if (mode == CREATE || mode == BUILD) {
+					pr_err("a RAID level is needed to create or build an array.\n");
+					exit(2);
+				}
+			}
 			dv = xmalloc(sizeof(*dv));
 			dv->devname = optarg;
 			dv->disposition = devmode;
-- 
2.10.2

[PATCH 2/4] mdadm:external bitmap only supports ext filesystem

[Zhilong Liu]

mdadm: ensure that the external bitmap_file is
stored by ext[2-4] file system, because bmap()
of linux/driver/md/bitmap.c exits directly when
the bitmap_file isn't suitable. mdadm should make
users aware of this scenario and give a prompt.

Signed-off-by: Zhilong Liu <zlliu@suse.com>

diff --git a/Create.c b/Create.c
index 2721884..9a951b0 100644
--- a/Create.c
+++ b/Create.c
@@ -831,11 +831,6 @@ int Create(struct supertype *st, char *mddev,
 			goto abort_locked;
 		}
 		bitmap_fd = open(s->bitmap_file, O_RDWR);
-		if (bitmap_fd < 0) {
-			pr_err("weird: %s cannot be openned\n",
-				s->bitmap_file);
-			goto abort_locked;
-		}
 		if (ioctl(mdfd, SET_BITMAP_FILE, bitmap_fd) < 0) {
 			pr_err("Cannot set bitmap file for %s: %s\n",
 				mddev, strerror(errno));
diff --git a/mdadm.c b/mdadm.c
index d6ad8dc..19a06db 100644
--- a/mdadm.c
+++ b/mdadm.c
@@ -28,6 +28,7 @@
 #include "mdadm.h"
 #include "md_p.h"
 #include <ctype.h>
+#include <sys/vfs.h>
 
 static int scan_assemble(struct supertype *ss,
 			 struct context *c,
@@ -1143,6 +1144,21 @@ int main(int argc, char *argv[])
 			    strcmp(optarg, "none") == 0 ||
 			    strchr(optarg, '/') != NULL) {
 				s.bitmap_file = optarg;
+				if (strchr(s.bitmap_file, '/') != NULL) {
+					bitmap_fd = open(s.bitmap_file, O_RDWR);
+					if (bitmap_fd < 0) {
+						pr_err("weird: %s cannot be openned\n", s.bitmap_file);
+						exit(2);
+					}
+					close(bitmap_fd);
+					struct statfs ext_bitmap;
+					statfs(s.bitmap_file, &ext_bitmap);
+					if (ext_bitmap.f_type != 0xEF53){
+						pr_err("external bitmap only supports ext[2-4] filesystem, %s.\n",
+							s.bitmap_file);
+						exit(2);
+					}
+				}
 				continue;
 			}
 			if (strcmp(optarg, "clustered") == 0) {
-- 
2.10.2

Re: [PATCH 2/4] mdadm:external bitmap only supports ext filesystem

[Liu Zhilong]

      as this patch's purpose, just wanna improve the prompt when using the
external bitmap mode, and this patch maybe a little redundant.
      For the errno rule, RUN_ARRAY returned EINVAL indeed and the man page
has indicated that external bitmap only works with ext[2-4] file system.
      I think it would be more user-friendly if prints one prompt and 
returned
EINVAL at the same time when the bmap() got failure.
      Such as:

diff --git a/drivers/md/bitmap.c b/drivers/md/bitmap.c
index 9fb2cca..0bff96b 100644
--- a/drivers/md/bitmap.c
+++ b/drivers/md/bitmap.c
@@ -381,6 +381,7 @@ static int read_page(struct file *file, unsigned 
long index,
                         bh->b_blocknr = bmap(inode, block);
                         if (bh->b_blocknr == 0) {
                                 /* Cannot use this file! */
+                               pr_err("The external bitmap only works 
with ext[2-4] filesystem.\n");
                                 ret = -EINVAL;
                                 goto out;
                         }
Thanks,
-Zhilong

On 03/08/2017 02:51 AM, Zhilong Liu wrote:
> mdadm: ensure that the external bitmap_file is
> stored by ext[2-4] file system, because bmap()
> of linux/driver/md/bitmap.c exits directly when
> the bitmap_file isn't suitable. mdadm should make
> users aware of this scenario and give a prompt.
>
> Signed-off-by: Zhilong Liu <zlliu@suse.com>
>
> diff --git a/Create.c b/Create.c
> index 2721884..9a951b0 100644
> --- a/Create.c
> +++ b/Create.c
> @@ -831,11 +831,6 @@ int Create(struct supertype *st, char *mddev,
>   			goto abort_locked;
>   		}
>   		bitmap_fd = open(s->bitmap_file, O_RDWR);
> -		if (bitmap_fd < 0) {
> -			pr_err("weird: %s cannot be openned\n",
> -				s->bitmap_file);
> -			goto abort_locked;
> -		}
>   		if (ioctl(mdfd, SET_BITMAP_FILE, bitmap_fd) < 0) {
>   			pr_err("Cannot set bitmap file for %s: %s\n",
>   				mddev, strerror(errno));
> diff --git a/mdadm.c b/mdadm.c
> index d6ad8dc..19a06db 100644
> --- a/mdadm.c
> +++ b/mdadm.c
> @@ -28,6 +28,7 @@
>   #include "mdadm.h"
>   #include "md_p.h"
>   #include <ctype.h>
> +#include <sys/vfs.h>
>   
>   static int scan_assemble(struct supertype *ss,
>   			 struct context *c,
> @@ -1143,6 +1144,21 @@ int main(int argc, char *argv[])
>   			    strcmp(optarg, "none") == 0 ||
>   			    strchr(optarg, '/') != NULL) {
>   				s.bitmap_file = optarg;
> +				if (strchr(s.bitmap_file, '/') != NULL) {
> +					bitmap_fd = open(s.bitmap_file, O_RDWR);
> +					if (bitmap_fd < 0) {
> +						pr_err("weird: %s cannot be openned\n", s.bitmap_file);
> +						exit(2);
> +					}
> +					close(bitmap_fd);
> +					struct statfs ext_bitmap;
> +					statfs(s.bitmap_file, &ext_bitmap);
> +					if (ext_bitmap.f_type != 0xEF53){
> +						pr_err("external bitmap only supports ext[2-4] filesystem, %s.\n",
> +							s.bitmap_file);
> +						exit(2);
> +					}
> +				}
>   				continue;
>   			}
>   			if (strcmp(optarg, "clustered") == 0) {

Re: [PATCH 2/4] mdadm:external bitmap only supports ext filesystem

[zhilong]

      as the purpose to improve the prompt when using the external bitmap
mode, maybe push this patch to mdadm is a little redundant.
      For errno rule, RUN_ARRAY returned EINVAL indeed and the man-page
has indicated that external bitmap only works with ext[2-4] file system.
      I think it would be more user-friendly if prints one prompt and 
returned
EINVAL at the same time when the bmap() got failure, so that user knows
where the EINVAL comes.
      Such as:

diff --git a/drivers/md/bitmap.c b/drivers/md/bitmap.c
index 9fb2cca..0bff96b 100644
--- a/drivers/md/bitmap.c
+++ b/drivers/md/bitmap.c
@@ -381,6 +381,7 @@ static int read_page(struct file *file, unsigned 
long index,
                         bh->b_blocknr = bmap(inode, block);
                         if (bh->b_blocknr == 0) {
                                 /* Cannot use this file! */
+                               pr_err("writing external bitmap only 
supports a file under ext[2-4] filesystem.\n");
                                 ret = -EINVAL;
                                 goto out;
                         }
Thanks,
-Zhilong

On 03/08/2017 03:51 PM, Zhilong Liu wrote:
> mdadm: ensure that the external bitmap_file is
> stored by ext[2-4] file system, because bmap()
> of linux/driver/md/bitmap.c exits directly when
> the bitmap_file isn't suitable. mdadm should make
> users aware of this scenario and give a prompt.
>
> Signed-off-by: Zhilong Liu <zlliu@suse.com>
>
> diff --git a/Create.c b/Create.c
> index 2721884..9a951b0 100644
> --- a/Create.c
> +++ b/Create.c
> @@ -831,11 +831,6 @@ int Create(struct supertype *st, char *mddev,
>   			goto abort_locked;
>   		}
>   		bitmap_fd = open(s->bitmap_file, O_RDWR);
> -		if (bitmap_fd < 0) {
> -			pr_err("weird: %s cannot be openned\n",
> -				s->bitmap_file);
> -			goto abort_locked;
> -		}
>   		if (ioctl(mdfd, SET_BITMAP_FILE, bitmap_fd) < 0) {
>   			pr_err("Cannot set bitmap file for %s: %s\n",
>   				mddev, strerror(errno));
> diff --git a/mdadm.c b/mdadm.c
> index d6ad8dc..19a06db 100644
> --- a/mdadm.c
> +++ b/mdadm.c
> @@ -28,6 +28,7 @@
>   #include "mdadm.h"
>   #include "md_p.h"
>   #include <ctype.h>
> +#include <sys/vfs.h>
>   
>   static int scan_assemble(struct supertype *ss,
>   			 struct context *c,
> @@ -1143,6 +1144,21 @@ int main(int argc, char *argv[])
>   			    strcmp(optarg, "none") == 0 ||
>   			    strchr(optarg, '/') != NULL) {
>   				s.bitmap_file = optarg;
> +				if (strchr(s.bitmap_file, '/') != NULL) {
> +					bitmap_fd = open(s.bitmap_file, O_RDWR);
> +					if (bitmap_fd < 0) {
> +						pr_err("weird: %s cannot be openned\n", s.bitmap_file);
> +						exit(2);
> +					}
> +					close(bitmap_fd);
> +					struct statfs ext_bitmap;
> +					statfs(s.bitmap_file, &ext_bitmap);
> +					if (ext_bitmap.f_type != 0xEF53){
> +						pr_err("external bitmap only supports ext[2-4] filesystem, %s.\n",
> +							s.bitmap_file);
> +						exit(2);
> +					}
> +				}
>   				continue;
>   			}
>   			if (strcmp(optarg, "clustered") == 0) {

[PATCH 3/4] mdadm:triggers core dump when stat2devnm return NULL

[Zhilong Liu]

ensure that the device should be a block device when uses
--wait parameter, such as the 'f' and 'd' type file would
be triggered core dumped.
./mdadm --wait /dev/md/, happened core dump.

Signed-off-by: Zhilong Liu <zlliu@suse.com>

diff --git a/Monitor.c b/Monitor.c
index 802a9d9..1900db3 100644
--- a/Monitor.c
+++ b/Monitor.c
@@ -1002,6 +1002,10 @@ int Wait(char *dev)
 			strerror(errno));
 		return 2;
 	}
+	if ((S_IFMT & stb.st_mode) != S_IFBLK) {
+		pr_err("%s is not a block device.\n", dev);
+		return 2;
+	}
 	strcpy(devnm, stat2devnm(&stb));
 
 	while(1) {
diff --git a/lib.c b/lib.c
index b640634..7116298 100644
--- a/lib.c
+++ b/lib.c
@@ -89,9 +89,6 @@ char *devid2kname(int devid)
 
 char *stat2kname(struct stat *st)
 {
-	if ((S_IFMT & st->st_mode) != S_IFBLK)
-		return NULL;
-
 	return devid2kname(st->st_rdev);
 }
 
-- 
2.10.2

Re: [PATCH 3/4] mdadm:triggers core dump when stat2devnm return NULL

[NeilBrown]

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

On Wed, Mar 08 2017, Zhilong Liu wrote:

> ensure that the device should be a block device when uses
> --wait parameter, such as the 'f' and 'd' type file would
> be triggered core dumped.
> ./mdadm --wait /dev/md/, happened core dump.
>
> Signed-off-by: Zhilong Liu <zlliu@suse.com>
>
> diff --git a/Monitor.c b/Monitor.c
> index 802a9d9..1900db3 100644
> --- a/Monitor.c
> +++ b/Monitor.c
> @@ -1002,6 +1002,10 @@ int Wait(char *dev)
>  			strerror(errno));
>  		return 2;
>  	}
> +	if ((S_IFMT & stb.st_mode) != S_IFBLK) {
> +		pr_err("%s is not a block device.\n", dev);
> +		return 2;
> +	}
>  	strcpy(devnm, stat2devnm(&stb));

Surely it would be cleaner to do something like:

  tmp = stat2devnm(&stb);
  if (!tmp) {
      pr_err("%s is not a block device.\n", dev);
      return 2;
  }
  strcpy(devnm, tmp);

This makes it more obvious how you have fixed the crash.

>  
>  	while(1) {
> diff --git a/lib.c b/lib.c
> index b640634..7116298 100644
> --- a/lib.c
> +++ b/lib.c
> @@ -89,9 +89,6 @@ char *devid2kname(int devid)
>  
>  char *stat2kname(struct stat *st)
>  {
> -	if ((S_IFMT & st->st_mode) != S_IFBLK)
> -		return NULL;
> -
>  	return devid2kname(st->st_rdev);
>  }

Why are you removing this test?  It has nothing to do with the other
part of the patch.

NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 3/4] mdadm:triggers core dump when stat2devnm return NULL

[zhilong]

On 03/13/2017 07:00 AM, NeilBrown wrote:
> On Wed, Mar 08 2017, Zhilong Liu wrote:
>
>> ensure that the device should be a block device when uses
>> --wait parameter, such as the 'f' and 'd' type file would
>> be triggered core dumped.
>> ./mdadm --wait /dev/md/, happened core dump.
>>
>> Signed-off-by: Zhilong Liu <zlliu@suse.com>
>>
>> diff --git a/Monitor.c b/Monitor.c
>> index 802a9d9..1900db3 100644
>> --- a/Monitor.c
>> +++ b/Monitor.c
>> @@ -1002,6 +1002,10 @@ int Wait(char *dev)
>>   			strerror(errno));
>>   		return 2;
>>   	}
>> +	if ((S_IFMT & stb.st_mode) != S_IFBLK) {
>> +		pr_err("%s is not a block device.\n", dev);
>> +		return 2;
>> +	}
>>   	strcpy(devnm, stat2devnm(&stb));
> Surely it would be cleaner to do something like:
>
>    tmp = stat2devnm(&stb);
>    if (!tmp) {
>        pr_err("%s is not a block device.\n", dev);
>        return 2;
>    }
>    strcpy(devnm, tmp);
>
> This makes it more obvious how you have fixed the crash.

Yes, this method is much better than I did. Great thanks for your 
improvement.

>>   
>>   	while(1) {
>> diff --git a/lib.c b/lib.c
>> index b640634..7116298 100644
>> --- a/lib.c
>> +++ b/lib.c
>> @@ -89,9 +89,6 @@ char *devid2kname(int devid)
>>   
>>   char *stat2kname(struct stat *st)
>>   {
>> -	if ((S_IFMT & st->st_mode) != S_IFBLK)
>> -		return NULL;
>> -
>>   	return devid2kname(st->st_rdev);
>>   }
> Why are you removing this test?  It has nothing to do with the other
> part of the patch.

Yes, I would remove this part in next revision.

Thanks,
-Zhilong

> NeilBrown

[PATCH 3/4 v1] mdadm:triggers core dump when stat2devnm return NULL

[Zhilong Liu]

monitor: ensure that the device should be a block
device when uses --wait parameter, such as the 'f'
and 'd' type file would be triggered core dumped.
./mdadm --wait /dev/md/, happened core dump.

Reviewed-by: NeilBrown <neilb@suse.com>
Signed-off-by: Zhilong Liu <zlliu@suse.com>
---
 Monitor.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/Monitor.c b/Monitor.c
index 802a9d9..f8850d3 100644
--- a/Monitor.c
+++ b/Monitor.c
@@ -1002,7 +1002,12 @@ int Wait(char *dev)
 			strerror(errno));
 		return 2;
 	}
-	strcpy(devnm, stat2devnm(&stb));
+	char *tmp = stat2devnm(&stb);
+	if (!tmp) {
+		pr_err("%s is not a block device.\n", dev);
+		return 2;
+	}
+	strcpy(devnm, tmp);
 
 	while(1) {
 		struct mdstat_ent *ms = mdstat_read(1, 0);
-- 
2.6.6

Re: [PATCH 0/4] mdadm:checking level once mode has been set

[NeilBrown]

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

On Wed, Mar 08 2017, Zhilong Liu wrote:

> mdadm: it would be better to check --level ealier,
> because it would fall to different prompt if user
> forgets to specify the --level. such as:
> ./mdadm -CR /dev/md0 -b internal -n2 -x1 /dev/loop[0-2]

When I run that command I get:

  mdadm: a RAID level is needed to create an array.


What do you get?

NeilBrown


>
> Signed-off-by: Zhilong Liu <zlliu@suse.com>
>
> diff --git a/Create.c b/Create.c
> index 9a951b0..beec29f 100644
> --- a/Create.c
> +++ b/Create.c
> @@ -125,10 +125,6 @@ int Create(struct supertype *st, char *mddev,
>  	memset(&info, 0, sizeof(info));
>  	if (s->level == UnSet && st && st->ss->default_geometry)
>  		st->ss->default_geometry(st, &s->level, NULL, NULL);
> -	if (s->level == UnSet) {
> -		pr_err("a RAID level is needed to create an array.\n");
> -		return 1;
> -	}
>  	if (s->raiddisks < 4 && s->level == 6) {
>  		pr_err("at least 4 raid-devices needed for level 6\n");
>  		return 1;
> diff --git a/mdadm.c b/mdadm.c
> index 19a06db..ad24bdf 100644
> --- a/mdadm.c
> +++ b/mdadm.c
> @@ -350,6 +350,12 @@ int main(int argc, char *argv[])
>  				pr_err("Must give -a/--add for devices to add: %s\n", optarg);
>  				exit(2);
>  			}
> +			if (devs_found > 0 && s.level == UnSet && !devmode) {
> +				if (mode == CREATE || mode == BUILD) {
> +					pr_err("a RAID level is needed to create or build an array.\n");
> +					exit(2);
> +				}
> +			}
>  			dv = xmalloc(sizeof(*dv));
>  			dv->devname = optarg;
>  			dv->disposition = devmode;
> -- 
> 2.10.2
>
> --
> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 0/4] mdadm:checking level once mode has been set

[zhilong]

On 03/13/2017 06:54 AM, NeilBrown wrote:
> On Wed, Mar 08 2017, Zhilong Liu wrote:
>
>> mdadm: it would be better to check --level ealier,
>> because it would fall to different prompt if user
>> forgets to specify the --level. such as:
>> ./mdadm -CR /dev/md0 -b internal -n2 -x1 /dev/loop[0-2]
> When I run that command I get:
>
>    mdadm: a RAID level is needed to create an array.
>
>
> What do you get?

I'm sorry I have provided the wrong command, for this scenario,
issues "mdadm --build /dev/md0 -n2 /dev/loop[0-1]" should be
proper. it's the purpose to check --level earlier.

If this patch is useful, I would send the v1 patch for it, correct the
comments.

Thanks,
-Zhilong
> NeilBrown
>
>
>> Signed-off-by: Zhilong Liu <zlliu@suse.com>
>>
>> diff --git a/Create.c b/Create.c
>> index 9a951b0..beec29f 100644
>> --- a/Create.c
>> +++ b/Create.c
>> @@ -125,10 +125,6 @@ int Create(struct supertype *st, char *mddev,
>>   	memset(&info, 0, sizeof(info));
>>   	if (s->level == UnSet && st && st->ss->default_geometry)
>>   		st->ss->default_geometry(st, &s->level, NULL, NULL);
>> -	if (s->level == UnSet) {
>> -		pr_err("a RAID level is needed to create an array.\n");
>> -		return 1;
>> -	}
>>   	if (s->raiddisks < 4 && s->level == 6) {
>>   		pr_err("at least 4 raid-devices needed for level 6\n");
>>   		return 1;
>> diff --git a/mdadm.c b/mdadm.c
>> index 19a06db..ad24bdf 100644
>> --- a/mdadm.c
>> +++ b/mdadm.c
>> @@ -350,6 +350,12 @@ int main(int argc, char *argv[])
>>   				pr_err("Must give -a/--add for devices to add: %s\n", optarg);
>>   				exit(2);
>>   			}
>> +			if (devs_found > 0 && s.level == UnSet && !devmode) {
>> +				if (mode == CREATE || mode == BUILD) {
>> +					pr_err("a RAID level is needed to create or build an array.\n");
>> +					exit(2);
>> +				}
>> +			}
>>   			dv = xmalloc(sizeof(*dv));
>>   			dv->devname = optarg;
>>   			dv->disposition = devmode;
>> -- 
>> 2.10.2
>>
>> --
>> 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 0/4] mdadm:checking level once mode has been set

[NeilBrown]

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

On Mon, Mar 13 2017, zhilong wrote:

> On 03/13/2017 06:54 AM, NeilBrown wrote:
>> On Wed, Mar 08 2017, Zhilong Liu wrote:
>>
>>> mdadm: it would be better to check --level ealier,
>>> because it would fall to different prompt if user
>>> forgets to specify the --level. such as:
>>> ./mdadm -CR /dev/md0 -b internal -n2 -x1 /dev/loop[0-2]
>> When I run that command I get:
>>
>>    mdadm: a RAID level is needed to create an array.
>>
>>
>> What do you get?
>
> I'm sorry I have provided the wrong command, for this scenario,
> issues "mdadm --build /dev/md0 -n2 /dev/loop[0-1]" should be
> proper. it's the purpose to check --level earlier.

OK, that makes sense.

>
> If this patch is useful, I would send the v1 patch for it, correct the
> comments.

My preference would be to have the check in Build.c, but Jes might have
different ideas.

NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 0/4] mdadm:checking level once mode has been set

[zhilong]

On 03/13/2017 11:48 AM, NeilBrown wrote:
> On Mon, Mar 13 2017, zhilong wrote:
>
>> On 03/13/2017 06:54 AM, NeilBrown wrote:
>>> On Wed, Mar 08 2017, Zhilong Liu wrote:
>>>
>>>> mdadm: it would be better to check --level ealier,
>>>> because it would fall to different prompt if user
>>>> forgets to specify the --level. such as:
>>>> ./mdadm -CR /dev/md0 -b internal -n2 -x1 /dev/loop[0-2]
>>> When I run that command I get:
>>>
>>>     mdadm: a RAID level is needed to create an array.
>>>
>>>
>>> What do you get?
>> I'm sorry I have provided the wrong command, for this scenario,
>> issues "mdadm --build /dev/md0 -n2 /dev/loop[0-1]" should be
>> proper. it's the purpose to check --level earlier.
> OK, that makes sense.
>
>> If this patch is useful, I would send the v1 patch for it, correct the
>> comments.
> My preference would be to have the check in Build.c, but Jes might have
> different ideas.

copy that, thanks very much.

Best regards,
-Zhilong

> NeilBrown

Re: [PATCH 0/4] mdadm:checking level once mode has been set

[jes.sorensen]

Zhilong Liu <zlliu@suse.com> writes:
> mdadm: it would be better to check --level ealier,
> because it would fall to different prompt if user
> forgets to specify the --level. such as:
> ./mdadm -CR /dev/md0 -b internal -n2 -x1 /dev/loop[0-2]
>
> Signed-off-by: Zhilong Liu <zlliu@suse.com>

This patchset is completely messed up - 0/4 contains actual code instead
of being a cover letter. 5/5 is a reply to 1/4 and you posted a v1 of
something which I assume needs to be the v2 given the v1 is the original
posting.

Would you mind sending out a clean set of these patches?

Thanks,
Jes