svn commit: r360068 - in head/sys: kern net sys

[Kristof Provost]

On 21 Apr 2020, at 4:34, Kyle Evans wrote:
> On Mon, Apr 20, 2020 at 9:14 PM Kyle Evans <kevans at freebsd.org> wrote:
>>
>> On Mon, Apr 20, 2020 at 8:15 PM Eric van Gyzen <eric at 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