Re: [PATCH V2] Grow: Close cfd file descriptor

[Jes Sorensen <jes@trained-monkey.org>]

On 8/30/21 9:32 AM, Mateusz Kusiak wrote:
> Unclosed file descriptor causes resource leak if error occurs.
> 
> Signed-off-by: Mateusz Kusiak <mateusz.kusiak@intel.com>

Hi Mateusz,

Thanks for looking at this. Please could you provide a more elaborate
commit message when you submit these commits. It is also unclear what
changed between v1 and v2 of your patch.

Grow_reshape() is a beast and it really could do with some cleaning up.
This would also be a good time to work on returning meaningful error
codes instead of those horrible 1 and 2 values for errors.

While I think your patch is technically correct, I think it makes the
code harder to read and discourages cleaning up the error codes. In
particular I don't like the goto error which is just a shortcut for not
setting rv before the jump. Can we please drop that and maybe longer
term focus on returning more meaningful error codes?

Given that Grow_reshape() is such a beast, what do you think of
splitting off the section from around line 1869 into a helper function,
something like Validate_External_Reshape() or something like that?
Basically the code covered in this if() statement:

	/* in the external case we need to check that the requested reshape is
	 * supported, and perform an initial check that the container holds the
	 * pre-requisite spare devices (mdmon owns final validation)
	 */
	if (st->ss->external) {

If you want to send me a smaller patch that adds the cleanup to release:
and the additional jumps to there, then I am happy to apply that for now.

Thanks,
Jes


> ---
>  Grow.c | 82 +++++++++++++++++++++++++---------------------------------
>  1 file changed, 35 insertions(+), 47 deletions(-)
> 
> diff --git a/Grow.c b/Grow.c
> index 7506ab46..dec6b742 100644
> --- a/Grow.c
> +++ b/Grow.c
> @@ -1796,7 +1796,7 @@ int Grow_reshape(char *devname, int fd,
>  	struct supertype *st;
>  	char *subarray = NULL;
>  
> -	int frozen;
> +	int frozen = 0;
>  	int changed = 0;
>  	char *container = NULL;
>  	int cfd = -1;
> @@ -1805,7 +1805,7 @@ int Grow_reshape(char *devname, int fd,
>  	int added_disks;
>  
>  	struct mdinfo info;
> -	struct mdinfo *sra;
> +	struct mdinfo *sra = NULL;
>  
>  	if (md_get_array_info(fd, &array) < 0) {
>  		pr_err("%s is not an active md array - aborting\n",
> @@ -1851,14 +1851,14 @@ int Grow_reshape(char *devname, int fd,
>  	}
>  	if (s->raiddisks > st->max_devs) {
>  		pr_err("Cannot increase raid-disks on this array beyond %d\n", st->max_devs);
> -		return 1;
> +		goto error;
>  	}
>  	if (s->level == 0 && (array.state & (1 << MD_SB_BITMAP_PRESENT)) &&
>  		!(array.state & (1 << MD_SB_CLUSTERED)) && !st->ss->external) {
>  		array.state &= ~(1 << MD_SB_BITMAP_PRESENT);
>  		if (md_set_array_info(fd, &array) != 0) {
>  			pr_err("failed to remove internal bitmap.\n");
> -			return 1;
> +			goto error;
>  		}
>  	}
>  
> @@ -1880,16 +1880,14 @@ int Grow_reshape(char *devname, int fd,
>  		}
>  		if (cfd < 0) {
>  			pr_err("Unable to open container for %s\n", devname);
> -			free(subarray);
> -			return 1;
> +			goto error;
>  		}
>  
>  		retval = st->ss->load_container(st, cfd, NULL);
>  
>  		if (retval) {
>  			pr_err("Cannot read superblock for %s\n", devname);
> -			free(subarray);
> -			return 1;
> +			goto error;
>  		}
>  
>  		/* check if operation is supported for metadata handler */
> @@ -1900,6 +1898,7 @@ int Grow_reshape(char *devname, int fd,
>  			cc = st->ss->container_content(st, subarray);
>  			for (content = cc; content ; content = content->next) {
>  				int allow_reshape = 1;
> +				rv = 1;
>  
>  				/* check if reshape is allowed based on metadata
>  				 * indications stored in content.array.status
> @@ -1913,26 +1912,23 @@ int Grow_reshape(char *devname, int fd,
>  				if (!allow_reshape) {
>  					pr_err("cannot reshape arrays in container with unsupported metadata: %s(%s)\n",
>  					       devname, container);
> -					sysfs_free(cc);
> -					free(subarray);
> -					return 1;
> +					break;
>  				}
>  				if (content->consistency_policy ==
>  				    CONSISTENCY_POLICY_PPL) {
>  					pr_err("Operation not supported when ppl consistency policy is enabled\n");
> -					sysfs_free(cc);
> -					free(subarray);
> -					return 1;
> +					break;
>  				}
>  				if (content->consistency_policy ==
>  				    CONSISTENCY_POLICY_BITMAP) {
>  					pr_err("Operation not supported when write-intent bitmap is enabled\n");
> -					sysfs_free(cc);
> -					free(subarray);
> -					return 1;
> +					break;
>  				}
> +				rv = 0;
>  			}
>  			sysfs_free(cc);
> +			if (rv == 1)
> +				goto release;
>  		}
>  		if (mdmon_running(container))
>  			st->update_tail = &st->updates;
> @@ -1950,7 +1946,7 @@ int Grow_reshape(char *devname, int fd,
>  		       s->raiddisks - array.raid_disks,
>  		       s->raiddisks - array.raid_disks == 1 ? "" : "s",
>  		       array.spare_disks + added_disks);
> -		return 1;
> +		goto error;
>  	}
>  
>  	sra = sysfs_read(fd, NULL, GET_LEVEL | GET_DISKS | GET_DEVS |
> @@ -1963,17 +1959,15 @@ int Grow_reshape(char *devname, int fd,
>  	} else {
>  		pr_err("failed to read sysfs parameters for %s\n",
>  			devname);
> -		return 1;
> +		goto error;
>  	}
>  	frozen = freeze(st);
>  	if (frozen < -1) {
>  		/* freeze() already spewed the reason */
> -		sysfs_free(sra);
> -		return 1;
> +		goto error;
>  	} else if (frozen < 0) {
>  		pr_err("%s is performing resync/recovery and cannot be reshaped\n", devname);
> -		sysfs_free(sra);
> -		return 1;
> +		goto error;
>  	}
>  
>  	/* ========= set size =============== */
> @@ -1989,15 +1983,13 @@ int Grow_reshape(char *devname, int fd,
>  
>  		if (orig_size == 0) {
>  			pr_err("Cannot set device size in this type of array.\n");
> -			rv = 1;
> -			goto release;
> +			goto error;
>  		}
>  
>  		if (reshape_super(st, s->size, UnSet, UnSet, 0, 0, UnSet, NULL,
>  				  devname, APPLY_METADATA_CHANGES,
>  				  c->verbose > 0)) {
> -			rv = 1;
> -			goto release;
> +			goto error;
>  		}
>  		sync_metadata(st);
>  		if (st->ss->external) {
> @@ -2126,8 +2118,7 @@ size_change_error:
>  			if (err == EBUSY &&
>  			    (array.state & (1<<MD_SB_BITMAP_PRESENT)))
>  				cont_err("Bitmap must be removed before size can be changed\n");
> -			rv = 1;
> -			goto release;
> +			goto error;
>  		}
>  		if (s->assume_clean) {
>  			/* This will fail on kernels older than 3.0 unless
> @@ -2183,10 +2174,7 @@ size_change_error:
>  		err = remove_disks_for_takeover(st, sra, array.layout);
>  		if (err) {
>  			dprintf("Array cannot be reshaped\n");
> -			if (cfd > -1)
> -				close(cfd);
> -			rv = 1;
> -			goto release;
> +			goto error;
>  		}
>  		/* Make sure mdmon has seen the device removal
>  		 * and updated metadata before we continue with
> @@ -2200,8 +2188,7 @@ size_change_error:
>  	info.array = array;
>  	if (sysfs_init(&info, fd, NULL)) {
>  		pr_err("failed to initialize sysfs.\n");
> -		rv = 1;
> -		goto release;
> +		goto error;
>  	}
>  	strcpy(info.text_version, sra->text_version);
>  	info.component_size = s->size*2;
> @@ -2222,8 +2209,7 @@ size_change_error:
>  			pr_err("%s has a non-standard layout.  If you wish to preserve this\n", devname);
>  			cont_err("during the reshape, please specify --layout=preserve\n");
>  			cont_err("If you want to change it, specify a layout or use --layout=normalise\n");
> -			rv = 1;
> -			goto release;
> +			goto error;
>  		}
>  	} else if (strcmp(s->layout_str, "normalise") == 0 ||
>  		   strcmp(s->layout_str, "normalize") == 0) {
> @@ -2239,8 +2225,7 @@ size_change_error:
>  			}
>  		} else {
>  			pr_err("%s is only meaningful when reshaping a RAID6 array.\n", s->layout_str);
> -			rv = 1;
> -			goto release;
> +			goto error;
>  		}
>  	} else if (strcmp(s->layout_str, "preserve") == 0) {
>  		/* This means that a non-standard RAID6 layout
> @@ -2261,8 +2246,7 @@ size_change_error:
>  			info.new_layout = map_name(r6layout, l);
>  		} else {
>  			pr_err("%s in only meaningful when reshaping to RAID6\n", s->layout_str);
> -			rv = 1;
> -			goto release;
> +			goto error;
>  		}
>  	} else {
>  		int l = info.new_level;
> @@ -2283,14 +2267,12 @@ size_change_error:
>  			break;
>  		default:
>  			pr_err("layout not meaningful with this level\n");
> -			rv = 1;
> -			goto release;
> +			goto error;
>  		}
>  		if (info.new_layout == UnSet) {
>  			pr_err("layout %s not understood for this level\n",
>  				s->layout_str);
> -			rv = 1;
> -			goto release;
> +			goto error;
>  		}
>  	}
>  
> @@ -2359,8 +2341,7 @@ size_change_error:
>  				  info.array.raid_disks, info.delta_disks,
>  				  c->backup_file, devname,
>  				  APPLY_METADATA_CHANGES, c->verbose)) {
> -			rv = 1;
> -			goto release;
> +			goto error;
>  		}
>  		sync_metadata(st);
>  		rv = reshape_array(container, fd, devname, st, &info, c->force,
> @@ -2369,10 +2350,17 @@ size_change_error:
>  		frozen = 0;
>  	}
>  release:
> +	if (cfd > -1)
> +		close(cfd);
> +	free(subarray);
>  	sysfs_free(sra);
>  	if (frozen > 0)
>  		unfreeze(st);
> +	free(st);
>  	return rv;
> +error:
> +	rv = 1;
> +	goto release;
>  }
>  
>  /* verify_reshape_position()
>