novice coding in /linux/net/ipv4/util.c From: DJ Barrow <dj.barrow@asitatech.com>

novice coding in /linux/net/ipv4/util.c From: DJ Barrow <dj.barrow@asitatech.com>

[DJ Barrow]

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

Re: novice coding in /linux/net/ipv4/util.c From: DJ Barrow <dj.barrow@asitatech.com>

[Richard B. Johnson]

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.

Re: novice coding in /linux/net/ipv4/util.c From: DJ Barrow <dj.barrow@asitatech.com>

[DJ Barrow]

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.

Re: novice coding in /linux/net/ipv4/util.c From: DJ Barrow <dj.barrow@asitatech.com>

[Ben Greear]

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 <greearb@candelatech.com>       <Ben_Greear AT excite.com>
President of Candela Technologies Inc      http://www.candelatech.com
ScryMUD:  http://scry.wanfear.com     http://scry.wanfear.com/~greear

Re: novice coding in /linux/net/ipv4/util.c From: DJ Barrow <dj.barrow@asitatech.com>

[Olaf Titz]

> 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

Re: novice coding in /linux/net/ipv4/util.c From: DJ Barrow <dj.barrow@asitatech.com>

[David S. Miller]

   From: Andi Kleen <ak@suse.de>
   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.

Re: novice coding in /linux/net/ipv4/util.c From: DJ Barrow <dj.barrow@asitatech.com>

[DJ Barrow]

Sounds reasonable to me.

On Monday 22 April 2002 18:07, Andi Kleen wrote:
> DJ Barrow <dj.barrow@asitatech.com> 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

Re: novice coding in /linux/net/ipv4/util.c From: DJ Barrow <dj.barrow@asitatech.com>

[Andi Kleen]

DJ Barrow <dj.barrow@asitatech.com> 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