From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from gandalf.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id 2C7E55A0272 for ; Fri, 8 Mar 2024 01:49:44 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202312; t=1709858977; bh=G2B9ueq4vp3MNw0WXfmdktNp4z57+fgBxLceEHT2j0A=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=Yju0iekII/lorXJWN+NTtsiTVzVVDw9OcPdcgh1Hs1Pk4YrEngRbdqv/FBmphl2WA bOCU0o6X+LdSkjiGWHDpiiyRlVjx+uyYXx+3PoHJbnhR0VD8oOyksOa5PUWiXA38rU PF9b5Y7r/ZyTPPCvcaHwfBm5H6PEpZFY2kmxBOlaIh3eGwX/kd5oz+eoRXZZlk0RoQ UEAjGlfscVF8a8VDvE6zl6/GbqkAC9aJfA+F28ExEwy6s/gBUjd7uEvPnclF6F1TEd BEWM3MdFFKlF7PwQcNVM/YiTw3/slZ/1zjDHH/EycShIw9IILPzqke/IcG/GLAcMGq 0JTPaSnB9pz6w== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4TrSHP686fz4wc9; Fri, 8 Mar 2024 11:49:37 +1100 (AEDT) Date: Fri, 8 Mar 2024 11:49:27 +1100 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH 2/3] icmp: Flow based error reporting Message-ID: References: <20240229041534.2573559-1-david@gibson.dropbear.id.au> <20240229041534.2573559-3-david@gibson.dropbear.id.au> <20240308002640.15b631b9@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="qa9z3xyuG9ix/2ZU" Content-Disposition: inline In-Reply-To: <20240308002640.15b631b9@elisabeth> Message-ID-Hash: PQK63ANKDV2UNHXRJGR5AX42QMJDBCH6 X-Message-ID-Hash: PQK63ANKDV2UNHXRJGR5AX42QMJDBCH6 X-MailFrom: dgibson@gandalf.ozlabs.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: --qa9z3xyuG9ix/2ZU Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Mar 08, 2024 at 12:26:40AM +0100, Stefano Brivio wrote: > On Thu, 29 Feb 2024 15:15:33 +1100 > David Gibson wrote: >=20 > > Use flow_dbg() and flow_err() helpers to generate flow-linked error > > messages in most places. Make a few small improvements to the messages > > while we're at it. This allows us to avoid the awkward 'pname' variabl= es > > since whether we're dealing with ICMP or ICMPv6 is already built into t= he > > flow type which these helpers include. > >=20 > > Signed-off-by: David Gibson > > --- > > icmp.c | 31 ++++++++++++++----------------- > > 1 file changed, 14 insertions(+), 17 deletions(-) > >=20 > > diff --git a/icmp.c b/icmp.c > > index 1caf791d..63adafcf 100644 > > --- a/icmp.c > > +++ b/icmp.c > > @@ -61,7 +61,6 @@ void icmp_sock_handler(const struct ctx *c, sa_family= _t af, union epoll_ref ref) > > { > > struct icmp_ping_flow *pingf =3D af =3D=3D AF_INET > > ? icmp_id_map[V4][ref.icmp.id] : icmp_id_map[V6][ref.icmp.id]; > > - const char *const pname =3D af =3D=3D AF_INET ? "ICMP" : "ICMPv6"; > > union sockaddr_inany sr; > > socklen_t sl =3D sizeof(sr); > > char buf[USHRT_MAX]; > > @@ -75,8 +74,7 @@ void icmp_sock_handler(const struct ctx *c, sa_family= _t af, union epoll_ref ref) > > =20 > > n =3D recvfrom(ref.fd, buf, sizeof(buf), 0, &sr.sa, &sl); > > if (n < 0) { > > - warn("%s: recvfrom() error on ping socket: %s", > > - pname, strerror(errno)); > > + flow_err(pingf, "recvfrom() error: %s", strerror(errno)); > > return; > > } > > if (sr.sa_family !=3D af) > > @@ -113,8 +111,9 @@ void icmp_sock_handler(const struct ctx *c, sa_fami= ly_t af, union epoll_ref ref) > > pingf->seq =3D seq; > > } > > =20 > > - debug("%s: echo reply to tap, ID: %"PRIu16", seq: %"PRIu16, pname, > > - ref.icmp.id, seq); > > + flow_dbg(pingf, "echo reply to tap, ID: %"PRIu16", seq: %"PRIu16, > > + ref.icmp.id, seq); > > + > > if (af =3D=3D AF_INET) > > tap_icmp4_send(c, sr.sa4.sin_addr, tap_ip4_daddr(c), buf, n); > > else if (af =3D=3D AF_INET6) > > @@ -123,7 +122,7 @@ void icmp_sock_handler(const struct ctx *c, sa_fami= ly_t af, union epoll_ref ref) > > return; > > =20 > > unexpected: > > - warn("%s: Unexpected packet on ping socket", pname); > > + flow_err(pingf, "Unexpected packet on ping socket"); > > } > > =20 > > /** > > @@ -158,7 +157,6 @@ static struct icmp_ping_flow *icmp_ping_new(const s= truct ctx *c, > > struct icmp_ping_flow **id_sock, > > sa_family_t af, uint16_t id) > > { > > - const char *const pname =3D af =3D=3D AF_INET ? "ICMP" : "ICMPv6"; > > uint8_t flowtype =3D af =3D=3D AF_INET ? FLOW_PING4 : FLOW_PING6; > > union icmp_epoll_ref iref =3D { .id =3D id }; > > union flow *flow =3D flow_alloc(); > > @@ -195,9 +193,9 @@ static struct icmp_ping_flow *icmp_ping_new(const s= truct ctx *c, > > if (pingf->sock > FD_REF_MAX) > > goto cancel; > > =20 > > - *id_sock =3D pingf; > > + flow_dbg(pingf, "new socket %i for echo ID %"PRIu16, pingf->sock, id); > > =20 > > - debug("%s: new socket %i for echo ID %"PRIu16, pname, pingf->sock, id= ); > > + *id_sock =3D pingf; > > =20 > > return pingf; > > =20 > > @@ -222,7 +220,6 @@ int icmp_tap_handler(const struct ctx *c, uint8_t p= if, sa_family_t af, > > const void *saddr, const void *daddr, > > const struct pool *p, const struct timespec *now) > > { > > - const char *const pname =3D af =3D=3D AF_INET ? "ICMP" : "ICMPv6"; > > union sockaddr_inany sa =3D { .sa_family =3D af }; > > const socklen_t sl =3D af =3D=3D AF_INET ? sizeof(sa.sa4) : sizeof(sa= =2Esa6); > > struct icmp_ping_flow *pingf, **id_sock; > > @@ -276,13 +273,13 @@ int icmp_tap_handler(const struct ctx *c, uint8_t= pif, sa_family_t af, > > =20 > > pingf->ts =3D now->tv_sec; > > =20 > > - if (sendto(pingf->sock, pkt, plen, MSG_NOSIGNAL, &sa.sa, sl) < 0) { > > - debug("%s: failed to relay request to socket: %s", > > - pname, strerror(errno)); > > - } else { > > - debug("%s: echo request to socket, ID: %"PRIu16", seq: %"PRIu16, > > - pname, id, seq); > > - } > > + if (sendto(pingf->sock, pkt, plen, MSG_NOSIGNAL, &sa.sa, sl) < 0) > > + flow_dbg(pingf, "failed to relay request to socket: %s", > > + strerror(errno)); > > + else > > + flow_dbg(pingf, > > + "echo request to socket, ID: %"PRIu16", seq: %"PRIu16, > > + id, seq); >=20 > I can add the usual curly brackets on merge. The rest of the series > looks good to me. Thanks. It may take me a minute to internalise the "braces if > 1 physical line" rule. --=20 David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson --qa9z3xyuG9ix/2ZU Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmXqYJYACgkQzQJF27ox 2GcnFw/+MQUywwIvYGpaeyVGusLLfLYHYcV7TB7c6PHmKY5u5WxXl52JjCohtuzs O9zEAo6CmkqLj86Vix8muU8M5Z5LRib0rXvobwJmznP4HVMYRIsACzDKCibpH2rq 1+nYIFv70+tUCFX7gqE/SqD5w2fuMnzs+C1GA1/lm9mygBLU/K0gJERwnf9poTdP pGBvZLO8ubk9B7PcGnKy39k0ofORW9xGv6A8gSLxCfb3FGvB+1mHaU9TzOWv2Eag 4+bBog6Ag0LpOQEuLYXpyaNf6C5WEBgMqrWEGrhLzw8QfA7Kr/OF2rKTtrUvhaim 1buOl6mtPb+F4284YZIvybzQ2yhdpc91CyR68txZLLxDSr9dKSUtSSPN4qCl+3y+ 2UJ4k5dXXeh3lgwuK5kydKKQuKZj17hHRAEHYBZETiDM0wgLOEkfj8syHP8iu/2L OlXg50ZdhX/AdnrTdrynFMHYG2v/VxK2w/Y0e0BbOinvwKP1bP8Av5w4AhM3aU3J rFN0O83TtH2WOL4RB1Bb0FamWBliYlsenCpAsZ/ECxTI3ARZe1BvndXyTKpNTwlR 44oc+N5VrIVkoPIQDUVITPRYTJEmyo0qer7f6aOuwBx3vhqq1igK3P3xe74niRQ6 IbLlBpXQtGmqb1YOBC6RU+eyvbtszb7DgMSsxFr1z/gMmmoSDlg= =MDgi -----END PGP SIGNATURE----- --qa9z3xyuG9ix/2ZU--