* Re: [PATCH 6/7] staging: lustre: obdclass: expand the GOTO macro + break
@ 2014-09-09 12:54 ` Dan Carpenter
0 siblings, 0 replies; 19+ messages in thread
From: Dan Carpenter @ 2014-09-09 12:54 UTC (permalink / raw)
To: Julia Lawall
Cc: Greg Kroah-Hartman, devel, Dilger, Andreas, Peng Tao,
kernel-janitors, linux-kernel, Drokin, Oleg
On Sun, Sep 07, 2014 at 06:18:34PM +0200, Julia Lawall wrote:
> diff --git a/drivers/staging/lustre/lustre/obdclass/obd_config.c b/drivers/staging/lustre/lustre/obdclass/obd_config.c
> index f41695d..8a9752f 100644
> --- a/drivers/staging/lustre/lustre/obdclass/obd_config.c
> +++ b/drivers/staging/lustre/lustre/obdclass/obd_config.c
> @@ -1226,25 +1226,25 @@ int class_process_config(struct lustre_cfg *lcfg)
> }
> case LCFG_POOL_NEW: {
> err = obd_pool_new(obd, lustre_cfg_string(lcfg, 2));
> - GOTO(out, err = 0);
> - break;
> + err = 0;
> + goto out;
[ Warning: this email is long and not related to your code. It's just
the frustrations of dealing with lustre. ]
So now the code reads:
err = obd_pool_new(obd, lustre_cfg_string(lcfg, 2));
err = 0;
That was there in the original code, and your patch is correct to leave
the suspicous looking code as is. We used to have a GCC warning for
this but the linux kernel source has too much bad code so we had to
disable the warning.
I wonder what happens if obd_pool_new() fails? Unfortunately "make
cscope" and "vim -t obd_pool_new" doesn't work with lustre so you have
to grep for it.
grep obd_pool_new drivers/staging/lustre/ -R |egrep '\.[ch]:'
This is the only caller so we can't compare with the other callers to
see if they check the return value.
Here is how the obd_pool_new() function is implemented.
1051 static inline int obd_pool_new(struct obd_device *obd, char *poolname)
1052 {
1053 int rc;
1054
1055 OBD_CHECK_DT_OP(obd, pool_new, -EOPNOTSUPP);
1056 OBD_COUNTER_INCREMENT(obd, pool_new);
1057
1058 rc = OBP(obd, pool_new)(obd, poolname);
1059 return rc;
1060 }
This whole function is just macros. Let's see what they do:
460 #define OBD_CHECK_DT_OP(obd, op, err) \
461 do { \
462 if (!OBT(obd) || !OBP((obd), op)) { \
463 if (err) \
464 CERROR("obd_" #op ": dev %d no operation\n", \
465 obd->obd_minor); \
466 return err; \
467 } \
468 } while (0)
Wow! What a terrible macro! None of the '\' are in a line. There is
a hidden return statement in it which is a terrible thing and flow
control statements are not allowed inside macros.
I can't tell what OBT() and OBP() because the names are very ambiguous.
328 #define OBT(dev) (dev)->obd_type
329 #define OBP(dev, op) (dev)->obd_type->typ_dt_ops->o_ ## op
330 #define MDP(dev, op) (dev)->obd_type->typ_md_ops->m_ ## op
Ok. "OB" stands for "obd". T stands for "type". The "P" probably
stands for pointer or operation. MD is clear enough.
The OBP() macro adds an "o_" to the function pointer and the MDP() macro
adds an "m_". That totally sucks because it makes the function pointer
hard to grep for. There isn't another explanation, whoever wrote this
code is just being ornery.
Summary so far: OBD_CHECK_DT_OP() checks to see if a function pointer
is NULL.
Let's see what OBD_COUNTER_INCREMENT() does.
361 #define OBD_COUNTER_INCREMENT(obdx, op) \
362 if ((obdx)->obd_stats != NULL) { \
363 unsigned int coffset; \
364 coffset = (unsigned int)((obdx)->obd_cntr_base) + \
365 OBD_COUNTER_OFFSET(op); \
366 LASSERT(coffset < (obdx)->obd_stats->ls_num); \
367 lprocfs_counter_incr((obdx)->obd_stats, coffset); \
368 }
369
That's a densely packed block of messy code but it basically does what
you'd expect from the name. Fair enough.
So the obd_pool_new() function verifies that ->o_pool_new is non-NULL,
it increments a counter and calls ->o_pool_new(). Let's take a look at
which functions implement ->o_pool_new().
grep -w o_pool_new drivers/staging/lustre/ -R | egrep '\.[ch]:'
The only implementation of this function is lov_pool_new(). Why do
we have a function pointer if there is only one implementation? We
should remove this.
The lov_pool_new() function definitely can fail unexpectedly with
-ENOMEM.
Let's go back to the original code and see how the error should be
handled... Oh... Apparently this is an optional thing so we would
just ignore the error and continue. The code is fine.
In any other subsystem it would have taken 30 seconds to read the code
because cscope would work and there wouldn't be the layers of
indirection.
I'm not convinced that having a function counter for calls to
lov_pool_new() is useful. We could get the same information from ftrace
or other tools. In my view, we could get rid of all the horrible macros
and the function pointers and the splitting names into half and the
layers of indirection and the debugging code and just call
lov_pool_new() directly.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 6/7] staging: lustre: obdclass: expand the GOTO macro + break
2014-09-09 12:54 ` Dan Carpenter
@ 2014-09-09 13:05 ` Julia Lawall
-1 siblings, 0 replies; 19+ messages in thread
From: Julia Lawall @ 2014-09-09 13:05 UTC (permalink / raw)
To: Dan Carpenter
Cc: Julia Lawall, Greg Kroah-Hartman, devel, Dilger, Andreas,
Peng Tao, kernel-janitors, linux-kernel, Drokin, Oleg
On Tue, 9 Sep 2014, Dan Carpenter wrote:
> On Sun, Sep 07, 2014 at 06:18:34PM +0200, Julia Lawall wrote:
> > diff --git a/drivers/staging/lustre/lustre/obdclass/obd_config.c b/drivers/staging/lustre/lustre/obdclass/obd_config.c
> > index f41695d..8a9752f 100644
> > --- a/drivers/staging/lustre/lustre/obdclass/obd_config.c
> > +++ b/drivers/staging/lustre/lustre/obdclass/obd_config.c
> > @@ -1226,25 +1226,25 @@ int class_process_config(struct lustre_cfg *lcfg)
> > }
> > case LCFG_POOL_NEW: {
> > err = obd_pool_new(obd, lustre_cfg_string(lcfg, 2));
> > - GOTO(out, err = 0);
> > - break;
> > + err = 0;
> > + goto out;
>
> [ Warning: this email is long and not related to your code. It's just
> the frustrations of dealing with lustre. ]
>
> So now the code reads:
>
> err = obd_pool_new(obd, lustre_cfg_string(lcfg, 2));
> err = 0;
>
> That was there in the original code, and your patch is correct to leave
> the suspicous looking code as is. We used to have a GCC warning for
> this but the linux kernel source has too much bad code so we had to
> disable the warning.
>
> I wonder what happens if obd_pool_new() fails? Unfortunately "make
> cscope" and "vim -t obd_pool_new" doesn't work with lustre so you have
> to grep for it.
>
> grep obd_pool_new drivers/staging/lustre/ -R |egrep '\.[ch]:'
>
> This is the only caller so we can't compare with the other callers to
> see if they check the return value.
>
> Here is how the obd_pool_new() function is implemented.
>
> 1051 static inline int obd_pool_new(struct obd_device *obd, char *poolname)
> 1052 {
> 1053 int rc;
> 1054
> 1055 OBD_CHECK_DT_OP(obd, pool_new, -EOPNOTSUPP);
> 1056 OBD_COUNTER_INCREMENT(obd, pool_new);
> 1057
> 1058 rc = OBP(obd, pool_new)(obd, poolname);
> 1059 return rc;
> 1060 }
>
> This whole function is just macros. Let's see what they do:
>
> 460 #define OBD_CHECK_DT_OP(obd, op, err) \
> 461 do { \
> 462 if (!OBT(obd) || !OBP((obd), op)) { \
> 463 if (err) \
> 464 CERROR("obd_" #op ": dev %d no operation\n", \
> 465 obd->obd_minor); \
> 466 return err; \
> 467 } \
> 468 } while (0)
>
> Wow! What a terrible macro! None of the '\' are in a line. There is
> a hidden return statement in it which is a terrible thing and flow
> control statements are not allowed inside macros.
>
> I can't tell what OBT() and OBP() because the names are very ambiguous.
>
> 328 #define OBT(dev) (dev)->obd_type
> 329 #define OBP(dev, op) (dev)->obd_type->typ_dt_ops->o_ ## op
> 330 #define MDP(dev, op) (dev)->obd_type->typ_md_ops->m_ ## op
>
> Ok. "OB" stands for "obd". T stands for "type". The "P" probably
> stands for pointer or operation. MD is clear enough.
>
> The OBP() macro adds an "o_" to the function pointer and the MDP() macro
> adds an "m_". That totally sucks because it makes the function pointer
> hard to grep for. There isn't another explanation, whoever wrote this
> code is just being ornery.
>
> Summary so far: OBD_CHECK_DT_OP() checks to see if a function pointer
> is NULL.
>
> Let's see what OBD_COUNTER_INCREMENT() does.
>
> 361 #define OBD_COUNTER_INCREMENT(obdx, op) \
> 362 if ((obdx)->obd_stats != NULL) { \
> 363 unsigned int coffset; \
> 364 coffset = (unsigned int)((obdx)->obd_cntr_base) + \
> 365 OBD_COUNTER_OFFSET(op); \
> 366 LASSERT(coffset < (obdx)->obd_stats->ls_num); \
> 367 lprocfs_counter_incr((obdx)->obd_stats, coffset); \
> 368 }
> 369
>
> That's a densely packed block of messy code but it basically does what
> you'd expect from the name. Fair enough.
>
> So the obd_pool_new() function verifies that ->o_pool_new is non-NULL,
> it increments a counter and calls ->o_pool_new(). Let's take a look at
> which functions implement ->o_pool_new().
>
> grep -w o_pool_new drivers/staging/lustre/ -R | egrep '\.[ch]:'
>
> The only implementation of this function is lov_pool_new(). Why do
> we have a function pointer if there is only one implementation? We
> should remove this.
>
> The lov_pool_new() function definitely can fail unexpectedly with
> -ENOMEM.
>
> Let's go back to the original code and see how the error should be
> handled... Oh... Apparently this is an optional thing so we would
> just ignore the error and continue. The code is fine.
>
> In any other subsystem it would have taken 30 seconds to read the code
> because cscope would work and there wouldn't be the layers of
> indirection.
>
> I'm not convinced that having a function counter for calls to
> lov_pool_new() is useful. We could get the same information from ftrace
> or other tools. In my view, we could get rid of all the horrible macros
> and the function pointers and the splitting names into half and the
> layers of indirection and the debugging code and just call
> lov_pool_new() directly.
I will look into it. I can try to study up on several of the functions,
and submit patches making a few changes, to be sure that I have not gotten
rid of anything that seems important.
If anyone knows the code well enough to know some of these
transformations in advance, that would be very helpful.
julia
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 6/7] staging: lustre: obdclass: expand the GOTO macro + break
@ 2014-09-09 13:05 ` Julia Lawall
0 siblings, 0 replies; 19+ messages in thread
From: Julia Lawall @ 2014-09-09 13:05 UTC (permalink / raw)
To: Dan Carpenter
Cc: Julia Lawall, Greg Kroah-Hartman, devel, Dilger, Andreas,
Peng Tao, kernel-janitors, linux-kernel, Drokin, Oleg
On Tue, 9 Sep 2014, Dan Carpenter wrote:
> On Sun, Sep 07, 2014 at 06:18:34PM +0200, Julia Lawall wrote:
> > diff --git a/drivers/staging/lustre/lustre/obdclass/obd_config.c b/drivers/staging/lustre/lustre/obdclass/obd_config.c
> > index f41695d..8a9752f 100644
> > --- a/drivers/staging/lustre/lustre/obdclass/obd_config.c
> > +++ b/drivers/staging/lustre/lustre/obdclass/obd_config.c
> > @@ -1226,25 +1226,25 @@ int class_process_config(struct lustre_cfg *lcfg)
> > }
> > case LCFG_POOL_NEW: {
> > err = obd_pool_new(obd, lustre_cfg_string(lcfg, 2));
> > - GOTO(out, err = 0);
> > - break;
> > + err = 0;
> > + goto out;
>
> [ Warning: this email is long and not related to your code. It's just
> the frustrations of dealing with lustre. ]
>
> So now the code reads:
>
> err = obd_pool_new(obd, lustre_cfg_string(lcfg, 2));
> err = 0;
>
> That was there in the original code, and your patch is correct to leave
> the suspicous looking code as is. We used to have a GCC warning for
> this but the linux kernel source has too much bad code so we had to
> disable the warning.
>
> I wonder what happens if obd_pool_new() fails? Unfortunately "make
> cscope" and "vim -t obd_pool_new" doesn't work with lustre so you have
> to grep for it.
>
> grep obd_pool_new drivers/staging/lustre/ -R |egrep '\.[ch]:'
>
> This is the only caller so we can't compare with the other callers to
> see if they check the return value.
>
> Here is how the obd_pool_new() function is implemented.
>
> 1051 static inline int obd_pool_new(struct obd_device *obd, char *poolname)
> 1052 {
> 1053 int rc;
> 1054
> 1055 OBD_CHECK_DT_OP(obd, pool_new, -EOPNOTSUPP);
> 1056 OBD_COUNTER_INCREMENT(obd, pool_new);
> 1057
> 1058 rc = OBP(obd, pool_new)(obd, poolname);
> 1059 return rc;
> 1060 }
>
> This whole function is just macros. Let's see what they do:
>
> 460 #define OBD_CHECK_DT_OP(obd, op, err) \
> 461 do { \
> 462 if (!OBT(obd) || !OBP((obd), op)) { \
> 463 if (err) \
> 464 CERROR("obd_" #op ": dev %d no operation\n", \
> 465 obd->obd_minor); \
> 466 return err; \
> 467 } \
> 468 } while (0)
>
> Wow! What a terrible macro! None of the '\' are in a line. There is
> a hidden return statement in it which is a terrible thing and flow
> control statements are not allowed inside macros.
>
> I can't tell what OBT() and OBP() because the names are very ambiguous.
>
> 328 #define OBT(dev) (dev)->obd_type
> 329 #define OBP(dev, op) (dev)->obd_type->typ_dt_ops->o_ ## op
> 330 #define MDP(dev, op) (dev)->obd_type->typ_md_ops->m_ ## op
>
> Ok. "OB" stands for "obd". T stands for "type". The "P" probably
> stands for pointer or operation. MD is clear enough.
>
> The OBP() macro adds an "o_" to the function pointer and the MDP() macro
> adds an "m_". That totally sucks because it makes the function pointer
> hard to grep for. There isn't another explanation, whoever wrote this
> code is just being ornery.
>
> Summary so far: OBD_CHECK_DT_OP() checks to see if a function pointer
> is NULL.
>
> Let's see what OBD_COUNTER_INCREMENT() does.
>
> 361 #define OBD_COUNTER_INCREMENT(obdx, op) \
> 362 if ((obdx)->obd_stats != NULL) { \
> 363 unsigned int coffset; \
> 364 coffset = (unsigned int)((obdx)->obd_cntr_base) + \
> 365 OBD_COUNTER_OFFSET(op); \
> 366 LASSERT(coffset < (obdx)->obd_stats->ls_num); \
> 367 lprocfs_counter_incr((obdx)->obd_stats, coffset); \
> 368 }
> 369
>
> That's a densely packed block of messy code but it basically does what
> you'd expect from the name. Fair enough.
>
> So the obd_pool_new() function verifies that ->o_pool_new is non-NULL,
> it increments a counter and calls ->o_pool_new(). Let's take a look at
> which functions implement ->o_pool_new().
>
> grep -w o_pool_new drivers/staging/lustre/ -R | egrep '\.[ch]:'
>
> The only implementation of this function is lov_pool_new(). Why do
> we have a function pointer if there is only one implementation? We
> should remove this.
>
> The lov_pool_new() function definitely can fail unexpectedly with
> -ENOMEM.
>
> Let's go back to the original code and see how the error should be
> handled... Oh... Apparently this is an optional thing so we would
> just ignore the error and continue. The code is fine.
>
> In any other subsystem it would have taken 30 seconds to read the code
> because cscope would work and there wouldn't be the layers of
> indirection.
>
> I'm not convinced that having a function counter for calls to
> lov_pool_new() is useful. We could get the same information from ftrace
> or other tools. In my view, we could get rid of all the horrible macros
> and the function pointers and the splitting names into half and the
> layers of indirection and the debugging code and just call
> lov_pool_new() directly.
I will look into it. I can try to study up on several of the functions,
and submit patches making a few changes, to be sure that I have not gotten
rid of anything that seems important.
If anyone knows the code well enough to know some of these
transformations in advance, that would be very helpful.
julia
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 6/7] staging: lustre: obdclass: expand the GOTO macro + break
2014-09-09 12:54 ` Dan Carpenter
@ 2014-09-09 13:37 ` Drokin, Oleg
-1 siblings, 0 replies; 19+ messages in thread
From: Drokin, Oleg @ 2014-09-09 13:37 UTC (permalink / raw)
To: Dan Carpenter
Cc: Julia Lawall, Greg Kroah-Hartman, devel, Dilger, Andreas,
Peng Tao, kernel-janitors, Linux Kernel Mailing List, Hammond,
John
Hello!
On Sep 9, 2014, at 8:54 AM, Dan Carpenter wrote:
> 460 #define OBD_CHECK_DT_OP(obd, op, err) \
> 461 do { \
> 462 if (!OBT(obd) || !OBP((obd), op)) { \
> 463 if (err) \
> 464 CERROR("obd_" #op ": dev %d no operation\n", \
> 465 obd->obd_minor); \
> 466 return err; \
> 467 } \
> 468 } while (0)
>
> Wow! What a terrible macro! None of the '\' are in a line. There is
This is most likely an artifact of the way spaces were converted to tabs automatically by somebody.
> a hidden return statement in it which is a terrible thing and flow
> control statements are not allowed inside macros.
>
> I can't tell what OBT() and OBP() because the names are very ambiguous.
>
> 328 #define OBT(dev) (dev)->obd_type
> 329 #define OBP(dev, op) (dev)->obd_type->typ_dt_ops->o_ ## op
> 330 #define MDP(dev, op) (dev)->obd_type->typ_md_ops->m_ ## op
>
> Ok. "OB" stands for "obd". T stands for "type". The "P" probably
> stands for pointer or operation. MD is clear enough.
>
> The OBP() macro adds an "o_" to the function pointer and the MDP() macro
> adds an "m_". That totally sucks because it makes the function pointer
> hard to grep for. There isn't another explanation, whoever wrote this
> code is just being ornery.
I guess I just became numb to this sort of thing over time dealing
with those functions (that frequently irritated me too at first, I guess.)
> So the obd_pool_new() function verifies that ->o_pool_new is non-NULL,
> it increments a counter and calls ->o_pool_new(). Let's take a look at
> which functions implement ->o_pool_new().
>
> grep -w o_pool_new drivers/staging/lustre/ -R | egrep '\.[ch]:'
>
> The only implementation of this function is lov_pool_new(). Why do
> we have a function pointer if there is only one implementation? We
> should remove this.
Well, of course in reality you are only looking at half the code (the client),
and client and server share quite a bit of common glue (used to share
if we are talking about upstream client). There are other implementors of
this method on the server side (not that we care about it here).
There all these different obd layers that do similar operations and
could be (or used to be able to be) stacked on top of each able.
Old (long abandoned idea) was such that you should be able to stack these layers
in almost any order on each other. That never worked out because there
were ever so slight changes to stuff you pass down to the next layer.
But all the supporting infrastructure remained in place. Some of it
still being used and some being there since old times even though
layer order became fixed. After working with lustre sufficiently long
it's just engraved into your head that e.g. "oh, llite always calls into
lov_xxx and lov always calls into osc_xxx via these obd macroses.
Still on the other hand obd is really like an object, it not only has
the methods called in this convoluted manner, but also data fields.
It needs to be seen if things will become even worse if we start calling
these methods directly, and pass around strangely derived structure pointers
with data.
> The lov_pool_new() function definitely can fail unexpectedly with
> -ENOMEM.
>
> Let's go back to the original code and see how the error should be
> handled... Oh... Apparently this is an optional thing so we would
> just ignore the error and continue. The code is fine.
>
> In any other subsystem it would have taken 30 seconds to read the code
> because cscope would work and there wouldn't be the layers of
> indirection.
>
> I'm not convinced that having a function counter for calls to
> lov_pool_new() is useful. We could get the same information from ftrace
> or other tools. In my view, we could get rid of all the horrible macros
The counter thing was already noticed by Greg separately and it's in the TODO
list for the perf tracking integration of some sort to replace the obd calls
accounting (see - the "object" benefit, you implement an obd method and
magically there's all this call tracking and other goodies for you that
you can use without even thinking).
> and the function pointers and the splitting names into half and the
> layers of indirection and the debugging code and just call
> lov_pool_new() directly.
I guess the reason for this somewhat apologetic response is to ask to be
a bit more careful and not to throw the baby with the water or something.
Losing ENTRY/EXIT/GOTO was hard enough as I still have not figured out
how do I trace back what happened on a 1000+ node cluster from a
vague "something bad happened" condition with standard tools
without knowing in advance what node was the reason for the badness
(so I cannot just activate a couple of tracepoints).
Bye,
Oleg
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 6/7] staging: lustre: obdclass: expand the GOTO macro + break
@ 2014-09-09 13:37 ` Drokin, Oleg
0 siblings, 0 replies; 19+ messages in thread
From: Drokin, Oleg @ 2014-09-09 13:37 UTC (permalink / raw)
To: Dan Carpenter
Cc: Julia Lawall, Greg Kroah-Hartman, devel, Dilger, Andreas,
Peng Tao, kernel-janitors, Linux Kernel Mailing List, Hammond,
John
Hello!
On Sep 9, 2014, at 8:54 AM, Dan Carpenter wrote:
> 460 #define OBD_CHECK_DT_OP(obd, op, err) \
> 461 do { \
> 462 if (!OBT(obd) || !OBP((obd), op)) { \
> 463 if (err) \
> 464 CERROR("obd_" #op ": dev %d no operation\n", \
> 465 obd->obd_minor); \
> 466 return err; \
> 467 } \
> 468 } while (0)
>
> Wow! What a terrible macro! None of the '\' are in a line. There is
This is most likely an artifact of the way spaces were converted to tabs automatically by somebody.
> a hidden return statement in it which is a terrible thing and flow
> control statements are not allowed inside macros.
>
> I can't tell what OBT() and OBP() because the names are very ambiguous.
>
> 328 #define OBT(dev) (dev)->obd_type
> 329 #define OBP(dev, op) (dev)->obd_type->typ_dt_ops->o_ ## op
> 330 #define MDP(dev, op) (dev)->obd_type->typ_md_ops->m_ ## op
>
> Ok. "OB" stands for "obd". T stands for "type". The "P" probably
> stands for pointer or operation. MD is clear enough.
>
> The OBP() macro adds an "o_" to the function pointer and the MDP() macro
> adds an "m_". That totally sucks because it makes the function pointer
> hard to grep for. There isn't another explanation, whoever wrote this
> code is just being ornery.
I guess I just became numb to this sort of thing over time dealing
with those functions (that frequently irritated me too at first, I guess.)
> So the obd_pool_new() function verifies that ->o_pool_new is non-NULL,
> it increments a counter and calls ->o_pool_new(). Let's take a look at
> which functions implement ->o_pool_new().
>
> grep -w o_pool_new drivers/staging/lustre/ -R | egrep '\.[ch]:'
>
> The only implementation of this function is lov_pool_new(). Why do
> we have a function pointer if there is only one implementation? We
> should remove this.
Well, of course in reality you are only looking at half the code (the client),
and client and server share quite a bit of common glue (used to share
if we are talking about upstream client). There are other implementors of
this method on the server side (not that we care about it here).
There all these different obd layers that do similar operations and
could be (or used to be able to be) stacked on top of each able.
Old (long abandoned idea) was such that you should be able to stack these layers
in almost any order on each other. That never worked out because there
were ever so slight changes to stuff you pass down to the next layer.
But all the supporting infrastructure remained in place. Some of it
still being used and some being there since old times even though
layer order became fixed. After working with lustre sufficiently long
it's just engraved into your head that e.g. "oh, llite always calls into
lov_xxx and lov always calls into osc_xxx via these obd macroses.
Still on the other hand obd is really like an object, it not only has
the methods called in this convoluted manner, but also data fields.
It needs to be seen if things will become even worse if we start calling
these methods directly, and pass around strangely derived structure pointers
with data.
> The lov_pool_new() function definitely can fail unexpectedly with
> -ENOMEM.
>
> Let's go back to the original code and see how the error should be
> handled... Oh... Apparently this is an optional thing so we would
> just ignore the error and continue. The code is fine.
>
> In any other subsystem it would have taken 30 seconds to read the code
> because cscope would work and there wouldn't be the layers of
> indirection.
>
> I'm not convinced that having a function counter for calls to
> lov_pool_new() is useful. We could get the same information from ftrace
> or other tools. In my view, we could get rid of all the horrible macros
The counter thing was already noticed by Greg separately and it's in the TODO
list for the perf tracking integration of some sort to replace the obd calls
accounting (see - the "object" benefit, you implement an obd method and
magically there's all this call tracking and other goodies for you that
you can use without even thinking).
> and the function pointers and the splitting names into half and the
> layers of indirection and the debugging code and just call
> lov_pool_new() directly.
I guess the reason for this somewhat apologetic response is to ask to be
a bit more careful and not to throw the baby with the water or something.
Losing ENTRY/EXIT/GOTO was hard enough as I still have not figured out
how do I trace back what happened on a 1000+ node cluster from a
vague "something bad happened" condition with standard tools
without knowing in advance what node was the reason for the badness
(so I cannot just activate a couple of tracepoints).
Bye,
Oleg
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH] checkpatch: Warn on macros with flow control statements
2014-09-09 12:54 ` Dan Carpenter
@ 2014-09-09 20:38 ` Joe Perches
-1 siblings, 0 replies; 19+ messages in thread
From: Joe Perches @ 2014-09-09 20:38 UTC (permalink / raw)
To: Dan Carpenter, Andrew Morton
Cc: Andy Whitcroft, Julia Lawall, Greg Kroah-Hartman, devel, Dilger,
Andreas, Peng Tao, kernel-janitors, linux-kernel, Drokin, Oleg
Macros with flow control statements (goto and return) are
not very nice to read as any flow movement is unexpected.
Try to highlight them and emit a warning on their definition.
Avoid warning on macros that use argument concatenation as
those macros commonly create another function where the
concatenation is used in the function name definition like:
#define FOO_FUNC(name, rtn_type) \
rtn_type func##name(arg1, ...) \
{ \
rtn_type rtn; \
[code...] \
return rtn; \
}
Signed-off-by: Joe Perches <joe@perches.com>
---
> 460 #define OBD_CHECK_DT_OP(obd, op, err) \
> 461 do { \
> 462 if (!OBT(obd) || !OBP((obd), op)) { \
> 463 if (err) \
> 464 CERROR("obd_" #op ": dev %d no operation\n", \
> 465 obd->obd_minor); \
> 466 return err; \
> 467 } \
> 468 } while (0)
>
> Wow! What a terrible macro! None of the '\' are in a line. There is
> a hidden return statement in it which is a terrible thing and flow
> control statements are not allowed inside macros.
scripts/checkpatch.pl | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index da7b2d4..dc5434a 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -4071,12 +4071,17 @@ sub process {
my $cnt = $realcnt;
my ($off, $dstat, $dcond, $rest);
my $ctx = '';
+ my $has_flow_statement = 0;
+ my $has_arg_concat = 0;
($dstat, $dcond, $ln, $cnt, $off) =
ctx_statement_block($linenr, $realcnt, 0);
$ctx = $dstat;
#print "dstat<$dstat> dcond<$dcond> cnt<$cnt> off<$off>\n";
#print "LINE<$lines[$ln-1]> len<" . length($lines[$ln-1]) . "\n";
+ $has_flow_statement = 1 if ($ctx =~ /\b(goto|return)\b/);
+ $has_arg_concat = 1 if ($ctx =~ /\#\#/);
+
$dstat =~ s/^.\s*\#\s*define\s+$Ident(?:\([^\)]*\))?\s*//;
$dstat =~ s/$;//g;
$dstat =~ s/\\\n.//g;
@@ -4141,6 +4146,19 @@ sub process {
}
}
+# check for macros with flow control, but without ## concatenation
+# ## concatenation is commonly a macro that defines a function so ignore those
+ if ($has_flow_statement && !$has_arg_concat) {
+ my $herectx = $here . "\n";
+ my $cnt = statement_rawlines($ctx);
+
+ for (my $n = 0; $n < $cnt; $n++) {
+ $herectx .= raw_line($linenr, $n) . "\n";
+ }
+ WARN("MACRO_WITH_FLOW_CONTROL",
+ "Macros with flow control statements should be avoided\n" . "$herectx");
+ }
+
# check for line continuations outside of #defines, preprocessor #, and asm
} else {
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH] checkpatch: Warn on macros with flow control statements
@ 2014-09-09 20:38 ` Joe Perches
0 siblings, 0 replies; 19+ messages in thread
From: Joe Perches @ 2014-09-09 20:38 UTC (permalink / raw)
To: Dan Carpenter, Andrew Morton
Cc: Andy Whitcroft, Julia Lawall, Greg Kroah-Hartman, devel, Dilger,
Andreas, Peng Tao, kernel-janitors, linux-kernel, Drokin, Oleg
Macros with flow control statements (goto and return) are
not very nice to read as any flow movement is unexpected.
Try to highlight them and emit a warning on their definition.
Avoid warning on macros that use argument concatenation as
those macros commonly create another function where the
concatenation is used in the function name definition like:
#define FOO_FUNC(name, rtn_type) \
rtn_type func##name(arg1, ...) \
{ \
rtn_type rtn; \
[code...] \
return rtn; \
}
Signed-off-by: Joe Perches <joe@perches.com>
---
> 460 #define OBD_CHECK_DT_OP(obd, op, err) \
> 461 do { \
> 462 if (!OBT(obd) || !OBP((obd), op)) { \
> 463 if (err) \
> 464 CERROR("obd_" #op ": dev %d no operation\n", \
> 465 obd->obd_minor); \
> 466 return err; \
> 467 } \
> 468 } while (0)
>
> Wow! What a terrible macro! None of the '\' are in a line. There is
> a hidden return statement in it which is a terrible thing and flow
> control statements are not allowed inside macros.
scripts/checkpatch.pl | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index da7b2d4..dc5434a 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -4071,12 +4071,17 @@ sub process {
my $cnt = $realcnt;
my ($off, $dstat, $dcond, $rest);
my $ctx = '';
+ my $has_flow_statement = 0;
+ my $has_arg_concat = 0;
($dstat, $dcond, $ln, $cnt, $off) ctx_statement_block($linenr, $realcnt, 0);
$ctx = $dstat;
#print "dstat<$dstat> dcond<$dcond> cnt<$cnt> off<$off>\n";
#print "LINE<$lines[$ln-1]> len<" . length($lines[$ln-1]) . "\n";
+ $has_flow_statement = 1 if ($ctx =~ /\b(goto|return)\b/);
+ $has_arg_concat = 1 if ($ctx =~ /\#\#/);
+
$dstat =~ s/^.\s*\#\s*define\s+$Ident(?:\([^\)]*\))?\s*//;
$dstat =~ s/$;//g;
$dstat =~ s/\\\n.//g;
@@ -4141,6 +4146,19 @@ sub process {
}
}
+# check for macros with flow control, but without ## concatenation
+# ## concatenation is commonly a macro that defines a function so ignore those
+ if ($has_flow_statement && !$has_arg_concat) {
+ my $herectx = $here . "\n";
+ my $cnt = statement_rawlines($ctx);
+
+ for (my $n = 0; $n < $cnt; $n++) {
+ $herectx .= raw_line($linenr, $n) . "\n";
+ }
+ WARN("MACRO_WITH_FLOW_CONTROL",
+ "Macros with flow control statements should be avoided\n" . "$herectx");
+ }
+
# check for line continuations outside of #defines, preprocessor #, and asm
} else {
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH] checkpatch: Warn on macros with flow control statements
2014-09-09 20:38 ` Joe Perches
@ 2014-09-10 8:43 ` Dan Carpenter
-1 siblings, 0 replies; 19+ messages in thread
From: Dan Carpenter @ 2014-09-10 8:43 UTC (permalink / raw)
To: Joe Perches
Cc: Andrew Morton, devel, Dilger, Andreas, Greg Kroah-Hartman,
Peng Tao, kernel-janitors, linux-kernel, Drokin, Oleg,
Julia Lawall, Andy Whitcroft
[-- Attachment #1: Type: text/plain, Size: 1173 bytes --]
On Tue, Sep 09, 2014 at 01:38:13PM -0700, Joe Perches wrote:
> Macros with flow control statements (goto and return) are
> not very nice to read as any flow movement is unexpected.
>
> Try to highlight them and emit a warning on their definition.
>
> Avoid warning on macros that use argument concatenation as
> those macros commonly create another function where the
> concatenation is used in the function name definition like:
> #define FOO_FUNC(name, rtn_type) \
> rtn_type func##name(arg1, ...) \
> { \
> rtn_type rtn; \
> [code...] \
> return rtn; \
> }
>
It adds 382 new warnings.
The '##' trick doesn't remove all then macros which create functions.
I can't think of a better way to do that though.
We will eventually get rid of almost all the warnings in staging. The
one that makes sense to keep is:
drivers/staging/lustre/lnet/selftest/selftest.h:559
#define STATE2STR(x) case x: return #x
My guess is that other maintainers won't be as excited to change these...
Some of the macros have "RETURN", "RET" or "EXIT" in the name so the
return is not really hidden.
I'll attach the list of warning locations.
regards,
dan carpenter
[-- Attachment #2: err-list --]
[-- Type: text/plain, Size: 13089 bytes --]
arch/alpha/lib/memcpy.c:24
arch/alpha/lib/memcpy.c:31
arch/alpha/math-emu/sfp-util.h:30
arch/arc/kernel/traps.c:65
arch/arc/kernel/unaligned.c:45
arch/arc/kernel/unaligned.c:56
arch/arc/kernel/unaligned.c:71
arch/arc/kernel/unaligned.c:98
arch/arm64/net/bpf_jit_comp.c:351
arch/arm/mach-omap2/gpmc.c:326
arch/arm/mach-omap2/gpmc.c:331
arch/arm/mm/alignment.c:215
arch/arm/mm/alignment.c:232
arch/arm/mm/alignment.c:253
arch/arm/mm/alignment.c:285
arch/blackfin/include/asm/uaccess.h:171
arch/blackfin/include/asm/uaccess.h:174
arch/frv/kernel/setup.c:665
arch/m68k/include/asm/bootstd.h:37
arch/m68k/include/asm/uaccess_no.h:138
arch/m68k/include/asm/uaccess_no.h:140
arch/m68k/math-emu/fp_emu.h:78
arch/m68k/math-emu/fp_emu.h:84
arch/mips/alchemy/common/clock.c:1016
arch/mips/cavium-octeon/executive/cvmx-spi.c:43
arch/mips/include/asm/octeon/cvmx-bootinfo.h:249
arch/mips/include/asm/octeon/cvmx-bootinfo.h:337
arch/mips/math-emu/me-debugfs.c:44
arch/mn10300/include/asm/uaccess.h:119
arch/mn10300/include/asm/uaccess.h:121
arch/mn10300/include/asm/uaccess.h:123
arch/mn10300/include/asm/uaccess.h:125
arch/mn10300/unit-asb2364/include/unit/serial.h:126
arch/parisc/include/asm/unistd.h:106
arch/parisc/include/asm/unistd.h:112
arch/parisc/include/asm/unistd.h:118
arch/parisc/include/asm/unistd.h:124
arch/parisc/include/asm/unistd.h:130
arch/parisc/include/asm/unistd.h:137
arch/parisc/kernel/module.c:81
arch/parisc/kernel/pci-dma.c:230
arch/parisc/lib/memcpy.c:71
arch/powerpc/include/asm/bitops.h:102
arch/powerpc/include/asm/io.h:406
arch/powerpc/include/asm/sfp-machine.h:353
arch/powerpc/platforms/powermac/pfunc_core.c:123
arch/powerpc/platforms/powernv/pci-ioda.c:44
arch/s390/hypfs/hypfs_vm.c:110
arch/s390/math-emu/math.c:75
arch/s390/math-emu/math.c:81
arch/s390/math-emu/math.c:87
arch/s390/math-emu/math.c:93
arch/sh/math-emu/math.c:276
arch/sh/math-emu/math.c:54
arch/sh/math-emu/math.c:55
arch/sh/math-emu/sfp-util.h:68
arch/sparc/include/asm/sfp-machine_32.h:198
arch/sparc/include/asm/sfp-machine_64.h:87
arch/sparc/include/asm/uaccess_32.h:169
arch/sparc/include/asm/uaccess_32.h:191
arch/sparc/include/asm/uaccess_64.h:150
arch/sparc/math-emu/sfp-util_32.h:108
arch/sparc/math-emu/sfp-util_64.h:113
arch/sparc/net/bpf_jit_comp.c:689
arch/unicore32/mm/alignment.c:101
arch/unicore32/mm/alignment.c:111
arch/unicore32/mm/alignment.c:135
arch/unicore32/mm/alignment.c:165
arch/unicore32/mm/alignment.c:182
arch/x86/include/asm/archrandom.h:69
arch/x86/include/asm/archrandom.h:86
arch/x86/include/asm/io.h:44
arch/x86/include/asm/rmwcc.h:24
arch/x86/kernel/nmi.c:464
arch/x86/kernel/nmi.c:475
arch/x86/kernel/ptrace.c:949
arch/x86/kernel/uprobes.c:552
arch/x86/kernel/uprobes.c:567
arch/x86/kernel/vm86_32.c:436
arch/x86/kernel/vm86_32.c:444
arch/x86/kernel/vm86_32.c:455
arch/x86/kernel/vm86_32.c:472
arch/x86/kernel/vm86_32.c:481
arch/x86/kernel/vm86_32.c:493
arch/x86/kernel/vm86_32.c:581
arch/x86/kvm/emulate.c:761
arch/x86/kvm/emulate.c:773
arch/x86/lib/insn.c:39
arch/x86/lib/insn.c:42
arch/x86/mm/pf_in.c:135
block/cfq-iosched.c:4488
block/cfq-iosched.c:4511
block/deadline-iosched.c:391
block/deadline-iosched.c:407
crypto/asymmetric_keys/verify_pefile.c:40
drivers/base/core.c:2102
drivers/block/drbd/drbd_vli.h:150
drivers/block/drbd/drbd_vli.h:176
drivers/char/dsp56k.c:60
drivers/char/dsp56k.c:77
drivers/char/dsp56k.c:87
drivers/char/ipmi/ipmi_bt_sm.c:90
drivers/char/ipmi/ipmi_bt_sm.c:92
drivers/char/pcmcia/cm4000_cs.c:204
drivers/cpufreq/cpufreq_governor.h:111
drivers/gpu/drm/drm_crtc.c:49
drivers/gpu/drm/gma500/gma_display.c:697
drivers/gpu/drm/i915/intel_display.c:10629
drivers/gpu/drm/i915/intel_display.c:10638
drivers/gpu/drm/i915/intel_display.c:10652
drivers/gpu/drm/i915/intel_display.c:10663
drivers/gpu/drm/i915/intel_display.c:10672
drivers/gpu/drm/i915/intel_display.c:542
drivers/gpu/drm/i915/intel_uncore.c:551
drivers/gpu/drm/mga/mga_drv.h:227
drivers/gpu/drm/mga/mga_drv.h:241
drivers/gpu/drm/r128/r128_drv.h:425
drivers/gpu/drm/r128/r128_drv.h:433
drivers/gpu/drm/r128/r128_drv.h:450
drivers/gpu/drm/radeon/radeon_drv.h:2006
drivers/gpu/drm/radeon/radeon_kms.c:805
drivers/gpu/drm/radeon/radeon_state.c:1824
drivers/infiniband/hw/usnic/usnic_uiom_interval_tree.c:30
drivers/input/input.c:1557
drivers/input/input.c:1564
drivers/input/input.c:1571
drivers/input/serio/i8042.c:1048
drivers/input/serio/serio.c:913
drivers/isdn/act2000/capi.c:119
drivers/isdn/hisax/l3dss1.c:359
drivers/isdn/i4l/isdn_tty.c:2595
drivers/isdn/i4l/isdn_tty.c:2596
drivers/isdn/i4l/isdn_ttyfax.c:25
drivers/macintosh/via-cuda.c:249
drivers/md/bcache/alloc.c:280
drivers/md/bcache/closure.h:319
drivers/md/bcache/closure.h:348
drivers/md/bcache/closure.h:364
drivers/md/bcache/journal.c:146
drivers/md/bcache/sysfs.h:88
drivers/md/bcache/sysfs.h:97
drivers/md/bcache/util.h:485
drivers/media/dvb-frontends/dib3000mb_priv.h:24
drivers/media/i2c/soc_camera/mt9t112.c:51
drivers/media/rc/rc-main.c:1183
drivers/media/tuners/tuner-i2c.h:129
drivers/media/usb/tlg2300/pd-video.c:214
drivers/net/dsa/mv88e6060.c:27
drivers/net/dsa/mv88e6060.c:44
drivers/net/dsa/mv88e6xxx.h:74
drivers/net/dsa/mv88e6xxx.h:84
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c:2190
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c:2196
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c:2202
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c:2208
drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c:12812
drivers/net/ethernet/chelsio/cxgb4/t4_hw.c:674
drivers/net/ethernet/cisco/enic/enic_res.c:54
drivers/net/ethernet/cisco/enic/vnic_vic.h:65
drivers/net/ethernet/intel/e1000e/ethtool.c:809
drivers/net/ethernet/intel/e1000e/ethtool.c:817
drivers/net/ethernet/intel/igb/igb_ethtool.c:1237
drivers/net/ethernet/intel/igb/igb_ethtool.c:1243
drivers/net/ethernet/neterion/vxge/vxge-config.c:24
drivers/net/ethernet/neterion/vxge/vxge-config.h:870
drivers/net/ethernet/tehuti/tehuti.h:543
drivers/net/ethernet/tehuti/tehuti.h:553
drivers/net/wimax/i2400m/debugfs.c:231
drivers/net/wimax/i2400m/usb.c:384
drivers/net/wireless/at76c50x-usb.c:708
drivers/net/wireless/at76c50x-usb.c:753
drivers/net/wireless/ath/ar5523/ar5523.c:368
drivers/net/wireless/ath/ath10k/wmi.h:177
drivers/net/wireless/ath/ath5k/ath5k.h:932
drivers/net/wireless/ath/ath5k/eeprom.h:244
drivers/net/wireless/ath/ath9k/ath9k.h:414
drivers/net/wireless/ath/carl9170/cmd.h:120
drivers/net/wireless/ath/carl9170/cmd.h:139
drivers/net/wireless/ath/carl9170/cmd.h:87
drivers/net/wireless/cw1200/fwio.c:65
drivers/net/wireless/cw1200/fwio.c:71
drivers/net/wireless/cw1200/fwio.c:77
drivers/net/wireless/cw1200/fwio.c:83
drivers/net/wireless/cw1200/wsm.c:30
drivers/net/wireless/cw1200/wsm.c:37
drivers/net/wireless/cw1200/wsm.c:45
drivers/net/wireless/cw1200/wsm.c:59
drivers/net/wireless/cw1200/wsm.c:68
drivers/net/wireless/iwlegacy/common.c:3788
drivers/net/wireless/iwlegacy/common.c:3794
drivers/net/wireless/iwlegacy/common.h:1526
drivers/net/wireless/iwlegacy/debug.c:136
drivers/net/wireless/iwlegacy/debug.c:144
drivers/net/wireless/iwlwifi/dvm/debugfs.c:47
drivers/net/wireless/iwlwifi/dvm/debugfs.c:55
drivers/net/wireless/iwlwifi/dvm/debugfs.c:63
drivers/net/wireless/iwlwifi/dvm/rxon.c:862
drivers/net/wireless/iwlwifi/dvm/rxon.c:868
drivers/net/wireless/iwlwifi/iwl-io.c:206
drivers/net/wireless/iwlwifi/pcie/trans.c:1450
drivers/net/wireless/p54/p54usb.c:640
drivers/net/wireless/p54/p54usb.c:648
drivers/net/wireless/rndis_wlan.c:624
drivers/parisc/ccio-dma.c:295
drivers/pcmcia/i82365.c:1154
drivers/pcmcia/m32r_cfc.c:611
drivers/pcmcia/m32r_pcc.c:599
drivers/pinctrl/pinctrl-single.c:1495
drivers/pnp/pnpacpi/core.c:36
drivers/pnp/pnpacpi/core.c:39
drivers/scsi/aha1542.c:150
drivers/scsi/aha1542.c:163
drivers/scsi/aic94xx/aic94xx_tmf.c:100
drivers/scsi/aic94xx/aic94xx_tmf.c:118
drivers/scsi/csiostor/csio_hw.c:369
drivers/scsi/fnic/fnic_res.c:39
drivers/scsi/sym53c8xx_2/sym_glue.c:1020
drivers/scsi/sym53c8xx_2/sym_glue.c:1025
drivers/ssb/pcmcia.c:595
drivers/ssb/sdio.c:465
drivers/staging/board/board.h:9
drivers/staging/dgnc/dgnc_sysfs.c:134
drivers/staging/line6/driver.h:55
drivers/staging/line6/driver.h:62
drivers/staging/lustre/include/linux/libcfs/libcfs_debug.h:246
drivers/staging/lustre/lnet/selftest/selftest.h:559
drivers/staging/lustre/lustre/include/lprocfs_status.h:673
drivers/staging/lustre/lustre/include/obd_class.h:335
drivers/staging/lustre/lustre/include/obd_class.h:344
drivers/staging/lustre/lustre/include/obd_class.h:431
drivers/staging/lustre/lustre/include/obd_class.h:441
drivers/staging/lustre/lustre/include/obd_class.h:460
drivers/staging/lustre/lustre/include/obd_class.h:470
drivers/staging/lustre/lustre/include/obd_class.h:487
drivers/staging/lustre/lustre/ptlrpc/llog_client.c:52
drivers/staging/rts5208/trace.h:49
drivers/staging/rts5208/trace.h:66
drivers/staging/rts5208/trace.h:83
drivers/staging/rts5208/trace.h:84
drivers/staging/unisys/common-spar/include/channels/iochannel.h:772
drivers/staging/unisys/virtpci/virtpci.c:301
drivers/usb/renesas_usbhs/fifo.c:222
drivers/video/fbdev/aty/atyfb_base.c:117
drivers/video/fbdev/aty/atyfb_base.c:122
drivers/video/fbdev/hgafb.c:54
drivers/video/fbdev/intelfb/intelfbdrv.c:492
drivers/video/fbdev/sis/sis.h:52
drivers/video/fbdev/vga16fb.c:281
fs/btrfs/send.c:591
fs/btrfs/send.c:609
fs/btrfs/send.c:615
fs/btrfs/send.c:622
fs/btrfs/send.c:628
fs/coda/upcall.c:60
fs/fat/dir.c:704
fs/nfsd/nfs2acl.c:16
fs/nfsd/nfs3acl.c:15
fs/nfsd/nfs3proc.c:17
fs/nfsd/nfs4xdr.c:114
fs/nfsd/nfs4xdr.c:84
fs/nfsd/nfs4xdr.c:98
fs/ntfs/super.c:114
fs/ntfs/super.c:124
fs/ntfs/super.c:132
fs/ntfs/super.c:144
fs/ntfs/super.c:156
fs/ntfs/super.c:164
fs/ntfs/super.c:171
fs/ufs/balloc.c:561
fs/xfs/xfs_error.h:43
fs/xfs/xfs_error.h:55
include/acpi/acoutput.h:372
include/acpi/acoutput.h:381
include/acpi/acoutput.h:391
include/acpi/acoutput.h:450
include/acpi/acoutput.h:451
include/acpi/acoutput.h:452
include/acpi/acoutput.h:453
include/acpi/acoutput.h:454
include/acpi/acoutput.h:455
include/acpi/acpixf.h:291
include/acpi/acpixf.h:294
include/acpi/acpixf.h:297
include/acpi/acpixf.h:314
include/acpi/acpixf.h:331
include/acpi/acpixf.h:348
include/acpi/platform/aclinux.h:105
include/acpi/platform/aclinux.h:107
include/acpi/platform/aclinux.h:109
include/acpi/platform/aclinux.h:111
include/acpi/platform/aclinux.h:113
include/drm/drmP.h:244
include/linux/ceph/decode.h:220
include/linux/ceph/decode.h:55
include/linux/compiler-gcc4.h:78
include/linux/genl_magic_func.h:178
include/linux/genl_magic_func.h:225
include/linux/genl_magic_func.h:357
include/linux/init.h:327
include/linux/init.h:333
include/linux/platform_device.h:277
include/linux/platform_device.h:291
include/linux/tracepoint.h:118
include/linux/usb/serial.h:398
include/linux/wait.h:208
include/trace/ftrace.h:322
include/trace/ftrace.h:331
include/trace/ftrace.h:346
kernel/bpf/core.c:249
kernel/bpf/core.c:250
kernel/events/internal.h:84
kernel/trace/trace_events.c:147
kernel/trace/trace_export.c:109
kernel/trace/trace_export.c:120
kernel/trace/trace_export.c:77
kernel/trace/trace_export.c:86
kernel/trace/trace_export.c:96
kernel/trace/trace_output.h:38
kernel/trace/trace_output.h:44
kernel/trace/trace_probe.c:39
kernel/trace/trace_probe.h:54
lib/inflate.c:232
lib/kstrtox.c:324
lib/lru_cache.c:46
lib/lzo/lzo1x_decompress_safe.c:30
lib/lzo/lzo1x_decompress_safe.c:36
lib/lzo/lzo1x_decompress_safe.c:42
lib/ts_fsm.c:151
lib/zlib_deflate/deflate.c:839
lib/zlib_inflate/inflate.c:196
net/bridge/netfilter/ebt_vlan.c:35
net/core/dev.c:7067
net/core/filter.c:421
net/decnet/netfilter/dn_rtmsg.c:101
net/ipv6/route.c:776
net/irda/irnet/irnet.h:364
net/irda/irnet/irnet.h:369
net/irda/irnet/irnet.h:375
net/mac80211/rx.c:2965
net/mac80211/rx.c:3023
net/mac80211/tx.c:1345
net/netfilter/nf_conntrack_h323_asn1.c:106
net/netfilter/nfnetlink_log.c:592
net/netfilter/nfnetlink_queue_core.c:800
net/rxrpc/ar-key.c:1133
net/rxrpc/ar-key.c:1139
net/rxrpc/ar-key.c:1150
net/rxrpc/rxkad.c:866
net/sched/cls_flow.c:126
net/sched/cls_flow.c:135
net/sched/cls_route.c:108
net/sched/cls_rsvp.h:119
net/vmw_vsock/af_vsock.c:1374
net/vmw_vsock/af_vsock.c:1455
net/wimax/debugfs.c:31
net/wireless/nl80211.c:4908
scripts/dtc/libfdt/fdt_rw.c:85
scripts/dtc/libfdt/fdt_sw.c:66
scripts/dtc/libfdt/libfdt_internal.h:58
sound/aoa/codecs/onyx.c:931
sound/isa/sb/emu8000_pcm.c:420
sound/oss/dmasound/dmasound.h:45
sound/oss/dmasound/dmasound_paula.c:188
sound/oss/dmasound/dmasound_paula.c:239
sound/oss/swarm_cs4297a.c:195
sound/pci/ca0106/ca0106_mixer.c:767
tools/lib/traceevent/trace-seq.c:41
tools/perf/builtin-kvm.c:1130
tools/perf/tests/attr.c:44
tools/perf/tests/keep-tracking.c:12
tools/perf/tests/keep-tracking.c:19
tools/perf/tests/perf-time-to-tsc.c:14
tools/perf/tests/perf-time-to-tsc.c:21
tools/perf/tests/sample-parsing.c:11
tools/perf/tests/sample-parsing.c:18
tools/perf/tests/tests.h:4
tools/perf/util/evsel.c:1251
tools/perf/util/parse-events.c:488
tools/perf/util/probe-event.c:2204
tools/perf/util/unwind-libunwind.c:154
tools/perf/util/unwind-libunwind.c:92
tools/testing/selftests/powerpc/utils.h:25
tools/testing/selftests/powerpc/utils.h:37
tools/usb/ffs-test.c:202
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH] checkpatch: Warn on macros with flow control statements
@ 2014-09-10 8:43 ` Dan Carpenter
0 siblings, 0 replies; 19+ messages in thread
From: Dan Carpenter @ 2014-09-10 8:43 UTC (permalink / raw)
To: Joe Perches
Cc: Andrew Morton, devel, Dilger, Andreas, Greg Kroah-Hartman,
Peng Tao, kernel-janitors, linux-kernel, Drokin, Oleg,
Julia Lawall, Andy Whitcroft
[-- Attachment #1: Type: text/plain, Size: 1173 bytes --]
On Tue, Sep 09, 2014 at 01:38:13PM -0700, Joe Perches wrote:
> Macros with flow control statements (goto and return) are
> not very nice to read as any flow movement is unexpected.
>
> Try to highlight them and emit a warning on their definition.
>
> Avoid warning on macros that use argument concatenation as
> those macros commonly create another function where the
> concatenation is used in the function name definition like:
> #define FOO_FUNC(name, rtn_type) \
> rtn_type func##name(arg1, ...) \
> { \
> rtn_type rtn; \
> [code...] \
> return rtn; \
> }
>
It adds 382 new warnings.
The '##' trick doesn't remove all then macros which create functions.
I can't think of a better way to do that though.
We will eventually get rid of almost all the warnings in staging. The
one that makes sense to keep is:
drivers/staging/lustre/lnet/selftest/selftest.h:559
#define STATE2STR(x) case x: return #x
My guess is that other maintainers won't be as excited to change these...
Some of the macros have "RETURN", "RET" or "EXIT" in the name so the
return is not really hidden.
I'll attach the list of warning locations.
regards,
dan carpenter
[-- Attachment #2: err-list --]
[-- Type: text/plain, Size: 13089 bytes --]
arch/alpha/lib/memcpy.c:24
arch/alpha/lib/memcpy.c:31
arch/alpha/math-emu/sfp-util.h:30
arch/arc/kernel/traps.c:65
arch/arc/kernel/unaligned.c:45
arch/arc/kernel/unaligned.c:56
arch/arc/kernel/unaligned.c:71
arch/arc/kernel/unaligned.c:98
arch/arm64/net/bpf_jit_comp.c:351
arch/arm/mach-omap2/gpmc.c:326
arch/arm/mach-omap2/gpmc.c:331
arch/arm/mm/alignment.c:215
arch/arm/mm/alignment.c:232
arch/arm/mm/alignment.c:253
arch/arm/mm/alignment.c:285
arch/blackfin/include/asm/uaccess.h:171
arch/blackfin/include/asm/uaccess.h:174
arch/frv/kernel/setup.c:665
arch/m68k/include/asm/bootstd.h:37
arch/m68k/include/asm/uaccess_no.h:138
arch/m68k/include/asm/uaccess_no.h:140
arch/m68k/math-emu/fp_emu.h:78
arch/m68k/math-emu/fp_emu.h:84
arch/mips/alchemy/common/clock.c:1016
arch/mips/cavium-octeon/executive/cvmx-spi.c:43
arch/mips/include/asm/octeon/cvmx-bootinfo.h:249
arch/mips/include/asm/octeon/cvmx-bootinfo.h:337
arch/mips/math-emu/me-debugfs.c:44
arch/mn10300/include/asm/uaccess.h:119
arch/mn10300/include/asm/uaccess.h:121
arch/mn10300/include/asm/uaccess.h:123
arch/mn10300/include/asm/uaccess.h:125
arch/mn10300/unit-asb2364/include/unit/serial.h:126
arch/parisc/include/asm/unistd.h:106
arch/parisc/include/asm/unistd.h:112
arch/parisc/include/asm/unistd.h:118
arch/parisc/include/asm/unistd.h:124
arch/parisc/include/asm/unistd.h:130
arch/parisc/include/asm/unistd.h:137
arch/parisc/kernel/module.c:81
arch/parisc/kernel/pci-dma.c:230
arch/parisc/lib/memcpy.c:71
arch/powerpc/include/asm/bitops.h:102
arch/powerpc/include/asm/io.h:406
arch/powerpc/include/asm/sfp-machine.h:353
arch/powerpc/platforms/powermac/pfunc_core.c:123
arch/powerpc/platforms/powernv/pci-ioda.c:44
arch/s390/hypfs/hypfs_vm.c:110
arch/s390/math-emu/math.c:75
arch/s390/math-emu/math.c:81
arch/s390/math-emu/math.c:87
arch/s390/math-emu/math.c:93
arch/sh/math-emu/math.c:276
arch/sh/math-emu/math.c:54
arch/sh/math-emu/math.c:55
arch/sh/math-emu/sfp-util.h:68
arch/sparc/include/asm/sfp-machine_32.h:198
arch/sparc/include/asm/sfp-machine_64.h:87
arch/sparc/include/asm/uaccess_32.h:169
arch/sparc/include/asm/uaccess_32.h:191
arch/sparc/include/asm/uaccess_64.h:150
arch/sparc/math-emu/sfp-util_32.h:108
arch/sparc/math-emu/sfp-util_64.h:113
arch/sparc/net/bpf_jit_comp.c:689
arch/unicore32/mm/alignment.c:101
arch/unicore32/mm/alignment.c:111
arch/unicore32/mm/alignment.c:135
arch/unicore32/mm/alignment.c:165
arch/unicore32/mm/alignment.c:182
arch/x86/include/asm/archrandom.h:69
arch/x86/include/asm/archrandom.h:86
arch/x86/include/asm/io.h:44
arch/x86/include/asm/rmwcc.h:24
arch/x86/kernel/nmi.c:464
arch/x86/kernel/nmi.c:475
arch/x86/kernel/ptrace.c:949
arch/x86/kernel/uprobes.c:552
arch/x86/kernel/uprobes.c:567
arch/x86/kernel/vm86_32.c:436
arch/x86/kernel/vm86_32.c:444
arch/x86/kernel/vm86_32.c:455
arch/x86/kernel/vm86_32.c:472
arch/x86/kernel/vm86_32.c:481
arch/x86/kernel/vm86_32.c:493
arch/x86/kernel/vm86_32.c:581
arch/x86/kvm/emulate.c:761
arch/x86/kvm/emulate.c:773
arch/x86/lib/insn.c:39
arch/x86/lib/insn.c:42
arch/x86/mm/pf_in.c:135
block/cfq-iosched.c:4488
block/cfq-iosched.c:4511
block/deadline-iosched.c:391
block/deadline-iosched.c:407
crypto/asymmetric_keys/verify_pefile.c:40
drivers/base/core.c:2102
drivers/block/drbd/drbd_vli.h:150
drivers/block/drbd/drbd_vli.h:176
drivers/char/dsp56k.c:60
drivers/char/dsp56k.c:77
drivers/char/dsp56k.c:87
drivers/char/ipmi/ipmi_bt_sm.c:90
drivers/char/ipmi/ipmi_bt_sm.c:92
drivers/char/pcmcia/cm4000_cs.c:204
drivers/cpufreq/cpufreq_governor.h:111
drivers/gpu/drm/drm_crtc.c:49
drivers/gpu/drm/gma500/gma_display.c:697
drivers/gpu/drm/i915/intel_display.c:10629
drivers/gpu/drm/i915/intel_display.c:10638
drivers/gpu/drm/i915/intel_display.c:10652
drivers/gpu/drm/i915/intel_display.c:10663
drivers/gpu/drm/i915/intel_display.c:10672
drivers/gpu/drm/i915/intel_display.c:542
drivers/gpu/drm/i915/intel_uncore.c:551
drivers/gpu/drm/mga/mga_drv.h:227
drivers/gpu/drm/mga/mga_drv.h:241
drivers/gpu/drm/r128/r128_drv.h:425
drivers/gpu/drm/r128/r128_drv.h:433
drivers/gpu/drm/r128/r128_drv.h:450
drivers/gpu/drm/radeon/radeon_drv.h:2006
drivers/gpu/drm/radeon/radeon_kms.c:805
drivers/gpu/drm/radeon/radeon_state.c:1824
drivers/infiniband/hw/usnic/usnic_uiom_interval_tree.c:30
drivers/input/input.c:1557
drivers/input/input.c:1564
drivers/input/input.c:1571
drivers/input/serio/i8042.c:1048
drivers/input/serio/serio.c:913
drivers/isdn/act2000/capi.c:119
drivers/isdn/hisax/l3dss1.c:359
drivers/isdn/i4l/isdn_tty.c:2595
drivers/isdn/i4l/isdn_tty.c:2596
drivers/isdn/i4l/isdn_ttyfax.c:25
drivers/macintosh/via-cuda.c:249
drivers/md/bcache/alloc.c:280
drivers/md/bcache/closure.h:319
drivers/md/bcache/closure.h:348
drivers/md/bcache/closure.h:364
drivers/md/bcache/journal.c:146
drivers/md/bcache/sysfs.h:88
drivers/md/bcache/sysfs.h:97
drivers/md/bcache/util.h:485
drivers/media/dvb-frontends/dib3000mb_priv.h:24
drivers/media/i2c/soc_camera/mt9t112.c:51
drivers/media/rc/rc-main.c:1183
drivers/media/tuners/tuner-i2c.h:129
drivers/media/usb/tlg2300/pd-video.c:214
drivers/net/dsa/mv88e6060.c:27
drivers/net/dsa/mv88e6060.c:44
drivers/net/dsa/mv88e6xxx.h:74
drivers/net/dsa/mv88e6xxx.h:84
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c:2190
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c:2196
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c:2202
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c:2208
drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c:12812
drivers/net/ethernet/chelsio/cxgb4/t4_hw.c:674
drivers/net/ethernet/cisco/enic/enic_res.c:54
drivers/net/ethernet/cisco/enic/vnic_vic.h:65
drivers/net/ethernet/intel/e1000e/ethtool.c:809
drivers/net/ethernet/intel/e1000e/ethtool.c:817
drivers/net/ethernet/intel/igb/igb_ethtool.c:1237
drivers/net/ethernet/intel/igb/igb_ethtool.c:1243
drivers/net/ethernet/neterion/vxge/vxge-config.c:24
drivers/net/ethernet/neterion/vxge/vxge-config.h:870
drivers/net/ethernet/tehuti/tehuti.h:543
drivers/net/ethernet/tehuti/tehuti.h:553
drivers/net/wimax/i2400m/debugfs.c:231
drivers/net/wimax/i2400m/usb.c:384
drivers/net/wireless/at76c50x-usb.c:708
drivers/net/wireless/at76c50x-usb.c:753
drivers/net/wireless/ath/ar5523/ar5523.c:368
drivers/net/wireless/ath/ath10k/wmi.h:177
drivers/net/wireless/ath/ath5k/ath5k.h:932
drivers/net/wireless/ath/ath5k/eeprom.h:244
drivers/net/wireless/ath/ath9k/ath9k.h:414
drivers/net/wireless/ath/carl9170/cmd.h:120
drivers/net/wireless/ath/carl9170/cmd.h:139
drivers/net/wireless/ath/carl9170/cmd.h:87
drivers/net/wireless/cw1200/fwio.c:65
drivers/net/wireless/cw1200/fwio.c:71
drivers/net/wireless/cw1200/fwio.c:77
drivers/net/wireless/cw1200/fwio.c:83
drivers/net/wireless/cw1200/wsm.c:30
drivers/net/wireless/cw1200/wsm.c:37
drivers/net/wireless/cw1200/wsm.c:45
drivers/net/wireless/cw1200/wsm.c:59
drivers/net/wireless/cw1200/wsm.c:68
drivers/net/wireless/iwlegacy/common.c:3788
drivers/net/wireless/iwlegacy/common.c:3794
drivers/net/wireless/iwlegacy/common.h:1526
drivers/net/wireless/iwlegacy/debug.c:136
drivers/net/wireless/iwlegacy/debug.c:144
drivers/net/wireless/iwlwifi/dvm/debugfs.c:47
drivers/net/wireless/iwlwifi/dvm/debugfs.c:55
drivers/net/wireless/iwlwifi/dvm/debugfs.c:63
drivers/net/wireless/iwlwifi/dvm/rxon.c:862
drivers/net/wireless/iwlwifi/dvm/rxon.c:868
drivers/net/wireless/iwlwifi/iwl-io.c:206
drivers/net/wireless/iwlwifi/pcie/trans.c:1450
drivers/net/wireless/p54/p54usb.c:640
drivers/net/wireless/p54/p54usb.c:648
drivers/net/wireless/rndis_wlan.c:624
drivers/parisc/ccio-dma.c:295
drivers/pcmcia/i82365.c:1154
drivers/pcmcia/m32r_cfc.c:611
drivers/pcmcia/m32r_pcc.c:599
drivers/pinctrl/pinctrl-single.c:1495
drivers/pnp/pnpacpi/core.c:36
drivers/pnp/pnpacpi/core.c:39
drivers/scsi/aha1542.c:150
drivers/scsi/aha1542.c:163
drivers/scsi/aic94xx/aic94xx_tmf.c:100
drivers/scsi/aic94xx/aic94xx_tmf.c:118
drivers/scsi/csiostor/csio_hw.c:369
drivers/scsi/fnic/fnic_res.c:39
drivers/scsi/sym53c8xx_2/sym_glue.c:1020
drivers/scsi/sym53c8xx_2/sym_glue.c:1025
drivers/ssb/pcmcia.c:595
drivers/ssb/sdio.c:465
drivers/staging/board/board.h:9
drivers/staging/dgnc/dgnc_sysfs.c:134
drivers/staging/line6/driver.h:55
drivers/staging/line6/driver.h:62
drivers/staging/lustre/include/linux/libcfs/libcfs_debug.h:246
drivers/staging/lustre/lnet/selftest/selftest.h:559
drivers/staging/lustre/lustre/include/lprocfs_status.h:673
drivers/staging/lustre/lustre/include/obd_class.h:335
drivers/staging/lustre/lustre/include/obd_class.h:344
drivers/staging/lustre/lustre/include/obd_class.h:431
drivers/staging/lustre/lustre/include/obd_class.h:441
drivers/staging/lustre/lustre/include/obd_class.h:460
drivers/staging/lustre/lustre/include/obd_class.h:470
drivers/staging/lustre/lustre/include/obd_class.h:487
drivers/staging/lustre/lustre/ptlrpc/llog_client.c:52
drivers/staging/rts5208/trace.h:49
drivers/staging/rts5208/trace.h:66
drivers/staging/rts5208/trace.h:83
drivers/staging/rts5208/trace.h:84
drivers/staging/unisys/common-spar/include/channels/iochannel.h:772
drivers/staging/unisys/virtpci/virtpci.c:301
drivers/usb/renesas_usbhs/fifo.c:222
drivers/video/fbdev/aty/atyfb_base.c:117
drivers/video/fbdev/aty/atyfb_base.c:122
drivers/video/fbdev/hgafb.c:54
drivers/video/fbdev/intelfb/intelfbdrv.c:492
drivers/video/fbdev/sis/sis.h:52
drivers/video/fbdev/vga16fb.c:281
fs/btrfs/send.c:591
fs/btrfs/send.c:609
fs/btrfs/send.c:615
fs/btrfs/send.c:622
fs/btrfs/send.c:628
fs/coda/upcall.c:60
fs/fat/dir.c:704
fs/nfsd/nfs2acl.c:16
fs/nfsd/nfs3acl.c:15
fs/nfsd/nfs3proc.c:17
fs/nfsd/nfs4xdr.c:114
fs/nfsd/nfs4xdr.c:84
fs/nfsd/nfs4xdr.c:98
fs/ntfs/super.c:114
fs/ntfs/super.c:124
fs/ntfs/super.c:132
fs/ntfs/super.c:144
fs/ntfs/super.c:156
fs/ntfs/super.c:164
fs/ntfs/super.c:171
fs/ufs/balloc.c:561
fs/xfs/xfs_error.h:43
fs/xfs/xfs_error.h:55
include/acpi/acoutput.h:372
include/acpi/acoutput.h:381
include/acpi/acoutput.h:391
include/acpi/acoutput.h:450
include/acpi/acoutput.h:451
include/acpi/acoutput.h:452
include/acpi/acoutput.h:453
include/acpi/acoutput.h:454
include/acpi/acoutput.h:455
include/acpi/acpixf.h:291
include/acpi/acpixf.h:294
include/acpi/acpixf.h:297
include/acpi/acpixf.h:314
include/acpi/acpixf.h:331
include/acpi/acpixf.h:348
include/acpi/platform/aclinux.h:105
include/acpi/platform/aclinux.h:107
include/acpi/platform/aclinux.h:109
include/acpi/platform/aclinux.h:111
include/acpi/platform/aclinux.h:113
include/drm/drmP.h:244
include/linux/ceph/decode.h:220
include/linux/ceph/decode.h:55
include/linux/compiler-gcc4.h:78
include/linux/genl_magic_func.h:178
include/linux/genl_magic_func.h:225
include/linux/genl_magic_func.h:357
include/linux/init.h:327
include/linux/init.h:333
include/linux/platform_device.h:277
include/linux/platform_device.h:291
include/linux/tracepoint.h:118
include/linux/usb/serial.h:398
include/linux/wait.h:208
include/trace/ftrace.h:322
include/trace/ftrace.h:331
include/trace/ftrace.h:346
kernel/bpf/core.c:249
kernel/bpf/core.c:250
kernel/events/internal.h:84
kernel/trace/trace_events.c:147
kernel/trace/trace_export.c:109
kernel/trace/trace_export.c:120
kernel/trace/trace_export.c:77
kernel/trace/trace_export.c:86
kernel/trace/trace_export.c:96
kernel/trace/trace_output.h:38
kernel/trace/trace_output.h:44
kernel/trace/trace_probe.c:39
kernel/trace/trace_probe.h:54
lib/inflate.c:232
lib/kstrtox.c:324
lib/lru_cache.c:46
lib/lzo/lzo1x_decompress_safe.c:30
lib/lzo/lzo1x_decompress_safe.c:36
lib/lzo/lzo1x_decompress_safe.c:42
lib/ts_fsm.c:151
lib/zlib_deflate/deflate.c:839
lib/zlib_inflate/inflate.c:196
net/bridge/netfilter/ebt_vlan.c:35
net/core/dev.c:7067
net/core/filter.c:421
net/decnet/netfilter/dn_rtmsg.c:101
net/ipv6/route.c:776
net/irda/irnet/irnet.h:364
net/irda/irnet/irnet.h:369
net/irda/irnet/irnet.h:375
net/mac80211/rx.c:2965
net/mac80211/rx.c:3023
net/mac80211/tx.c:1345
net/netfilter/nf_conntrack_h323_asn1.c:106
net/netfilter/nfnetlink_log.c:592
net/netfilter/nfnetlink_queue_core.c:800
net/rxrpc/ar-key.c:1133
net/rxrpc/ar-key.c:1139
net/rxrpc/ar-key.c:1150
net/rxrpc/rxkad.c:866
net/sched/cls_flow.c:126
net/sched/cls_flow.c:135
net/sched/cls_route.c:108
net/sched/cls_rsvp.h:119
net/vmw_vsock/af_vsock.c:1374
net/vmw_vsock/af_vsock.c:1455
net/wimax/debugfs.c:31
net/wireless/nl80211.c:4908
scripts/dtc/libfdt/fdt_rw.c:85
scripts/dtc/libfdt/fdt_sw.c:66
scripts/dtc/libfdt/libfdt_internal.h:58
sound/aoa/codecs/onyx.c:931
sound/isa/sb/emu8000_pcm.c:420
sound/oss/dmasound/dmasound.h:45
sound/oss/dmasound/dmasound_paula.c:188
sound/oss/dmasound/dmasound_paula.c:239
sound/oss/swarm_cs4297a.c:195
sound/pci/ca0106/ca0106_mixer.c:767
tools/lib/traceevent/trace-seq.c:41
tools/perf/builtin-kvm.c:1130
tools/perf/tests/attr.c:44
tools/perf/tests/keep-tracking.c:12
tools/perf/tests/keep-tracking.c:19
tools/perf/tests/perf-time-to-tsc.c:14
tools/perf/tests/perf-time-to-tsc.c:21
tools/perf/tests/sample-parsing.c:11
tools/perf/tests/sample-parsing.c:18
tools/perf/tests/tests.h:4
tools/perf/util/evsel.c:1251
tools/perf/util/parse-events.c:488
tools/perf/util/probe-event.c:2204
tools/perf/util/unwind-libunwind.c:154
tools/perf/util/unwind-libunwind.c:92
tools/testing/selftests/powerpc/utils.h:25
tools/testing/selftests/powerpc/utils.h:37
tools/usb/ffs-test.c:202
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH] checkpatch: Warn on macros with flow control statements
2014-09-10 8:43 ` Dan Carpenter
@ 2014-09-10 13:55 ` Joe Perches
-1 siblings, 0 replies; 19+ messages in thread
From: Joe Perches @ 2014-09-10 13:55 UTC (permalink / raw)
To: Dan Carpenter
Cc: Andrew Morton, devel, Dilger, Andreas, Greg Kroah-Hartman,
Peng Tao, kernel-janitors, linux-kernel, Drokin, Oleg,
Julia Lawall, Andy Whitcroft
On Wed, 2014-09-10 at 11:43 +0300, Dan Carpenter wrote:
> On Tue, Sep 09, 2014 at 01:38:13PM -0700, Joe Perches wrote:
> > Macros with flow control statements (goto and return) are
> > not very nice to read as any flow movement is unexpected.
break and continue are also flow control statements
but are those are frequently used in macros in
complete switch statements so were not added.
> > Try to highlight them and emit a warning on their definition.
> >
> > Avoid warning on macros that use argument concatenation as
> > those macros commonly create another function where the
> > concatenation is used in the function name definition like:
> > #define FOO_FUNC(name, rtn_type) \
> > rtn_type func##name(arg1, ...) \
> > { \
> > rtn_type rtn; \
> > [code...] \
> > return rtn; \
> > }
> >
>
> It adds 382 new warnings.
Thanks for running it over the tree.
> The '##' trick doesn't remove all then macros which create functions.
> I can't think of a better way to do that though.
Nor I. I suppose it could be a --strict CHK and not
a WARN message type though.
> We will eventually get rid of almost all the warnings in staging. The
> one that makes sense to keep is:
>
> drivers/staging/lustre/lnet/selftest/selftest.h:559
> #define STATE2STR(x) case x: return #x
Yup, there are a few of those and they should
definitely stay.
> My guess is that other maintainers won't be as excited to change these...
Do maintainers ever get excited about style?
> Some of the macros have "RETURN", "RET" or "EXIT" in the name so the
> return is not really hidden.
Not sure what to do about that.
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH] checkpatch: Warn on macros with flow control statements
@ 2014-09-10 13:55 ` Joe Perches
0 siblings, 0 replies; 19+ messages in thread
From: Joe Perches @ 2014-09-10 13:55 UTC (permalink / raw)
To: Dan Carpenter
Cc: Andrew Morton, devel, Dilger, Andreas, Greg Kroah-Hartman,
Peng Tao, kernel-janitors, linux-kernel, Drokin, Oleg,
Julia Lawall, Andy Whitcroft
On Wed, 2014-09-10 at 11:43 +0300, Dan Carpenter wrote:
> On Tue, Sep 09, 2014 at 01:38:13PM -0700, Joe Perches wrote:
> > Macros with flow control statements (goto and return) are
> > not very nice to read as any flow movement is unexpected.
break and continue are also flow control statements
but are those are frequently used in macros in
complete switch statements so were not added.
> > Try to highlight them and emit a warning on their definition.
> >
> > Avoid warning on macros that use argument concatenation as
> > those macros commonly create another function where the
> > concatenation is used in the function name definition like:
> > #define FOO_FUNC(name, rtn_type) \
> > rtn_type func##name(arg1, ...) \
> > { \
> > rtn_type rtn; \
> > [code...] \
> > return rtn; \
> > }
> >
>
> It adds 382 new warnings.
Thanks for running it over the tree.
> The '##' trick doesn't remove all then macros which create functions.
> I can't think of a better way to do that though.
Nor I. I suppose it could be a --strict CHK and not
a WARN message type though.
> We will eventually get rid of almost all the warnings in staging. The
> one that makes sense to keep is:
>
> drivers/staging/lustre/lnet/selftest/selftest.h:559
> #define STATE2STR(x) case x: return #x
Yup, there are a few of those and they should
definitely stay.
> My guess is that other maintainers won't be as excited to change these...
Do maintainers ever get excited about style?
> Some of the macros have "RETURN", "RET" or "EXIT" in the name so the
> return is not really hidden.
Not sure what to do about that.
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH] checkpatch: Warn on macros with flow control statements
2014-09-10 13:55 ` Joe Perches
@ 2014-09-10 14:06 ` Julia Lawall
-1 siblings, 0 replies; 19+ messages in thread
From: Julia Lawall @ 2014-09-10 14:06 UTC (permalink / raw)
To: Joe Perches
Cc: Dan Carpenter, Andrew Morton, devel, Dilger, Andreas,
Greg Kroah-Hartman, Peng Tao, kernel-janitors, linux-kernel,
Drokin, Oleg, Julia Lawall, Andy Whitcroft
On Wed, 10 Sep 2014, Joe Perches wrote:
> On Wed, 2014-09-10 at 11:43 +0300, Dan Carpenter wrote:
> > On Tue, Sep 09, 2014 at 01:38:13PM -0700, Joe Perches wrote:
> > > Macros with flow control statements (goto and return) are
> > > not very nice to read as any flow movement is unexpected.
>
> break and continue are also flow control statements
> but are those are frequently used in macros in
> complete switch statements so were not added.
Would it be possible to make a warning when there is a break or continue
but no while/switch/etc.
julia
> > > Try to highlight them and emit a warning on their definition.
> > >
> > > Avoid warning on macros that use argument concatenation as
> > > those macros commonly create another function where the
> > > concatenation is used in the function name definition like:
> > > #define FOO_FUNC(name, rtn_type) \
> > > rtn_type func##name(arg1, ...) \
> > > { \
> > > rtn_type rtn; \
> > > [code...] \
> > > return rtn; \
> > > }
> > >
> >
> > It adds 382 new warnings.
>
> Thanks for running it over the tree.
>
> > The '##' trick doesn't remove all then macros which create functions.
> > I can't think of a better way to do that though.
>
> Nor I. I suppose it could be a --strict CHK and not
> a WARN message type though.
>
> > We will eventually get rid of almost all the warnings in staging. The
> > one that makes sense to keep is:
> >
> > drivers/staging/lustre/lnet/selftest/selftest.h:559
> > #define STATE2STR(x) case x: return #x
>
> Yup, there are a few of those and they should
> definitely stay.
>
> > My guess is that other maintainers won't be as excited to change these...
>
> Do maintainers ever get excited about style?
>
> > Some of the macros have "RETURN", "RET" or "EXIT" in the name so the
> > return is not really hidden.
>
> Not sure what to do about that.
>
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH] checkpatch: Warn on macros with flow control statements
@ 2014-09-10 14:06 ` Julia Lawall
0 siblings, 0 replies; 19+ messages in thread
From: Julia Lawall @ 2014-09-10 14:06 UTC (permalink / raw)
To: Joe Perches
Cc: Dan Carpenter, Andrew Morton, devel, Dilger, Andreas,
Greg Kroah-Hartman, Peng Tao, kernel-janitors, linux-kernel,
Drokin, Oleg, Julia Lawall, Andy Whitcroft
On Wed, 10 Sep 2014, Joe Perches wrote:
> On Wed, 2014-09-10 at 11:43 +0300, Dan Carpenter wrote:
> > On Tue, Sep 09, 2014 at 01:38:13PM -0700, Joe Perches wrote:
> > > Macros with flow control statements (goto and return) are
> > > not very nice to read as any flow movement is unexpected.
>
> break and continue are also flow control statements
> but are those are frequently used in macros in
> complete switch statements so were not added.
Would it be possible to make a warning when there is a break or continue
but no while/switch/etc.
julia
> > > Try to highlight them and emit a warning on their definition.
> > >
> > > Avoid warning on macros that use argument concatenation as
> > > those macros commonly create another function where the
> > > concatenation is used in the function name definition like:
> > > #define FOO_FUNC(name, rtn_type) \
> > > rtn_type func##name(arg1, ...) \
> > > { \
> > > rtn_type rtn; \
> > > [code...] \
> > > return rtn; \
> > > }
> > >
> >
> > It adds 382 new warnings.
>
> Thanks for running it over the tree.
>
> > The '##' trick doesn't remove all then macros which create functions.
> > I can't think of a better way to do that though.
>
> Nor I. I suppose it could be a --strict CHK and not
> a WARN message type though.
>
> > We will eventually get rid of almost all the warnings in staging. The
> > one that makes sense to keep is:
> >
> > drivers/staging/lustre/lnet/selftest/selftest.h:559
> > #define STATE2STR(x) case x: return #x
>
> Yup, there are a few of those and they should
> definitely stay.
>
> > My guess is that other maintainers won't be as excited to change these...
>
> Do maintainers ever get excited about style?
>
> > Some of the macros have "RETURN", "RET" or "EXIT" in the name so the
> > return is not really hidden.
>
> Not sure what to do about that.
>
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH] checkpatch: Warn on macros with flow control statements
2014-09-10 14:06 ` Julia Lawall
@ 2014-09-10 14:36 ` Joe Perches
-1 siblings, 0 replies; 19+ messages in thread
From: Joe Perches @ 2014-09-10 14:36 UTC (permalink / raw)
To: Julia Lawall
Cc: Dan Carpenter, Andrew Morton, devel, Dilger, Andreas,
Greg Kroah-Hartman, Peng Tao, kernel-janitors, linux-kernel,
Drokin, Oleg, Andy Whitcroft
On Wed, 2014-09-10 at 16:06 +0200, Julia Lawall wrote:
> On Wed, 10 Sep 2014, Joe Perches wrote:
> > On Wed, 2014-09-10 at 11:43 +0300, Dan Carpenter wrote:
> > > On Tue, Sep 09, 2014 at 01:38:13PM -0700, Joe Perches wrote:
> > > > Macros with flow control statements (goto and return) are
> > > > not very nice to read as any flow movement is unexpected.
> >
> > break and continue are also flow control statements
> > but are those are frequently used in macros in
> > complete switch statements so were not added.
>
> Would it be possible to make a warning when there is a break or continue
> but no while/switch/etc.
I suppose the has_flow_statement could be extended.
Maybe something like:
if ($ctx =~ /\b(goto|return|break|continue)\b/ &&
$ctx !~ /\b(switch|if|do|while)\b/) {
has_flow_statement = 1;
}
but checkpatch isn't really capable of doing proper
flow logic analysis.
I'm not sure it's worthwhile.
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH] checkpatch: Warn on macros with flow control statements
@ 2014-09-10 14:36 ` Joe Perches
0 siblings, 0 replies; 19+ messages in thread
From: Joe Perches @ 2014-09-10 14:36 UTC (permalink / raw)
To: Julia Lawall
Cc: Dan Carpenter, Andrew Morton, devel, Dilger, Andreas,
Greg Kroah-Hartman, Peng Tao, kernel-janitors, linux-kernel,
Drokin, Oleg, Andy Whitcroft
On Wed, 2014-09-10 at 16:06 +0200, Julia Lawall wrote:
> On Wed, 10 Sep 2014, Joe Perches wrote:
> > On Wed, 2014-09-10 at 11:43 +0300, Dan Carpenter wrote:
> > > On Tue, Sep 09, 2014 at 01:38:13PM -0700, Joe Perches wrote:
> > > > Macros with flow control statements (goto and return) are
> > > > not very nice to read as any flow movement is unexpected.
> >
> > break and continue are also flow control statements
> > but are those are frequently used in macros in
> > complete switch statements so were not added.
>
> Would it be possible to make a warning when there is a break or continue
> but no while/switch/etc.
I suppose the has_flow_statement could be extended.
Maybe something like:
if ($ctx =~ /\b(goto|return|break|continue)\b/ &&
$ctx !~ /\b(switch|if|do|while)\b/) {
has_flow_statement = 1;
}
but checkpatch isn't really capable of doing proper
flow logic analysis.
I'm not sure it's worthwhile.
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH] checkpatch: Warn on macros with flow control statements
2014-09-10 14:36 ` Joe Perches
(?)
@ 2014-09-10 14:52 ` Drokin, Oleg
-1 siblings, 0 replies; 19+ messages in thread
From: Drokin, Oleg @ 2014-09-10 14:52 UTC (permalink / raw)
To: Joe Perches
Cc: Julia Lawall, Dan Carpenter, Andrew Morton,
<devel@driverdev.osuosl.org>,
Dilger, Andreas, Greg Kroah-Hartman, Peng Tao,
<kernel-janitors@vger.kernel.org>,
<linux-kernel@vger.kernel.org>,
Andy Whitcroft
Hello!
On Sep 10, 2014, at 10:36 AM, Joe Perches wrote:
> On Wed, 2014-09-10 at 16:06 +0200, Julia Lawall wrote:
>> On Wed, 10 Sep 2014, Joe Perches wrote:
>>> On Wed, 2014-09-10 at 11:43 +0300, Dan Carpenter wrote:
>>>> On Tue, Sep 09, 2014 at 01:38:13PM -0700, Joe Perches wrote:
>>>>> Macros with flow control statements (goto and return) are
>>>>> not very nice to read as any flow movement is unexpected.
>>>
>>> break and continue are also flow control statements
>>> but are those are frequently used in macros in
>>> complete switch statements so were not added.
>>
>> Would it be possible to make a warning when there is a break or continue
>> but no while/switch/etc.
>
> I suppose the has_flow_statement could be extended.
>
> Maybe something like:
>
> if ($ctx =~ /\b(goto|return|break|continue)\b/ &&
> $ctx !~ /\b(switch|if|do|while)\b/) {
> has_flow_statement = 1;
> }
>
> but checkpatch isn't really capable of doing proper
> flow logic analysis.
While possibly not really suitable in checkpatch, it might be a good addition to
some static code analyzer as a "future bugs possible due to this" check.
Bye,
Oleg
^ permalink raw reply [flat|nested] 19+ messages in thread