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]

Message-ID: <20200325094305.15f9ea24@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Wed, 25 Mar 2020 09:43:05 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Ido Schimmel <idosch@...sch.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 Wed, 25 Mar 2020 11:41:43 +0200 Ido Schimmel 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".

Hm, why the label exists to me does not mean identify the source of the
jump. It's quite logical to name the label after the target, because
then you can just read the body of the function, and validate the jumps
undo the right things based on their names. Is the reason to name
labels after source so that the label doesn't have to be renamed if
steps are inserted in the middle? Can't think of anything else.

Anyway, it's not a big deal, but please be prepared for me to keep
bringing this up :)

> 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.