From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751989AbdHAM4t (ORCPT ); Tue, 1 Aug 2017 08:56:49 -0400 Received: from mga03.intel.com ([134.134.136.65]:21171 "EHLO mga03.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751217AbdHAM4s (ORCPT ); Tue, 1 Aug 2017 08:56:48 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.41,306,1498546800"; d="scan'208";a="1157701309" Message-ID: <1501592205.29303.320.camel@linux.intel.com> Subject: Re: [PATCH v5] vfat: Deduplicate hex2bin() From: Andy Shevchenko To: OGAWA Hirofumi , Andy Shevchenko Cc: Andrew Morton , "linux-kernel@vger.kernel.org" , Joe Perches Date: Tue, 01 Aug 2017 15:56:45 +0300 In-Reply-To: <87vam8tjim.fsf@devron> References: <20170731143349.84629-1-andriy.shevchenko@linux.intel.com> <877eyov49l.fsf@devron> <87zibktpil.fsf@devron> <87vam8tjim.fsf@devron> Organization: Intel Finland Oy Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.22.6-1 Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, 2017-08-01 at 05:54 +0900, OGAWA Hirofumi wrote: > Andy Shevchenko 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 Intel Finland Oy