[PATCH] allow RAID5 to grow to RAID6 with a backup_file

[PATCH] allow RAID5 to grow to RAID6 with a backup_file

[Nigel Croxon]

This problem came in, as the user did not specify a full path with
the backup_file option when growing an RAID5 array to RAID6.
When the full path is specified, the symbolic link is created
properly (/run/mdadm/backup_file-mdX). But the code did not support
the symbolic link when looking for the backup_file. Added two
checks for symlink.

This addresses https://www.spinics.net/lists/raid/msg48910.html
and numerous customer reported problems.

Signed-off-by: Nigel Croxon <ncroxon@redhat.com>
---
 Grow.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/Grow.c b/Grow.c
index 764374f..6dce71c 100644
--- a/Grow.c
+++ b/Grow.c
@@ -1135,6 +1135,16 @@ int reshape_open_backup_file(char *backup_file,
 	unsigned int dev;
 	int i;
 
+	if (lstat(backup_file, &stb) != -1) {
+		switch (stb.st_mode & S_IFMT) {
+		case S_IFLNK:
+			return 1;
+			break;
+		default:
+			break;
+		}
+	}
+
 	*fdlist = open(backup_file, O_RDWR|O_CREAT|(restart ? O_TRUNC : O_EXCL),
 		       S_IRUSR | S_IWUSR);
 	*offsets = 8 * 512;
@@ -5236,8 +5246,17 @@ char *locate_backup(char *name)
 	char *fl = make_backup(name);
 	struct stat stb;
 
-	if (stat(fl, &stb) == 0 && S_ISREG(stb.st_mode))
+	lstat(fl, &stb);
+	switch (stb.st_mode & S_IFMT) {
+	case S_IFLNK:
 		return fl;
+		break;
+	case S_IFREG:
+		return fl;
+		break;
+	default:
+		break;
+	}
 
 	free(fl);
 	return NULL;
-- 
2.20.1

Re: [PATCH] allow RAID5 to grow to RAID6 with a backup_file

[Jes Sorensen]

On 5/4/20 12:31 PM, Nigel Croxon wrote:
> This problem came in, as the user did not specify a full path with
> the backup_file option when growing an RAID5 array to RAID6.
> When the full path is specified, the symbolic link is created
> properly (/run/mdadm/backup_file-mdX). But the code did not support
> the symbolic link when looking for the backup_file. Added two
> checks for symlink.
> 
> This addresses https://www.spinics.net/lists/raid/msg48910.html
> and numerous customer reported problems.
> 
> Signed-off-by: Nigel Croxon <ncroxon@redhat.com>
> ---
>  Grow.c | 21 ++++++++++++++++++++-
>  1 file changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/Grow.c b/Grow.c
> index 764374f..6dce71c 100644
> --- a/Grow.c
> +++ b/Grow.c
> @@ -1135,6 +1135,16 @@ int reshape_open_backup_file(char *backup_file,
>  	unsigned int dev;
>  	int i;
>  
> +	if (lstat(backup_file, &stb) != -1) {
> +		switch (stb.st_mode & S_IFMT) {
> +		case S_IFLNK:
> +			return 1;
> +			break;

It doesn't make sense to have a break after a return here.

> +		default:
> +			break;
> +		}
> +	}
> +
>  	*fdlist = open(backup_file, O_RDWR|O_CREAT|(restart ? O_TRUNC : O_EXCL),
>  		       S_IRUSR | S_IWUSR);
>  	*offsets = 8 * 512;
> @@ -5236,8 +5246,17 @@ char *locate_backup(char *name)
>  	char *fl = make_backup(name);
>  	struct stat stb;
>  
> -	if (stat(fl, &stb) == 0 && S_ISREG(stb.st_mode))> +	lstat(fl, &stb);

You ditched the error check for lstat here. You cannot count on the
contents of stb being valid in this case.

> +	switch (stb.st_mode & S_IFMT) {
> +	case S_IFLNK:
>  		return fl;
> +		break;
> +	case S_IFREG:
> +		return fl;
> +		break;

Same with the break statements here.

Mind fixing this up and submitting a v2.

Thanks,
Jes

> +	default:
> +		break;
> +	}
>  
>  	free(fl);
>  	return NULL;
>