Re: [PATCH v2 6/7] udf: Remove struct ustr as non-needed intermediate storage

[Jan Kara]

On Thu 24-12-15 10:25:37, Andrew Gabbasov wrote:
> Although 'struct ustr' tries to structurize the data by combining
> the string and its length, it doesn't actually make much benefit,
> since it saves only one parameter, but introduces an extra copying
> of the whole buffer, serving as an intermediate storage. It looks
> quite inefficient and not actually needed.
> 
> This commit gets rid of the struct ustr by changing the parameters
> of some functions appropriately.
> 
> Also, it removes using 'dstring' type, since it doesn't make much
> sense too.
> 
> Just using the occasion, add a 'const' qualifier to udf_get_filename
> to make consistent parameters sets.

Some comments below:

> @@ -211,15 +163,18 @@ static int udf_name_to_CS0(dstring *ocu, struct ustr *uni, int length,
>  	wchar_t uni_char;
>  	int u_len, u_ch;
>  
> -	memset(ocu, 0, sizeof(dstring) * length);
> +	if (ocu_max_len <= 0)
> +		return 0;
> +
> +	memset(ocu, 0, ocu_max_len);
>  	ocu[0] = 8;
>  	max_val = 0xff;
>  	u_ch = 1;
>  
>  try_again:
> -	u_len = 0;
> -	for (i = 0; (i < uni->u_len) && ((u_len + 1 + u_ch) < length); i++) {
> -		len = conv_f(&uni->u_name[i], uni->u_len - i, &uni_char);
> +	u_len = 1;
> +	for (i = 0; (i < str_len) && ((u_len + u_ch) <= ocu_max_len); i++) {
> +		len = conv_f(&str_i[i], str_len - i, &uni_char);
>  		if (!len)
>  			continue;
>  		/* Invalid character, deal with it */
> @@ -236,41 +191,37 @@ try_again:
>  		}
>  
>  		if (max_val == 0xffff)
> -			ocu[++u_len] = (uint8_t)(uni_char >> 8);
> -		ocu[++u_len] = (uint8_t)(uni_char & 0xff);
> +			ocu[u_len++] = (uint8_t)(uni_char >> 8);
> +		ocu[u_len++] = (uint8_t)(uni_char & 0xff);
>  		i += len - 1;
>  	}
>  
> -	ocu[length - 1] = (uint8_t)u_len + 1;
> -	return u_len + 1;
> +	return u_len;

It seems you removed setting of the length in the resulting CS0 string.

> -int udf_CS0toUTF8(struct ustr *utf_o, const struct ustr *ocu_i)
> +int udf_CS0toUTF8(uint8_t *utf_o, int o_len, const uint8_t *ocu_i, int i_len)
>  {
> -	return udf_name_from_CS0(utf_o, ocu_i, udf_uni2char_utf8);
> +	return udf_name_from_CS0(utf_o, o_len, ocu_i, i_len,
> +				 udf_uni2char_utf8);
>  }
>  
> -int udf_get_filename(struct super_block *sb, uint8_t *sname, int slen,
> +int udf_get_filename(struct super_block *sb, const uint8_t *sname, int slen,
>  		     uint8_t *dname, int dlen)
>  {
> -	struct ustr *filename, *unifilename;
> +	uint8_t *filename;
>  	int (*conv_f)(wchar_t, unsigned char *, int);
>  	int ret;
>  
>  	if (!slen)
>  		return -EIO;
>  
> -	filename = kmalloc(sizeof(struct ustr), GFP_NOFS);
> +	if (dlen <= 0)
> +		return 0;
> +
> +	filename = kmalloc(dlen, GFP_NOFS);
>  	if (!filename)
>  		return -ENOMEM;
>  
> -	unifilename = kmalloc(sizeof(struct ustr), GFP_NOFS);
> -	if (!unifilename) {
> -		ret = -ENOMEM;
> -		goto out1;
> -	}
> -
> -	udf_build_ustr_exact(unifilename, sname, slen);
>  	if (UDF_QUERY_FLAG(sb, UDF_FLAG_UTF8)) {
>  		conv_f = udf_uni2char_utf8;
>  	} else if (UDF_QUERY_FLAG(sb, UDF_FLAG_NLS_MAP)) {
> @@ -278,21 +229,17 @@ int udf_get_filename(struct super_block *sb, uint8_t *sname, int slen,
>  	} else
>  		BUG();
>  
> -	ret = udf_name_from_CS0(filename, unifilename, conv_f);
> +	ret = udf_name_from_CS0(filename, dlen, sname, slen, conv_f);
>  	if (ret < 0) {
>  		udf_debug("Failed in udf_get_filename: sname = %s\n", sname);
>  		goto out2;
>  	}
>  
> -	ret = udf_translate_to_linux(dname, dlen,
> -				     filename->u_name, filename->u_len,
> -				     unifilename->u_name, unifilename->u_len);
> +	ret = udf_translate_to_linux(dname, dlen, filename, dlen, sname, slen);

Umm, but here you pass CS0 name to udf_translate_to_linux() including
beginning encoding character which didn't happen before your patch. So in
case we have to compute CRC it will be different.


								Honza
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/