On 8/5/24 23:01, Jouni Malinen wrote:
On Thu, Aug 01, 2024 at 10:21:38PM +0530, Aditya Kumar Singh wrote:
diff --git a/hostapd/hostapd_cli.c b/hostapd/hostapd_cli.c
@@ -54,7 +54,11 @@ static void usage(void)
+#ifdef CONFIG_IEEE80211BE
+ "usage: hostapd_cli [-p<path>] [-i<ifname>] [-l<link_id>] [-hvBr] "
+#else
"usage: hostapd_cli [-p<path>] [-i<ifname>] [-hvBr] "
+#endif /* CONFIG_IEEE80211BE */
Please avoid duplicated versions by splitting that into
"usage: hostapd_cli [-p<path>] [-i<ifname>] "
#ifdef CONFIG_IEEE80211BE
"[-l<link_id>] "
#endif /* CONFIG_IEEE80211BE */
"[-hvBr] "
Sure will do.
+#ifdef CONFIG_IEEE80211BE
+ " -l<link_id> Link ID of the interface in case of Multi-Link\n"
+ " Operation\n"
That "Operation" fits fine into the end of the previous printed line..
Okay.
@@ -2205,19 +2214,26 @@ static void hostapd_cli_action(struct wpa_ctrl *ctrl)
eloop_unregister_read_sock(fd);
}
-
int main(int argc, char *argv[])
Please no unrelated whitespace cleanup (especially when it is actually
incorrect for the coding style used in hostap.git).
Got it. This came accidentally. Thanks for pointing it out.
+#ifdef CONFIG_IEEE80211BE
+ c = getopt(argc, argv, "a:BhG:i:l:p:P:rs:v");
+#else
c = getopt(argc, argv, "a:BhG:i:p:P:rs:v");
+#endif /* CONFIG_IEEE80211BE */
Please avoid duplicated things. I would go with that CONFIG_IEEE80211BE
case for both since the default case in the switch will handle that fine
as-is.
Sure, got it.
@@ -2252,6 +2268,16 @@ int main(int argc, char *argv[])
+#ifdef CONFIG_IEEE80211BE
+ case 'l':
+ link_id = atoi(optarg);
+ os_memset(buf, '\0', sizeof(buf));
+ os_snprintf(buf, sizeof(buf), "%s_%s%d",
+ ctrl_ifname, WPA_CTRL_IFACE_LINK_NAME, link_id);
No os_memset() is needed before os_snprintf(), but use of
os_snprintf_error() would be recommended.
Okay.
diff --git a/src/ap/hostapd.c b/src/ap/hostapd.c
+static void hostapd_set_ctrl_sock_iface(struct hostapd_data *hapd)
+{
+#ifdef CONFIG_IEEE80211BE
+ os_memset(hapd->ctrl_sock_iface, '\0',
+ sizeof(hapd->ctrl_sock_iface));
+ os_strlcpy(hapd->ctrl_sock_iface, hapd->conf->iface,
+ sizeof(hapd->ctrl_sock_iface));
No os_memset() before os_strlcpy()..
Got it.
+ if (hapd->conf->mld_ap) {
+ char buf[128];
+
+ os_memset(buf, '\0', sizeof(buf));
+ os_snprintf(buf, sizeof(buf), "%s_%s%d",
+ hapd->conf->iface, WPA_CTRL_IFACE_LINK_NAME,
+ hapd->mld_link_id);
+ os_memset(hapd->ctrl_sock_iface, '\0',
+ sizeof(hapd->ctrl_sock_iface));
+ os_strlcpy(hapd->ctrl_sock_iface, buf, sizeof(buf));
No os_memset() before os_snprintf()/os_strlcpy().
That last sizeof(buf) is very wrong for the os_strlcpy().. It is
supposed to be the size of the target buffer. This could result in
buffer overflow..
Why is that buf[] stack buffer used here? Couldn't this simply
os_snprintf() to hapd->ctrl_sock_iface?
Instead of first writing the non-mld_ap value into hapd->ctrl_sock_iface
and then overwriting it, this would be cleaner by having if-else..
Sure will move to to if-else. Thanks for the suggestion.
diff --git a/src/ap/hostapd.h b/src/ap/hostapd.h
@@ -476,6 +476,7 @@ struct hostapd_data {
struct hostapd_mld *mld;
struct dl_list link;
u8 mld_link_id;
+ char ctrl_sock_iface[IFNAMSIZ + 1];
Is that large enough to include the "_link" + ID part?
Oops! Nope, typically tested with interface names not going beyond 6-7
chars. But ideally this should be be "IFNAMSIZ + 7+ 1". Thanks for
pointing this out.
Thanks for your review Jouni. I will address these and send next version
for review.
--
Aditya
_______________________________________________
Hostap mailing list
Hostap@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/hostap
