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
