buglwIP - A Lightweight TCP/IP stack - Bugs: bug #43499, Wrong argument order in...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #43499: Wrong argument order in src/include/netif/etharp.h defines

Submitted by:  goas <bgab001>
Submitted on:  Wed 29 Oct 2014 05:29:45 PM UTC  
 
Category:  None Severity:  2 - Minor
Item Group:  Change Request Status:  Fixed
Privacy:  Public Assigned to:  None
Open/Closed:  Closed Planned Release:  None
lwIP version:  git head

Wed 29 Oct 2014 06:01:02 PM UTC, comment #2: 

> not exactly (as far as I understood it) : the calls to the functions (from etharp.c, and probably only there) don't have to change, and would then be correctly self-documented.
> See (etharp.c@439)
>   ETHADDR32_COPY(&ethhdr->dest, dst);
>   ETHADDR16_COPY(&ethhdr->src, src);
> which expands to
>   SMEMCPY(&ethhdr->dest, dst , ETHARP_HWADDR_LEN);
>   SMEMCPY(&ethhdr->src, src , ETHARP_HWADDR_LEN);
> which expands to
>   memcpy(&ethhdr->dest, dst , ETHARP_HWADDR_LEN);
>   memcpy(&ethhdr->src, src , ETHARP_HWADDR_LEN);
>
> The copy is done from the right side variable onto the left side variable anyway, so I don't see why the macro
>  #define ETHADDR32_COPY(src, dst)  SMEMCPY(src, dst, ETHARP_HWADDR_LEN)
> names the destination src and the source dst.
> Even if it does work...
>
> No API change is needed anywhere if you swap BOTH src and dst names. Just the "self documentation" (= this h file) is fixed.
> As far as I know ;-) And it worked on my target.
> Except if I wrote my bug report wrong, I'm not suggesting to change only one of the dst,src pair, which would indeed force all user code to


Indeed, this wasn't clear at first for me this was only about a cosmetic change.

Fixed, thank you ! :)

Sylvain Rochet <gradator>
Project Member
Wed 29 Oct 2014 05:35:09 PM UTC, comment #1: 

What you are suggesting is a silent API break which is going to put ALL lwIP users on nerves.

Sylvain Rochet <gradator>
Project Member
Wed 29 Oct 2014 05:29:45 PM UTC, original submission:  

Hi

In file src/include/netif/etharp.h, there are the macros
#define ETHADDR32_COPY(src, dst)  SMEMCPY(src, dst, ETHARP_HWADDR_LEN)
#define ETHADDR16_COPY(src, dst)  SMEMCPY(src, dst, ETHARP_HWADDR_LEN)

It works fine, but in reality all memcpy() functions I ever saw actually use (dst, src), as does the one on my ARM board, called after expanding all macros.
So it's easy to mess up a port or not understand what the code is supposed to do with this wrong argument order.

Can it be fixed to the following ?
#define ETHADDR32_COPY(dst, src)  SMEMCPY(dst, src, ETHARP_HWADDR_LEN)
#define ETHADDR16_COPY(dst, src)  SMEMCPY(dst, src, ETHARP_HWADDR_LEN)

B.G.

goas <bgab001>

 

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by gradator (Posted a comment)
  • -email is unavailable- added by bgab001 (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

     

     

    Follow 3 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2014-10-29 gradator Severity3 - Normal => 2 - Minor
        StatusNone => Fixed
        Open/ClosedOpen => Closed

    Back to the top


    Powered by Savane 3.5