Re: ping.c minor bug discrepancy between reported size of icmp packet

2018-06-09 Thread Richard Procter
Hi, 

On 9/06/2018, at 7:52 AM, Tom Smyth wrote:

> Hello I see a small discrepancy between the measurement
> of sent and received packets as displayed by ping command
> 
> on the wire the sent and received packets are the same size
> I had a brief go
> 
> foo# ping 5.134.88.1
> PING 5.134.88.1 (5.134.88.1): 56 data bytes
> 64 bytes from 5.134.88.1: icmp_seq=0 ttl=53 time=91.719 ms
> 
> it would appear that one measurement is the ICMP Payload only
> and the other is the ICMP payload + ICMP header  (8 byte difference)

Note that these values do not necessarily differ by 8 as one 
may receive something other than an ECHOREPLY in return e.g.

$ ping -s 40 -v 192.168.1.99
PING 192.168.1.99 (192.168.1.99): 40 data bytes
36 bytes from 192.168.5.1: Destination Host Unreachable

To my mind the two measurements are distinct, distinguished, 
and useful in a diagnostic tool.

As an aside, I have long hankered for ping(1) to advise me on how 
to hit or exceed MTU but it is unable (and ought to be unable) 
to do so, as that would involve multiple layer violations. 
For the same reason, it is unable to indicate total bytes sent. 
See the second paragraph of icmp(4) for starters.

best, 
Richard. 




> 
> 
> foo# grep -n " data bytes" /root/ping.c
> 760:printf("%s): %d data bytes\n", pr_addr(dst, dst->sa_len), datalen);
> foo# grep -n " bytes from" /root/ping.c
> 1248:   printf("%d bytes from %s: icmp_seq=%u", cc,
> 1292:   printf("%d bytes from %s: ", cc, pr_addr(from, fromlen));
> 
> looking at the source code it looks like the size = %d but %d is presenting
> different values
> I didnt see where %d was being changed between line 760 and line 1248
> It has been a while since I looked at C programming in anger and im a bit
> rusty...
> any pointers on where i should be looking so that I can submit a patch
> 
> -- 
> Kindest regards,
> Tom Smyth
> 



Re: ping.c minor bug discrepancy between reported size of icmp packet

2018-06-09 Thread Tom Smyth
Hello Paul, All,

Thanks for your clarification,
I appreciate your help on this please find my reponses in line

On 9 June 2018 at 09:40, Paul de Weerd  wrote:
> Hi Tom,
>
> This is documented in ping(8):
>
>  -s packetsize
>  Specify the number of data bytes to be sent.  The default
>  is 56, which translates into 64 ICMP data bytes when
>  combined with the 8 bytes of ICMP header data.  The
>  maximum packet size is 65467 for IPv4 and 65527 for IPv6.
>
> You can play around with the 56 bytes, but those 8 are non-negotiable:
> they're always added.

I get you ...  the ICMP data bytes can be adjusted but not the ICMP
header bytes,

Note that it says 56 *data* bytes versus 64
> (total) bytes.
the issue I was highlighting is that when the user specifies
a certain size in the command , the command says
sending 1000 bytes
but then says the reply 1008 bytes

I think this is confusing to the user of the system.
also I think the total bytes is incorrect because it only takes
account of the ICMP header and not the IP header,

Perhaps the Approach I should take to fix this (my confusion)
( and possibly future users confusion)
would be to make the displayed values consistent and the
displayed label consistent

>
> You seem to be misunderstanding the format string passed that you
> found.  %d is part of a format string, see printf(3).  The "%"
> indicates a conversion specification, the "d" indicates the type of
> conversion, in this case a signed integer.  The arguments following
> the format string are filled in where a conversion specification is
> given in the format string, in the order given.  So in this example:
>

> #include ;
>
> int main() {
> int number;
> const char* test;
>
> text = "Hi Tom!";
> number = 42;
>
> printf("string: %s\nnumber: %d\n", string, number)
> return 0;
> }
>
> The printf(3) function gets called with three arguments.  The first is
> the format string: "string: %s\nnumber: %d\n".  As you can see,
> there's two conversion specifications in there, "%s" and "%d".  "%s"
> is for a "char*" argument, it gets replaced with the second argument
> to the function (the variable 'string', which we gave the value "Hi
> Tom!").  "%d" is for the integer argument, it gets replaced with a
> decimal representation of the value of the third argument to the
> function (the variable 'number', which we gave the value 42).
>
Thanks for your help on printf(3) :)  Ill work on this

> Applying this knowledge to the three lines you found in the ping
> source:
> In line 760, %d gets the value from variable 'datalen'.
> In lines 1248 and 1292, %d gets the value from variable 'cc'.
>
> Note that 'cc' could be changed between those two lines, so the value
> printed in the end doesn't *have* to be the same - that depends on the
> rest of the code.

I think we could  display (datalen+8)   bytes  in line 760

or
preferably display   (cc -8)  bytes in lines 1248 and 1292

and do a ping man page   update based on the agreed approach

thanks for your help and input
Tom Smyth

>
> Cheers,
>
> Paul
>
> --
>>[<++>-]<+++.>+++[<-->-]<.>+++[<+
> +++>-]<.>++[<>-]<+.--.[-]
>  http://www.weirdnet.nl/



-- 
Kindest regards,
Tom Smyth



Re: ping.c minor bug discrepancy between reported size of icmp packet

2018-06-09 Thread Florian Obser
On Fri, Jun 08, 2018 at 08:52:17PM +0100, Tom Smyth wrote:
> Hello I see a small discrepancy between the measurement
> of sent and received packets as displayed by ping command
> 
> on the wire the sent and received packets are the same size
> I had a brief go
> 
> foo# ping 5.134.88.1
> PING 5.134.88.1 (5.134.88.1): 56 data bytes
> 64 bytes from 5.134.88.1: icmp_seq=0 ttl=53 time=91.719 ms
> 
> it would appear that one measurement is the ICMP Payload only
> and the other is the ICMP payload + ICMP header  (8 byte difference)
> 

This analysis is correct.

This discrepancy always bugged me but I suspect that people got so
used to it that changing it is a bad idea. I certainly do the math
automatically in my head.

I'd like to improve this but I don't think it's possible, this stuff
is just around for too long to change.

> 
> foo# grep -n " data bytes" /root/ping.c
> 760:printf("%s): %d data bytes\n", pr_addr(dst, dst->sa_len), datalen);
> foo# grep -n " bytes from" /root/ping.c
> 1248:   printf("%d bytes from %s: icmp_seq=%u", cc,
> 1292:   printf("%d bytes from %s: ", cc, pr_addr(from, fromlen));
> 
> looking at the source code it looks like the size = %d but %d is presenting
> different values
> I didnt see where %d was being changed between line 760 and line 1248
> It has been a while since I looked at C programming in anger and im a bit
> rusty...
> any pointers on where i should be looking so that I can submit a patch
> 
> -- 
> Kindest regards,
> Tom Smyth
> 

-- 
I'm not entirely sure you are real.



Re: ping.c minor bug discrepancy between reported size of icmp packet

2018-06-09 Thread Paul de Weerd
Hi Tom,

This is documented in ping(8):

 -s packetsize
 Specify the number of data bytes to be sent.  The default
 is 56, which translates into 64 ICMP data bytes when
 combined with the 8 bytes of ICMP header data.  The
 maximum packet size is 65467 for IPv4 and 65527 for IPv6.

You can play around with the 56 bytes, but those 8 are non-negotiable:
they're always added.  Note that it says 56 *data* bytes versus 64
(total) bytes.

On Fri, Jun 08, 2018 at 08:52:17PM +0100, Tom Smyth wrote:
| Hello I see a small discrepancy between the measurement
| of sent and received packets as displayed by ping command
| 
| on the wire the sent and received packets are the same size
| I had a brief go
| 
| foo# ping 5.134.88.1
| PING 5.134.88.1 (5.134.88.1): 56 data bytes
| 64 bytes from 5.134.88.1: icmp_seq=0 ttl=53 time=91.719 ms
| 
| it would appear that one measurement is the ICMP Payload only
| and the other is the ICMP payload + ICMP header  (8 byte difference)
| 
| 
| foo# grep -n " data bytes" /root/ping.c
| 760:printf("%s): %d data bytes\n", pr_addr(dst, dst->sa_len), datalen);
| foo# grep -n " bytes from" /root/ping.c
| 1248:   printf("%d bytes from %s: icmp_seq=%u", cc,
| 1292:   printf("%d bytes from %s: ", cc, pr_addr(from, fromlen));
| 
| looking at the source code it looks like the size = %d but %d is presenting
| different values
| I didnt see where %d was being changed between line 760 and line 1248
| It has been a while since I looked at C programming in anger and im a bit
| rusty...
| any pointers on where i should be looking so that I can submit a patch

You seem to be misunderstanding the format string passed that you
found.  %d is part of a format string, see printf(3).  The "%"
indicates a conversion specification, the "d" indicates the type of
conversion, in this case a signed integer.  The arguments following
the format string are filled in where a conversion specification is
given in the format string, in the order given.  So in this example:

#include ;

int main() {
int number;
const char* test;

text = "Hi Tom!";
number = 42;

printf("string: %s\nnumber: %d\n", string, number)
return 0;
}

The printf(3) function gets called with three arguments.  The first is
the format string: "string: %s\nnumber: %d\n".  As you can see,
there's two conversion specifications in there, "%s" and "%d".  "%s"
is for a "char*" argument, it gets replaced with the second argument
to the function (the variable 'string', which we gave the value "Hi
Tom!").  "%d" is for the integer argument, it gets replaced with a
decimal representation of the value of the third argument to the
function (the variable 'number', which we gave the value 42).

Applying this knowledge to the three lines you found in the ping
source:
In line 760, %d gets the value from variable 'datalen'.
In lines 1248 and 1292, %d gets the value from variable 'cc'.

Note that 'cc' could be changed between those two lines, so the value
printed in the end doesn't *have* to be the same - that depends on the
rest of the code.

Cheers,

Paul

-- 
>[<++>-]<+++.>+++[<-->-]<.>+++[<+
+++>-]<.>++[<>-]<+.--.[-]
 http://www.weirdnet.nl/ 



ping.c minor bug discrepancy between reported size of icmp packet

2018-06-08 Thread Tom Smyth
Hello I see a small discrepancy between the measurement
of sent and received packets as displayed by ping command

on the wire the sent and received packets are the same size
I had a brief go

foo# ping 5.134.88.1
PING 5.134.88.1 (5.134.88.1): 56 data bytes
64 bytes from 5.134.88.1: icmp_seq=0 ttl=53 time=91.719 ms

it would appear that one measurement is the ICMP Payload only
and the other is the ICMP payload + ICMP header  (8 byte difference)


foo# grep -n " data bytes" /root/ping.c
760:printf("%s): %d data bytes\n", pr_addr(dst, dst->sa_len), datalen);
foo# grep -n " bytes from" /root/ping.c
1248:   printf("%d bytes from %s: icmp_seq=%u", cc,
1292:   printf("%d bytes from %s: ", cc, pr_addr(from, fromlen));

looking at the source code it looks like the size = %d but %d is presenting
different values
I didnt see where %d was being changed between line 760 and line 1248
It has been a while since I looked at C programming in anger and im a bit
rusty...
any pointers on where i should be looking so that I can submit a patch

-- 
Kindest regards,
Tom Smyth