Re: [PATCH] iputils ping: add (non-raw) ICMP socket support

[Salvatore Mesoraca <s.mesoraca16@gmail.com>]

In data mercoledì 8 aprile 2015 18:20:43, YOSHIFUJI Hideaki ha scritto:
> Salvatore Mesoraca wrote:
> > YOSHIFUJI Hideaki wrote:
> >> Hi,
> > 
> > Hi,
> > thank you for thaking to the time to answer
> > 
> >> Salvatore Mesoraca wrote:
> >>> From: Vasiliy Kulikov <segooon@gmail.com>
> >>> 
> >>> This patch adds non-raw IPPROTO_ICMP socket kind support that was added
> >>> to the Linux 3.0.  The patch is backward-compatible: if ICMP socket kind
> >>> is not enabled in the kernel (either in case of an old kernel or
> >>> explicitly disabled via /proc/sys/net/ipv4/ping_group_range), ping uses
> >>> old privileged raw sockets as a fallback.
> >>> 
> >>> This patch is going to be included in Ubuntu 15.10 and it is already
> >>> included in Gentoo stable tree (at the moment of the writing ping has
> >>> CAP_NET_RAW still enabled by default) it is also included in OpenWall
> >>> since 2011.
> >>> This patch also tries to sneak in a fix for a missing colon in a printf.
> >>> I've tested it on Linux 3.17.7 and it worked without issues.
> >> 
> >> Please do not mix changes in a single commit.
> >> Thanks.
> > 
> > I had some doubt about that additional change (it is so small that I don't
> > think that anyone will ever make a separate patch for it), but it was
> > present in the original patch and I decided to add it anyway and wait for
> > feedback. I'll delete it.
> > Thank you again for you comment.
> 
> What I meant was changes not for supporting non-raw icmp socket
> should be formed separately.  It seems that the patch try to
> change other things as well, in receive_error_msg() for example.
> 
> --yoshfuji

Thank you for the clarification.
If I understood correctly you were referring to these lines:
> @@ -652,9 +687,18 @@ int receive_error_msg()
>             goto out;
>         }
> 
> -       acknowledge(ntohs(icmph.un.echo.sequence));
> +       error_pkt = (e->ee_type != ICMP_REDIRECT &&
> +                e->ee_type != ICMP_SOURCE_QUENCH);
> +       if (error_pkt) {
> +           acknowledge(ntohs(icmph.un.echo.sequence));
> +           net_errors++;
> +           nerrors++;
> +       }
> +       else {
> +           saved_errno = 0;
> +       }


In my understanding these changes are required to support non-raw ICMP sockets 
because they cannot use setsockopt(icmp_sock, SOL_RAW, ICMP_FILTER...) and 
they need to get rid of ICMP_REDIRECT and ICMP_SOURCE_QUENCH in a different 
way. IMHO this change alone does not make much sense.
Anyway if you think that this and other changes should be in different commits 
I'll post a split PATCHv2.
Thank you again for your time.

Salvatore Mesoraca