My_ntoa()


  • Discourse touched me in a no-no place

    Copied verbatim (no anonymization, no changes, no nothing) from the source code generated by a recently departed (they're still alive) collegue:

    /*
    * my_ntoa -- IPv6 compatible inet_ntoa replacement
    *
    * Inputs:
    *
    * addr The IP address
    *
    * Returns:
    *
    * Pointer to the string representation of the IP address.
    *
    * This currently only supports IPv4.
    */
    const char *
    my_ntoa(struct in_addr addr) {
    static char ip_str[MAXLINE];
    const char *cp;

    cp = inet_ntop(AF_INET, &addr, ip_str, MAXLINE);

    return cp;
    }
    Lest you be distracted by it, MAXLINE was #defined as 255.


  • Honestly, I don't see the problem. I was once hired to do basically exactly this to some code. They wanted the ability to easily identify what needed to be changed to support IPv6, so the job was:

    1) Identify all parts of the code that needed to be modified to support IPv6.

    2) Isolate the code that needed to be modified into functions that only perform networking functions.

    3) Mark that code with a specific text tag (like 'only supports IPv4') in comments where code needs to be modified.

    Replacing calls to 'inet_ntoa' with calls to this function perfectly meets this, quite reasonable IMO, requirements.



  • @joelkatz said:

    Honestly, I don't see the problem. I was once hired to do basically exactly this to some code. They wanted the ability to easily identify what needed to be changed to support IPv6, so the job was: []cut]

    This maybe all very well, but inet_ntoa isn't code that needs to be modified to support IPv6 — the code using it is. And this my_ntoa is useless anyway, because it looks and does exactly the same as inet_ntoa.

    The "IPv6 compatible inet_ntoa replacement" already exists — it's inet_ntop. Now maybe a wrapper around it, which would return a pointer to a static buffer would be useful for porting old code, but my_ntoa is not it.

    Oh, and the name is silly.



  • Yup, although at first glance "IPv6 compatible inet_ntoa replacement" followed almost immediately by "This currently only supports IPv4." sounds like a WTF, it's just isolating code for the future. 

    I've done this kind of work before. 

    255 characters for the string representation seems a little large though, when 8*4+7 = 39 is good enough (40 including the null terminator, considering the above code is written in C). 

    And why they/he decided on the name "my_ntoa" wants a slap.



  • It is also not thread safe.



  • @Mole said:

    255 characters for the string representation seems a little large though, when 8*4+7 = 39 is good enough (40 including the null terminator, considering the above code is written in C).

    "FFFF:FFFF:FFFF:FFFF:FFFF:FFFF:255.255.255.255" is 45 characters. And there's no need to invent your own constant, INET6_ADDRSTRLEN = 46 is readily available.



  • @Spectre said:

    @joelkatz said:
    Honestly, I don't see the problem. I was once hired to do basically exactly this to some code. They wanted the ability to easily identify what needed to be changed to support IPv6, so the job was: [cut]

    This maybe all very well, but inet_ntoa isn't code that needs to be modified to support IPv6 — the code using it is. And this my_ntoa is useless anyway, because it looks and does exactly the same as inet_ntoa.

    The "IPv6 compatible inet_ntoa replacement" already exists — it's inet_ntop. Now maybe a wrapper around it, which would return a pointer to a static buffer would be useful for porting old code, but my_ntoa is not it.

    Oh, and the name is silly.

    I don't know what to say other than to ask you to read my post again and hope that you understand it. This code doesn't modify inet_ntoa, it modifies code that calls inet_ntoa, just as you said is necessary. This my_ntoa function is not useless -- in encapsulates the lack of IPv6 support in all the callers of inet_ntoa into one place.

     You are correct that my_ntoa is not currently the wrapper. Its purpose is to clearly document exactly what you said, that a a wrapper around inet_ntop is needed to port old code.

     This code says, in code, exactly what you are saying in your comments. It doesn't fix a defect (presumably because it didn't need fixing at the time) but it does document and isolate it. This is a common first step in adding IPv6 support

    1) Identify what code needs to be changed and comment it.

    2) Encapsulate code that needs to be changed.

    3) Add support for the new capability.

    It is not always a good idea to start on 3 until 1 and 2 are done. If this is from code that's somewhere in this process, it makes perfect sense.

     I do agree about the name though. Searching through my own code, I found many examples of this -- my_fwrite, my_itoa, my_strsep, my_strftime.Yuck. It's a bad habit.



  • @Auction_God said:

    It is also not thread safe.

    Since inet_ntoa is not required to be thread-safe, I don't see why my_ntoa should be required to be.

    http://www.opengroup.org/onlinepubs/000095399/functions/inet_addr.html

    "The inet_ntoa() function need not be reentrant. A function that is not required to be reentrant is not required to be thread-safe."

    Unless the code base this function came from doesn't use threads at all and doesn't look like it ever might be modified to do so, failure to at least point this out, though, is a defect.

     

    And since this code included no explanation of why it was a WTF, and is not self-evidently a WTF, ...



  • @joelkatz said:

    @Auction_God said:

    It is also not thread safe.

    Since inet_ntoa is not required to be thread-safe, I don't see why my_ntoa should be required to be.

    http://www.opengroup.org/onlinepubs/000095399/functions/inet_addr.html

    "The inet_ntoa() function need not be reentrant. A function that is not required to be reentrant is not required to be thread-safe."

    Unless the code base this function came from doesn't use threads at all and doesn't look like it ever might be modified to do so, failure to at least point this out, though, is a defect.

    And since this code included no explanation of why it was a WTF, and is not self-evidently a WTF, ...

    inet_ntoa() doesn't need to be reentrant because it doesn't make use of any global data, it just does a conversion.  The my_ntoa() function makes use of a static buffer, making it non-reentrant.



  • @morbiuswilters said:

    inet_ntoa() doesn't need to be reentrant because it doesn't make use of any global data, it just does a conversion.  The my_ntoa() function makes use of a static buffer, making it non-reentrant.

    Um, inet_ntoa is not required to be reentrant, because a typical implementation makes use of a static buffer. And I don't see your point, anyway.


  • Discourse touched me in a no-no place

    @joelkatz said:

    @Auction_God said:

    It is also not thread safe.

    Since inet_ntoa is not required to be thread-safe, I don't see why my_ntoa should be required to be.

    Something that no-one has picked up on yet - the function does in fact use a thread-safe version of inet_ntoa() - inet_ntop()


  • @PJH said:

    @joelkatz said:

    @Auction_God said:

    It is also not thread safe.

    Since inet_ntoa is not required to be thread-safe, I don't see why my_ntoa should be required to be.

    Something that no-one has picked up on yet - the function does in fact use a thread-safe version of inet_ntoa() - inet_ntop()
    Correct.  And it feeds it a static buffer for storing the result, thus nullifying the thread-safety.  But since my_ntoa is intended as a replacement for inet_ntoa (which may not be thread-safe), it should not be called in a way that requires thread-safety.


  • @morbiuswilters said:

    inet_ntoa() doesn't need to be reentrant because it doesn't make use of any global data, it just does a conversion.

    You've got that wrong way around.  Not using any global data is one of the requirements of reentrancy, not a reason to not need to be reentrant.


  • @tdb said:

    @morbiuswilters said:

    inet_ntoa() doesn't need to be reentrant because it doesn't make use of any global data, it just does a conversion.

    You've got that wrong way around.  Not using any global data is one of the requirements of reentrancy, not a reason to not need to be reentrant.

    Yeah, I ended up goofing that up.  Your comment directly above sets it straight.  Thanks.


  • Discourse touched me in a no-no place

    @tdb said:

    But since my_ntoa is intended as a replacement for inet_ntoa (which may not be thread-safe), it should not be called in a way that requires thread-safety.
    To be honest, no-one [at work] knows what this function was intended for, since all apparent uses of the function could have been replaced with inet_ntoa() (whether or not it should may be up for debate - see below,) there was/is no requirement for this program to be IPv6 compliant since everything else this program is used with is not IPv6 compliant, (and he was certainly not told to make it IPv6 compliant.)

    Even if we were to move to IPv6, at best this function saves no work.

    NB: This program does use threads. Whether or not this is relevant to this function I've yet to determine - and I'm unlikely to. Should it be me who picks up this piece of work, it'll be written from scratch. As it stands, of that 6K lines, a substantial proportion appears to have been copypasta'd from the source of arping.

    This from the guy who copypasta'd a substantial proportion of (IIRC) wget source to a program that as the main psrt of it's functionality was to ping other boxes. (This was found out when some obscure comment was found in his code, and when I was asked about it by another colleague, suggested he google the comment.)

     



  •  @Mole said:

    Yup, although at first glance "IPv6 compatible inet_ntoa replacement" followed almost immediately by "This currently only supports IPv4." sounds like a WTF, it's just isolating code for the future. 

    I've done this kind of work before. 

    255 characters for the string representation seems a little large though, when 8*4+7 = 39 is good enough (40 including the null terminator, considering the above code is written in C). 

    And why they/he decided on the name "my_ntoa" wants a slap.

    Maybe they were former oracle engineers.

    1: Hey, we need a new varchar!

    2: Well, "varchar" is already taken. What else can we call it?

    1: varchar2?

    2: Done!  Let's go play snood.


Log in to reply