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

[Kyle Evans]

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.

Thanks,

Kyle Evans