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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 25 Mar 2020 11:41:43 +0200
From:   Ido Schimmel <idosch@...sch.org>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, jiri@...lanox.com,
        andrew@...n.ch, f.fainelli@...il.com, vivien.didelot@...il.com,
        roopa@...ulusnetworks.com, nikolay@...ulusnetworks.com,
        mlxsw@...lanox.com, Ido Schimmel <idosch@...lanox.com>
Subject: Re: [PATCH net-next 01/15] devlink: Add packet trap policers support

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