Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1700091 > unrolled thread

[PATCH v5] vfat: Deduplicate hex2bin()

Back to article view | Back to linux.kernel

Contents

  [PATCH v5] vfat: Deduplicate hex2bin() Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2017-07-31 16:40 +0200
    [PATCH v5] vfat: Deduplicate hex2bin() OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> - 2017-07-31 20:50 +0200
      Re: [PATCH v5] vfat: Deduplicate hex2bin() Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-07-31 21:40 +0200
        Re: [PATCH v5] vfat: Deduplicate hex2bin() OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> - 2017-07-31 23:00 +0200
          Re: [PATCH v5] vfat: Deduplicate hex2bin() Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2017-08-01 15:00 +0200
    Re: [PATCH v5] vfat: Deduplicate hex2bin() OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> - 2017-07-31 20:50 +0200
      Re: [PATCH v5] vfat: Deduplicate hex2bin() Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2017-07-31 21:40 +0200
        Re: [PATCH v5] vfat: Deduplicate hex2bin() OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> - 2017-07-31 23:00 +0200

#1700091 — [PATCH v5] vfat: Deduplicate hex2bin()

We may use hex2bin() instead of custom approach.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 This is blast from the past (2011). Instead of dirty looking bitwise
 types here, though __be16 would be correct, I decide to go with
 an array of two u8 items.

 This patch relies on
 http://marc.info/?l=linux-kernel&m=150150935411183&w=2 being applied,
 which is currently not.

 fs/fat/namei_vfat.c | 35 ++++++++++-------------------------
 1 file changed, 10 insertions(+), 25 deletions(-)

diff --git a/fs/fat/namei_vfat.c b/fs/fat/namei_vfat.c
index 6a7152d0c250..56127f76c41f 100644
--- a/fs/fat/namei_vfat.c
+++ b/fs/fat/namei_vfat.c
@@ -17,6 +17,7 @@
 
 #include <linux/module.h>
 #include <linux/ctype.h>
+#include <linux/kernel.h>
 #include <linux/slab.h>
 #include <linux/namei.h>
 #include "fat.h"
@@ -510,11 +511,10 @@ xlate_to_uni(const unsigned char *name, int len, unsigned char *outname,
 	     struct nls_table *nls)
 {
 	const unsigned char *ip;
-	unsigned char nc;
 	unsigned char *op;
-	unsigned int ec;
-	int i, k, fill;
+	int i, fill;
 	int charlen;
+	u8 hc[2];
 
 	if (utf8) {
 		*outlen = utf8s_to_utf16s(name, len, UTF16_HOST_ENDIAN,
@@ -532,31 +532,16 @@ xlate_to_uni(const unsigned char *name, int len, unsigned char *outname,
 			if (escape && (*ip == ':')) {
 				if (i > len - 5)
 					return -EINVAL;
-				ec = 0;
-				for (k = 1; k < 5; k++) {
-					nc = ip[k];
-					ec <<= 4;
-					if (nc >= '0' && nc <= '9') {
-						ec |= nc - '0';
-						continue;
-					}
-					if (nc >= 'a' && nc <= 'f') {
-						ec |= nc - ('a' - 10);
-						continue;
-					}
-					if (nc >= 'A' && nc <= 'F') {
-						ec |= nc - ('A' - 10);
-						continue;
-					}
-					return -EINVAL;
-				}
-				*op++ = ec & 0xFF;
-				*op++ = ec >> 8;
+
+				fill = hex2bin(hc, ip + 1, 2);
+				if (fill)
+					return fill;
+				*op++ = hc[1];
+				*op++ = hc[0];
 				ip += 5;
 				i += 5;
 			} else {
-				charlen = nls->char2uni(ip, len - i,
-									(wchar_t *)op);
+				charlen = nls->char2uni(ip, len - i, (wchar_t *)op);
 				if (charlen < 0)
 					return -EINVAL;
 				ip += charlen;
-- 
2.13.2

[toc] | [next] | [standalone]

#1700314

We may use hex2bin() instead of custom approach.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
---

 fs/fat/namei_vfat.c |   35 ++++++++++++-----------------------
 1 file changed, 12 insertions(+), 23 deletions(-)

diff -puN fs/fat/namei_vfat.c~fat-dont-use-custom-hex2bin fs/fat/namei_vfat.c
--- linux/fs/fat/namei_vfat.c~fat-dont-use-custom-hex2bin	2017-01-07 09:52:14.981693213 +0900
+++ linux-hirofumi/fs/fat/namei_vfat.c	2017-01-07 09:52:14.982693219 +0900
@@ -19,6 +19,8 @@
 #include <linux/ctype.h>
 #include <linux/slab.h>
 #include <linux/namei.h>
+#include <linux/kernel.h>
+
 #include "fat.h"
 
 static inline unsigned long vfat_d_version(struct dentry *dentry)
@@ -510,10 +512,8 @@ xlate_to_uni(const unsigned char *name,
 	     struct nls_table *nls)
 {
 	const unsigned char *ip;
-	unsigned char nc;
 	unsigned char *op;
-	unsigned int ec;
-	int i, k, fill;
+	int i, fill;
 	int charlen;
 
 	if (utf8) {
@@ -530,33 +530,22 @@ xlate_to_uni(const unsigned char *name,
 			 i < len && *outlen < FAT_LFN_LEN;
 			 *outlen += 1) {
 			if (escape && (*ip == ':')) {
+				u8 uc[2];
+
 				if (i > len - 5)
 					return -EINVAL;
-				ec = 0;
-				for (k = 1; k < 5; k++) {
-					nc = ip[k];
-					ec <<= 4;
-					if (nc >= '0' && nc <= '9') {
-						ec |= nc - '0';
-						continue;
-					}
-					if (nc >= 'a' && nc <= 'f') {
-						ec |= nc - ('a' - 10);
-						continue;
-					}
-					if (nc >= 'A' && nc <= 'F') {
-						ec |= nc - ('A' - 10);
-						continue;
-					}
+
+				if (hex2bin(uc, ip + 1, 2) < 0)
 					return -EINVAL;
-				}
-				*op++ = ec & 0xFF;
-				*op++ = ec >> 8;
+
+				*(wchar_t *)op = uc[0] << 8 | uc[1];
+
+				op += 2;
 				ip += 5;
 				i += 5;
 			} else {
 				charlen = nls->char2uni(ip, len - i,
-									(wchar_t *)op);
+							(wchar_t *)op);
 				if (charlen < 0)
 					return -EINVAL;
 				ip += charlen;
_

-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

[toc] | [prev] | [next] | [standalone]

#1700351

On Mon, Jul 31, 2017 at 9:44 PM, OGAWA Hirofumi
<hirofumi@mail.parknet.co.jp> wrote:
> We may use hex2bin() instead of custom approach.

>  #include <linux/ctype.h>
>  #include <linux/slab.h>
>  #include <linux/namei.h>
> +#include <linux/kernel.h>

Would not be better to squeeze it somehow alphabetically ordered (in
most ordered part)?


> +
> +                               *(wchar_t *)op = uc[0] << 8 | uc[1];
> +
> +                               op += 2;

This had been in the original patch 6 years ago and had been refused
because of endianess issues.

>                                 charlen = nls->char2uni(ip, len - i,
> -                                                                       (wchar_t *)op);
> +                                                       (wchar_t *)op);

It perfectly fits one line.

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1700385

Andy Shevchenko <andy.shevchenko@gmail.com> writes:

>> +
>> +                               *(wchar_t *)op = uc[0] << 8 | uc[1];
>> +
>> +                               op += 2;
>
> This had been in the original patch 6 years ago and had been refused
> because of endianess issues.

Sorry, I forgot what I said completely. Maybe I changed my mind? 

			if (uni_xlate == 1) {
				*op++ = ':';
				op = hex_byte_pack(op, ec >> 8);
				op = hex_byte_pack(op, ec);
				len -= 5;

Here is output. So "uc[0] << 8 | uc[1]" is right code, isn't it?

>>                                 charlen = nls->char2uni(ip, len - i,
>> -                                                                       (wchar_t *)op);
>> +                                                       (wchar_t *)op);
>
> It perfectly fits one line.

It over 80 column.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

[toc] | [prev] | [next] | [standalone]

#1701009

On Tue, 2017-08-01 at 05:54 +0900, OGAWA Hirofumi wrote:
> Andy Shevchenko <andy.shevchenko@gmail.com> writes:
> 
> > > +
> > > +                               *(wchar_t *)op = uc[0] << 8 |
> > > uc[1];
> > > +
> > > +                               op += 2;
> > 
> > This had been in the original patch 6 years ago and had been refused
> > because of endianess issues.
> 
> Sorry, I forgot what I said completely. Maybe I changed my mind? 
> 
> 			if (uni_xlate == 1) {
> 				*op++ = ':';
> 				op = hex_byte_pack(op, ec >> 8);
> 				op = hex_byte_pack(op, ec);
> 				len -= 5;
> 
> Here is output. So "uc[0] << 8 | uc[1]" is right code, isn't it?

Yes, the right part of the expression is correct.

If *(wchar_t *)op is what we are expecting, that it's okay.

> > >                                 charlen = nls->char2uni(ip, len -
> > > i,
> > > -                                                                 
> > >       (wchar_t *)op);
> > > +                                                       (wchar_t
> > > *)op);
> > 
> > It perfectly fits one line.
> 
> It over 80 column.

For one character? :-)

>>> > +                          fill = hex2bin(hc, ip + 1, 2);
>>> > +                          if (fill)
>>> > +                                  return fill;
 
>>> This should not use random errno (in this case, it is -1 (EPERM)).

>> You perhaps missed the side note I put after --- line.
>> It reflects this change.

> Sure, I missed to read it. But same here, hex2bin() doesn't care FS's
> errno, it is what "random errno" I meant (I.e. hex2bin() might change
it
> to bool or any other errno again).

I see your point. 

I'm fine with shadowing in cases when it's strictly needed, otherwise
how can it be changed to bool without compile time issues? Any other
error code changes for a such widely used helper might be a disaster not
only for this driver, so it's quite unlikely.

At the end it's up to you to decide. So, I'm fine with the patch Andrew
took.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[toc] | [prev] | [next] | [standalone]

#1700317

Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:

> We may use hex2bin() instead of custom approach.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

[...]

> +	u8 hc[2];

Let's move this to following more local scope.

>  	if (utf8) {
>  		*outlen = utf8s_to_utf16s(name, len, UTF16_HOST_ENDIAN,
> @@ -532,31 +532,16 @@ xlate_to_uni(const unsigned char *name, int len, unsigned char *outname,
>  			if (escape && (*ip == ':')) {
				u8 uc[2];

Here.

>  				if (i > len - 5)
>  					return -EINVAL;

[...]

> +				fill = hex2bin(hc, ip + 1, 2);
> +				if (fill)
> +					return fill;

This should not use random errno (in this case, it is -1 (EPERM)).

> +				*op++ = hc[1];
> +				*op++ = hc[0];

Maybe, originally endian bug?

>  				ip += 5;
>  				i += 5;
>  			} else {
> -				charlen = nls->char2uni(ip, len - i,
> -									(wchar_t *)op);
> +				charlen = nls->char2uni(ip, len - i, (wchar_t *)op);
>  				if (charlen < 0)
>  					return -EINVAL;
>  				ip += charlen;

I will send a modified patch.

Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

[toc] | [prev] | [next] | [standalone]

#1700355

On Tue, 2017-08-01 at 03:40 +0900, OGAWA Hirofumi wrote:
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
> 
> > We may use hex2bin() instead of custom approach.
> > 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> 
> 

> > +				fill = hex2bin(hc, ip + 1, 2);
> > +				if (fill)
> > +					return fill;
> 
> This should not use random errno (in this case, it is -1 (EPERM)).

You perhaps missed the side note I put after --- line.
It reflects this change.

> 
> > +				*op++ = hc[1];
> > +				*op++ = hc[0];
> 
> Maybe, originally endian bug?

No problem reported. And as you noticed quite ago it is __be16
originally as it goes hi-lo.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[toc] | [prev] | [next] | [standalone]

#1700387

Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:

>> > +				fill = hex2bin(hc, ip + 1, 2);
>> > +				if (fill)
>> > +					return fill;
>> 
>> This should not use random errno (in this case, it is -1 (EPERM)).
>
> You perhaps missed the side note I put after --- line.
> It reflects this change.

Sure, I missed to read it. But same here, hex2bin() doesn't care FS's
errno, it is what "random errno" I meant (I.e. hex2bin() might change it
to bool or any other errno again).
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web