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

[punit vara]

On Sun, Oct 11, 2015 at 1:52 AM, punit vara <punitvara@xxxxxxxxx> wrote:
> On Sun, Oct 11, 2015 at 1:20 AM, Julia Lawall <julia.lawall@xxxxxxx> wrote:
>>
>>
>> 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
>
> Thanks Julia I didn't know about that dwarf utility I will go through
> it .sorry I have forgot to write MacAddress.
>
> So here solution could be either
>
> ether_addr_equal(bssid, pnetwork->network.MacAddress) or
> ether_addr_equal_unaligned(bssid->network.MacAddress)
>
> I will learn debugging with help of pahole and again send the patch.

Thanks Julia I didn't know about that dwarves utility I will go through
it .sorry I have forgot to write MacAddress.

So here solution could be either

ether_addr_equal(bssid, pnetwork->network.MacAddress) or
ether_addr_equal_unaligned(bssid, pnetwork->network.MacAddress)

I will learn debugging with help of pahole and again send the patch.
Sorry for inconvenience previous mail send before adding because my
laptop was hanged.
--
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/