Re: [PATCH] NLS: improve UTF8 -> UTF16 string conversion routine

[NamJae Jeon]

2011/11/18 Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>:
> The utf8s_to_utf16s conversion routine needs to be improved. ÂUnlike
> its utf16s_to_utf8s sibling, it doesn't accept arguments specifying
> the maximum length of the output buffer or the endianness of its
> 16-bit output.
>
> This patch (as1501) adds the two missing arguments, and adjusts the
> only two places in the kernel where the function is called. ÂA
> follow-on patch will add a third caller that does utilize the new
> capabilities.
>
> The two conversion routines are still annoyingly inconsistent in the
> way they handle invalid byte combinations. ÂBut that's a subject for a
> different patch.
>
> Signed-off-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
> CC: Clemens Ladisch <clemens@xxxxxxxxxx>
>
> ---
>
> Âdrivers/hv/hv_kvp.c | Â 10 ++++++----
> Âfs/fat/namei_vfat.c | Â Â3 ++-
> Âfs/nls/nls_base.c  |  43 +++++++++++++++++++++++++++++++++----------
> Âinclude/linux/nls.h | Â Â5 +++--
> Â4 files changed, 44 insertions(+), 17 deletions(-)
>
> Index: usb-3.2/include/linux/nls.h
> ===================================================================
> --- usb-3.2.orig/include/linux/nls.h
> +++ usb-3.2/include/linux/nls.h
> @@ -43,7 +43,7 @@ enum utf16_endian {
> Â Â Â ÂUTF16_BIG_ENDIAN
> Â};
>
> -/* nls.c */
> +/* nls_base.c */
> Âextern int register_nls(struct nls_table *);
> Âextern int unregister_nls(struct nls_table *);
> Âextern struct nls_table *load_nls(char *);
> @@ -52,7 +52,8 @@ extern struct nls_table *load_nls_defaul
>
> Âextern int utf8_to_utf32(const u8 *s, int len, unicode_t *pu);
> Âextern int utf32_to_utf8(unicode_t u, u8 *s, int maxlen);
> -extern int utf8s_to_utf16s(const u8 *s, int len, wchar_t *pwcs);
> +extern int utf8s_to_utf16s(const u8 *s, int len,
> + Â Â Â Â Â Â Â enum utf16_endian endian, wchar_t *pwcs, int maxlen);
> Âextern int utf16s_to_utf8s(const wchar_t *pwcs, int len,
> Â Â Â Â Â Â Â Âenum utf16_endian endian, u8 *s, int maxlen);
>
> Index: usb-3.2/fs/nls/nls_base.c
> ===================================================================
> --- usb-3.2.orig/fs/nls/nls_base.c
> +++ usb-3.2/fs/nls/nls_base.c
> @@ -114,34 +114,57 @@ int utf32_to_utf8(unicode_t u, u8 *s, in
> Â}
> ÂEXPORT_SYMBOL(utf32_to_utf8);
>
> -int utf8s_to_utf16s(const u8 *s, int len, wchar_t *pwcs)
> +static inline void put_utf16(wchar_t *s, unsigned c, enum utf16_endian endian)
> +{
> + Â Â Â switch (endian) {
> + Â Â Â default:
> + Â Â Â Â Â Â Â *s = (wchar_t) c;
> + Â Â Â Â Â Â Â break;
> + Â Â Â case UTF16_LITTLE_ENDIAN:
> + Â Â Â Â Â Â Â *s = __cpu_to_le16(c);
> + Â Â Â Â Â Â Â break;
> + Â Â Â case UTF16_BIG_ENDIAN:
> + Â Â Â Â Â Â Â *s = __cpu_to_be16(c);
> + Â Â Â Â Â Â Â break;
> + Â Â Â }
> +}
> +
> +int utf8s_to_utf16s(const u8 *s, int len, enum utf16_endian endian,
> + Â Â Â Â Â Â Â wchar_t *pwcs, int maxlen)
> Â{
> Â Â Â Âu16 *op;
> Â Â Â Âint size;
> Â Â Â Âunicode_t u;
>
> Â Â Â Âop = pwcs;
> - Â Â Â while (*s && len > 0) {
> + Â Â Â while (len > 0 && maxlen > 0 && *s) {
> Â Â Â Â Â Â Â Âif (*s & 0x80) {
> Â Â Â Â Â Â Â Â Â Â Â Âsize = utf8_to_utf32(s, len, &u);
> Â Â Â Â Â Â Â Â Â Â Â Âif (size < 0)
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âreturn -EINVAL;
> + Â Â Â Â Â Â Â Â Â Â Â s += size;
> + Â Â Â Â Â Â Â Â Â Â Â len -= size;
Why did you move this code to here ?
>
> Â Â Â Â Â Â Â Â Â Â Â Âif (u >= PLANE_SIZE) {
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â if (maxlen < 2)
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â break;
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âu -= PLANE_SIZE;
> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â *op++ = (wchar_t) (SURROGATE_PAIR |
> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ((u >> 10) & SURROGATE_BITS));
> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â *op++ = (wchar_t) (SURROGATE_PAIR |
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â put_utf16(op++, SURROGATE_PAIR |
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ((u >> 10) & SURROGATE_BITS),
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â endian);
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â put_utf16(op++, SURROGATE_PAIR |
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ÂSURROGATE_LOW |
> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â (u & SURROGATE_BITS));
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â (u & SURROGATE_BITS),
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â endian);
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â maxlen -= 2;

Why did you use contants value(-2) instead of maxlen -= size; value ?

> Â Â Â Â Â Â Â Â Â Â Â Â} else {
> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â *op++ = (wchar_t) u;
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â put_utf16(op++, u, endian);
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â maxlen--;
> Â Â Â Â Â Â Â Â Â Â Â Â}
> - Â Â Â Â Â Â Â Â Â Â Â s += size;
> - Â Â Â Â Â Â Â Â Â Â Â len -= size;
> Â Â Â Â Â Â Â Â} else {
> - Â Â Â Â Â Â Â Â Â Â Â *op++ = *s++;
> + Â Â Â Â Â Â Â Â Â Â Â put_utf16(op++, *s++, endian);
> Â Â Â Â Â Â Â Â Â Â Â Âlen--;
> + Â Â Â Â Â Â Â Â Â Â Â maxlen--;
> Â Â Â Â Â Â Â Â}
> Â Â Â Â}
> Â Â Â Âreturn op - pwcs;
> Index: usb-3.2/fs/fat/namei_vfat.c
> ===================================================================
> --- usb-3.2.orig/fs/fat/namei_vfat.c
> +++ usb-3.2/fs/fat/namei_vfat.c
> @@ -512,7 +512,8 @@ xlate_to_uni(const unsigned char *name,
> Â Â Â Âint charlen;
>
> Â Â Â Âif (utf8) {
> - Â Â Â Â Â Â Â *outlen = utf8s_to_utf16s(name, len, (wchar_t *)outname);
> + Â Â Â Â Â Â Â *outlen = utf8s_to_utf16s(name, len, UTF16_HOST_ENDIAN,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â (wchar_t *) outname, FAT_LFN_LEN + 2);
Is there the reason why you plus 2 to FAT_LFN_LEN ?
> Â Â Â Â Â Â Â Âif (*outlen < 0)
> Â Â Â Â Â Â Â Â Â Â Â Âreturn *outlen;
> Â Â Â Â Â Â Â Âelse if (*outlen > FAT_LFN_LEN)
                          return -ENAMETOOLONG;
"else if (*outlen > FAT_LFN_LEN)" code  is needed ? Is there the case
that *outlen is over FAT_LFN_LEN in your patch ?

Thanks.
> Index: usb-3.2/drivers/hv/hv_kvp.c
> ===================================================================
> --- usb-3.2.orig/drivers/hv/hv_kvp.c
> +++ usb-3.2/drivers/hv/hv_kvp.c
> @@ -212,11 +212,13 @@ kvp_respond_to_host(char *key, char *val
> Â Â Â Â * The windows host expects the key/value pair to be encoded
> Â Â Â Â * in utf16.
> Â Â Â Â */
> - Â Â Â keylen = utf8s_to_utf16s(key_name, strlen(key_name),
> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â (wchar_t *)kvp_data->data.key);
> + Â Â Â keylen = utf8s_to_utf16s(key_name, strlen(key_name), UTF16_HOST_ENDIAN,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â (wchar_t *) kvp_data->data.key,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â HV_KVP_EXCHANGE_MAX_KEY_SIZE / 2);
> Â Â Â Âkvp_data->data.key_size = 2*(keylen + 1); /* utf16 encoding */
> - Â Â Â valuelen = utf8s_to_utf16s(value, strlen(value),
> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â (wchar_t *)kvp_data->data.value);
> + Â Â Â valuelen = utf8s_to_utf16s(value, strlen(value), UTF16_HOST_ENDIAN,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â (wchar_t *) kvp_data->data.value,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â HV_KVP_EXCHANGE_MAX_VALUE_SIZE / 2);
> Â Â Â Âkvp_data->data.value_size = 2*(valuelen + 1); /* utf16 encoding */
>
> Â Â Â Âkvp_data->data.value_type = REG_SZ; /* all our values are strings */
>
> --
> 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/
>
¢éì®&Þ~º&¶¬–+-±éÝ¥Šw®žË±Êâmébžìdz¹Þ)í…æèw*jg¬±¨¶‰šŽŠÝj/êäz¹ÞŠà2ŠÞ¨è­Ú&¢)ß«a¶Úþø®G«éh®æj:+v‰¨Šwè†Ù>Wš±êÞiÛaxPjØm¶ŸÿÃ-»+ƒùdš_