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

Submitter:  goas <bgab001>
Submitted:  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>
Group 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>
Group 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>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

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)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    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.13-d3ae.
    Corresponding source code