Hi Luiz,
On Tue, Sep 8, 2020 at 10:25 AM Luiz Augusto von Dentz
<luiz.dentz@xxxxxxxxx> wrote:
>
> Hi Miao,
>
> On Tue, Aug 18, 2020 at 3:31 PM Miao-chen Chou <mcchou@xxxxxxxxxxxx> 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@xxxxxxxxxx>
> > Reviewed-by: Manish Mandlik <mmandlik@xxxxxxxxxxxx>
> > Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@xxxxxxxxxxxx>
> > ---
> >
> > 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
