Re: [PATCH 2/4] Staging: rtl8712: Use ether_addr_equal() over memcmp()

[Julia Lawall]

On Sun, 11 Oct 2015, punit vara wrote:

> On Sat, Oct 10, 2015 at 11:04 PM, Larry Finger
> <Larry.Finger@xxxxxxxxxxxx> wrote:
> > On 10/10/2015 11:58 AM, Punit Vara wrote:
> >>
> >> This patch is to the rtl871x_ioctl_linux.c file that fixes up following
> >> warning reported by checkpatch.pl :
> >>
> >> -Prefer ether_addr_equal() or ether_addr_equal_unaligned() over memcmp()
> >>
> >> bssid has datatype u8 and pnetwork->network.MacAddress has data type
> >>   unsigned char.
> >>
> >> Signed-off-by: Punit Vara <punitvara@xxxxxxxxx>
> >> ---
> >>   drivers/staging/rtl8712/rtl871x_ioctl_linux.c | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
> >> b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
> >> index 2ba055d..1c9092e 100644
> >> --- a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
> >> +++ b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
> >> @@ -2007,7 +2007,7 @@ static int r871x_get_ap_info(struct net_device *dev,
> >>                         return -EINVAL;
> >>                 }
> >>                 netdev_info(dev, "r8712u: BSSID:%pM\n", bssid);
> >> -               if (!memcmp(bssid, pnetwork->network.MacAddress,
> >> ETH_ALEN)) {
> >> +               if (!ether_addr_equal(bssid, pnetwork->network)) {
> >>                         /* BSSID match, then check if supporting wpa/wpa2
> >> */
> >>                         pbuf =
> >> r8712_get_wpa_ie(&pnetwork->network.IEs[12],
> >>                                &wpa_ielen, pnetwork->network.IELength-12);
> >>
> >
> > The types of the variables are not what is important - it is the alignment!
> >
> > I suggest that you read the definition of ether_addr_equal() and consider
> > what happens if either of the two addresses is not aligned to u16!
> >
> > You also have a logic error. Routine memcmp() returns zero when the two
> > arguments are equal. Thus !memcmp() will be true when they are equal.
> > Routine ether_addr_equal() returns true when the arguments are equal.
> > Whenever !memcmp() is correct and the arguments are aligned, then you need
> > to replace it with ether_addr_equal().
> >
> > The checkpatch script is a good tool for locating things to be changed;
> > however, if the change is other than cosmetic, you *MUST THINK* about what
> > is happening. Yes, rtl8712 is ugly code, but it works. Please do not break
> > it.
> >
> > NACK.
> >
> > Larry
> >
> 
> @Larry I am thankful for your high quality feedback.
> if both address are aligned to u16 then we should use
> ether_addr_equal() if not then ether_addr_equal_unaligned().
> 
> Both return true if addr1 and addr2 matches. If I am understand
> correctly bssid and MacAdress is not aligned to u16 .So right
> correction would be
> if(ether_addr_equal_unaligned(bssid, pnetwork->network))
> 
> right ?

You can use pahole to check the alignment.  Use git log to find other 
patches that mention the use of this tool to see what information to 
provide in the commit message.

julia
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/