linux-kernel - Re: [PATCH v2 15/15] staging: r8188eu: correct long line warnings near prior DBG

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, 16 Feb 2022 23:45:29 +0000
From:   Phillip Potter <phil@...lpotter.co.uk>
To:     David Laight <David.Laight@...lab.com>
Cc:     "dan.carpenter@...cle.com" <dan.carpenter@...cle.com>,
        "Larry.Finger@...inger.net" <Larry.Finger@...inger.net>,
        "straube.linux@...il.com" <straube.linux@...il.com>,
        "martin@...ser.cx" <martin@...ser.cx>,
        "linux-staging@...ts.linux.dev" <linux-staging@...ts.linux.dev>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "paskripkin@...il.com" <paskripkin@...il.com>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>
Subject: Re: [PATCH v2 15/15] staging: r8188eu: correct long line warnings
 near prior DBG_88E calls

On Wed, Feb 16, 2022 at 10:01:18AM +0000, David Laight wrote:
> From: Phillip Potter
> > Sent: 16 February 2022 01:07
> > 
> > Where it is possible (without out-of-patch-series-scope large scale
> > refactoring), correct code to remove checkpatch warnings about lines
> > that are too long, also correcting operator spacing where appropriate
> > for these lines as well. These warnings occur mostly due to so many
> > DBG_88E removals and parentheses tweaks etc. being adjacent to such
> > long lines, which are therefore included in the resultant diff.
> ...
> 
> Somewhere my copy of this seems to have got its tabs deleted.
> I blame outlook :-)
> 
> > diff --git a/drivers/staging/r8188eu/core/rtw_br_ext.c b/drivers/staging/r8188eu/core/rtw_br_ext.c
> > index ddc3a2c8aaca..d68611ef22f8 100644
> > --- a/drivers/staging/r8188eu/core/rtw_br_ext.c
> > +++ b/drivers/staging/r8188eu/core/rtw_br_ext.c
> > @@ -382,7 +382,7 @@ int nat25_db_handle(struct adapter *priv, struct sk_buff *skb, int method)
> >  if (protocol == ETH_P_IP) {
> >  struct iphdr *iph = (struct iphdr *)(skb->data + ETH_HLEN);
> > 
> > -if (((unsigned char *)(iph) + (iph->ihl<<2)) >= (skb->data + ETH_HLEN + skb->len))
> > +if (((unsigned char *)(iph) + (iph->ihl << 2)) >= (skb->data + ETH_HLEN + skb->len))
> 
> You can delete at least three sets of () from that line.
> 
> >  return -1;
> > 
> >  switch (method) {
> > @@ -451,7 +451,11 @@ int nat25_db_handle(struct adapter *priv, struct sk_buff *skb, int method)
> >  pOldTag = (struct pppoe_tag *)__nat25_find_pppoe_tag(ph, ntohs(PTT_RELAY_SID));
> >  if (pOldTag) { /*  if SID existed, copy old value and delete it */
> >  old_tag_len = ntohs(pOldTag->tag_len);
> > -if (old_tag_len+TAG_HDR_LEN+MAGIC_CODE_LEN+RTL_RELAY_TAG_LEN > sizeof(tag_buf))
> > +if (old_tag_len +
> > +    TAG_HDR_LEN +
> > +    MAGIC_CODE_LEN +
> > +    RTL_RELAY_TAG_LEN >
> > +    sizeof(tag_buf))
> >  return -1;
> 
> That change really doesn't help readability at all.
> There isn't much point shortening it that much like that, especially
> since the here is a line that is nearly as long just above.
> 
> The real fix is to reduce the number of levels of indentation
> to something sane.
> I suspect that use of continue, break and return will help.
> 
> The other line length changes have much the same problem
> but not as sever.
> 
> 	David
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
> 

Dear David,

Thank you for your feedback, and yes I totally agree - this patch was
more for procedure's sake to quieten the checkpatch warnings, but I was
in two minds about whether I should include it.

The indentation level is absolutely what is the problem here, but it is
arguably not in scope for this particular patch set given these are
pre-existing lines that have the issue. Certainly needs fixing though
for sure - just that this is more substantial and worthy of a separate
patch set in my opinion.

Looks like I need to do V3 anyway as I missed an unused-but-set warning
in patch 5 of the series. I may therefore drop this patch in V3 and
perhaps work on the indentation problem more generally.

Regards,
Phil