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 3496F5A0274 for ; Thu, 3 Aug 2023 04:27:11 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=201602; t=1691029621; bh=RwkRjw4ht6sc+7aQkl7L7a05Fcg+7d2Fm/efWOC9p70=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=oshaYq5nKsVdqz5WEGemfbwoRgOfb2QzWdz32EC4P63vPhfYnC/bEhHvbwyiFIXHM EZ5jBnhSfcR4zZr5hTyy/9JGpGEjuAPjZv2BenKwNw0oUjIRGmqYGK+IcUx3U8XHCQ InzIgOJfg/vvoSNATIikLMhl5l9PVQFbMdV0Sk0w= Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4RGXmP5059z4wy0; Thu, 3 Aug 2023 12:27:01 +1000 (AEST) Date: Thu, 3 Aug 2023 12:24:41 +1000 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH 10/17] netlink: Add nl_do() helper for simple operations with error checking Message-ID: References: <20230724060936.952659-1-david@gibson.dropbear.id.au> <20230724060936.952659-11-david@gibson.dropbear.id.au> <20230803004807.7d9eb817@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="Dyuxqkr29Pad7tJi" Content-Disposition: inline In-Reply-To: <20230803004807.7d9eb817@elisabeth> Message-ID-Hash: FEBCOETVTKGTQSX7LRVIPC56PGDGBI6L X-Message-ID-Hash: FEBCOETVTKGTQSX7LRVIPC56PGDGBI6L 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: --Dyuxqkr29Pad7tJi Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Aug 03, 2023 at 12:48:07AM +0200, Stefano Brivio wrote: > On Mon, 24 Jul 2023 16:09:29 +1000 > David Gibson wrote: >=20 > > So far we never checked for errors reported on netlink operations via > > NLMSG_ERROR messages. This has led to several subtle and tricky to deb= ug > > situations which would have been obvious if we knew that certain netlink > > operations had failed. > >=20 > > Introduce a nl_do() helper that performs netlink "do" operations (that = is > > making a single change without retreiving complex information) with much > > more thorough error checking. As well as returning an error code if we > > get an NLMSG_ERROR message, we also check for unexpected behaviour in > > several places. That way if we've made a mistake in our assumptions ab= out > > how netlink works it should result in a clear error rather than some su= btle > > misbehaviour. > >=20 > > We update those calls to nl_req() that can use the new wrapper to do so. > > We will extend those to better handle errors in future. We don't touch > > non-"do" operations for now, those are a bit trickier. > >=20 > > Link: https://bugs.passt.top/show_bug.cgi?id=3D60 > >=20 > > Signed-off-by: David Gibson > > --- > > netlink.c | 59 ++++++++++++++++++++++++++++++++++++++++++++----------- > > 1 file changed, 47 insertions(+), 12 deletions(-) > >=20 > > diff --git a/netlink.c b/netlink.c > > index 3170344..cdd65c0 100644 > > --- a/netlink.c > > +++ b/netlink.c > > @@ -148,6 +148,47 @@ static ssize_t nl_req(int s, char *buf, void *req, > > return n; > > } > > =20 > > +/** > > + * nl_do() - Send netlink "do" request, and wait for acknowledgement > > + * @s: Netlink socket > > + * @req: Request (will fill netlink header) > > + * @type: Request type > > + * @flags: Extra request flags (NLM_F_REQUEST and NLM_F_ACK assumed) > > + * @len: Request length > > + * > > + * Return: 0 on success, negative error code on error > > + */ > > +static int nl_do(int s, void *req, uint16_t type, uint16_t flags, ssiz= e_t len) > > +{ > > + struct nlmsghdr *nh; > > + char buf[NLBUFSIZ]; > > + uint16_t seq; > > + ssize_t n; > > + > > + n =3D nl_req(s, buf, req, type, flags, len); > > + seq =3D ((struct nlmsghdr *)req)->nlmsg_seq; > > + > > + for (nh =3D (struct nlmsghdr *)buf; > > + NLMSG_OK(nh, n); nh =3D NLMSG_NEXT(nh, n)) { > > + struct nlmsgerr *errmsg; > > + > > + if (nh->nlmsg_seq !=3D seq) > > + die("netlink: Unexpected response sequence number"); > > + > > + switch (nh->nlmsg_type) { > > + case NLMSG_DONE: > > + return 0; > > + case NLMSG_ERROR: > > + errmsg =3D (struct nlmsgerr *)NLMSG_DATA(nh); > > + return errmsg->error; >=20 > This is an errno, we should probably print it here ...and, now reading > 14/17 and 16/17: saving repeated strerror() calls there. On the other > hand this has the advantage of one single error message instead of two, > but... hmm. No, this is deliberate. We use this for the "write" side of the dup operations. So for the routes we don't want this to print errors for all the times we get a net unreachable, then again for all the duplicated routes as we try repeatedly. --=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 --Dyuxqkr29Pad7tJi Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmTLD+IACgkQzQJF27ox 2GcdPA/8C3ma5DXAZBBVt4NE2IZ8+uUNXY7XBPYdajwsSuCMASlCkWtP0R3OlgrW BZngXKhldNJyQbQ3ubWJYmbL1r5Dt3WhS/g9QrCE/k+0TuoaIWQa5KLDXsNiiqA9 phFI8DA5zh4yZIGmBhRRzZQyshSTFN1Y1+io7krdV3setk4+9tCetoN6Ej0OVcrm 8tax3hkOXF2e9lH4VLOmAFhsd3ruUvVw+psiEEEW3Moq7UYh06EPomUxvRlQ8578 3iKz0yx2+/i/R2Kdc631jCzb74+AvxAeDhgiDCADzcuakHs4x6WramYt8oWatdJh KWRdmeCGJFvQM7gUGEc8mzSrV9tLso3YE+fHvT6CR+HDZXg0m7GOPqvCi3Xqd34n VGIC2bP4rPvO6vmo1I7MCCqlY54cw8t7LziIi8tx19rLceg8j28KqYIsFdmk2q1M ptDQvkDi6pWNHzGbWJSV+Hkry83VNRyx87jJ2UGIXVhsX6fQulARecoiqbwcq1yy 6Ll9rOBG4JMt5jamv8uINCjucU37BGjEddfMdi59GoaQWT3iLOmsCCgmLNDkbUHB 0gAW9tItrX290Ebm85/fYpb9F7Y4C4LR2+DyzUarieqeaoASYF6bndkbBskx/wcK KTxgh9N+3AS/o+y0ADw+cBmTUfypqUp1jAmzTI6kl4r+CXZecHc= =qm8I -----END PGP SIGNATURE----- --Dyuxqkr29Pad7tJi--