From owner-svn-src-head@freebsd.org Wed May 3 21:05:16 2017 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 117D3D5CCF3; Wed, 3 May 2017 21:05:16 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from mail.baldwin.cx (bigwig.baldwin.cx [IPv6:2001:470:1f11:75::1]) (using TLSv1 with cipher DHE-RSA-CAMELLIA256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id DFCF71E20; Wed, 3 May 2017 21:05:15 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from ralph.baldwin.cx (c-73-231-226-104.hsd1.ca.comcast.net [73.231.226.104]) by mail.baldwin.cx (Postfix) with ESMTPSA id 95B7B10A82D; Wed, 3 May 2017 17:05:14 -0400 (EDT) From: John Baldwin To: Ian Lepore Cc: Ryan Stone , Alan Somers , "src-committers@freebsd.org" , "svn-src-all@freebsd.org" , "svn-src-head@freebsd.org" Subject: Re: svn commit: r317755 - head/sbin/ifconfig Date: Wed, 03 May 2017 14:00:33 -0700 Message-ID: <2536795.sxmMB4gFZB@ralph.baldwin.cx> User-Agent: KMail/4.14.10 (FreeBSD/11.0-STABLE; KDE/4.14.10; amd64; ; ) In-Reply-To: <1493838199.80042.14.camel@freebsd.org> References: <201705031721.v43HL2vS071819@repo.freebsd.org> <1493838199.80042.14.camel@freebsd.org> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.4.3 (mail.baldwin.cx); Wed, 03 May 2017 17:05:14 -0400 (EDT) X-Virus-Scanned: clamav-milter 0.99.2 at mail.baldwin.cx X-Virus-Status: Clean X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 03 May 2017 21:05:16 -0000 On Wednesday, May 03, 2017 01:03:19 PM Ian Lepore wrote: > On Wed, 2017-05-03 at 14:07 -0400, Ryan Stone wrote: > > On Wed, May 3, 2017 at 1:39 PM, Ryan Stone wrote: > > > > > > > > > > > > > > On Wed, May 3, 2017 at 1:21 PM, Alan Somers wrote: > > > > > > > > > > > Author: asomers > > > > Date: Wed May 3 17:21:01 2017 > > > > New Revision: 317755 > > > > URL: https://svnweb.freebsd.org/changeset/base/317755 > > > > > > > > Log: > > > > Various Coverity fixes in ifconfig(8) > > > > > > > > * Exit early if kldload(2) fails (1011259). This is the only change that > > > > affects ifconfig's behavior. > > > > > > > > > > > Please revert this ASAP. kldload is expected to fail for a number of > > > benign reasons and this change is likely to prevent any network > > > configuration from being applied to systems, breaking remote access. > > > > > > > > It's been pointed out to me off-list that the situation is not quite as > > dire as I had originally believed. The ifconfig code in question already > > searches to check if the module in question is loaded before calling > > kldload. However, there is at least one driver (mlx4_en) that does not > > follow the "if_" kld module naming convention that this code depends > > on, so this change will make it impossible to apply configuration to > > mlx4_en interfaces. Additionally, it is possible that other drivers use > > the naming convention for their kld file but not for the module declared in > > the C code, in which case this change would also break configuration of > > those interfaces. > > > > jhb@ suggests that ifconfig should only attempt to load a module if the > > interface doesn't already exist, by calling if_nametoindex to check for the > > existence of the interface. That seems to be a reasonable fix for me, but > > in the interest of not breaking users' networking configuration > > (potentially making it impossible to fix a remote machine), I'd recommend > > that the part of the change that checks the return code from kldload() be > > reverted while a fix for this issue is worked on. > > It should be noted that the existing code uses if_nametoindex() > immediately after ifmaybeload() returns, and handles errors > accordingly. I.e., there wasn't really anything wrong with the code as > originally written/structured. Except it's really klunky. The loop searching the module list is a lot more code than ignoring EEXIST errors from kldload. I would structure the code like this: ifindex = if_nametoindex(ifname); if (ifindex == 0) { ifmaybeload(ifname); ifindex = if_nametoindex(ifname); } if (ifindex == 0) { /* existing code */ } Further, I would probably simplify ifmaybeload() to be something like this: static void ifmaybeload(const char *name) { if (noload) return; /* existing code to generate 'ifkind' */ if (kldload(ifkind) == -1) { if (errno != EEXIST) err(...); } } One could argue for ignoring ENOENT errors as well, but Alan's specific use case is one that wanted an explicit kldload error for ENOENT. -- John Baldwin