[PATCH v5 4/6] hostapd_cli: MLO: add status command for MLD socket

[Aditya Kumar Singh]

On 9/1/24 14:33, Jouni Malinen wrote:
> On Tue, Aug 13, 2024 at 02:08:50PM +0530, Aditya Kumar Singh wrote:
>> Add MLD level 'status' command. Currently each link level socket has got
>> 'status' command. When the same is passed on MLD level socket without any
>> link id, it routes it to first BSS of the MLD if available. Handle this
>> now properly.
>>
>> If link id is not passed then it will be treated as MLD level status
>> command.
> 
> This is not a hostapd_cli change, but a hostapd change to add a STATUS
> control interface command. Sure, that new command could be used with
> hostapd_cli, but this patch does not introduce any new hostapd_cli
> changes. The commit message should be clearer on that.

Sure, noted. Thanks for correcting.

> 
>>> status
>> name=wlan0
>> mld_address=AA:BB:CC:DD:EE:FF
>> num_links=2
>> LINK INFORMATION
> 
> That "LINK INFORMATION" line feels a bit strange. Why would it be needed
> here?

Just to separate MLD level info from link level info. Can be omitted if 
that's not really required.

> 
>> link_id=0
>> link_addr=AA:BB:CC:DD:EE:EE
>> link_id=1
>> link_addr=AA:BB:CC:DD:FF:FF
> 
> link_addr[0]=AA:BB:CC:DD:EE:EE
> link_addr[1]=AA:BB:CC:DD:FF:FF
> 
> Might be easier to use in a parser since that would provide unique
> <name>=<value> lines from the command.

Yeah, see your point.

> 
>> diff --git a/hostapd/ctrl_iface.c b/hostapd/ctrl_iface.c
>> @@ -4724,6 +4724,49 @@ done:
>> +int hostapd_ctrl_mld_iface_status(struct hostapd_mld *mld, char *buf,
>> +				  size_t buflen)
> 
>> +	if (!mld->fbss) {
>> +		ret = os_snprintf(buf + len, buflen - len,
>> +				  "\n No Link information present\n");
>> +		if (os_snprintf_error(buflen - len, ret))
>> +			return len;
>> +		len += ret;
>> +	}
> 
> What is that " No Link information present" supposed to do here and why
> does this continue printing "LINK INFORMATION" after this?

Oops! return should be there. Thanks for pointing it out. This check is 
there, let's say you have MLD with 2 links initially. Now, you have 
disabled both of the links. So during this time, link info will not 
printed. Hence, that check is there.

> 
> That initial space on the line and now equals sign makes this somewhat
> inconvenient if one were to want to use a generic parser of
> <name>=<value> pairs.

Oh okay. Got it.

> 
>> +	ret = os_snprintf(buf + len, buflen - len,
>> +			 "LINK INFORMATION\n");
> 
> This would be added after that " No Link information present" line..

Yeah. return should be there in the if().


Having said that, do you want me to re-spin a new version? Or will you 
correct it while applying?

-- 
Aditya