Received-SPF: pass (google.com: best guess record for domain of linux-wireless-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [RFCv3 2/3] nl80211: Support >4096 byte NEW_WIPHY event nlmsg
To:     Johannes Berg <johannes@sipsolutions.net>,
        linux-wireless@vger.kernel.org
References: <20190906154303.9303-1-denkenz@gmail.com>
 <20190906154303.9303-2-denkenz@gmail.com>
 <4397b67b63d1b1a332afa9010e7f48abd54b49b2.camel@sipsolutions.net>
 <5bd58103-bdb7-b72c-0b64-76c8573ca380@gmail.com>
 <1eac4f853b835fef85cdf33d971382b2f6e7c5a9.camel@sipsolutions.net>
From:   Denis Kenzior <denkenz@gmail.com>
Message-ID: <8b22b64e-5efc-beac-6581-778e47625c89@gmail.com>
Date:   Wed, 11 Sep 2019 08:51:01 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.8.0
MIME-Version: 1.0
In-Reply-To: <1eac4f853b835fef85cdf33d971382b2f6e7c5a9.camel@sipsolutions.net>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

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