Re: [PATCHv2] fat: don't use custom hex_to_bin()

[Andy Shevchenko]

On Tue, Sep 27, 2011 at 8:19 PM, OGAWA Hirofumi
<hirofumi@xxxxxxxxxxxxxxxxxx> wrote:
> Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> writes:
>
> Looks good to me.
>
> Acked-by: OGAWA Hirofumi <hirofumi@xxxxxxxxxxxxxxxxxx>
>
> Or if it's needed, I'll apply this to fat tree.
Please, hold on. Read my comments below.

>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
>> Cc: OGAWA Hirofumi <hirofumi@xxxxxxxxxxxxxxxxxx>
>> ---
>> Âfs/fat/namei_vfat.c | Â 30 ++++++++----------------------
>> Â1 files changed, 8 insertions(+), 22 deletions(-)
>>
>> diff --git a/fs/fat/namei_vfat.c b/fs/fat/namei_vfat.c
>> index bb3f29c..cf0fd96 100644
>> --- a/fs/fat/namei_vfat.c
>> +++ b/fs/fat/namei_vfat.c
>> @@ -21,6 +21,8 @@
>> Â#include <linux/slab.h>
>> Â#include <linux/buffer_head.h>
>> Â#include <linux/namei.h>
>> +#include <linux/kernel.h>
>> +
>> Â#include "fat.h"
>>
>> Â/*
>> @@ -505,10 +507,8 @@ 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, rc, fill;
>> Â Â Â int charlen;
>>
>> Â Â Â if (utf8) {
>> @@ -528,26 +528,12 @@ 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;
>> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â }
>> +
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â rc = hex2bin(op, ip + 1, 2);
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â if (rc < 0)
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â return -EINVAL;
>> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â }
>> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â *op++ = ec & 0xFF;
>> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â *op++ = ec >> 8;
Actually here we will change endianess.
So, my question is what endianess is right here? If the original code
okay, then patch should be rewritten like this:

     rc = hex2bin(op++, ip + 3, 1);
     if (rc < 0)
          return -EINVAL;

     rc = hex2bin(op++, ip + 1, 1);
     if (rc < 0)
          return -EINVAL;

>> +
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â op += 2;
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ip += 5;
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â i += 5;
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â } else {
>
> --
> OGAWA Hirofumi <hirofumi@xxxxxxxxxxxxxxxxxx>
> --
> 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/
>



-- 
With Best Regards,
Andy Shevchenko
èº{.nÇ+‰·Ÿ®‰­†+%ŠËlzwm…ébëæìr¸›zX§»®w¥Š{ayºÊÚë,j­¢f£¢·hš‹àz¹®w¥¢¸¢·¦j:+v‰¨ŠwèjØm¶Ÿÿ¾«‘êçzZ+ƒùšŽŠÝj"ú!¶iO•æ¬z·švØ^¶m§ÿðÃnÆàþY&—