vfat: Deduplicate hex2bin()
PATCH v5 1/1 vfat: Deduplicate hex2bin()
We may use hex2bin() instead of custom approach.
---
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(-)
fs/fat/namei_vfat.c +10 -25
@@ -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;
Signature · 1 line
2.13.2
OGAWA Hirofumi
maintainerQuoted 3 lines · Andy Shevchenko
We may use
hex2bin()instead of custom approach.Signed-off-by Andy Shevchenko <[email protected]>
[...]
fs/fat/namei_vfat.c xlate_to_uni(const unsigned char *name, int len, unsigned char *outname, · line 517
Quoted 1 line
fs/fat/namei_vfat.c +1
-0+u8 hc[2 ];
Let's move this to following more local scope.
fs/fat/namei_vfat.c xlate_to_uni(const unsigned char *name, int len, unsigned char *outname, · line 532
Quoted 4 lines
fs/fat/namei_vfat.c +0
-0if (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.
Quoted 2 lines
(quoted diff) +0
-0if (i > len - 5) return -EINVAL;
[...]
fs/fat/namei_vfat.c xlate_to_uni(const unsigned char *name, int len, unsigned char *outname, · line 538
Quoted 3 lines
fs/fat/namei_vfat.c +3
-0+ fill =hex2bin (hc, ip +1 ,2 ); +if (fill) +return fill;
This should not use random errno (in this case, it is -1 (EPERM)).
fs/fat/namei_vfat.c xlate_to_uni(const unsigned char *name, int len, unsigned char *outname, · line 540
Quoted 2 lines
fs/fat/namei_vfat.c +2
-0+ *op++ = hc[1 ]; + *op++ = hc[0 ];
Maybe, originally endian bug?
fs/fat/namei_vfat.c xlate_to_uni(const unsigned char *name, int len, unsigned char *outname, · line 547
Quoted 9 lines
fs/fat/namei_vfat.c +1
-2ip +=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.
Signature · 1 line
OGAWA Hirofumi <[email protected]>
OGAWA Hirofumi
maintainerPATCH v5 1/1 vfat: Deduplicate hex2bin()
We may use hex2bin() instead of custom approach.
---
fs/fat/namei_vfat.c | 35 ++++++++++++----------------------- 1 file changed, 12 insertions(+), 23 deletions(-)
fs/fat/namei_vfat.c renamed from fs/fat/namei_vfat.c~fat-dont-use-custom-hex2bin +12 -23
@@ -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;
_
Signature · 1 line
OGAWA Hirofumi <[email protected]>
Quoted 6 lines · OGAWA Hirofumi
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)?
fs/fat/namei_vfat.c xlate_to_uni(const unsigned char *name, · line 543
Quoted 4 lines
fs/fat/namei_vfat.c renamed from fs/fat/namei_vfat.c~fat-dont-use-custom-hex2bin +4
-0+ + *(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.
fs/fat/namei_vfat.c xlate_to_uni(const unsigned char *name, · line 548
Quoted 3 lines
charlen = nls->char2uni(ip, len - i,fs/fat/namei_vfat.c renamed from fs/fat/namei_vfat.c~fat-dont-use-custom-hex2bin +1
-1- (wchar_t *)op); + (wchar_t *)op);
It perfectly fits one line.
Signature · 2 lines
With Best Regards, Andy Shevchenko
OGAWA Hirofumi
maintainerQuoted 7 lines · Andy Shevchenko
fs/fat/namei_vfat.c xlate_to_uni(const unsigned char *name, · line 543
Quoted 4 lines
fs/fat/namei_vfat.c renamed from fs/fat/namei_vfat.c~fat-dont-use-custom-hex2bin +4
-0+ + *(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?
Quoted 5 lines
fs/fat/namei_vfat.c xlate_to_uni(const unsigned char *name, · line 548
Quoted 3 lines
charlen = nls->char2uni(ip, len - i,fs/fat/namei_vfat.c renamed from fs/fat/namei_vfat.c~fat-dont-use-custom-hex2bin +1
-1- (wchar_t *)op); + (wchar_t *)op);It perfectly fits one line.
It over 80 column.
Signature · 1 line
OGAWA Hirofumi <[email protected]>
Quoted 13 lines · OGAWA Hirofumi
Quoted 8 lines · Andy Shevchenko
Quoted 5 lines
+
+ *(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?
Quoted 7 lines
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.
Quoted 10 lines
Quoted 8 lines
Quoted 6 lines
charlen = nls->char2uni(ip, len - i,
-
(wchar_t *)op);
+ (wchar_t *)op);It perfectly fits one line.
It over 80 column.
For one character? :-)
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
+ fill =
hex2bin(hc, ip + 1, 2); + if (fill)
+ return fill;
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
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
Quoted 1 line
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.
Signature · 2 lines
Andy Shevchenko <[email protected]> Intel Finland Oy
Quoted 9 lines · OGAWA Hirofumi
Quoted 3 lines · Andy Shevchenko
We may use
hex2bin()instead of custom approach.Signed-off-by Andy Shevchenko <[email protected]>
Quoted 5 lines
fs/fat/namei_vfat.c xlate_to_uni(const unsigned char *name, int len, unsigned char *outname, · line 538
Quoted 3 lines
fs/fat/namei_vfat.c +3
-0+ 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.
Quoted 5 lines
fs/fat/namei_vfat.c xlate_to_uni(const unsigned char *name, int len, unsigned char *outname, · line 540
Quoted 3 lines
fs/fat/namei_vfat.c +2
-0+ *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.
Signature · 2 lines
Andy Shevchenko <[email protected]> Intel Finland Oy
OGAWA Hirofumi
maintainerQuoted 8 lines · Andy Shevchenko
Quoted 5 lines
fs/fat/namei_vfat.c xlate_to_uni(const unsigned char *name, int len, unsigned char *outname, · line 538
Quoted 3 lines
fs/fat/namei_vfat.c +3
-0+ 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).
Signature · 1 line
OGAWA Hirofumi <[email protected]>