Re: checkpatch falsepositives in Lustre code

[Oleg Drokin]

On Feb 15, 2016, at 10:05 PM, Joe Perches wrote:

> On Mon, 2016-02-15 at 21:45 -0500, Oleg Drokin wrote:
>> On Feb 15, 2016, at 9:27 PM, Joe Perches wrote:
>> 
>>> On Mon, 2016-02-15 at 20:57 -0500, Oleg Drokin wrote:
>>>> On Feb 15, 2016, at 7:56 PM, Joe Perches wrote:
>>>>> [etc...]
>>>>> 
>>>>> Yeah, that's a defect of some type.
>>>> 
>>>> Also while I have your attention, here's another one:
>>>> 
>>>> struct cfs_percpt_lock *
>>>> cfs_percpt_lock_alloc(struct cfs_cpt_table *cptab)
>>>> {
>>>>         struct cfs_percpt_lock  *pcl;
>>>>         spinlock_t              *lock;
>>>>         int                     i;
>>>> …
>>>>         cfs_percpt_for_each(lock, i, pcl->pcl_locks)
>>>>                 spin_lock_init(lock);
>>>> 
>>>> The declaration of the spinlock pointer produces:
>>>> CHECK: spinlock_t definition without comment
>>>> 
>>>> Should spinlock pointers really be included in the check, it's obvious that
>>>> they themselves are not really protecting anything, esp. considering it's a
>>>> local function variable here.
>>> 
>>> I don't have an opinion here.
>>> 
>>> spinlock_t pointers are relatively rare.
>> 
>> I guess they are. And I understand why you would want a comment for the actual
>> spinlock, but pointexr - much less so.
>> 
>> Anyway, I have some more questions:
>> 
>> ERROR: Macros with complex values should be enclosed in parentheses
>> #8720: FILE: drivers/staging/lustre/lustre/libcfs/tracefile.h:189:
>> +#define cfs_tcd_for_each(tcd, i, j)                                   \
>> +       for (i = 0; cfs_trace_data[i]; i++)                             \
>> +               for (j = 0, ((tcd) = &(*cfs_trace_data[i])[j].tcd);     \
>> +                    j < num_possible_cpus();                            \
>> +                    j++, (tcd) = &(*cfs_trace_data[i])[j].tcd)
>> 
>> This is a macros with complex value alright, but the whole idea of this one
>> is to not be enclosed. Any ideas about this one and similar?
> 
> checkpatch is brainless script and a not a real parser.
> Ignoring its stupid and incorrect messages is a good idea.

It also asks to notify the authors ;)
I guess this could be ignored too, but since Lustre lives in staging
on the condition of improving its code style, I wanted to at least
give it a good go and clean up as much stuff as makes sense.

> fyi: There are many of these messages that exist like below.

"define.*for_each" seems to be a recurring theme?

> 
> I can't think of a reasonable way to automatically identify
> and not show the defective error messages for these.  Andy?
> 
> ---
> 
> ERROR: Macros with complex values should be enclosed in parentheses
> #86: FILE: include/linux/dmar.h:86:
> +#define for_each_active_drhd_unit(drhd)					\
> +	list_for_each_entry_rcu(drhd, &dmar_drhd_units, list)		\
> +		if (drhd->ignored) {} else
> 
> ERROR: Macros with complex values should be enclosed in parentheses
> #90: FILE: include/linux/dmar.h:90:
> +#define for_each_active_iommu(i, drhd)					\
> +	list_for_each_entry_rcu(drhd, &dmar_drhd_units, list)		\
> +		if (i=drhd->iommu, drhd->ignored) {} else
> 
> ERROR: Macros with complex values should be enclosed in parentheses
> #94: FILE: include/linux/dmar.h:94:
> +#define for_each_iommu(i, drhd)						\
> +	list_for_each_entry_rcu(drhd, &dmar_drhd_units, list)		\
> +		if (i=drhd->iommu, 0) {} else 
> 
> ERROR: Macros with complex values should be enclosed in parentheses
> #110: FILE: include/linux/dmar.h:110:
> +#define	for_each_active_dev_scope(a, c, p, d)	\
> +	for_each_dev_scope((a), (c), (p), (d))	if (!(d)) { continue; } else
> 
> total: 4 errors, 0 warnings, 285 lines checked