bug-inetutils
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: ping


From: Alain Magloire
Subject: Re: ping
Date: Tue, 19 Jun 2001 00:02:57 -0400 (EDT)

Bonjour

> OK, I have added libicmp. Currently it looks more like `libping',
> so I'll be expanding its functionality step by step. Ping seems to
> be working fine, I will enable SUBDIR when I'm finished testing it
> on Linux and Solaris.

I had a few space cycle and enable ping to compile for QNX/Neutrino
ifconfig will not work since I'm running a tiny tcpip stack module.

So to get ping/libicmp to compile:

<sys/signal.h> ==> <signal.h>

<sys/signal.h> is not portable and should not be use, but I do not
have my copy of POSIX std to back this out 8-).


nitpicking through the code:

- descrepencies in functions that are suppose to return
  "int"s but instead do "return;" or do not returning anything.

- I would advocate to use "const char *" when appropriate
  decode_type() is a good candidate.

- missing prototypes.  ping_xmit, ping_recv,
  icmp_cksum  etc ..

- some functions should be static especially for the libs
  to not walk on namespace. For example the _packet_xxxx();

- Missing the usual trick :
  funct (int a, int b_unused)
  {
    (void)b_unused;
   ..
  }
  To shutup gcc about warning: "unused parameter".  In some
  old compiler, we use to do: b_unused = b_unused;

- libicmp/icmp_address.c(icmp_address_encode) looks more then
  dangerous, icmp is not allocated prior of being use, but
  I think that you probably did not implement it
  and it is just a stub, still thought I should mention it.

int
icmp_address_encode(..)
{
  icmphdr_t *icmp;
  
  if (bufsize < 12)
    return -1;

  icmp->icmp_mask = 0;     <---- oops, icmp is not initiliase/allocated.
...
}

- Inconsistencies between signed and unsigned, most are harmless ...
  but since I was nitpicking lets through that in for good measure 8-)

   warning: comparison between signed and unsigned

- I would advocate not to use variables names that will class
  whith function names.  For example, signal, sin, optarg, dup, ping
  etc ...   There was an amusing bug where the compiler did not
  warn about an uninitialize variable because it was the name
  of a function ... althoug compilers are smart enough to detect
  the context nowadays.

Of course this is "nitpicking", and some remarks are boderline
religion style/advocate, you may discard everything 8-)

A simple diff follows, for libicmp/* and ping/* nothing serious.




reply via email to

[Prev in Thread] Current Thread [Next in Thread]