Re: [RFCv3 2/3] nl80211: Support >4096 byte NEW_WIPHY event nlmsg

[Denis Kenzior <denkenz@gmail.com>]

Hi Johannes,

On 9/11/19 10:28 AM, Johannes Berg wrote:
> On Wed, 2019-09-11 at 07:41 -0500, Denis Kenzior wrote:
>>
>> For my dual band cards the channel dump all fits into a ~1k attribute if
>> I recall correctly.  So there may not really be a need for this.  Or at
>> the very least we could keep things simple(r) and only split at the band
>> level, not at the individual channel level.
> 
> Yeah, that does seem reasonable, especially if we're moving to bigger
> messages anyway. If we do add something huge to each channel, we can
> recover that code I suppose.

So do you want me to drop the channel splitting logic and only allow 
this for bands?  Or just keep this since it is already done?

>> The current logic uses last_channel_pos for some of the trickery in
>> addition to last_good_pos.  So nla_put_failure would have to be made
>> aware of that.  Perhaps we can store last_good_pos in the stack, but the
>> split mechanism only allows the splits to be done at certain points...
> 
> Hmm, not sure I understand. last_channel_pos and last_good_pos are
> basically equivalent, no? In fact, I'm not sure why you need
> last_channel_pos vs. just using last_good_pos instead? Maybe I'm missing
> something.

Sort of.  The way I did it, last_channel_pos keeps track of whether any 
channel info was added or not.  So if NULL, we simply backtrack to 
last_good_pos in nla_put_failure. You can probably use last_good_pos for 
everything and an extra variable for the channel info tracking.

> 
> To me, conceptually, the "state->band_start" and "state->chan_start" is
> basically a sub-state of "state->split", so this is underneath state-
>> split == 3 (I think?), you basically get 3.0.0, 3.0.1, 3.0.2, ...,
> 3.1.0, 3.1.1 ... for the state? Which you have to unwind in terms of
> message formatting, but the last_good_pos should be sufficient?

Right.  And as I mentioned above, this could be done, but you probably 
need another state variable..

> 
> IOW, the only difference I see between the normal split states 1, 2, ...
> and the band/channel split states 3.0.0, 3.0.1, ... is the fact that we
> have to also fix up the nested attributes after we trim to last_good_pos
> on failures. Where am I wrong?
> 

Didn't say that you were ;)

>> Right now only the channel dump uses this (and I'm still not fully
>> convinced we should go to all the trouble), so one argument would be not
>> to introduce something this generic until another user of it manifests
>> itself?
> 
> I was thinking it'd actually be less complex, but I guess I have to try
> to write it to see for myself.

To be clear, I think it is a good approach and can be made to work.  My 
main hesitation is whether doing it now is worth it given the discussion 
at the very top.  But I can see what I can come up with if you want.

Regards,
-Denis