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>] [day] [month] [year] [list]
Date:   Fri, 25 Aug 2017 17:57:16 -0700
From:   Joe Perches <joe@...ches.com>
To:     Harsha Sharma <harshasharmaiitr@...il.com>
Cc:     gregkh@...uxfoundation.org, devel@...verdev.osuosl.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] staging:rtl8712:xmit_linux.c: Avoid CamelCase

On Sat, 2017-08-26 at 01:21 +0530, Harsha Sharma wrote:
> Hello,
> 
> Sorry, this was my first contribution in linux-kernel. I will take care
> about this from next time.
> Do I need to send these patches once again one by one?

No, just pick the "best" one of what you sent
without any of the "style" modifications like
the 80 column adjustments and see what happens.

Read the code outside of staging for awhile
to get a feel for what style seems appropriate
before you send style-only patches.

For instance, this proposed change:

diff --git a/drivers/staging/unisys/visornic/visornic_main.c b/drivers/staging/unisys/visornic/visornic_main.c
> index 0b39676..57fc65f 100644
> --- a/drivers/staging/unisys/visornic/visornic_main.c
> +++ b/drivers/staging/unisys/visornic/visornic_main.c
> @@ -253,10 +253,9 @@ static int visor_copy_fragsinfo_from_skb(struct sk_buff *skb,
>  		for (frag = 0; frag < numfrags; frag++) {
>  			count = add_physinfo_entries(page_to_pfn(
>  				skb_frag_page(&skb_shinfo(skb)->frags[frag])),
> -					      skb_shinfo(skb)->frags[frag].
> -					      page_offset,
> -					      skb_shinfo(skb)->frags[frag].
> -					      size, count, frags_max, frags);
> +				skb_shinfo(skb)->frags[frag].page_offset,
> +				skb_shinfo(skb)->frags[frag].size,
> +				count, frags_max, frags);
>  			/* add_physinfo_entries only returns
>  			 * zero if the frags array is out of room
>  			 * That should never happen because we

while it's nice that you put the multi-line dereferences
together, it's not nice that you change the indentation.

Sometimes it's simply better to exceed 80 columns.
But here, it'd probably be nicer to use temporaries.

Something like:
---
 drivers/staging/unisys/visornic/visornic_main.c | 26 +++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/unisys/visornic/visornic_main.c b/drivers/staging/unisys/visornic/visornic_main.c
index 2891622eef18..aafd849dc1e8 100644
--- a/drivers/staging/unisys/visornic/visornic_main.c
+++ b/drivers/staging/unisys/visornic/visornic_main.c
@@ -195,7 +195,7 @@ visor_copy_fragsinfo_from_skb(struct sk_buff *skb, unsigned int firstfraglen,
 			      unsigned int frags_max,
 			      struct phys_info frags[])
 {
-	unsigned int count = 0, frag, size, offset = 0, numfrags;
+	unsigned int count = 0, size, offset = 0, numfrags;
 	unsigned int total_count;
 
 	numfrags = skb_shinfo(skb)->nr_frags;
@@ -234,21 +234,23 @@ visor_copy_fragsinfo_from_skb(struct sk_buff *skb, unsigned int firstfraglen,
 		count++;
 	}
 	if (numfrags) {
+		int i;
+
 		if ((count + numfrags) > frags_max)
 			return -EINVAL;
 
-		for (frag = 0; frag < numfrags; frag++) {
-			count = add_physinfo_entries(page_to_pfn(
-				skb_frag_page(&skb_shinfo(skb)->frags[frag])),
-					      skb_shinfo(skb)->frags[frag].
-					      page_offset,
-					      skb_shinfo(skb)->frags[frag].
-					      size, count, frags_max, frags);
-			/* add_physinfo_entries only returns
-			 * zero if the frags array is out of room
-			 * That should never happen because we
-			 * fail above, if count+numfrags > frags_max.
+		for (i = 0; i < numfrags; i++) {
+			skb_frag_t *frag = &skb_shinfo(skb)->frags[i];
+			unsigned long pfn = page_to_pfn(skb_frag_page(frag));
+
+			/* add_physinfo_entries only returns zero if the
+			 * frags array is out of room.
+			 * That should never happen because we fail above,
+			 * if count+numfrags > frags_max.
 			 */
+			count = add_physinfo_entries(pfn, frag->page_offset,
+						     frag->size, count,
+						     frags_max, frags);
 			if (!count)
 				return -EINVAL;
 		}

Powered by blists - more mailing lists