Re: [PATCH v2 2/3] lib/string_helpers.c: Refactor string_escape_mem

[Rasmus Villemoes <linux@rasmusvillemoes.dk>]

On Thu, Jan 29 2015, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

>>  
>> -static int escape_space(unsigned char c, char **dst, size_t *osz)
>> +static bool escape_space(unsigned char c, char **dst, char *end)
>>  {
>>  	char *out = *dst;
>>  	unsigned char to;
>>  
>> -	if (*osz < 2)
>> -		return -ENOMEM;
>> -
>>  	switch (c) {
>>  	case '\n':
>>  		to = 'n';
>> @@ -283,26 +275,23 @@ static int escape_space(unsigned char c, char **dst, size_t *osz)
>>  		to = 'f';
>>  		break;
>>  	default:
>> -		return 0;
>> +		return false;
>>  	}
>>  
>> -	*out++ = '\\';
>> -	*out++ = to;
>> +	if (out + 0 < end)
>> +		out[0] = '\\';
>> +	if (out + 1 < end)
>> +		out[1] = to;
>
> Could we do this in the same way like for hex_string, i.e.
>
> if (out < end)
>  *out = '\\';
> ++out;
>
> …
>
> *dst = out;
> return true;
>
> ?

We could, but I don't think either is more readable than the
other. Hence I chose the one requiring 2n+1 lines instead of 3n+1
lines. Had this been in vsprintf.c I would stick to the pattern you
suggest.


>> @@ -440,9 +424,10 @@ static int escape_hex(unsigned char c, char **dst, size_t *osz)
>>  int string_escape_mem(const char *src, size_t isz, char **dst, size_t osz,
>>  		      unsigned int flags, const char *esc)
>>  {
>> -	char *out = *dst, *p = out;
>> +	char *p = *dst;
>
> Leave 'out' here and…
>
>> +	char *end = p + osz;
>>  	bool is_dict = esc && *esc;
>> -	int ret = 0;
>> +	int ret;
>>  
>>  	while (isz--) {
>>  		unsigned char c = *src++;
>> @@ -462,55 +447,32 @@ int string_escape_mem(const char *src, size_t isz, char **dst, size_t osz,
>>  		    (is_dict && !strchr(esc, c))) {
>>  			/* do nothing */
>>  		} else {
>> -			if (flags & ESCAPE_SPACE) {
>> -				ret = escape_space(c, &p, &osz);
>> -				if (ret < 0)
>> -					break;
>> -				if (ret > 0)
>> -					continue;
>> -			}
>> -
>> -			if (flags & ESCAPE_SPECIAL) {
>> -				ret = escape_special(c, &p, &osz);
>> -				if (ret < 0)
>> -					break;
>> -				if (ret > 0)
>> -					continue;
>> -			}
>> -
>> -			if (flags & ESCAPE_NULL) {
>> -				ret = escape_null(c, &p, &osz);
>> -				if (ret < 0)
>> -					break;
>> -				if (ret > 0)
>> -					continue;
>> -			}
>> +			if (flags & ESCAPE_SPACE && escape_space(c, &p, end))
>> +				continue;
>> +
>> +			if (flags & ESCAPE_SPECIAL && escape_special(c, &p, end))
>> +				continue;
>> +
>> +			if (flags & ESCAPE_NULL && escape_null(c, &p, end))
>> +				continue;
>>  
>>  			/* ESCAPE_OCTAL and ESCAPE_HEX always go last */
>> -			if (flags & ESCAPE_OCTAL) {
>> -				ret = escape_octal(c, &p, &osz);
>> -				if (ret < 0)
>> -					break;
>> +			if (flags & ESCAPE_OCTAL && escape_octal(c, &p, end))
>>  				continue;
>> -			}
>> -			if (flags & ESCAPE_HEX) {
>> -				ret = escape_hex(c, &p, &osz);
>> -				if (ret < 0)
>> -					break;
>> +
>> +			if (flags & ESCAPE_HEX && escape_hex(c, &p, end))
>>  				continue;
>> -			}
>>  		}
>>  
>> -		ret = escape_passthrough(c, &p, &osz);
>> -		if (ret < 0)
>> -			break;
>> +		escape_passthrough(c, &p, end);
>> +	}
>
> + black line.
>
>> +	if (p > end) {
>> +		*dst = end;
>> +		return -ENOMEM;
>>  	}
>>  
>> +	ret = p - *dst;
>>  	*dst = p;
>> -
>> -	if (ret < 0)
>> -		return ret;
>> -
>> -	return p - out;
>
> …and do not change the logic right now. Just substitute if (ret < 0) by
> above if (p > end).
>

I'm not sure I follow. How does this change the logic? We return -ENOMEM
if and only if the entire output didn't fit, while still updating *dst
to point to one past the last output character. If the output did fit,
we return the size of the output.

One thing that only occured to me now is that we may now leave a partial
escape sequence at the end of the buffer. I can't see how this can
reasonably be avoided while still doing a meaningful refactorization
preparing it for the next patch.

Rasmus