From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753917AbcADMca (ORCPT ); Mon, 4 Jan 2016 07:32:30 -0500 Received: from mx2.suse.de ([195.135.220.15]:56468 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753238AbcADMc1 (ORCPT ); Mon, 4 Jan 2016 07:32:27 -0500 Date: Mon, 4 Jan 2016 13:32:33 +0100 From: Jan Kara To: Andrew Gabbasov Cc: Jan Kara , linux-kernel@vger.kernel.org Subject: Re: [PATCH v2 6/7] udf: Remove struct ustr as non-needed intermediate storage Message-ID: <20160104123233.GB13014@quack.suse.cz> References: <1450974338-22762-1-git-send-email-andrew_gabbasov@mentor.com> <1450974338-22762-7-git-send-email-andrew_gabbasov@mentor.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1450974338-22762-7-git-send-email-andrew_gabbasov@mentor.com> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 SUSE Labs, CR