netdev - Re: [PATCH v1 2/6] staging: qlge: coredump via devlink health reporter

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] [day] [month] [year] [list]

Message-ID: <20201013011810.GB41031@f3>
Date:   Tue, 13 Oct 2020 10:18:10 +0900
From:   Benjamin Poirier <benjamin.poirier@...il.com>
To:     Coiby Xu <coiby.xu@...il.com>
Cc:     devel@...verdev.osuosl.org, Shung-Hsi Yu <shung-hsi.yu@...e.com>,
        Manish Chopra <manishc@...vell.com>,
        "supporter:QLOGIC QLGE 10Gb ETHERNET DRIVER" 
        <GR-Linux-NIC-Dev@...vell.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "open list:QLOGIC QLGE 10Gb ETHERNET DRIVER" <netdev@...r.kernel.org>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v1 2/6] staging: qlge: coredump via devlink health
 reporter

On 2020-10-12 19:51 +0800, Coiby Xu wrote:
> On Sat, Oct 10, 2020 at 10:22:30PM +0900, Benjamin Poirier wrote:
> > On 2020-10-10 18:02 +0800, Coiby Xu wrote:
> > [...]
> > > > > +	do {                                                           \
> > > > > +		err = fill_seg_(fmsg, &dump->seg_hdr, dump->seg_regs); \
> > > > > +		if (err) {					       \
> > > > > +			kvfree(dump);                                  \
> > > > > +			return err;				       \
> > > > > +		}                                                      \
> > > > > +	} while (0)
> > > > > +
> > > > > +static int qlge_reporter_coredump(struct devlink_health_reporter *reporter,
> > > > > +				  struct devlink_fmsg *fmsg, void *priv_ctx,
> > > > > +				  struct netlink_ext_ack *extack)
> > > > > +{
> > > > > +	int err = 0;
> > > > > +
> > > > > +	struct qlge_devlink *dev = devlink_health_reporter_priv(reporter);
> > > >
> > > > Please name this variable ql_devlink, like in qlge_probe().
> > > 
> > > I happened to find the following text in drivers/staging/qlge/TODO
> > > > * in terms of namespace, the driver uses either qlge_, ql_ (used by
> > > >  other qlogic drivers, with clashes, ex: ql_sem_spinlock) or nothing (with
> > > >  clashes, ex: struct ob_mac_iocb_req). Rename everything to use the "qlge_"
> > > >  prefix.
> > 
> > This comment applies to global identifiers, not local variables.
> 
> Thank you for the explanation! Are you suggesting we should choose
> different naming styles so we better tell global identifiers from local
> variables?

That's not the main purpose IMO. Using a consistent prefix for global
identifiers (ex. "qlge_") is to avoid clashes (two drivers using the
same name, as in the examples above). Strictly speaking, it is not a
problem for symbols with internal linkage (ex. static functions) or type
definitions in local header files but it makes the code clearer because
it shows immediately that this identifier was defined in the driver.

For local variables, the name is more a matter of personal taste I think
but it should be consistent within the driver and with other users of
the same api, where applicable. A prefix is not needed but the name is
sometimes a simpler version of a type name which includes a prefix.

> > > So I will adopt qlge_ instead. Besides I prefer qlge_dl to ql_devlink.
> > 
> > Up to you but personally, I think ql_devlink is better. In any case,
> > "dev" is too general and often used for struct net_device pointers
> > instead.
> 
> Thank you for the suggestion. Another reason to use qlge_dl is many
> other network drivers supporting devlink interface also adopt this kind
> of style.

Sounds good. On second thought I regretted suggesting ql_devlink. While
local variable don't need any prefix; if they do have one, better not
mix qlge_ and ql_.