[dm-devel] [PATCH 3/4] libmultipath: add_feature: allow only 1 feature

[Hannes Reinecke]

On 08/29/2017 12:05 AM, Martin Wilck wrote:
> The existing test "if feature is already present" doesn't work for
> multiple features, and we are only using add_feature() for single
> feature additions anyway. Simplify the code by not allowing spaces
> in the feature string to be added. This way we can drop the
> complex "count new features" code. Moreover, print an error message if
> the existing features string is malformed.
> 
> The old code calculated the width of the feature count twice, and the
> second time messed up the buffer length field passed to snprintf(); fix
> that, too.  Finally, replace several strcat() invocations by one
> strncpy() call to make the code easier to review.
> 
> Signed-off-by: Martin Wilck <mwilck at suse.com>
> ---
>  libmultipath/structs.c | 64 +++++++++++++++++---------------------------------
>  1 file changed, 21 insertions(+), 43 deletions(-)
> 
> diff --git a/libmultipath/structs.c b/libmultipath/structs.c
> index 11e33676..828e7907 100644
> --- a/libmultipath/structs.c
> +++ b/libmultipath/structs.c
> @@ -516,8 +516,7 @@ void setup_feature(struct multipath *mpp, char *feature)
>  int add_feature(char **f, const char *n)
>  {
>  	int c = 0, d, l;
> -	char *e, *p, *t;
> -	const char *q;
> +	char *e, *t;
>  
>  	if (!f)
>  		return 1;
> @@ -526,6 +525,11 @@ int add_feature(char **f, const char *n)
>  	if (!n || *n == '0')
>  		return 0;
>  
> +	if (strchr(n, ' ') != NULL) {
> +		condlog(0, "internal error: feature \"%s\" contains spaces", n);
> +		return 1;
> +	}
> +
>  	/* default feature is null */
>  	if(!*f)
>  	{
> @@ -543,55 +547,29 @@ int add_feature(char **f, const char *n)
>  
>  	/* Get feature count */
>  	c = strtoul(*f, &e, 10);
> -	if (*f == e)
> -		/* parse error */
> +	if (*f == e || (*e != ' ' && *e != '\0')) {
> +		condlog(0, "parse error in feature string \"%s\"", *f);
>  		return 1;
> -
> -	/* Check if we need to increase feature count space */
> -	l = strlen(*f) + strlen(n) + 1;
> -
> -	/* Count new features */
> -	if ((c % 10) == 9)
> -		l++;
> -	c++;
> -	q = n;
> -	while (*q != '\0') {
> -		if (*q == ' ' && q[1] != '\0' && q[1] != ' ') {
> -			if ((c % 10) == 9)
> -				l++;
> -			c++;
> -		}
> -		q++;
>  	}
>  
> +	/* Add 1 digit and 1 space */
> +	l = strlen(e) + strlen(n) + 2;
> +
> +	c++;
> +	/* Check if we need more digits for feature count */
> +	for (d = c; d >= 10; d /= 10)
> +		l++;
> +
>  	t = MALLOC(l + 1);
>  	if (!t)
>  		return 1;
>  
> -	memset(t, 0, l + 1);
> +	/* e: old feature string with leading space, or "" */
> +	if (*e == ' ')
> +		while (*(e + 1) == ' ')
> +			e++;
>  
> -	/* Update feature count */
> -	d = c;
> -	l = 1;
> -	while (d > 9) {
> -		d /= 10;
> -		l++;
> -	}
> -	p = t;
> -	snprintf(p, l + 2, "%0d ", c);
> -
> -	/* Copy the feature string */
> -	p = strchr(*f, ' ');
> -
> -	if (p) {
> -		while (*p == ' ')
> -			p++;
> -		strcat(t, p);
> -		strcat(t, " ");
> -	} else {
> -		p = t + strlen(t);
> -	}
> -	strcat(t, n);
> +	snprintf(t, l + 1, "%0d%s %s", c, e, n);
>  
>  	FREE(*f);
>  	*f = t;
> 
I did say we should have a testcase, right?

Reviewed-by: Hannes Reinecke <hare at suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare at suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)