diff mbox series

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

Message ID 20240813083852.3945773-5-quic_adisi@quicinc.com
State Changes Requested
Headers show
Series MLO control socket changes | expand

Commit Message

Aditya Kumar Singh Aug. 13, 2024, 8:38 a.m. UTC 
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.

$ hostapd_cli -i wlan0
....
Interactive mode

> status
name=wlan0
mld_address=AA:BB:CC:DD:EE:FF
num_links=2
LINK INFORMATION
link_id=0
link_addr=AA:BB:CC:DD:EE:EE
link_id=1
link_addr=AA:BB:CC:DD:FF:FF

Signed-off-by: Aditya Kumar Singh <quic_adisi@quicinc.com>
---
 hostapd/ctrl_iface.c | 46 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

Comments

Jouni Malinen Sept. 1, 2024, 9:03 a.m. UTC | #1 
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.

> > 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?

> 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.

> 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?

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.

> +	ret = os_snprintf(buf + len, buflen - len,
> +			 "LINK INFORMATION\n");

This would be added after that " No Link information present" line..
Aditya Kumar Singh Sept. 2, 2024, 7:37 a.m. UTC | #2 
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?
diff mbox series

Patch

diff --git a/hostapd/ctrl_iface.c b/hostapd/ctrl_iface.c
index fe46c37a7930..e868b826d040 100644
--- a/hostapd/ctrl_iface.c
+++ b/hostapd/ctrl_iface.c
@@ -4724,6 +4724,49 @@  done:
 
 #ifdef CONFIG_IEEE80211BE
 #ifndef CONFIG_CTRL_IFACE_UDP
+int hostapd_ctrl_mld_iface_status(struct hostapd_mld *mld, char *buf,
+				  size_t buflen)
+{
+	struct hostapd_data *link_hapd;
+	int len = 0, ret;
+
+	ret = os_snprintf(buf + len, buflen - len,
+			  "name=%s\n"
+			  "mld_address=" MACSTR "\n"
+			  "num_links=%d\n",
+			  mld->name, MAC2STR(mld->mld_addr), mld->num_links);
+	if (os_snprintf_error(buflen - len, ret))
+		return len;
+	len += ret;
+
+	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;
+	}
+
+	ret = os_snprintf(buf + len, buflen - len,
+			 "LINK INFORMATION\n");
+	if (os_snprintf_error(buflen - len, ret))
+		return len;
+	len += ret;
+
+	dl_list_for_each(link_hapd, &mld->links, struct hostapd_data, link) {
+		ret = os_snprintf(buf + len, buflen - len,
+				 "link_id=%d\n"
+				 "link_addr=" MACSTR "\n",
+				 link_hapd->mld_link_id, MAC2STR(link_hapd->own_addr));
+		if (os_snprintf_error(buflen - len, ret))
+			return len;
+		len += ret;
+	}
+
+	return len;
+}
+
+
 static int hostapd_mld_ctrl_iface_receive_process(struct hostapd_mld *mld,
 						  char *buf, char *reply,
 						  int reply_size,
@@ -4784,6 +4827,9 @@  static int hostapd_mld_ctrl_iface_receive_process(struct hostapd_mld *mld,
 	} else if (os_strcmp(cmd, "DETACH") == 0) {
 		if (ctrl_iface_detach(&mld->ctrl_dst, from, fromlen))
 			reply_len = -1;
+	} else if (os_strcmp(buf, "STATUS") == 0 && link_id == -1){
+		reply_len = hostapd_ctrl_mld_iface_status(mld, reply,
+							  reply_size);
 	} else {
 		if (link_id == -1)
 			wpa_printf(MSG_DEBUG,