Date: Tue, 21 Apr 2020 10:15:03 +0200 From: "Kristof Provost" <kp@FreeBSD.org> To: "Kyle Evans" <kevans@freebsd.org> Cc: "Eric van Gyzen" <eric@vangyzen.net>, "Shawn Webb" <shawn.webb@hardenedbsd.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: <C45542E7-84F0-488B-9F38-D5A49797A9BE@FreeBSD.org> In-Reply-To: <CACNAnaFLyhM8GjVuUgGtHJK=CmYPdwbVsHr5Ckfq3kcP9WFhzg@mail.gmail.com> 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> <CACNAnaHVZozWLcNVT%2BZA4aptc5oSYD68OPpX7_sLvomZYia-fw@mail.gmail.com> <CACNAnaFLyhM8GjVuUgGtHJK=CmYPdwbVsHr5Ckfq3kcP9WFhzg@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On 21 Apr 2020, at 4:34, Kyle Evans wrote:
> On Mon, Apr 20, 2020 at 9:14 PM Kyle Evans <kevans@freebsd.org> wrote:
>>
>> 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.
>>
>
> 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.
For what it’s worth, I’m in your camp: a few hundred bytes of stack
use doesn’t matter much here. Straightforward code is more important.
Best regards,
Kristof
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?C45542E7-84F0-488B-9F38-D5A49797A9BE>