From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933168AbcBDXhG (ORCPT ); Thu, 4 Feb 2016 18:37:06 -0500 Received: from mail-wm0-f47.google.com ([74.125.82.47]:35223 "EHLO mail-wm0-f47.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932783AbcBDXhE (ORCPT ); Thu, 4 Feb 2016 18:37:04 -0500 From: Rasmus Villemoes To: Andrzej Hajda Cc: Andrew Morton , Bartlomiej Zolnierkiewicz , Marek Szyprowski , linux-kernel@vger.kernel.org (open list) Subject: Re: [PATCH v3] err.h: allow IS_ERR_VALUE to handle properly more types Organization: D03 References: <20160202163350.f7d42f4b97f48756f3900e9a@linux-foundation.org> <1454505328-26019-1-git-send-email-a.hajda@samsung.com> X-Hashcash: 1:20:160204:linux-kernel@vger.kernel.org::dyFCnuejC78ZwHke:0000000000000000000000000000000000PJn X-Hashcash: 1:20:160204:a.hajda@samsung.com::7y16crxqernQpaJE:0000000000000000000000000000000000000000001s81 X-Hashcash: 1:20:160204:akpm@linux-foundation.org::yMHXfRmVg5s5W0fx:0000000000000000000000000000000000003T8P X-Hashcash: 1:20:160204:m.szyprowski@samsung.com::qhp00YDXql3PQR1h:00000000000000000000000000000000000005yNq X-Hashcash: 1:20:160204:b.zolnierkie@samsung.com::Q7t3tRGhnfzN0/Ov:0000000000000000000000000000000000000Gf8A Date: Fri, 05 Feb 2016 00:37:00 +0100 In-Reply-To: <1454505328-26019-1-git-send-email-a.hajda@samsung.com> (Andrzej Hajda's message of "Wed, 03 Feb 2016 14:15:28 +0100") Message-ID: <877fiknj37.fsf@rasmusvillemoes.dk> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Feb 03 2016, Andrzej Hajda wrote: > Current implementation of IS_ERR_VALUE works correctly only with > following types: > - unsigned long, > - short, int, long. > Other types are handled incorrectly either on 32-bit either on 64-bit > either on both architectures. > The patch fixes it by comparing argument with MAX_ERRNO casted > to argument's type for unsigned types and comparing with zero for signed > types. As a result all integer types bigger than char are handled properly. > > > Signed-off-by: Andrzej Hajda > --- > v3: > - use '<= -1' instead of '< 0' to silence verbose warnings for gcc > older than 4.8, > v2: > - use '<= 0' instead of '< 0' to silence gcc verbose warnings, > - expand commit message. > --- > include/linux/err.h | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/include/linux/err.h b/include/linux/err.h > index 56762ab..b7d4a9f 100644 > --- a/include/linux/err.h > +++ b/include/linux/err.h > @@ -18,7 +18,9 @@ > > #ifndef __ASSEMBLY__ > > -#define IS_ERR_VALUE(x) unlikely((x) >= (unsigned long)-MAX_ERRNO) > +#define IS_ERR_VALUE(x) ((typeof(x))(-1) <= 0 \ > + ? unlikely((x) <= -1) \ > + : unlikely((x) >= (typeof(x))-MAX_ERRNO)) > I'm a bit worried that you consider any negative value an error when x is signed - at least that's a change which deserves some comment why that's ok. For example, I could imagine someone using e.g. INT_MIN as a sentinel return value meaning 'not an error, but something special still'. I think that, since we're there, one should stick in a BUILD_BUG_ON to catch people passing in a u8 or s8. Something that seems that it would work for all (wide enough) types is ({ typeof(x) _x = (x); BUILD_BUG_ON(sizeof(_x) < 2); unlikely(_x >= (typeof(x))-MAX_ERRNO && _x <= (typeof(x))-1); }) Whether x is signed or not we want the first condition. If x is unsigned and at least as wide as int, the -1 in the second condition will be promoted to the max value typeof(x) can hold, so gcc should trivially remove the check, and if x is signed, this keeps the previous semantics. It's also a pattern I expect gcc to be able to optimize pretty well (that is, if x has type signed int, I expect it'll recognize that these tests are better done as a single unsigned comparison). Maybe it'll still give a -Wtype-limit warning for the unsigned case. If we go with the above, maybe one should also stick in something which ensures x is not a pointer; (void)(_x+_x); should do it. Rasmus