Re: [PATCH net-next 01/15] devlink: Add packet trap policers support

[Ido Schimmel <idosch@idosch.org>]

On Tue, Mar 24, 2020 at 08:31:09PM -0700, Jakub Kicinski wrote:
> On Tue, 24 Mar 2020 21:32:36 +0200 Ido Schimmel wrote:
> > +/**
> > + * devlink_trap_policers_register - Register packet trap policers with devlink.
> > + * @devlink: devlink.
> > + * @policers: Packet trap policers.
> > + * @policers_count: Count of provided packet trap policers.
> > + *
> > + * Return: Non-zero value on failure.
> > + */
> > +int
> > +devlink_trap_policers_register(struct devlink *devlink,
> > +			       const struct devlink_trap_policer *policers,
> > +			       size_t policers_count)
> > +{
> > +	int i, err;
> > +
> > +	mutex_lock(&devlink->lock);
> > +	for (i = 0; i < policers_count; i++) {
> > +		const struct devlink_trap_policer *policer = &policers[i];
> > +
> > +		if (WARN_ON(policer->id == 0)) {
> > +			err = -EINVAL;
> > +			goto err_trap_policer_verify;
> > +		}
> > +
> > +		err = devlink_trap_policer_register(devlink, policer);
> > +		if (err)
> > +			goto err_trap_policer_register;
> > +	}
> > +	mutex_unlock(&devlink->lock);
> > +
> > +	return 0;
> > +
> > +err_trap_policer_register:
> > +err_trap_policer_verify:
> 
> nit: as you probably know the label names are not really in compliance
> with:
> https://www.kernel.org/doc/html/latest/process/coding-style.html#centralized-exiting-of-functions
> ;)

Hi Jakub, thanks for the thorough review!

I assume you're referring to the fact that the label does not say what
the goto does? It seems that the coding style guide also allows for
labels that indicate why the label exists: "Choose label names which say
what the goto does or why the goto exists".

This is the form I'm used to, but I do adjust the names in code that
uses the other form (such as in netdevsim).

I already used this form in previous devlink submissions, so I would
like to stick to it unless you/Jiri have strong preference here.

> 
> > +	for (i--; i >= 0; i--)
> > +		devlink_trap_policer_unregister(devlink, &policers[i]);
> > +	mutex_unlock(&devlink->lock);
> > +	return err;
> > +}
> > +EXPORT_SYMBOL_GPL(devlink_trap_policers_register);