Hi Jouni,
Thank you for the suggestions. I have modified as you suggested and posted the new patches.
Could you help to review?
Thank you.
Chin-Ran
> -----Original Message-----
> From: Jouni Malinen <j@xxxxx>
> Sent: Thursday, August 29, 2024 4:34 PM
> To: Chin-Ran Lo <chin-ran.lo@xxxxxxx>
> Cc: hostap@xxxxxxxxxxxxxxxxxxx; Pete Hsieh <tsung-hsien.hsieh@xxxxxxx>
> Subject: [EXT] Re: [PATCH v7 2/2] Add the similar USD APIs to dbus control
> interface that other apps can use the functions
>
> Caution: This is an external email. Please take care when clicking links or
> opening attachments. When in doubt, report the message using the 'Report
> this email' button
>
>
> 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.
Chin-Ran> It's changed in the latest patch
>
> 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.
Chin-Ran> It's because nan_de_cancel_publish() needs publish_id. I think dbus should just pass down what the required parameters the engine requested.
>
> What about the new signals? It would be good to document them in
> dbus.doxygen.
Chin-Ran> They are added
>
> > 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?
Chin-Ran> It's unexpected. I have rolled it back.
>
> > 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..
Chin-Ran> It's changed in the latest patch
>
> > @@ -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.
>
Chin-Ran> It's added / changed in the latest patch
> > + { "NanPublishterminated", WPAS_DBUS_NEW_IFACE_INTERFACE,
>
> Why would this use inconsistent spelling of NAN? I would uses
> "NANPublishTerminated"
>
Chin-Ran> It's changed in the latest patch
> > + { "NanSubscribeterminated", WPAS_DBUS_NEW_IFACE_INTERFACE,
>
> and "NANSubscribeTerminated".
>
Chin-Ran> It's changed in the latest patch
> And this list of signals seems to be missing "NANReplied".
>
Chin-Ran> It's added in the latest patch
> > +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".
Chin-Ran> It's changed in the latest patch
>
> > +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.
Chin-Ran> It's removed in the latest patch
>
> > + 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.
Chin-Ran> It's changed in the latest patch
>
> > + memcpy(reply_info.peer_addr, peer_addr, ETH_ALEN);
>
> Please use os_memcpy() instead of memcpy().
>
Chin-Ran> It's fixed in the latest patch
> > +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"
>
Chin-Ran> It's changed in the latest patch
> > +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.
Chin-Ran> It's removed in the latest patch
>
> > +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"
>
Chin-Ran> It's changed in the latest patch
> > + 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().
Chin-Ran> It's removed in the latest patch
>
> > 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?
Chin-Ran> It's unexpected and removed in the latest patch
>
> > 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?
>
Chin-Ran> It's changed in the latest patch
> 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.
>
Chin-Ran> It's removed in the latest patch, since the parameters are different
> > +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.
Chin-Ran> It's changed in the latest patch
>
> > + 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.
>
Chin-Ran> It's changed in the latest patch
> --
> Jouni Malinen PGP id
> EFC895FA
_______________________________________________
Hostap mailing list
Hostap@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/hostap
