Quotes of the week [LWN.net]

Quotes of the week

Posted Apr 5, 2012 19:20 UTC (Thu) by RobSeace (subscriber, #4435)
In reply to: Quotes of the week by juliank
Parent article: Quotes of the week

> Swapping bytes in C code is just wrong in almost all cases (probably in all cases).

Well, those of us writing lots of socket-related code are pretty well stuck with byte-swapping (or doing nothing on big-endian systems) via {n,h}to{h,n}{s,l}() for many uses... If you want to pull a port# from a sockaddr_{in,in6}, you pretty much have to ntohs() it to work with it in any sane manner... If you want to set one, you have to htons() it... If you want to set the raw sockaddr_in.sin_addr.s_addr to one of the INADDR_* values (like INADDR_LOOPBACK), you need to htonl() them... Etc...

Now, you could argue that having those sockaddr_* fields be in network byte order was a mistake, and I'd probably agree... But, there's nothing much that can be done about it but live with it at this point... (And, thankfully getaddrinfo() makes it less necessary to poke with the sockaddr_* internals these days, anyway...)

(Log in to post comments)

Quotes of the week

Posted Apr 5, 2012 19:53 UTC (Thu) by RobSeace (subscriber, #4435) [Link]

> I think you missed Rob's point, the ntoh*() functions are the equivalent of the code he posted.

Well, not really, since internally they do byte-swap (if you're on a little-endian host)... They are just abstracting away the ugly #ifdef behavior out of your code directly and hiding it in libc... It doesn't really change its wrongness...

> #ifdef host_is_little_endian
> i = ntohl(i)
> #endif

I've never seen anyone do anything that nutty... It's basically just duplicating exactly what ntohl() already does for you!

Quotes of the week

Posted Apr 5, 2012 20:30 UTC (Thu) by samroberts (subscriber, #46749) [Link]

> They are just abstracting away the ugly #ifdef

Again, Rob's point was that writing code conditional on the *host's* byte-order is unnecessary for dealing with external data.

libc doesn't necessarily have an ifdef, ntoh* can be written in portable code with no ifdef. Plan9's libc, according to Rob, had no such ifdef in it.

> I've never seen anyone do anything that nutty...

https://github.com/sam-github/libnet/blob/master/libnet/s...

Now you have. And no, I didn't write that.

Quotes of the week

Posted Apr 5, 2012 21:00 UTC (Thu) by RobSeace (subscriber, #4435) [Link]

> libc doesn't necessarily have an ifdef, ntoh* can be written in portable
> code with no ifdef. Plan9's libc, according to Rob, had no such ifdef in it.

I suppose so... But, glibc at least does this:

uint16_t
htons (x)
uint16_t x;
{
#if BYTE_ORDER == BIG_ENDIAN
return x;
#elif BYTE_ORDER == LITTLE_ENDIAN
return __bswap_16 (x);
#else
# error "What kind of system is this?"
#endif
}

And, then defines ntohs() as the same thing... And, htonl()/ntohl() is the same but with __bswap_32() of course...

Which I believe is exactly what he was talking about... So, like I said, you're merely abstracting away the ugliness... (Which isn't a bad thing in any way! If you need to have ugliness, better to hide it away...)

> Now you have.

Ok, that's just stupid... And, unless I'm missing something, it does absolutely nothing... Because on big-endian, htons() is effectively a no-op function... So, it seems to me it's doing the same direct assignment on both big and little-endian systems... Perhaps they wanted the reverse test? Which would then be merely redundant (since htons() effectively does it for you already) rather than flat out insane...

Anyway, that seems less like someone doing byte-swapping when not needed, but more just someone fundamentally misunderstanding how htons() works... Maybe they thought it was always a byte-swap routine, and didn't realize that on big-endian systems it's going to be a no-op? That would make their code make some sense (they're trying to force it to little-endian always)...

Quotes of the week

Posted Apr 5, 2012 23:30 UTC (Thu) by viro (subscriber, #7872) [Link]

Egads... OK, here's the deal: anybody thinking in terms of "swapped" and "native" is bloody wrong. Doing it that way is *the* recipe for hard to find bugs. There are 3 things: native, little-endian and big-endian. That's what one ought to keep track of, not "have I done byteswap odd or even number of times". Forget about the fact that conversions are involutions. Sure, on any realistic platform it will be true that htonl(x) is always equal to ntohl(x). Which is not interesting for anything other than low-level implementation of those primitives. bswap32 is the wrong thing to do outside of the internals of their implementation.

BTW, code doing n = ntohl(n) is asking for trouble, period. Don't do that - if I ever see that kind of crap in patches touching fs/*, you can be sure that you'll get it back with unkind comments. Don't reuse variables between e.g. native and big-endian; sooner or later you'll get confused and do something really dumb like mismatched number of flips on different paths. And that's not just a theory - I've seen quite a few bugs of that kind.

uint32_t is _not_ "one of {__le32,__be32}, depending on host endianness". It's a separate type and should be treated as such. If the variable can be reused, leave doing that to compiler.

Having the same struct used both with host- and fixed-endian contents is a really bad practice; better split it in two types and take care about which one is getting used in any particular place. And no, "I'll just convert these fields in place here" is *not* a good idea. Been there, fixed quite a few of resulting bugs. Hadn't been fun at all...

Quotes of the week

Posted Apr 6, 2012 0:51 UTC (Fri) by RobSeace (subscriber, #4435) [Link]

Did you mean to reply to my comment? Because I agree with everything you said... If I gave any impression otherwise, I apologize... But, I'm confused as to where I gave that impression...