netdev - Re: [PATCH v5 09/17] wifi: cc33xx: Add main.c

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: <59b318b2d6719a009189e10949df35f855790d63.camel@sipsolutions.net>
Date: Tue, 12 Nov 2024 16:39:45 +0100
From: Johannes Berg <johannes@...solutions.net>
To: "Nemanov, Michael" <michael.nemanov@...com>, Kalle Valo
 <kvalo@...nel.org>,  "David S . Miller" <davem@...emloft.net>, Eric Dumazet
 <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni
 <pabeni@...hat.com>, Rob Herring <robh@...nel.org>,  Krzysztof Kozlowski
 <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
 linux-wireless@...r.kernel.org, netdev@...r.kernel.org, 
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Cc: Sabeeh Khan <sabeeh-khan@...com>
Subject: Re: [PATCH v5 09/17] wifi: cc33xx: Add main.c

On Tue, 2024-11-12 at 17:34 +0200, Nemanov, Michael wrote:
> 
> > > +static int parse_control_message(struct cc33xx *cc,
> > > +				 const u8 *buffer, size_t buffer_length)
> > > +{
> > > +	u8 *const end_of_payload = (u8 *const)buffer + buffer_length;
> > > +	u8 *const start_of_payload = (u8 *const)buffer;
> > 
> > I don't think the "u8 *const" is useful here, and the cast is awkward.
> > If anything you'd want "const u8 *const" (which should make it not need
> > the cast), but the const you have adds no value... do you even know what
> > it means? ;-)
> > 
> 
> My intent was to express that start and end pointers are fixed and will 
> not change in the loop below. When reading this again I agree this hurts 
> more than it helps, I'll drop it.

Well, I don't even mind the const so much rather than the cast, I'd
probably not have commented on it if it were

	const u8 *const end_of_payload = buffer + buffer_length;
	const u8 *const start_of_payload = buffer;

I'd still think the second const (for the variable) isn't all that
useful, but really the lack of first const (for the object pointed to)
makes the casts necessary and (IMHO) that's what hurts.

> const u8 *buffer in the prototype illustrates that parse_control_message 
> will not change the data so I'll keep it if there a re no objections.

Sure.

> > > +	struct NAB_header *nab_header;
> > 
> > surely checkpatch complained about CamelCase or so with the struct name
> > like that?
> > 
> 
> Double-checked, no warnings from checkpatch:

Hah, ok :) I'm surprised because it complained about _Generic in my
patch, and that's something you really can't even change since it's C11
standard ...

johannes