On Fri, Aug 23, 2024 at 09:41:19AM +0000, Chin-Ran Lo wrote:
> diff --git a/doc/dbus.doxygen b/doc/dbus.doxygen
> + <h3>NANPublish ( (sybbbqbbbqqqqvv) : nan_args ) --> nothing</h3>
> + <p>Set the parameters of nan-publish for the interface.</p>
This looks very different style from existing methods. Could you please
clarify the reasoning behind this vs. the "a{sv} : args" style used in
many existing methods? For example, see how the Scan() method is used
with a large number of optional named arguments in a dictionary.
That "--> nothing" part looks incorrect. Isn't this returning
"publish_id : i"?
> + <h3>NANCancelPublish ( i : nan_args ) --> nothing</h3>
Calling that nan_args feels unnecessarily confusing when this is
publish_id from NANPublish.
What about the new signals? It would be good to document them in
dbus.doxygen.
> diff --git a/src/common/nan_de.h b/src/common/nan_de.h
> @@ -96,7 +96,7 @@ struct nan_publish_params {
> unsigned int freq;
>
> /* Multi-channel frequencies (publishChannelList) */
> - const int *freq_list;
> + int *freq_list;
Why?
> diff --git a/wpa_supplicant/dbus/dbus_new.c b/wpa_supplicant/dbus/dbus_new.c
> @@ -3605,6 +3605,52 @@ static const struct wpa_dbus_method_desc wpas_dbus_interface_methods[] = {
> +#ifdef CONFIG_NAN_USD
> + { "NANPublish", WPAS_DBUS_NEW_IFACE_INTERFACE,
> + (WPADBusMethodHandler) wpas_dbus_handler_nan_publish,
> + {
> + { "nan_args", "(sybbbqbbbqqqqvv)", ARG_IN },
> + { "publish_id", "i", ARG_OUT },
> + END_ARGS
> + }
> + },
> + { "NANCancelPublish", WPAS_DBUS_NEW_IFACE_INTERFACE,
> + (WPADBusMethodHandler) wpas_dbus_handler_nan_cancel_publish,
> + {
> + { "nan_args", "i", ARG_IN },
> + END_ARGS
> + }
> + },
The comments above on dbus.doxygen applies here as well..
> @@ -3983,6 +4029,38 @@ static const struct wpa_dbus_signal_desc wpas_dbus_interface_signals[] = {
> +#ifdef CONFIG_NAN_USD
> + { "NANDiscoveryresult", WPAS_DBUS_NEW_IFACE_INTERFACE,
And these are the undocumented signals..
I would use upper case 'r' in Result here, i.e., NANDiscoveryResult.
> + { "NanPublishterminated", WPAS_DBUS_NEW_IFACE_INTERFACE,
Why would this use inconsistent spelling of NAN? I would uses
"NANPublishTerminated"
> + { "NanSubscribeterminated", WPAS_DBUS_NEW_IFACE_INTERFACE,
and "NANSubscribeTerminated".
And this list of signals seems to be missing "NANReplied".
> +void wpas_dbus_signal_nan_discovery_result(struct wpa_supplicant *wpa_s,
> + msg = dbus_message_new_signal(wpa_s->dbus_new_path,
> + WPAS_DBUS_NEW_IFACE_INTERFACE,
> + "NanDiscoveryresult");
That name is not consistent with the list of signals, i.e., this should
be "NANDiscoveryResult".
> +void wpas_dbus_signal_nan_replied(struct wpa_supplicant *wpa_s,
> + wpa_printf(MSG_INFO, "DBUS NanReplied");
I would not add that print at INFO level here.
> + msg = dbus_message_new_signal(wpa_s->dbus_new_path,
> + WPAS_DBUS_NEW_IFACE_INTERFACE,
> + "NanReplied");
The name should be "NANReplied" to be consistent with other methods and
signals.
> + memcpy(reply_info.peer_addr, peer_addr, ETH_ALEN);
Please use os_memcpy() instead of memcpy().
> +void wpas_dbus_signal_nan_receive(struct wpa_supplicant *wpa_s,
> + msg = dbus_message_new_signal(wpa_s->dbus_new_path,
> + WPAS_DBUS_NEW_IFACE_INTERFACE,
> + "NanReceive");
"NANReceive"
> +void wpas_dbus_signal_nan_publish_terminated(struct wpa_supplicant *wpa_s,
> + int publish_id, int reason)
> + msg = dbus_message_new_signal(wpa_s->dbus_new_path,
> + WPAS_DBUS_NEW_IFACE_INTERFACE,
> + "NanPublishterminated");
"NANPublishTerminated"
> + if (!dbus_message_append_args(msg, DBUS_TYPE_INT32, &dpub_id,
> + DBUS_TYPE_INVALID) ||
> + !dbus_message_append_args(msg, DBUS_TYPE_INT32, &dreason,
> + DBUS_TYPE_INVALID))
> + wpa_printf(MSG_ERROR, "dbus: Failed to construct signal");
> + else {
> + dbus_connection_send(iface->con, msg, NULL);
> + wpa_msg(wpa_s, MSG_INFO, NAN_SUBSCRIBE_TERMINATED
> + "wpas_dbus_signal_nan_subscribe_terminated() dbus_connection_send (int)");
> + }
Why would the D-Bus interface implementation send out that control
interface NAN_SUBSCRIBE_TERMINATED event with wpa_msg()? Please remove
it. wpa_printf() could be used for debug prints, but I don't see need
for this entry on success.
> +void wpas_dbus_signal_nan_subscribe_terminated(struct wpa_supplicant *wpa_s,
> + msg = dbus_message_new_signal(wpa_s->dbus_new_path,
> + WPAS_DBUS_NEW_IFACE_INTERFACE,
> + "NanSubscribeterminated");
"NANSubscribeTerminated"
> + if (!dbus_message_append_args(msg, DBUS_TYPE_INT32, &dsub_id,
> + DBUS_TYPE_INVALID) ||
> + !dbus_message_append_args(msg, DBUS_TYPE_INT32, &dreason,
> + DBUS_TYPE_INVALID))
> + wpa_printf(MSG_ERROR, "dbus: Failed to construct signal");
> + else {
> + dbus_connection_send(iface->con, msg, NULL);
> + wpa_msg(wpa_s, MSG_INFO, NAN_SUBSCRIBE_TERMINATED
> + "wpas_dbus_signal_nan_subscribe_terminated() dbus_connection_send (int)");
> + }
Same here about wpa_msg().
> diff --git a/wpa_supplicant/dbus/dbus_new.h b/wpa_supplicant/dbus/dbus_new.h
> +++ b/wpa_supplicant/dbus/dbus_new.h
> @@ -21,6 +21,7 @@ struct wpa_bss;
> struct wps_event_m2d;
> struct wps_event_fail;
> struct wps_credential;
> +struct wpa_dbus_discov_info;
Why?
> diff --git a/wpa_supplicant/dbus/dbus_new_handlers.c b/wpa_supplicant/dbus/dbus_new_handlers.c
> +#define MAX_NAN_SRV_NAME_LEN 256
> +#define NAN_FREQ_LIST_ALL 0xff
That second name and use of hex looks strange here since
NAN_FREQ_LIST_ALL is used as the maximum number of frequencies. Why not
"MAX_NAN_FREQS 255" to be consistent with the other limit?
Is there particular need for these explicit limits?
> +int unit_len(int dbus_type)
That should be a static function since it is not used outside this file.
> +{
> + switch (dbus_type) {
> + case DBUS_TYPE_BYTE:
> + case DBUS_TYPE_BOOLEAN:
> + return 1;
> + case DBUS_TYPE_INT16:
> + case DBUS_TYPE_UINT16:
> + return 2;
> + case DBUS_TYPE_INT32:
> + case DBUS_TYPE_UINT32:
> + return 4;
> + case DBUS_TYPE_INT64:
> + case DBUS_TYPE_UINT64:
> + case DBUS_TYPE_DOUBLE:
> + return 8;
> + }
> + return 0;
> +}
I don't really like this at all.. Isn't there any shared function from
the library to get this information? Then again, I don't think this
should be needed at all since the overall design with fixed sequence of
arguments looks really inconvenient.
> +static bool get_gvariant_items(DBusMessageIter *piter, int type, void* datpt, u32 dat_buf_len)
Why would this be needed? Wouldn't it be much more flexible and future
proof to use an array with a dictionary of named arguments like more or
less all other methods taking in multiple optional values use?
> +DBusMessage * wpas_dbus_handler_nan_publish(DBusMessage *message,
> + struct wpa_supplicant *wpa_s)
> + // Extract the elements
> + if (get_gvariant_items(&subiter, DBUS_TYPE_STRING, (void*)&psrv_name, sizeof(&psrv_name)) == false) {
> + wpa_printf(MSG_ERROR, "Error while fetching srv_name");
> + goto fail;
> + }
Isn't this style of get_gvariant_items() calls forcing all the arguments
to be present and in the exact this order? That seems to make this way
too strict and inconvenient to use. Dictionary of named arguments seems
to be much cleaner approach.
> + strncpy(service_name, psrv_name, MAX_NAN_SRV_NAME_LEN-1);
That could result in a truncated string. wpa_dbus_dict_*() helpers would
provide all the needed code to take care of this type of details and
would also get rid of the explicit MAX_NAN_SRV_NAME_LEN length limit.
--
Jouni Malinen PGP id EFC895FA
_______________________________________________
Hostap mailing list
Hostap@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/hostap
