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:46 +0100
From:   Jiri Pirko <jiri@...nulli.us>
To:     Ido Schimmel <idosch@...sch.org>
Cc:     Jakub Kicinski <kuba@...nel.org>, 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

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