From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by passt.top (Postfix) with ESMTP id 8062C5A026D for ; Thu, 3 Aug 2023 00:48:14 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1691016493; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=KHzDp9EtcOJ32pWFQtEHLXrGlgravpXciNY2IxYPCEE=; b=WRoKqreSbjkywKlUotEPaWzQUZgAK8hVDAEoEkwRtjjP+PUmjRnc/WNnY0ophr51nlgnGO gVPtN442kxy6wOMOrYkwt6hetgyQJvYK2ma0JXAYs9Fw67PI+nEGjMc76+wsMZBhAqos1d zmUQxmwSMBKyKIfweHY5Cd4xHpvVugo= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-516-1UD9HvCOM7ytJkyYJkZCRg-1; Wed, 02 Aug 2023 18:48:10 -0400 X-MC-Unique: 1UD9HvCOM7ytJkyYJkZCRg-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.rdu2.redhat.com [10.11.54.1]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id BB9DF185A78B; Wed, 2 Aug 2023 22:48:09 +0000 (UTC) Received: from elisabeth (unknown [10.39.208.24]) by smtp.corp.redhat.com (Postfix) with ESMTPS id E099340C206F; Wed, 2 Aug 2023 22:48:08 +0000 (UTC) Date: Thu, 3 Aug 2023 00:48:07 +0200 From: Stefano Brivio To: David Gibson Subject: Re: [PATCH 10/17] netlink: Add nl_do() helper for simple operations with error checking Message-ID: <20230803004807.7d9eb817@elisabeth> In-Reply-To: <20230724060936.952659-11-david@gibson.dropbear.id.au> References: <20230724060936.952659-1-david@gibson.dropbear.id.au> <20230724060936.952659-11-david@gibson.dropbear.id.au> Organization: Red Hat MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.1 on 10.11.54.1 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: M67FV374WPAAL7NKB26C563KYS4EVOSC X-Message-ID-Hash: M67FV374WPAAL7NKB26C563KYS4EVOSC X-MailFrom: sbrivio@redhat.com 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: On Mon, 24 Jul 2023 16:09:29 +1000 David Gibson wrote: > So far we never checked for errors reported on netlink operations via > NLMSG_ERROR messages. This has led to several subtle and tricky to debug > situations which would have been obvious if we knew that certain netlink > operations had failed. > > 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 about > how netlink works it should result in a clear error rather than some subtle > misbehaviour. > > 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. > > Link: https://bugs.passt.top/show_bug.cgi?id=60 > > Signed-off-by: David Gibson > --- > netlink.c | 59 ++++++++++++++++++++++++++++++++++++++++++++----------- > 1 file changed, 47 insertions(+), 12 deletions(-) > > 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; > } > > +/** > + * 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, ssize_t len) > +{ > + struct nlmsghdr *nh; > + char buf[NLBUFSIZ]; > + uint16_t seq; > + ssize_t n; > + > + n = nl_req(s, buf, req, type, flags, len); > + seq = ((struct nlmsghdr *)req)->nlmsg_seq; > + > + for (nh = (struct nlmsghdr *)buf; > + NLMSG_OK(nh, n); nh = NLMSG_NEXT(nh, n)) { > + struct nlmsgerr *errmsg; > + > + if (nh->nlmsg_seq != seq) > + die("netlink: Unexpected response sequence number"); > + > + switch (nh->nlmsg_type) { > + case NLMSG_DONE: > + return 0; > + case NLMSG_ERROR: > + errmsg = (struct nlmsgerr *)NLMSG_DATA(nh); > + return errmsg->error; 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. -- Stefano