From owner-svn-src-head@freebsd.org Tue Apr 21 02:14:28 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 BFB662B84A6; Tue, 21 Apr 2020 02:14:28 +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 495nFJ4WTLz4YJ1; Tue, 21 Apr 2020 02:14:28 +0000 (UTC) (envelope-from kevans@freebsd.org) Received: from mail-qt1-f182.google.com (mail-qt1-f182.google.com [209.85.160.182]) (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 83DBFF168; Tue, 21 Apr 2020 02:14:28 +0000 (UTC) (envelope-from kevans@freebsd.org) Received: by mail-qt1-f182.google.com with SMTP id n20so4202114qtp.9; Mon, 20 Apr 2020 19:14:28 -0700 (PDT) X-Gm-Message-State: AGi0PuaVvPFIcIYfQRzHfKe/nU1TX5Ca81vfrIiLYfGyNUGvWe4V4p/5 LiWG/vtR4xod04GEvGT0s5S0B09n0EuhxLyY0GE= X-Google-Smtp-Source: APiQypIchJs+asYA47BKFXHvSsMwtxLoYsa8n8gGIWCLW3t4YnvZI7F4bA/SHFHkgju2pgyrDMZtiCGpRL7eKXIqcZI= X-Received: by 2002:ac8:65cc:: with SMTP id t12mr18843323qto.310.1587435268017; Mon, 20 Apr 2020 19:14:28 -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: <523895fb-7f30-c4ce-78a7-d16b2ff4954c@vangyzen.net> From: Kyle Evans Date: Mon, 20 Apr 2020 21:14:16 -0500 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: svn commit: r360068 - in head/sys: kern net sys To: Eric van Gyzen Cc: 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:14:28 -0000 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. Thanks, Kyle Evans