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

Groups > linux.kernel > #1701009

Re: [PATCH v5] vfat: Deduplicate hex2bin()

Show all headers | View raw

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

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[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

csiph-web