Re: [PATCH v5 1/5] netdev: ethernet dev_alloc_skb to netdev_alloc_skb

From: Pradeep A. Dalvi
Date: Mon Feb 06 2012 - 12:37:44 EST


On Mon, Feb 6, 2012 at 10:15 PM, David Miller <davem@xxxxxxxxxxxxx> wrote:
> From: "Pradeep A. Dalvi" <netdev@xxxxxxxxxxxxxxxx>
> Date: Sun,  5 Feb 2012 18:19:09 +0530
>
>> From: Pradeep A Dalvi <netdev@xxxxxxxxxxxxxxxx>
>>
>> Replaced deprecating dev_alloc_skb with netdev_alloc_skb in drivers/net/ethernet
>>   - Removed extra skb->dev = dev after netdev_alloc_skb
>>
>> Signed-off-by: Pradeep A Dalvi <netdev@xxxxxxxxxxxxxxxx>
>
> Applied, but I had to fix several things up:
>
>> -                     if (pkt_len < rx_copybreak && (skb = dev_alloc_skb(pkt_len + 2)) != NULL) {
>> +                     if (pkt_len < rx_copybreak &&
>> +                                     (skb = netdev_alloc_skb(dev, pkt_len + 2)) != NULL) {
>
> This is not the correct way to format a multi-line conditional.
>
> All subsequent lines must start at the first column after the initial line's
> openning parenthesis:
>
>        if (format_it &&
>            like_this)
>
>        if (not &&
>                like_this)
>
> To be honest I have no idea what posses people to tab things out in such
> an incredibly ugly fashion in the first place.
>
>> -             if (!(lp->rx_skbuff[i] = dev_alloc_skb(lp->rx_buff_len))) {
>> +             lp->rx_skbuff[i] = netdev_alloc_skb(dev, lp->rx_buff_len);
>> +             if (!lp->rx_skbuff[i]) {
>
> You properly leave this test alone and keep it as "!foo" yet:
>
>> -                     if(!(new_skb = dev_alloc_skb(lp->rx_buff_len))){
>> +                     new_skb = netdev_alloc_skb(dev, lp->rx_buff_len);
>> +                     if (new_skb == NULL) {
>
> You change this one to the undesirable "== NULL" test, don't do that.
> "!foo" is the canonical and most efficient NULL pointer test.
>
>> -      skb = dev_alloc_skb(pkt_len+2);
>> +     skb = netdev_alloc_skb(dev, pkt_len + 2);
>
> Do not change the indentation in one place when the entire rest of the source
> file uses something else, fixing that would a seperate change from what you're
> doing.
>
>> -         skb = dev_alloc_skb(RX_BUFLEN + 2);
>> +             skb = netdev_alloc_skb(RX_BUFLEN + 2);
>
> Same problem.

Thanks a lot! I will make note of these points and shall not repeat again.

Regards,
Pradeep A. Dalvi
--
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/