Hi ,
While debugging last night with Brian O'Sullivan I found this beauty.
char *in_ntoa(__u32 in)
{
static char buff[18];
char *p;
p = (char *) ∈
sprintf(buff, "%d.%d.%d.%d",
(p[0] & 255), (p[1] & 255), (p[2] & 255), (p[3] & 255));
return(buff);
}
This textbook peice of novice coding which has existed since 2.2.14.
For those who can't spot the error, please note that this function is
returning a static string, excellent stuff if you are hoping to reuse the
same function like the following
printk("%s %s\n",in_ntoa(addr1),in_ntoa(addr2));
On Mon, 22 Apr 2002, DJ Barrow wrote:
> Hi ,
> While debugging last night with Brian O'Sullivan I found this beauty.
>
> char *in_ntoa(__u32 in)
> {
> static char buff[18];
> char *p;
>
> p = (char *) ∈
> sprintf(buff, "%d.%d.%d.%d",
> (p[0] & 255), (p[1] & 255), (p[2] & 255), (p[3] & 255));
> return(buff);
> }
>
> This textbook peice of novice coding which has existed since 2.2.14.
> For those who can't spot the error, please note that this function is
> returning a static string, excellent stuff if you are hoping to reuse the
> same function like the following
> printk("%s %s\n",in_ntoa(addr1),in_ntoa(addr2));
> -
I love it! Last guy wins! I wonder how you fix it without having to
pass it a pointer to something the caller owns? This is, truly,
non-trivial. Also, this is in ../linux/net, not something specific
to Intel, and there is no macro to handle the network-order. It
just 'comes-out-right' with Intel machines.
Cheers,
Dick Johnson
Penguin : Linux version 2.4.18 on an i686 machine (797.90 BogoMips).
Windows-2000/Professional isn't.
Richard,
I agree The least offensive way would be to pass in a sring from the caller,
I didn't spot the second endian bug till you mentioned it ;-).
I wish Linus would finally get rid of the errno global as this is equally
stupid on smp machines or else make an errno_array[NR_CPUS] array
& make errno a #define errno errno_array[smp_processor_id()] or
something similar, plenty of others have posted patches for that
rubbish.
Alan Cox is on the list of Authors, he must have wrote it ;-)
On Monday 22 April 2002 16:48, Richard B. Johnson wrote:
> On Mon, 22 Apr 2002, DJ Barrow wrote:
> > Hi ,
> > While debugging last night with Brian O'Sullivan I found this beauty.
> >
> > char *in_ntoa(__u32 in)
> > {
> > static char buff[18];
> > char *p;
> >
> > p = (char *) ∈
> > sprintf(buff, "%d.%d.%d.%d",
> > (p[0] & 255), (p[1] & 255), (p[2] & 255), (p[3] & 255));
> > return(buff);
> > }
> >
> > This textbook peice of novice coding which has existed since 2.2.14.
> > For those who can't spot the error, please note that this function is
> > returning a static string, excellent stuff if you are hoping to reuse the
> > same function like the following
> > printk("%s %s\n",in_ntoa(addr1),in_ntoa(addr2));
> > -
>
> I love it! Last guy wins! I wonder how you fix it without having to
> pass it a pointer to something the caller owns? This is, truly,
> non-trivial. Also, this is in ../linux/net, not something specific
> to Intel, and there is no macro to handle the network-order. It
> just 'comes-out-right' with Intel machines.
>
> Cheers,
> Dick Johnson
>
> Penguin : Linux version 2.4.18 on an i686 machine (797.90 BogoMips).
>
> Windows-2000/Professional isn't.
DJ Barrow wrote:
> Richard,
> I agree The least offensive way would be to pass in a sring from the caller,
> I didn't spot the second endian bug till you mentioned it ;-).
The input should always be in network-byte-order, so there is no
endian problem, at least. The other problem can be worked around
by the caller, though a second, similar, method that took an pre-allocated
buffer would definately be nice.
--
Ben Greear <[email protected]> <Ben_Greear AT excite.com>
President of Candela Technologies Inc http://www.candelatech.com
ScryMUD: http://scry.wanfear.com http://scry.wanfear.com/~greear
DJ Barrow <[email protected]> writes:
> This textbook peice of novice coding which has existed since 2.2.14.
Even earlier I think
BTW do you call libresolv novice coding too ?
> For those who can't spot the error, please note that this function is
> returning a static string, excellent stuff if you are hoping to reuse the
> same function like the following
> printk("%s %s\n",in_ntoa(addr1),in_ntoa(addr2));
That is why most of networking uses the NIPQUAD macro instead.
Best would be probably to convert the few remaining users of in_ntoa
to NIPQUAD and drop this function.
-Andi
Sounds reasonable to me.
On Monday 22 April 2002 18:07, Andi Kleen wrote:
> DJ Barrow <[email protected]> writes:
> > This textbook peice of novice coding which has existed since 2.2.14.
>
> Even earlier I think
>
> BTW do you call libresolv novice coding too ?
>
> > For those who can't spot the error, please note that this function is
> > returning a static string, excellent stuff if you are hoping to reuse the
> > same function like the following
> > printk("%s %s\n",in_ntoa(addr1),in_ntoa(addr2));
>
> That is why most of networking uses the NIPQUAD macro instead.
>
> Best would be probably to convert the few remaining users of in_ntoa
> to NIPQUAD and drop this function.
>
> -Andi
> I love it! Last guy wins! I wonder how you fix it without having to
> pass it a pointer to something the caller owns? This is, truly,
> non-trivial. Also, this is in ../linux/net, not something specific
This very discussion came up over CIPE several years ago. I wrote a
function which is even less beautiful but at least can be used
the natural way, i.e. like this:
dprintk(DEB_OUT, (KERN_INFO
"%s: sending %d from %s:%d to %s:%d\n",
dev->name, skb->len,
cipe_ntoa(iph->saddr), ntohs(udph->source),
cipe_ntoa(iph->daddr), ntohs(udph->dest)));
It uses a circular chain of internal buffers (statically allocated
because I decided that more effort Just Isn't Worth It [tm]).
I proposed something like this on l-k back then and most people
disagreed on the grounds that we could use NIPQUAD instead. This may
be efficient but it is problematic on two counts IMHO:
1. it is Wrong[tm] because an IP address is _one_ argument to printk,
not four
2. there is no easy way to make this IPv6 compatible. Printing IPv6
addresses seems to me still an unsolved problem in the kernel (the
stuff in icmpv6_rcv() can't be the last word on it, and ip6t_LOG.c
neither, I mean really...)
> to Intel, and there is no macro to handle the network-order. It
> just 'comes-out-right' with Intel machines.
no, it comes out right because the argument is in network order, or
else it would come out _wrong_ on Intel boxen. (I'm glad I have a
little endian box if only because of catching such errors early :-)
Olaf
From: Andi Kleen <[email protected]>
Date: 22 Apr 2002 19:07:38 +0200
Best would be probably to convert the few remaining users of in_ntoa
to NIPQUAD and drop this function.
I'm doing this in my tree.