[PATCH 6/7] staging: lustre: obdclass: expand the GOTO macro + break
Julia Lawall
julia.lawall at lip6.fr
Tue Sep 9 13:05:41 UTC 2014
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
More information about the devel
mailing list