Re: [PATCH 3/4] ndctl: switch to tools/include/linux/{kernel, list, bitmap}.h

[Dan Williams]

On Tue, Jul 25, 2017 at 4:55 PM, Arnaldo Carvalho de Melo
<acme@xxxxxxxxxx> wrote:
> Em Tue, Jul 25, 2017 at 03:36:26PM -0700, Dan Williams escreveu:
>> Replace the ccan implementation of list primitives, bitmap helpers and
>> small utility macros with the common definitions available in
>> tool/include/linux.
>
> You should first add what you need in separate patches, paving the way
> to then use it, and some stuff are already there, see below:

Ok, I'll break out those changes separately.

>
>> diff --git a/tools/include/linux/hashtable.h b/tools/include/linux/hashtable.h
>> index c65cc0aa2659..251eabf2a05e 100644
>> --- a/tools/include/linux/hashtable.h
>> +++ b/tools/include/linux/hashtable.h
>> @@ -13,10 +13,6 @@
>>  #include <linux/hash.h>
>>  #include <linux/log2.h>
>>
>> -#ifndef ARRAY_SIZE
>> -#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
>> -#endif
>> -
>
> This is already tools/include/linux/kernel.h as
>
> #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
>
> It was moved by:
>
> commit 68289cbd83eaa20faef7cc818121bc8e769065de
> Author: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
> Date:   Mon Apr 17 12:01:36 2017 -0300
>
>     tools include: Drop ARRAY_SIZE() definition from linux/hashtable.h
>
>     As tools/include/linux/kernel.h has it now, with the goodies present in
>     the kernel.h counterpart, i.e. checking that the parameter is an array
>     at build time.
>
> --------------------
>
>
>>  #define DEFINE_HASHTABLE(name, bits)                                         \
>>       struct hlist_head name[1 << (bits)] =                                   \
>>                       { [0 ... ((1 << (bits)) - 1)] = HLIST_HEAD_INIT }
>> diff --git a/tools/include/linux/kernel.h b/tools/include/linux/kernel.h
>> index 28607db02bd3..ebe5211ce086 100644
>> --- a/tools/include/linux/kernel.h
>> +++ b/tools/include/linux/kernel.h
>> @@ -45,6 +45,10 @@
>>       _min1 < _min2 ? _min1 : _min2; })
>>  #endif
>>
>> +#ifndef clamp
>> +#define clamp(v, f, c) (max(min((v), (c)), (f)))
>> +#endif
>> +
>
> The include/linux/kernel.h definition is:
>
>
> /**
>  * clamp - return a value clamped to a given range with strict typechecking
>  * @val: current value
>  * @lo: lowest allowable value
>  * @hi: highest allowable value
>  *
>  * This macro does strict typechecking of lo/hi to make sure they are of the
>  * same type as val.  See the unnecessary pointer comparisons.
>  */
> #define clamp(val, lo, hi) min((typeof(val))max(val, lo), hi)
>
> I suggest we try to track the kernel counterparts as much as possible, and
> put the definitions in the same order as in the kernel header, this way we
> can even look at how different they are using plain old 'diff'.

Sounds good.

>
>>  #ifndef roundup
>>  #define roundup(x, y) (                                \
>>  {                                                      \
>> @@ -54,6 +58,10 @@
>>  )
>>  #endif
>>
>> +#ifndef ARRAY_SIZE
>> +#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
>> +#endif
>> +
>>  #ifndef BUG_ON
>>  #ifdef NDEBUG
>>  #define BUG_ON(cond) do { if (cond) {} } while (0)
>> @@ -66,8 +74,10 @@
>>   * Both need more care to handle endianness
>>   * (Don't use bitmap_copy_le() for now)
>>   */
>> +#ifndef cpu_to_le64
>>  #define cpu_to_le64(x)       (x)
>>  #define cpu_to_le32(x)       (x)
>> +#endif
>
> This will not apply, there were changes here as well.

I should have mentioned that this tree was based on 4.10. I picked an
old merge base on the thought that it would reduce the chance of
people bisecting 4.13..4.14-rc1 ending up in the initial ndctl history
where only tools/ndctl/ exists. But, if I want to touch kernel.h this
old baseline is not going to work...

I'll play around with git bisect to see if the old merge base is
actually worth it, and roll forward otherwise.