LKML Archive on lore.kernel.org
 help / color / Atom feed
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>,
	Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Joe Perches <joe@perches.com>
Subject: Re: [PATCH v5] vfat: Deduplicate hex2bin()
Date: Tue, 01 Aug 2017 15:56:45 +0300
Message-ID: <1501592205.29303.320.camel@linux.intel.com> (raw)
In-Reply-To: <87vam8tjim.fsf@devron>

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

  reply index

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-07-31 14:33 Andy Shevchenko
2017-07-31 18:40 ` OGAWA Hirofumi
2017-07-31 18:44   ` OGAWA Hirofumi
2017-07-31 19:31     ` Andy Shevchenko
2017-07-31 20:54       ` OGAWA Hirofumi
2017-08-01 12:56         ` Andy Shevchenko [this message]
2017-07-31 19:39   ` Andy Shevchenko
2017-07-31 20:56     ` OGAWA Hirofumi

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1501592205.29303.320.camel@linux.intel.com \
    --to=andriy.shevchenko@linux.intel.com \
    --cc=akpm@linux-foundation.org \
    --cc=andy.shevchenko@gmail.com \
    --cc=hirofumi@mail.parknet.co.jp \
    --cc=joe@perches.com \
    --cc=linux-kernel@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git
	git clone --mirror https://lore.kernel.org/lkml/9 lkml/git/9.git
	git clone --mirror https://lore.kernel.org/lkml/10 lkml/git/10.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git