patch splitting

patch splitting

[Tobin C. Harding]

Is it easier to review this change for correctness if it is three
patches or one?

TLDR;
+	struct wpa_key_t *key = &priv->wpa.key[index];

-	memcpy(&priv->wpa.key[index].rx_seq[0], enc->rx_seq, IW_ENCODE_SEQ_MAX_SIZE);
+	memcpy(key->rx_seq, enc->rx_seq, IW_ENCODE_SEQ_MAX_SIZE);                       




Brief description of steps:
1. Add local pointer variable, defined to correct memory location.
2. Use newly defined local variable where suitable.
3. Remove unnecessary address operator (reasoning specified below).

Extended description of steps:

1. Add local variable (defined in one statement here for brevity)

struct wpa_key_t *key = &priv->wpa.key[index];

2. Use newly defined local variable where suitable.

sample original code

...
} else if (enc->ext_flags & IW_ENCODE_EXT_RX_SEQ_VALID) {
               memcpy(&priv->wpa.key[index].rx_seq[0],
                       enc->rx_seq, IW_ENCODE_SEQ_MAX_SIZE);
               ...

sample code after step 2:
...
} else if (enc->ext_flags & IW_ENCODE_EXT_RX_SEQ_VALID) {
               memcpy(&key->rx_seq[0], enc->rx_seq, IW_ENCODE_SEQ_MAX_SIZE);
               ...

Justification for step 3:

In the above call to memcpy() taking the address of the initial
element of the array adds no extra information to the call since the
source variable is named identically and does not take the address of
the first element.

sample final state:
...
} else if (enc->ext_flags & IW_ENCODE_EXT_RX_SEQ_VALID) {
               memcpy(key->rx_seq, enc->rx_seq, IW_ENCODE_SEQ_MAX_SIZE);
               ...

thanks for taking the time to read.

Tobin.

patch splitting

[valdis.kletnieks at vt.edu]

On Thu, 30 Mar 2017 11:22:40 +1100, "Tobin C. Harding" said:
> Is it easier to review this change for correctness if it is three
> patches or one?
>
> TLDR;
> +	struct wpa_key_t *key = &priv->wpa.key[index];
>
> -	memcpy(&priv->wpa.key[index].rx_seq[0], enc->rx_seq, IW_ENCODE_SEQ_MAX_SIZE);
> +	memcpy(key->rx_seq, enc->rx_seq, IW_ENCODE_SEQ_MAX_SIZE);

One patch for one thing.

> Brief description of steps:
> 1. Add local pointer variable, defined to correct memory location.
> 2. Use newly defined local variable where suitable.
> 3. Remove unnecessary address operator (reasoning specified below).

So, is this 3 things?  Or one thing:

"Simplify overly-complex first argument to memcpy()"?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 484 bytes
Desc: not available
Url : http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20170329/9422ca32/attachment.bin 

patch splitting

[Tobin C. Harding]

On Wed, Mar 29, 2017 at 08:55:02PM -0400, valdis.kletnieks at vt.edu wrote:
> On Thu, 30 Mar 2017 11:22:40 +1100, "Tobin C. Harding" said:
> > Is it easier to review this change for correctness if it is three
> > patches or one?
> >
> > TLDR;
> > +	struct wpa_key_t *key = &priv->wpa.key[index];
> >
> > -	memcpy(&priv->wpa.key[index].rx_seq[0], enc->rx_seq, IW_ENCODE_SEQ_MAX_SIZE);
> > +	memcpy(key->rx_seq, enc->rx_seq, IW_ENCODE_SEQ_MAX_SIZE);
> 
> One patch for one thing.
> 
> > Brief description of steps:
> > 1. Add local pointer variable, defined to correct memory location.
> > 2. Use newly defined local variable where suitable.
> > 3. Remove unnecessary address operator (reasoning specified below).
> 
> So, is this 3 things?  Or one thing:
> 
> "Simplify overly-complex first argument to memcpy()"?


Thanks Valdis, very well explained.

Tobin.