Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1334896 > unrolled thread

Re: checkpatch falsepositives in Lustre code

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: checkpatch falsepositives in Lustre code Joe Perches <joe@perches.com> - 2016-02-16 02:00 +0100
    Re: checkpatch falsepositives in Lustre code Oleg Drokin <green@linuxhacker.ru> - 2016-02-16 03:00 +0100
      Re: checkpatch falsepositives in Lustre code Joe Perches <joe@perches.com> - 2016-02-16 03:30 +0100
        Re: checkpatch falsepositives in Lustre code Oleg Drokin <green@linuxhacker.ru> - 2016-02-16 03:50 +0100
          Re: checkpatch falsepositives in Lustre code Joe Perches <joe@perches.com> - 2016-02-16 04:10 +0100
            Re: checkpatch falsepositives in Lustre code Oleg Drokin <green@linuxhacker.ru> - 2016-02-16 04:20 +0100

#1334896 — Re: checkpatch falsepositives in Lustre code

On Mon, 2016-02-15 at 18:49 -0500, Oleg Drokin wrote:
> Hello!
> 
> 
>    As I am going over Lustre to clean up the code style, I noticed this bunch below.
> 
>    Those all are function definitions, though I guess it might have been foiled by
>    return type on the previous line?
>    Now sure if anything could be done about this.
> 
>    Thanks.
> 
> ERROR: that open brace { should be on the previous line 
> #2098: FILE: drivers/staging/lustre/lustre/libcfs/hash.c:1358:
> +cfs_ash_for_each_enter(struct cfs_hash *hs)
> +{

[etc...]

Yeah, that's a defect of some type.

I'm not sure if it's really possible to handle it well though.

Maybe there could be a test added for something like

"^[\+ ](?:$Declare\s*|DeclareMisordered\s*)?\$Ident\("

to find what looks like function declarations in the
first column to avoid some of these false positives.

Andy?

[toc] | [next] | [standalone]

#1334916

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.

[toc] | [prev] | [next] | [standalone]

#1334949

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.

$ git grep -E "\bspinlock_t\s*\*\s*\w+\s*[=;]" | wc -l
327

~10% of them seem to have in-line comments.

$ git grep -E "\bspinlock_t\s*\*\s*\w+\s*[=;].*/\*" | wc -l
34

and just fyi, here's a top level directory breakdown:

$ git grep -E "\bspinlock_t\s*\*\s*\w+\s*[=;]" | cut -f1 -d"/" | uniq -c
      1 Documentation
     27 arch
      1 block
    119 drivers
     24 fs
     23 include
      5 kernel
      3 lib
     67 mm
     51 net
      4 security
      2 sound

[toc] | [prev] | [next] | [standalone]

#1334963

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?

[toc] | [prev] | [next] | [standalone]

#1334977

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.

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

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

[toc] | [prev] | [next] | [standalone]

#1334979

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web