Re: bridge sioc[gs]drvspec operations incompatible with COMPAT_NETBSD32

[Ryota Ozaki <ozaki-r%netbsd.org@localhost>]

On Mon, Jun 1, 2015 at 2:37 PM, Matt Thomas <matt%3am-software.com@localhost> wrote:
>
>> On May 31, 2015, at 8:36 PM, Ryota Ozaki <ozaki-r%netbsd.org@localhost> wrote:
>>
>> On Sun, May 31, 2015 at 6:50 PM, Roy Marples <roy%marples.name@localhost> wrote:
>>> Hi Matt
>>>
>>>
>>> On 2015-05-30 01:02, Matt Thomas wrote:
>>>>
>>>> The use of SIOC[GS]DRVSPEC to copyin or copyout other structures which
>>>> have pointers/size_t/u_long makes them very hard to deal with in
>>>> COMPAT_NETBSD32.
>>>>
>>>> The simplest solution is to eliminate the use of the ifbifconf and
>>>> ifbaconf structures in userland and have BRDGGIFS and BRDGRTS use the
>>>> ifdrv struct members ifd_len and ifb_data directly for their needs.
>>>> The netbsd32 compat code already deals with this so this just requires
>>>> a small change to if_bridge{.c,var.h}.  I also converted ifbareq to
>>>> use fixed types in the diff.
>>>>
>>>> Make brconfig to deal with the new method actually makes brconfig
>>>> simplier.
>>>>
>>>> There is the problem of missing compat code for the old ifbareq but
>>>> I'm not sure if it's really required.
>>>>
>>>> Comments?
>>>
>>>
>>> Thanks for working on this!
>>>
>>> I took your patch and adjusted it some more:
>>>  *  Added a check in the kernel if we have a function in the command
>>>     table as the ipfilter stuff is optional and I think the table
>>>     will now always grow beyond it.
>>>  *  added an extra parameter to do_cmd in brconfig.c so we can
>>>     get the returned length from the ioctl. This allows us to know
>>>     if we need a bigger buffer or not.
>>>
>>> Seems to be working fine now, at least for setup.
>>> Will be able to plug another interface in once PPPoE works to actually
>>> test it with though.
>>>
>>> While in brconfig, I notice that the kernel returns the length needed
>>> if the buffer is too small, but brconfig just does old length *2 and
>>> tries again. Is it worthwile fixing this to grow a buffer of what
>>> the kernel actually wants?
>>
>> I think so and changing to len = olen just works for me :)
>>
>> And bridge ATF tests passed (on amd64); I tried the tests with
>> intentional initial small buffers and the logic of growing buffers
>> looks working.
>>
>>>
>>> Comments on this welcome!
>>
>> LGTM except a nitpick, trailing spaces at [OBRDGGIFS] = ... and
>> [OBRDGRTS] = ... (they were originally there though :-/).
>
> new diff for if_bridge* is at http://www.netbsd.org/~matt/ifbridge-diff.txt
> new diff for brconfig.c is at http://www.netbsd.org/~matt/brconfig-diff.txt

Need s/for (;;) {/do {/ in show_interfaces.

>
> Just minor cleanups.
>

With the fix above, the patch works for me.

  ozaki-r