Date: Mon, 20 Apr 2020 21:14:16 -0500 From: Kyle Evans <kevans@freebsd.org> To: Eric van Gyzen <eric@vangyzen.net> Cc: Shawn Webb <shawn.webb@hardenedbsd.org>, Kristof Provost <kp@freebsd.org>, Ronald Klop <ronald-lists@klop.ws>, svn-src-head <svn-src-head@freebsd.org>, svn-src-all <svn-src-all@freebsd.org>, src-committers <src-committers@freebsd.org> Subject: Re: svn commit: r360068 - in head/sys: kern net sys Message-ID: <CACNAnaHVZozWLcNVT%2BZA4aptc5oSYD68OPpX7_sLvomZYia-fw@mail.gmail.com> In-Reply-To: <523895fb-7f30-c4ce-78a7-d16b2ff4954c@vangyzen.net> References: <202004180750.03I7oUK6032898@repo.freebsd.org> <op.0jbpatnnkndu52@sjakie> <67B55A62-848B-4876-8367-DE0D32A8B7D4@FreeBSD.org> <20200419142327.nhotyboqubk3vl2l@mutt-hbsd> <523895fb-7f30-c4ce-78a7-d16b2ff4954c@vangyzen.net>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, Apr 20, 2020 at 8:15 PM Eric van Gyzen <eric@vangyzen.net> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CACNAnaHVZozWLcNVT%2BZA4aptc5oSYD68OPpX7_sLvomZYia-fw>