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

[Jiri Pirko <jiri@resnulli.us>]

Wed, Mar 25, 2020 at 10:41:43AM CET, idosch@idosch.org wrote:
>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.

Yeah, let's have this per-code. In devlink, this is how it's done. Let's
be consistent.


>
>> 
>> > +	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);