From owner-svn-src-head@freebsd.org Tue Apr 21 02:34:24 2020 Return-Path: Delivered-To: svn-src-head@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id 626AF2B93F8 for ; Tue, 21 Apr 2020 02:34:24 +0000 (UTC) (envelope-from kevans@freebsd.org) Received: from smtp.freebsd.org (smtp.freebsd.org [IPv6:2610:1c1:1:606c::24b:4]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) server-signature RSA-PSS (4096 bits) client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "smtp.freebsd.org", Issuer "Let's Encrypt Authority X3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 495nhJ0Ck7z4ZvZ for ; Tue, 21 Apr 2020 02:34:24 +0000 (UTC) (envelope-from kevans@freebsd.org) Received: from mail-qv1-f54.google.com (mail-qv1-f54.google.com [209.85.219.54]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "GTS CA 1O1" (verified OK)) (Authenticated sender: kevans) by smtp.freebsd.org (Postfix) with ESMTPSA id D7744F3D0 for ; Tue, 21 Apr 2020 02:34:23 +0000 (UTC) (envelope-from kevans@freebsd.org) Received: by mail-qv1-f54.google.com with SMTP id p19so5885490qve.0 for ; Mon, 20 Apr 2020 19:34:23 -0700 (PDT) X-Gm-Message-State: AGi0PuYXil1vAJ9SGSmlwZ7UzL+bXtVY3WdXyrq0EkWWzRudvtKBCy+s oXCCRjur0B2p3D9zsJZ1brxlobgRTgEE6R9BRwM= X-Received: by 2002:a05:6214:28d:: with SMTP id l13mt18407373qvv.181.1587436463320; Mon, 20 Apr 2020 19:34:23 -0700 (PDT) MIME-Version: 1.0 References: <202004180750.03I7oUK6032898@repo.freebsd.org> <67B55A62-848B-4876-8367-DE0D32A8B7D4@FreeBSD.org> <20200419142327.nhotyboqubk3vl2l@mutt-hbsd> <523895fb-7f30-c4ce-78a7-d16b2ff4954c@vangyzen.net> In-Reply-To: From: Kyle Evans Date: Mon, 20 Apr 2020 21:34:12 -0500 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: svn commit: r360068 - in head/sys: kern net sys Cc: Eric van Gyzen , Shawn Webb , Kristof Provost , Ronald Klop , svn-src-head , svn-src-all , src-committers Content-Type: text/plain; charset="UTF-8" X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.29 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: Tue, 21 Apr 2020 02:34:24 -0000 On Mon, Apr 20, 2020 at 9:14 PM Kyle Evans wrote: > > On Mon, Apr 20, 2020 at 8:15 PM Eric van Gyzen wrote: > > > > >>>> + sz = asprintf(&buf, M_TEMP, "%s-%s-%s", uuid, if_name(ifp), > > >>>> + jailname); > > >>>> + if (sz < 0) { > > >>>> + /* Fall back to a random mac address. */ > > >>> > > >>> > > >>> I was wondering if it would be valuable to give this fall back something > > >>> like: > > >>> > > >>> printf("%s: unable to create fixed mac address; using random > > >>> mac address", if_name(ifp)); > > >>> > > >>> This will only be printed in rare circumstances. But in that case will > > >>> provide valuable information. > > >>> > > >> That would potentially be valuable, yes. On the other hand, we traditionally > > >> don???t sprinkle a lot of printf()s around in the kernel. This is extremely > > >> unlikely to happen, and if it does odds are attaching the interface will > > >> fail at an earlier or later point, you may struggle to pass packets and run > > >> into any number of other issues. > > >> It???s also possible to diagnose absent the printf(), because the MAC > > >> address will be locally administered rather than within the FreeBSD OUI. > > >> > > >> So, in short: not a bad idea. You can argue it both ways, and I find myself > > >> (weakly) on the opposite side. > > > > > > Would displaying the message only when verbose boot mode is enabled be > > > a suitable compromise? > > > > We could completely avoid the problems of dynamic allocation by calling > > SHA1Update three times, feeding each piece of data separately. > > > > For bonus points, use a single char[] to save stack space, too. Maybe > > use a union, for legibility, and to ensure the proper size without ugly > > assertions. > > > > To be honest, I'd be more inclined to just revert this part of it and > push it all back onto the stack. It's still < 512 bytes and pretty > much always called in short paths because it's generally only used > during initial creation of some ifnet; I found the concern about the > stack usage here, specifically, a bit dubious in the first place, and > this follow-up hasn't left me enjoying it any further. > Sorry, to clarify: I'm also pretty much OK with SHA1Update 3x if I'm alone in the "don't really care about this particular stack usage" camp, but I've found it useful that they're currently joined into a single buffer as I've had occasion to dump it in the past to confirm my understanding of the pedigree of the output, in case of, e.g., generated conflicts.