Messages in this thread | | | Subject | Re: [PATCH v2] err.h: allow IS_ERR_VALUE to handle properly more types | From | Andrzej Hajda <> | Date | Tue, 02 Feb 2016 09:22:54 +0100 |
| |
On 02/02/2016 07:23 AM, Andrew Morton wrote: > On Thu, 28 Jan 2016 09:27:28 +0100 Andrzej Hajda <a.hajda@samsung.com> 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. >> >> I have analyzed usage of IS_ERR_VALUE using coccinelle and in about 35 >> cases it is used incorrectly, ie it can hide errors depending of 32/64 bit >> architecture. Instead of fixing usage I propose to enhance the macro >> to cover more types. >> And just for the record: the macro is used 101 times with signed variables, >> I am not sure if it should be preferred over simple comparison "ret < 0", >> but the new version can do it as well. >> >> And below list of detected potential errors: >> >> ... >> >> --- 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) < 0) \ >> + : unlikely((x) >= (typeof(x))-MAX_ERRNO)) >> > hm, seems complicated. Can we simply cast the value to long? > > #define IS_ERR_VALUE(x) ((long)x < 0) && (long)x >= (long)-MAX_ERRNO) > > and simplify that to > > #define IS_ERR_VALUE(x) ((unsigned long)(long)x >= (unsigned long)-MAX_ERRNO) > > or something like that. It will not work with u32 on 64bit systems.
Short rationales behind my implementation:
1. Typical usage pattern of the macro looks like: T x; ... x = -ESOME_ERROR; ... if (IS_ERR_VALUE(x)) ...
In error assignment we have casting of -ESOME_ERROR to type T. Casting of -MAX_ERRNO to the same type in the macro assures that comparison will be sane, at least for types big enough. In short we ends at following expression (for unsigned types): (T)-ESOME_ERROR >= (T)-MAX_ERRNO In old implementation we ended at: (unsigned)(T)-ESOME_ERROR >= (unsigned)-MAX_ERRNO Different castings for -ESOME_ERROR and for -MAX_ERRNO makes this comparison incorrect for some types T.
2. Error checking is completely different for signed and unsigned vars: a. signed are compared to 0: ret < 0. b. unsigned are compared with some high value: ret >= (-MAX_ERRNO). This dualism is clearly visible and emphasized in this implementation. In old implementation IS_ERR_VALUE works correctly for some signed types due to obscure C casting rules.
Summarizing: current implementation is short but tricky, answering why it works/fails for certain types is quite challenging. On the other side proposed implementation is longer but more straightforward, and of course is correct for more types :)
Maybe, to make it more clear, it could be good to use separate macro for signedness:
#define IS_SIGNED_TYPE(t) ((t)(-1) <= 0)
#define IS_ERR_VALUE(x) (IS_SIGNED_TYPE(typeof(x)) \ ? unlikely((x) < 0) \ : unlikely((x) >= (typeof(x))-MAX_ERRNO)) Regards Andrzej
| |