* Re: checkpatch falsepositives in Lustre code
[not found] <1E5E2198-2E5C-4B6F-AAA5-C28E0A776714@linuxhacker.ru>
@ 2016-02-16 0:56 ` Joe Perches
2016-02-16 1:57 ` Oleg Drokin
0 siblings, 1 reply; 6+ messages in thread
From: Joe Perches @ 2016-02-16 0:56 UTC (permalink / raw)
To: Oleg Drokin, Andy Whitcroft; +Cc: LKML
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?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: checkpatch falsepositives in Lustre code
2016-02-16 0:56 ` checkpatch falsepositives in Lustre code Joe Perches
@ 2016-02-16 1:57 ` Oleg Drokin
2016-02-16 2:27 ` Joe Perches
0 siblings, 1 reply; 6+ messages in thread
From: Oleg Drokin @ 2016-02-16 1:57 UTC (permalink / raw)
To: Joe Perches; +Cc: Andy Whitcroft, LKML
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: checkpatch falsepositives in Lustre code
2016-02-16 1:57 ` Oleg Drokin
@ 2016-02-16 2:27 ` Joe Perches
2016-02-16 2:45 ` Oleg Drokin
0 siblings, 1 reply; 6+ messages in thread
From: Joe Perches @ 2016-02-16 2:27 UTC (permalink / raw)
To: Oleg Drokin; +Cc: Andy Whitcroft, LKML, Andrew Morton
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: checkpatch falsepositives in Lustre code
2016-02-16 2:27 ` Joe Perches
@ 2016-02-16 2:45 ` Oleg Drokin
2016-02-16 3:05 ` Joe Perches
0 siblings, 1 reply; 6+ messages in thread
From: Oleg Drokin @ 2016-02-16 2:45 UTC (permalink / raw)
To: Joe Perches; +Cc: Andy Whitcroft, LKML, Andrew Morton
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?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: checkpatch falsepositives in Lustre code
2016-02-16 2:45 ` Oleg Drokin
@ 2016-02-16 3:05 ` Joe Perches
2016-02-16 3:12 ` Oleg Drokin
0 siblings, 1 reply; 6+ messages in thread
From: Joe Perches @ 2016-02-16 3:05 UTC (permalink / raw)
To: Oleg Drokin; +Cc: Andy Whitcroft, LKML, Andrew Morton
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: checkpatch falsepositives in Lustre code
2016-02-16 3:05 ` Joe Perches
@ 2016-02-16 3:12 ` Oleg Drokin
0 siblings, 0 replies; 6+ messages in thread
From: Oleg Drokin @ 2016-02-16 3:12 UTC (permalink / raw)
To: Joe Perches; +Cc: Andy Whitcroft, LKML, Andrew Morton
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-02-16 3:12 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1E5E2198-2E5C-4B6F-AAA5-C28E0A776714@linuxhacker.ru>
2016-02-16 0:56 ` checkpatch falsepositives in Lustre code Joe Perches
2016-02-16 1:57 ` Oleg Drokin
2016-02-16 2:27 ` Joe Perches
2016-02-16 2:45 ` Oleg Drokin
2016-02-16 3:05 ` Joe Perches
2016-02-16 3:12 ` Oleg Drokin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox