| Message ID | 20200818152612.BlueZ.v1.3.I19ff9cdbd40fe453db0e81aec8bf94dd9490dce3@changeid (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [BlueZ,v1,1/7] adv_monitor: Introduce org.bluez.AdvertisementMonitorManager1 interface | expand |
Hi Miao, On Tue, Aug 18, 2020 at 3:31 PM Miao-chen Chou <mcchou@chromium.org> wrote: > > This implements the RegisterMonitor() method handler of ADV monitor > manager interface. > > The following tests were performed. > - Issue a RegisterMonitor() call with a valid path and expect a > success as return. > - Issue a RegisterMonitor() call with an invalid path and expect > org.bluez.Error.InvalidArguments as return. > - Issue two Registermonitor() calls with the same path and expect > org.bluez.Error.AlreadyExists. > - Verify the values of the registered paths with logging. > - Verify D-Bus disconnection callback was triggered when the client detach > from D-Bus. > > Reviewed-by: Yun-Hao Chung <howardchung@google.com> > Reviewed-by: Manish Mandlik <mmandlik@chromium.org> > Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> > --- > > src/adv_monitor.c | 167 +++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 166 insertions(+), 1 deletion(-) > > diff --git a/src/adv_monitor.c b/src/adv_monitor.c > index 4d02237e8..3d27ad18b 100644 > --- a/src/adv_monitor.c > +++ b/src/adv_monitor.c > @@ -22,7 +22,9 @@ > #endif > > #define _GNU_SOURCE > +#include <errno.h> > #include <stdint.h> > +#include <string.h> > > #include <glib.h> > #include <dbus/dbus.h> > @@ -34,7 +36,9 @@ > #include "adapter.h" > #include "dbus-common.h" > #include "log.h" > +#include "src/error.h" > #include "src/shared/mgmt.h" > +#include "src/shared/queue.h" > #include "src/shared/util.h" > > #include "adv_monitor.h" > @@ -52,12 +56,170 @@ struct btd_adv_monitor_manager { > uint16_t max_num_monitors; > uint8_t max_num_patterns; > > + struct queue *apps; /* apps who registered for Adv monitoring */ > }; > > +struct adv_monitor_app { > + struct btd_adv_monitor_manager *manager; > + char *owner; > + char *path; > + > + DBusMessage *reg; > + GDBusClient *client; > +}; > + > +struct app_match_data { > + const char *owner; > + const char *path; > +}; > + > +/* Replies to an app's D-Bus message and unref it */ > +static void app_reply_msg(struct adv_monitor_app *app, DBusMessage *reply) > +{ > + if (!app || !app->reg || !reply) > + return; > + > + g_dbus_send_message(btd_get_dbus_connection(), reply); > + dbus_message_unref(app->reg); > + app->reg = NULL; > +} > + > +/* Destroys an app object along with related D-Bus handlers */ > +static void app_destroy(void *data) > +{ > + struct adv_monitor_app *app = data; > + > + if (!app) > + return; > + > + DBG("Destroy Adv Monitor app %s at path %s", app->owner, app->path); > + > + if (app->reg) { > + app_reply_msg(app, btd_error_failed(app->reg, > + "Adv Monitor app destroyed")); > + } > + > + if (app->client) { > + g_dbus_client_set_disconnect_watch(app->client, NULL, NULL); > + g_dbus_client_set_proxy_handlers(app->client, NULL, NULL, NULL, > + NULL); > + g_dbus_client_set_ready_watch(app->client, NULL, NULL); > + g_dbus_client_unref(app->client); > + app->client = NULL; > + } > + > + g_free(app->owner); > + app->owner = NULL; > + g_free(app->path); > + app->path = NULL; Same comment as before, if the whole object would be free then there is no need to reset each individual field. > + > + g_free(app); > +} > + > +/* Handles a D-Bus disconnection event of an app */ > +static void app_disconnect_cb(DBusConnection *conn, void *user_data) > +{ > + struct adv_monitor_app *app = user_data; > + > + btd_info(app->manager->adapter_id, "Adv Monitor app %s disconnected " > + "from D-Bus", app->owner); > + if (app && queue_remove(app->manager->apps, app)) > + app_destroy(app); > +} > + > +/* Creates an app object, initiates it and sets D-Bus event handlers */ > +static struct adv_monitor_app *app_create(DBusConnection *conn, > + const char *sender, const char *path, > + struct btd_adv_monitor_manager *manager) > +{ > + struct adv_monitor_app *app; > + > + if (!path || !sender || !manager) > + return NULL; > + > + app = g_new0(struct adv_monitor_app, 1); Please use new0 instead of g_new0 in new code. > + if (!app) > + return NULL; > + > + app->owner = g_strdup(sender); > + app->path = g_strdup(path); > + app->manager = manager; > + app->reg = NULL; > + > + app->client = g_dbus_client_new(conn, sender, path); > + if (!app->client) { > + app_destroy(app); > + return NULL; > + } > + > + g_dbus_client_set_disconnect_watch(app->client, app_disconnect_cb, app); > + g_dbus_client_set_proxy_handlers(app->client, NULL, NULL, NULL, NULL); > + g_dbus_client_set_ready_watch(app->client, NULL, NULL); > + > + return app; > +} > + > +/* Matches an app based on its owner and path */ > +static bool app_match(const void *a, const void *b) > +{ > + const struct adv_monitor_app *app = a; > + const struct app_match_data *match = b; > + > + if (match->owner && strcmp(app->owner, match->owner)) > + return false; > + > + if (match->path && strcmp(app->path, match->path)) > + return false; > + > + return true; > +} > + > +/* Handles a RegisterMonitor D-Bus call */ > +static DBusMessage *register_monitor(DBusConnection *conn, DBusMessage *msg, > + void *user_data) > +{ > + DBusMessageIter args; > + struct app_match_data match; > + struct adv_monitor_app *app; > + struct btd_adv_monitor_manager *manager = user_data; > + > + if (!dbus_message_iter_init(msg, &args)) > + return btd_error_invalid_args(msg); > + > + if (dbus_message_iter_get_arg_type(&args) != DBUS_TYPE_OBJECT_PATH) > + return btd_error_invalid_args(msg); > + > + dbus_message_iter_get_basic(&args, &match.path); > + > + if (!strlen(match.path) || !g_str_has_prefix(match.path, "/")) > + return btd_error_invalid_args(msg); > + > + match.owner = dbus_message_get_sender(msg); > + > + if (queue_find(manager->apps, app_match, &match)) > + return btd_error_already_exists(msg); > + > + app = app_create(conn, match.owner, match.path, manager); > + if (!app) { > + btd_error(manager->adapter_id, > + "Failed to reserve %s for Adv Monitor app %s", > + match.path, match.owner); > + return btd_error_failed(msg, > + "Failed to create Adv Monitor app"); > + } > + > + queue_push_tail(manager->apps, app); > + > + btd_info(manager->adapter_id, "Path %s reserved for Adv Monitor app %s", > + match.path, match.owner); > + > + return dbus_message_new_method_return(msg); > +} > + > static const GDBusMethodTable adv_monitor_methods[] = { > { GDBUS_METHOD("RegisterMonitor", > GDBUS_ARGS({ "application", "o" }), > - NULL, NULL) }, > + NULL, register_monitor) }, > { GDBUS_ASYNC_METHOD("UnregisterMonitor", > GDBUS_ARGS({ "application", "o" }), > NULL, NULL) }, > @@ -157,6 +319,7 @@ static struct btd_adv_monitor_manager *manager_new( > manager->mgmt = mgmt_ref(mgmt); > manager->adapter_id = btd_adapter_get_index(adapter); > manager->path = g_strdup(adapter_get_path(manager->adapter)); > + manager->apps = queue_new(); > > return manager; > } > @@ -170,6 +333,8 @@ static void manager_free(struct btd_adv_monitor_manager *manager) > g_free(manager->path); > manager->path = NULL; > > + queue_destroy(manager->apps, app_destroy); > + > g_free(manager); > } > > -- > 2.26.2 >
Hi Luiz, On Tue, Sep 8, 2020 at 10:25 AM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > Hi Miao, > > On Tue, Aug 18, 2020 at 3:31 PM Miao-chen Chou <mcchou@chromium.org> wrote: > > > > This implements the RegisterMonitor() method handler of ADV monitor > > manager interface. > > > > The following tests were performed. > > - Issue a RegisterMonitor() call with a valid path and expect a > > success as return. > > - Issue a RegisterMonitor() call with an invalid path and expect > > org.bluez.Error.InvalidArguments as return. > > - Issue two Registermonitor() calls with the same path and expect > > org.bluez.Error.AlreadyExists. > > - Verify the values of the registered paths with logging. > > - Verify D-Bus disconnection callback was triggered when the client detach > > from D-Bus. > > > > Reviewed-by: Yun-Hao Chung <howardchung@google.com> > > Reviewed-by: Manish Mandlik <mmandlik@chromium.org> > > Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> > > --- > > > > src/adv_monitor.c | 167 +++++++++++++++++++++++++++++++++++++++++++++- > > 1 file changed, 166 insertions(+), 1 deletion(-) > > > > diff --git a/src/adv_monitor.c b/src/adv_monitor.c > > index 4d02237e8..3d27ad18b 100644 > > --- a/src/adv_monitor.c > > +++ b/src/adv_monitor.c > > @@ -22,7 +22,9 @@ > > #endif > > > > #define _GNU_SOURCE > > +#include <errno.h> > > #include <stdint.h> > > +#include <string.h> > > > > #include <glib.h> > > #include <dbus/dbus.h> > > @@ -34,7 +36,9 @@ > > #include "adapter.h" > > #include "dbus-common.h" > > #include "log.h" > > +#include "src/error.h" > > #include "src/shared/mgmt.h" > > +#include "src/shared/queue.h" > > #include "src/shared/util.h" > > > > #include "adv_monitor.h" > > @@ -52,12 +56,170 @@ struct btd_adv_monitor_manager { > > uint16_t max_num_monitors; > > uint8_t max_num_patterns; > > > > + struct queue *apps; /* apps who registered for Adv monitoring */ > > }; > > > > +struct adv_monitor_app { > > + struct btd_adv_monitor_manager *manager; > > + char *owner; > > + char *path; > > + > > + DBusMessage *reg; > > + GDBusClient *client; > > +}; > > + > > +struct app_match_data { > > + const char *owner; > > + const char *path; > > +}; > > + > > +/* Replies to an app's D-Bus message and unref it */ > > +static void app_reply_msg(struct adv_monitor_app *app, DBusMessage *reply) > > +{ > > + if (!app || !app->reg || !reply) > > + return; > > + > > + g_dbus_send_message(btd_get_dbus_connection(), reply); > > + dbus_message_unref(app->reg); > > + app->reg = NULL; > > +} > > + > > +/* Destroys an app object along with related D-Bus handlers */ > > +static void app_destroy(void *data) > > +{ > > + struct adv_monitor_app *app = data; > > + > > + if (!app) > > + return; > > + > > + DBG("Destroy Adv Monitor app %s at path %s", app->owner, app->path); > > + > > + if (app->reg) { > > + app_reply_msg(app, btd_error_failed(app->reg, > > + "Adv Monitor app destroyed")); > > + } > > + > > + if (app->client) { > > + g_dbus_client_set_disconnect_watch(app->client, NULL, NULL); > > + g_dbus_client_set_proxy_handlers(app->client, NULL, NULL, NULL, > > + NULL); > > + g_dbus_client_set_ready_watch(app->client, NULL, NULL); > > + g_dbus_client_unref(app->client); > > + app->client = NULL; > > + } > > + > > + g_free(app->owner); > > + app->owner = NULL; > > + g_free(app->path); > > + app->path = NULL; > > Same comment as before, if the whole object would be free then there > is no need to reset each individual field. Done. > > > + > > + g_free(app); > > +} > > + > > +/* Handles a D-Bus disconnection event of an app */ > > +static void app_disconnect_cb(DBusConnection *conn, void *user_data) > > +{ > > + struct adv_monitor_app *app = user_data; > > + > > + btd_info(app->manager->adapter_id, "Adv Monitor app %s disconnected " > > + "from D-Bus", app->owner); > > + if (app && queue_remove(app->manager->apps, app)) > > + app_destroy(app); > > +} > > + > > +/* Creates an app object, initiates it and sets D-Bus event handlers */ > > +static struct adv_monitor_app *app_create(DBusConnection *conn, > > + const char *sender, const char *path, > > + struct btd_adv_monitor_manager *manager) > > +{ > > + struct adv_monitor_app *app; > > + > > + if (!path || !sender || !manager) > > + return NULL; > > + > > + app = g_new0(struct adv_monitor_app, 1); > > Please use new0 instead of g_new0 in new code. Done. > > > + if (!app) > > + return NULL; > > + > > + app->owner = g_strdup(sender); > > + app->path = g_strdup(path); > > + app->manager = manager; > > + app->reg = NULL; > > + > > + app->client = g_dbus_client_new(conn, sender, path); > > + if (!app->client) { > > + app_destroy(app); > > + return NULL; > > + } > > + > > + g_dbus_client_set_disconnect_watch(app->client, app_disconnect_cb, app); > > + g_dbus_client_set_proxy_handlers(app->client, NULL, NULL, NULL, NULL); > > + g_dbus_client_set_ready_watch(app->client, NULL, NULL); > > + > > + return app; > > +} > > + > > +/* Matches an app based on its owner and path */ > > +static bool app_match(const void *a, const void *b) > > +{ > > + const struct adv_monitor_app *app = a; > > + const struct app_match_data *match = b; > > + > > + if (match->owner && strcmp(app->owner, match->owner)) > > + return false; > > + > > + if (match->path && strcmp(app->path, match->path)) > > + return false; > > + > > + return true; > > +} > > + > > +/* Handles a RegisterMonitor D-Bus call */ > > +static DBusMessage *register_monitor(DBusConnection *conn, DBusMessage *msg, > > + void *user_data) > > +{ > > + DBusMessageIter args; > > + struct app_match_data match; > > + struct adv_monitor_app *app; > > + struct btd_adv_monitor_manager *manager = user_data; > > + > > + if (!dbus_message_iter_init(msg, &args)) > > + return btd_error_invalid_args(msg); > > + > > + if (dbus_message_iter_get_arg_type(&args) != DBUS_TYPE_OBJECT_PATH) > > + return btd_error_invalid_args(msg); > > + > > + dbus_message_iter_get_basic(&args, &match.path); > > + > > + if (!strlen(match.path) || !g_str_has_prefix(match.path, "/")) > > + return btd_error_invalid_args(msg); > > + > > + match.owner = dbus_message_get_sender(msg); > > + > > + if (queue_find(manager->apps, app_match, &match)) > > + return btd_error_already_exists(msg); > > + > > + app = app_create(conn, match.owner, match.path, manager); > > + if (!app) { > > + btd_error(manager->adapter_id, > > + "Failed to reserve %s for Adv Monitor app %s", > > + match.path, match.owner); > > + return btd_error_failed(msg, > > + "Failed to create Adv Monitor app"); > > + } > > + > > + queue_push_tail(manager->apps, app); > > + > > + btd_info(manager->adapter_id, "Path %s reserved for Adv Monitor app %s", > > + match.path, match.owner); > > + > > + return dbus_message_new_method_return(msg); > > +} > > + > > static const GDBusMethodTable adv_monitor_methods[] = { > > { GDBUS_METHOD("RegisterMonitor", > > GDBUS_ARGS({ "application", "o" }), > > - NULL, NULL) }, > > + NULL, register_monitor) }, > > { GDBUS_ASYNC_METHOD("UnregisterMonitor", > > GDBUS_ARGS({ "application", "o" }), > > NULL, NULL) }, > > @@ -157,6 +319,7 @@ static struct btd_adv_monitor_manager *manager_new( > > manager->mgmt = mgmt_ref(mgmt); > > manager->adapter_id = btd_adapter_get_index(adapter); > > manager->path = g_strdup(adapter_get_path(manager->adapter)); > > + manager->apps = queue_new(); > > > > return manager; > > } > > @@ -170,6 +333,8 @@ static void manager_free(struct btd_adv_monitor_manager *manager) > > g_free(manager->path); > > manager->path = NULL; > > > > + queue_destroy(manager->apps, app_destroy); > > + > > g_free(manager); > > } > > > > -- > > 2.26.2 > > > > > -- > Luiz Augusto von Dentz Thanks, Miao
diff --git a/src/adv_monitor.c b/src/adv_monitor.c index 4d02237e8..3d27ad18b 100644 --- a/src/adv_monitor.c +++ b/src/adv_monitor.c @@ -22,7 +22,9 @@ #endif #define _GNU_SOURCE +#include <errno.h> #include <stdint.h> +#include <string.h> #include <glib.h> #include <dbus/dbus.h> @@ -34,7 +36,9 @@ #include "adapter.h" #include "dbus-common.h" #include "log.h" +#include "src/error.h" #include "src/shared/mgmt.h" +#include "src/shared/queue.h" #include "src/shared/util.h" #include "adv_monitor.h" @@ -52,12 +56,170 @@ struct btd_adv_monitor_manager { uint16_t max_num_monitors; uint8_t max_num_patterns; + struct queue *apps; /* apps who registered for Adv monitoring */ }; +struct adv_monitor_app { + struct btd_adv_monitor_manager *manager; + char *owner; + char *path; + + DBusMessage *reg; + GDBusClient *client; +}; + +struct app_match_data { + const char *owner; + const char *path; +}; + +/* Replies to an app's D-Bus message and unref it */ +static void app_reply_msg(struct adv_monitor_app *app, DBusMessage *reply) +{ + if (!app || !app->reg || !reply) + return; + + g_dbus_send_message(btd_get_dbus_connection(), reply); + dbus_message_unref(app->reg); + app->reg = NULL; +} + +/* Destroys an app object along with related D-Bus handlers */ +static void app_destroy(void *data) +{ + struct adv_monitor_app *app = data; + + if (!app) + return; + + DBG("Destroy Adv Monitor app %s at path %s", app->owner, app->path); + + if (app->reg) { + app_reply_msg(app, btd_error_failed(app->reg, + "Adv Monitor app destroyed")); + } + + if (app->client) { + g_dbus_client_set_disconnect_watch(app->client, NULL, NULL); + g_dbus_client_set_proxy_handlers(app->client, NULL, NULL, NULL, + NULL); + g_dbus_client_set_ready_watch(app->client, NULL, NULL); + g_dbus_client_unref(app->client); + app->client = NULL; + } + + g_free(app->owner); + app->owner = NULL; + g_free(app->path); + app->path = NULL; + + g_free(app); +} + +/* Handles a D-Bus disconnection event of an app */ +static void app_disconnect_cb(DBusConnection *conn, void *user_data) +{ + struct adv_monitor_app *app = user_data; + + btd_info(app->manager->adapter_id, "Adv Monitor app %s disconnected " + "from D-Bus", app->owner); + if (app && queue_remove(app->manager->apps, app)) + app_destroy(app); +} + +/* Creates an app object, initiates it and sets D-Bus event handlers */ +static struct adv_monitor_app *app_create(DBusConnection *conn, + const char *sender, const char *path, + struct btd_adv_monitor_manager *manager) +{ + struct adv_monitor_app *app; + + if (!path || !sender || !manager) + return NULL; + + app = g_new0(struct adv_monitor_app, 1); + if (!app) + return NULL; + + app->owner = g_strdup(sender); + app->path = g_strdup(path); + app->manager = manager; + app->reg = NULL; + + app->client = g_dbus_client_new(conn, sender, path); + if (!app->client) { + app_destroy(app); + return NULL; + } + + g_dbus_client_set_disconnect_watch(app->client, app_disconnect_cb, app); + g_dbus_client_set_proxy_handlers(app->client, NULL, NULL, NULL, NULL); + g_dbus_client_set_ready_watch(app->client, NULL, NULL); + + return app; +} + +/* Matches an app based on its owner and path */ +static bool app_match(const void *a, const void *b) +{ + const struct adv_monitor_app *app = a; + const struct app_match_data *match = b; + + if (match->owner && strcmp(app->owner, match->owner)) + return false; + + if (match->path && strcmp(app->path, match->path)) + return false; + + return true; +} + +/* Handles a RegisterMonitor D-Bus call */ +static DBusMessage *register_monitor(DBusConnection *conn, DBusMessage *msg, + void *user_data) +{ + DBusMessageIter args; + struct app_match_data match; + struct adv_monitor_app *app; + struct btd_adv_monitor_manager *manager = user_data; + + if (!dbus_message_iter_init(msg, &args)) + return btd_error_invalid_args(msg); + + if (dbus_message_iter_get_arg_type(&args) != DBUS_TYPE_OBJECT_PATH) + return btd_error_invalid_args(msg); + + dbus_message_iter_get_basic(&args, &match.path); + + if (!strlen(match.path) || !g_str_has_prefix(match.path, "/")) + return btd_error_invalid_args(msg); + + match.owner = dbus_message_get_sender(msg); + + if (queue_find(manager->apps, app_match, &match)) + return btd_error_already_exists(msg); + + app = app_create(conn, match.owner, match.path, manager); + if (!app) { + btd_error(manager->adapter_id, + "Failed to reserve %s for Adv Monitor app %s", + match.path, match.owner); + return btd_error_failed(msg, + "Failed to create Adv Monitor app"); + } + + queue_push_tail(manager->apps, app); + + btd_info(manager->adapter_id, "Path %s reserved for Adv Monitor app %s", + match.path, match.owner); + + return dbus_message_new_method_return(msg); +} + static const GDBusMethodTable adv_monitor_methods[] = { { GDBUS_METHOD("RegisterMonitor", GDBUS_ARGS({ "application", "o" }), - NULL, NULL) }, + NULL, register_monitor) }, { GDBUS_ASYNC_METHOD("UnregisterMonitor", GDBUS_ARGS({ "application", "o" }), NULL, NULL) }, @@ -157,6 +319,7 @@ static struct btd_adv_monitor_manager *manager_new( manager->mgmt = mgmt_ref(mgmt); manager->adapter_id = btd_adapter_get_index(adapter); manager->path = g_strdup(adapter_get_path(manager->adapter)); + manager->apps = queue_new(); return manager; } @@ -170,6 +333,8 @@ static void manager_free(struct btd_adv_monitor_manager *manager) g_free(manager->path); manager->path = NULL; + queue_destroy(manager->apps, app_destroy); + g_free(manager); }