* 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