From e423776000982ad5646dfae6f753226f743c36e3 Mon Sep 17 00:00:00 2001 From: haruInDisguise Date: Sun, 9 Mar 2025 22:28:59 +0100 Subject: [PATCH 1/7] module_mpris: Added 'query-timeout' option This enables us to configure the communication timeout with the dbus daemon. --- doc/yambar-modules-mpris.5.scd | 18 ++++++++++++------ modules/mpris.c | 23 ++++++++++++++++------- 2 files changed, 28 insertions(+), 13 deletions(-) diff --git a/doc/yambar-modules-mpris.5.scd b/doc/yambar-modules-mpris.5.scd index a52caba..510dc8f 100644 --- a/doc/yambar-modules-mpris.5.scd +++ b/doc/yambar-modules-mpris.5.scd @@ -55,6 +55,11 @@ mpris - This module provides MPRIS status such as currently playing artist/album : list of string : yes : A list of MPRIS client identities +| query_timeout +: int +: no +: Dbus/MPRIS client connection timeout in ms. Try setting/incrementing + this value if the module reports a timeout error. Defaults to 500. # EXAMPLES @@ -77,18 +82,19 @@ bar: # NOTE The 'identity' refers a part of your clients DBus bus name. -You can obtain a list of available bus names using: +You can obtain a list of active client names using: ``` Systemd: > busctl --user --list Playerctl: > playerctl --list-all -Libdbus: > dbus-send --session --print-reply --type=method_call --dest='org.freedesktop.DBus' /org org.freedesktop.DBus.ListNames ... | grep 'org.mpris.MediaPlayer2' +Libdbus: > dbus-send --session --print-reply --type=method_call \ + --dest='org.freedesktop.DBus' /org org.freedesktop.DBus.ListNames ``` -The identity refers to the part after 'org.mpris.MediaPlayer2'. -For example, firefox may use the bus name -'org.mpris.MediaPlayer2.firefox.instance_1_7' and its identity would be -'firefox' +MPRIS client bus names start with 'org.mpris.MediaPlayer2.'. +For example, firefox may use the bus name: +'org.mpris.MediaPlayer2.firefox.instance_1_7' which +gives us the identity 'firefox' # SEE ALSO diff --git a/modules/mpris.c b/modules/mpris.c index 538256d..c610176 100644 --- a/modules/mpris.c +++ b/modules/mpris.c @@ -14,16 +14,17 @@ #include #include "dbus.h" +#include "yml.h" #define LOG_MODULE "mpris" -#define LOG_ENABLE_DBG 1 +#define LOG_ENABLE_DBG 0 #include "../bar/bar.h" #include "../config-verify.h" #include "../config.h" #include "../log.h" #include "../plugin.h" -#define QUERY_TIMEOUT 100 +#define DEFAULT_QUERY_TIMEOUT 500 #define PATH "/org/mpris/MediaPlayer2" #define BUS_NAME "org.mpris.MediaPlayer2" @@ -99,6 +100,7 @@ struct private int refresh_abort_fd; size_t identities_count; + size_t timeout_ms; const char **identities; struct particle *label; @@ -649,10 +651,10 @@ context_new(struct private *m, struct context *context) sd_bus_message *reply = NULL; sd_bus_error error = {}; - status = sd_bus_call(NULL, message, QUERY_TIMEOUT, &error, &reply); + status = sd_bus_call(NULL, message, m->timeout_ms, &error, &reply); if (status < 0 && sd_bus_error_is_set(&error)) { - LOG_ERR("context_new: got error response with error: %s: %s (%d)", error.name, error.message, + LOG_ERR("context_new: got error response: %s: %s (%d)", error.name, error.message, sd_bus_error_get_errno(&error)); return false; } @@ -1035,7 +1037,7 @@ run(struct module *mod) break; } - if (!context_process_events(context, QUERY_TIMEOUT)) { + if (!context_process_events(context, m->timeout_ms)) { aborted = true; break; } @@ -1068,10 +1070,11 @@ description(const struct module *mod) } static struct module * -mpris_new(const char **ident, size_t ident_count, struct particle *label) +mpris_new(const char **ident, size_t ident_count, size_t timeout, struct particle *label) { struct private *priv = calloc(1, sizeof(*priv)); priv->label = label; + priv->timeout_ms = timeout; priv->identities = malloc(sizeof(*ident) * ident_count); priv->identities_count = ident_count; @@ -1093,8 +1096,13 @@ static struct module * from_conf(const struct yml_node *node, struct conf_inherit inherited) { const struct yml_node *ident_list = yml_get_value(node, "identities"); + const struct yml_node *query_timeout = yml_get_value(node, "query_timeout"); const struct yml_node *c = yml_get_value(node, "content"); + size_t timeout_ms = DEFAULT_QUERY_TIMEOUT * 1000; + if(query_timeout != NULL) + timeout_ms = yml_value_as_int(query_timeout) * 1000; + const size_t ident_count = yml_list_length(ident_list); const char *ident[ident_count]; size_t i = 0; @@ -1102,7 +1110,7 @@ from_conf(const struct yml_node *node, struct conf_inherit inherited) ident[i] = yml_value_as_string(iter.node); } - return mpris_new(ident, ident_count, conf_to_particle(c, inherited)); + return mpris_new(ident, ident_count, timeout_ms, conf_to_particle(c, inherited)); } static bool @@ -1116,6 +1124,7 @@ verify_conf(keychain_t *chain, const struct yml_node *node) { static const struct attr_info attrs[] = { {"identities", true, &conf_verify_indentities}, + {"query_timeout", false, &conf_verify_unsigned}, MODULE_COMMON_ATTRS, }; From dcf936fd9b04cb77c9125dd5be5856872ec3b405 Mon Sep 17 00:00:00 2001 From: haruInDisguise Date: Mon, 10 Mar 2025 00:32:14 +0100 Subject: [PATCH 2/7] module_mpris: Fixed 'use after free' --- modules/mpris.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/modules/mpris.c b/modules/mpris.c index c610176..b29e883 100644 --- a/modules/mpris.c +++ b/modules/mpris.c @@ -135,14 +135,17 @@ metadata_clear(struct metadata *metadata) if (metadata->album != NULL) { free(metadata->album); + metadata->album = NULL; } if (metadata->title != NULL) { free(metadata->title); + metadata->title = NULL; } if (metadata->trackid != NULL) { free(metadata->trackid); + metadata->trackid = NULL; } } @@ -296,7 +299,7 @@ metadata_parse_property(const char *property_name, sd_bus_message *message, stru goto unexpected_type; status = sd_bus_message_read(message, "v", argument_layout, &string); - if (status > 0) + if (status > 0 && strlen(string) > 0) buffer->trackid = strdup(string); /* FIXME: "strcmp matches both 'album' as well as 'albumArtist'" */ @@ -310,7 +313,7 @@ metadata_parse_property(const char *property_name, sd_bus_message *message, stru } else if (strcmp(property_name, "xesam:title") == 0) { status = sd_bus_message_read(message, "v", "s", &string); - if(status > 0) + if(status > 0 && strlen(string) > 0) buffer->title = strdup(string); } else if (strcmp(property_name, "mpris:length") == 0) { @@ -393,12 +396,12 @@ property_parse(struct property *prop, const char *property_name, sd_bus_message const char *string; if (strcmp(property_name, "PlaybackStatus") == 0) { status = sd_bus_message_read(message, "v", "s", &string); - if (status) + if (status && strlen(string) > 0) prop->playback_status = strdup(string); } else if (strcmp(property_name, "LoopStatus") == 0) { status = sd_bus_message_read(message, "v", "s", &string); - if (status) + if (status && strlen(string) > 0) prop->loop_status = strdup(string); } else if (strcmp(property_name, "Position") == 0) { From 0bcde5c453e7765deec020321e08484b0781b980 Mon Sep 17 00:00:00 2001 From: haruInDisguise Date: Mon, 10 Mar 2025 01:36:26 +0100 Subject: [PATCH 3/7] module_mpris: Fixed memory leak The identity list now uses tlllist and is deallocated properly --- modules/mpris.c | 43 +++++++++++++++++-------------------------- 1 file changed, 17 insertions(+), 26 deletions(-) diff --git a/modules/mpris.c b/modules/mpris.c index b29e883..74d2e95 100644 --- a/modules/mpris.c +++ b/modules/mpris.c @@ -83,11 +83,6 @@ struct context { sd_bus *monitor_connection; sd_bus_message *update_message; - /* FIXME: There is no nice way to pass the desired identities to - * the event handler for validation. */ - char **identities_ref; - size_t identities_count; - tll(struct client *) clients; struct client *current_client; @@ -99,14 +94,14 @@ struct private thrd_t refresh_thread_id; int refresh_abort_fd; - size_t identities_count; size_t timeout_ms; - const char **identities; struct particle *label; struct context context; }; +static tll(const char*) identity_list; + #if 0 static void debug_print_argument_type(sd_bus_message *message) @@ -225,10 +220,10 @@ client_change_unique_name(struct client *client, const char *new_name) } static bool -verify_bus_name(char **idents, const size_t ident_count, const char *name) +verify_bus_name(const char *name) { - for (size_t i = 0; i < ident_count; i++) { - const char *ident = idents[i]; + tll_foreach(identity_list, it) { + const char *ident = it->item; if (strlen(name) < strlen(BUS_NAME ".") + strlen(ident)) { continue; @@ -455,9 +450,13 @@ destroy(struct module *mod) sd_bus_close(context->monitor_connection); - module_default_destroy(mod); + tll_foreach(identity_list, it) { + free((char*)it->item); + } m->label->destroy(m->label); free(m); + + module_default_destroy(mod); } static void @@ -524,7 +523,7 @@ context_event_handle_name_acquired(sd_bus_message *message, struct context *cont return; } - if (verify_bus_name(context->identities_ref, context->identities_count, name)) { + if (verify_bus_name(name)) { const char *unique_name = sd_bus_message_get_destination(message); LOG_DBG("'NameAcquired': Acquired new client: %s unique: %s", name, unique_name); client_add(context, name, unique_name); @@ -667,8 +666,6 @@ context_new(struct private *m, struct context *context) (*context) = (struct context){ .monitor_connection = connection, - .identities_ref = (char **)m->identities, - .identities_count = m->identities_count, .clients = tll_init(), }; @@ -1061,8 +1058,8 @@ run(struct module *mod) bar->refresh(bar); } - LOG_DBG("exiting"); + LOG_DBG("exiting"); return ret; } @@ -1073,17 +1070,11 @@ description(const struct module *mod) } static struct module * -mpris_new(const char **ident, size_t ident_count, size_t timeout, struct particle *label) +mpris_new(size_t timeout, struct particle *label) { struct private *priv = calloc(1, sizeof(*priv)); priv->label = label; priv->timeout_ms = timeout; - priv->identities = malloc(sizeof(*ident) * ident_count); - priv->identities_count = ident_count; - - for (size_t i = 0; i < ident_count; i++) { - priv->identities[i] = strdup(ident[i]); - } struct module *mod = module_common_new(); mod->private = priv; @@ -1106,14 +1097,14 @@ from_conf(const struct yml_node *node, struct conf_inherit inherited) if(query_timeout != NULL) timeout_ms = yml_value_as_int(query_timeout) * 1000; - const size_t ident_count = yml_list_length(ident_list); - const char *ident[ident_count]; + // FIXME: This is a redundant copy size_t i = 0; for (struct yml_list_iter iter = yml_list_iter(ident_list); iter.node != NULL; yml_list_next(&iter), i++) { - ident[i] = yml_value_as_string(iter.node); + const char *string = strdup(yml_value_as_string(iter.node)); + tll_push_back(identity_list, string); } - return mpris_new(ident, ident_count, timeout_ms, conf_to_particle(c, inherited)); + return mpris_new(timeout_ms, conf_to_particle(c, inherited)); } static bool From 6a97b364a01b0bd4af54fa0c0e434acfc2ef704f Mon Sep 17 00:00:00 2001 From: haruInDisguise Date: Mon, 10 Mar 2025 11:13:17 +0100 Subject: [PATCH 4/7] module_mpris: Fixed inconsistent string validation checks This addresses changes requested by @mathstuf --- modules/mpris.c | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/modules/mpris.c b/modules/mpris.c index 74d2e95..31b8fd2 100644 --- a/modules/mpris.c +++ b/modules/mpris.c @@ -24,8 +24,9 @@ #include "../log.h" #include "../plugin.h" -#define DEFAULT_QUERY_TIMEOUT 500 +#define is_empty_string(str) ((str) == NULL || (str)[0] == '\0') +#define DEFAULT_QUERY_TIMEOUT 500 #define PATH "/org/mpris/MediaPlayer2" #define BUS_NAME "org.mpris.MediaPlayer2" #define SERVICE "org.mpris.MediaPlayer2" @@ -259,7 +260,7 @@ read_string_array(sd_bus_message *message, string_array *list) const char *string; while ((status = sd_bus_message_read_basic(message, SD_BUS_TYPE_STRING, &string)) > 0) { - if (strlen(string) > 0) { + if (!is_empty_string(string)) { tll_push_back(*list, strdup(string)); } } @@ -287,20 +288,20 @@ metadata_parse_property(const char *property_name, sd_bus_message *message, stru const char *argument_layout = NULL; sd_bus_message_peek_type(message, &argument_type, &argument_layout); assert(argument_type == SD_BUS_TYPE_VARIANT); - assert(argument_layout != NULL && strlen(argument_layout) > 0); + assert(!is_empty_string(argument_layout)); if (strcmp(property_name, "mpris:trackid") == 0) { if (argument_layout[0] != SD_BUS_TYPE_STRING && argument_layout[0] != SD_BUS_TYPE_OBJECT_PATH) goto unexpected_type; status = sd_bus_message_read(message, "v", argument_layout, &string); - if (status > 0 && strlen(string) > 0) + if (status > 0 && !is_empty_string(string)) buffer->trackid = strdup(string); /* FIXME: "strcmp matches both 'album' as well as 'albumArtist'" */ } else if (strcmp(property_name, "xesam:album") == 0) { status = sd_bus_message_read(message, "v", argument_layout, &string); - if (status > 0 && strlen(string) > 0) + if (status > 0 && !is_empty_string(string)) buffer->album = strdup(string); } else if (strcmp(property_name, "xesam:artist") == 0) { @@ -308,7 +309,7 @@ metadata_parse_property(const char *property_name, sd_bus_message *message, stru } else if (strcmp(property_name, "xesam:title") == 0) { status = sd_bus_message_read(message, "v", "s", &string); - if(status > 0 && strlen(string) > 0) + if(status > 0 && !is_empty_string(string)) buffer->title = strdup(string); } else if (strcmp(property_name, "mpris:length") == 0) { @@ -386,17 +387,17 @@ property_parse(struct property *prop, const char *property_name, sd_bus_message assert(status > 0); assert(argument_type == SD_BUS_TYPE_VARIANT); - assert(argument_layout != NULL && strlen(argument_layout) > 0); + assert(!is_empty_string(argument_layout)); const char *string; if (strcmp(property_name, "PlaybackStatus") == 0) { status = sd_bus_message_read(message, "v", "s", &string); - if (status && strlen(string) > 0) + if (status && !is_empty_string(string)) prop->playback_status = strdup(string); } else if (strcmp(property_name, "LoopStatus") == 0) { status = sd_bus_message_read(message, "v", "s", &string); - if (status && strlen(string) > 0) + if (status && !is_empty_string(string)) prop->loop_status = strdup(string); } else if (strcmp(property_name, "Position") == 0) { @@ -476,7 +477,7 @@ context_event_handle_name_owner_changed(sd_bus_message *message, struct context new_owner); #endif - if (strlen(new_owner) == 0 && strlen(old_owner) > 0) { + if (is_empty_string(new_owner) && !is_empty_string(old_owner)) { /* Target bus has been lost */ struct client *client = client_lookup_by_unique_name(context, old_owner); @@ -490,14 +491,14 @@ context_event_handle_name_owner_changed(sd_bus_message *message, struct context context->current_client = NULL; return; - } else if (strlen(old_owner) == 0 && strlen(new_owner) > 0) { + } else if (is_empty_string(old_owner) && !is_empty_string(new_owner)) { /* New unique name registered. Not used */ return; } /* Name changed */ - assert(new_owner != NULL && strlen(new_owner) > 0); - assert(old_owner != NULL && strlen(old_owner) > 0); + assert(!is_empty_string(new_owner)); + assert(!is_empty_string(old_owner)); struct client *client = client_lookup_by_unique_name(context, old_owner); LOG_DBG("'NameOwnerChanged': Name changed from '%s' to '%s' for client '%s'", old_owner, new_owner, From dcbb0f88ae9c3f4a9b429c0c69e183551c365559 Mon Sep 17 00:00:00 2001 From: haruInDisguise Date: Mon, 10 Mar 2025 11:34:29 +0100 Subject: [PATCH 5/7] module_mpris: Cleanup Fixed inconsistent variable naming/debug logging --- modules/mpris.c | 78 ++++++++++++++++++++----------------------------- 1 file changed, 31 insertions(+), 47 deletions(-) diff --git a/modules/mpris.c b/modules/mpris.c index 31b8fd2..79f1980 100644 --- a/modules/mpris.c +++ b/modules/mpris.c @@ -13,9 +13,6 @@ #include -#include "dbus.h" -#include "yml.h" - #define LOG_MODULE "mpris" #define LOG_ENABLE_DBG 0 #include "../bar/bar.h" @@ -24,14 +21,17 @@ #include "../log.h" #include "../plugin.h" +#include "dbus.h" +#include "yml.h" + #define is_empty_string(str) ((str) == NULL || (str)[0] == '\0') -#define DEFAULT_QUERY_TIMEOUT 500 -#define PATH "/org/mpris/MediaPlayer2" -#define BUS_NAME "org.mpris.MediaPlayer2" -#define SERVICE "org.mpris.MediaPlayer2" -#define INTERFACE_ROOT "org.mpris.MediaPlayer2" -#define INTERFACE_PLAYER INTERFACE_ROOT ".Player" +#define DEFAULT_QUERY_TIMEOUT_MS (500 * 1000) + +#define MPRIS_PATH "/org/mpris/MediaPlayer2" +#define MPRIS_BUS_NAME "org.mpris.MediaPlayer2" +#define MPRIS_SERVICE "org.mpris.MediaPlayer2" +#define MPRIS_INTERFACE_PLAYER "org.mpris.MediaPlayer2.Player" #define DBUS_PATH "/org/freedesktop/DBus" #define DBUS_BUS_NAME "org.freedesktop.DBus" @@ -101,29 +101,19 @@ struct private struct context context; }; -static tll(const char*) identity_list; +static tll(const char *) identity_list; -#if 0 -static void +#if defined(LOG_ENABLE_DBG) && LOG_ENABLE_DBG +static void __attribute__((unused)) debug_print_argument_type(sd_bus_message *message) { char type; const char *content; sd_bus_message_peek_type(message, &type, &content); - LOG_DBG("peek_message_type: %c -> %s", type, content); + LOG_DBG("argument type: %c -> %s", type, content); } #endif -#if defined(LOG_ENABLE_DBG) -#define dump_type(message) \ - { \ - char type; \ - const char *content; \ - sd_bus_message_peek_type(message, &type, &content); \ - LOG_DBG("argument layout: %c -> %s", type, content); \ - } -#endif - static void metadata_clear(struct metadata *metadata) { @@ -223,14 +213,15 @@ client_change_unique_name(struct client *client, const char *new_name) static bool verify_bus_name(const char *name) { - tll_foreach(identity_list, it) { + tll_foreach(identity_list, it) + { const char *ident = it->item; - if (strlen(name) < strlen(BUS_NAME ".") + strlen(ident)) { + if (strlen(name) < strlen(MPRIS_BUS_NAME ".") + strlen(ident)) { continue; } - const char *cmp = name + strlen(BUS_NAME "."); + const char *cmp = name + strlen(MPRIS_BUS_NAME "."); if (strncmp(cmp, ident, strlen(ident)) != 0) { continue; } @@ -309,7 +300,7 @@ metadata_parse_property(const char *property_name, sd_bus_message *message, stru } else if (strcmp(property_name, "xesam:title") == 0) { status = sd_bus_message_read(message, "v", "s", &string); - if(status > 0 && !is_empty_string(string)) + if (status > 0 && !is_empty_string(string)) buffer->title = strdup(string); } else if (strcmp(property_name, "mpris:length") == 0) { @@ -451,9 +442,7 @@ destroy(struct module *mod) sd_bus_close(context->monitor_connection); - tll_foreach(identity_list, it) { - free((char*)it->item); - } + tll_foreach(identity_list, it) { free((char *)it->item); } m->label->destroy(m->label); free(m); @@ -468,14 +457,11 @@ context_event_handle_name_owner_changed(sd_bus_message *message, struct context * it was acquired, lost or changed */ const char *bus_name = NULL, *old_owner = NULL, *new_owner = NULL; - int status __attribute__((unused)) - = sd_bus_message_read(message, "sss", &bus_name, &old_owner, &new_owner); + int status __attribute__((unused)) = sd_bus_message_read(message, "sss", &bus_name, &old_owner, &new_owner); assert(status > 0); -#if 1 LOG_DBG("event_handler: 'NameOwnerChanged': bus_name: '%s' old_owner: '%s' new_ower: '%s'", bus_name, old_owner, new_owner); -#endif if (is_empty_string(new_owner) && !is_empty_string(old_owner)) { /* Target bus has been lost */ @@ -514,13 +500,12 @@ context_event_handle_name_acquired(sd_bus_message *message, struct context *cont /* NameAcquired (STRING name) */ /* " This signal is sent to a specific application when it gains ownership of a name. " */ const char *name = NULL; - int status __attribute__((unused)) - = sd_bus_message_read_basic(message, SD_BUS_TYPE_STRING, &name); + int status __attribute__((unused)) = sd_bus_message_read_basic(message, SD_BUS_TYPE_STRING, &name); assert(status > 0); - /*LOG_DBG("event_handler: 'NameAcquired': name: '%s'", name);*/ + LOG_DBG("event_handler: 'NameAcquired': name: '%s'", name); - if (strncmp(name, BUS_NAME, strlen(BUS_NAME)) != 0) { + if (strncmp(name, MPRIS_BUS_NAME, strlen(MPRIS_BUS_NAME)) != 0) { return; } @@ -563,7 +548,8 @@ context_event_handler(sd_bus_message *message, void *userdata, sd_bus_error *ret /* Copy the 'PropertiesChanged/Seeked' message, so it can be parsed * later on */ - if (strcmp(path_name, PATH) == 0 && (strcmp(member, "PropertiesChanged") == 0 || strcmp(member, "Seeked") == 0)) { + if (strcmp(path_name, MPRIS_PATH) == 0 + && (strcmp(member, "PropertiesChanged") == 0 || strcmp(member, "Seeked") == 0)) { struct client *client = client_lookup_by_unique_name(context, sender); if (client == NULL) return 1; @@ -717,7 +703,7 @@ update_status_from_message(struct module *mod, sd_bus_message *message) const char *interface_name = NULL; sd_bus_message_read_basic(message, SD_BUS_TYPE_STRING, &interface_name); - if (strcmp(interface_name, INTERFACE_PLAYER) != 0) { + if (strcmp(interface_name, MPRIS_INTERFACE_PLAYER) != 0) { LOG_DBG("Ignoring interface: %s", interface_name); mtx_unlock(&mod->lock); return true; @@ -732,8 +718,7 @@ update_status_from_message(struct module *mod, sd_bus_message *message) while ((has_entries = sd_bus_message_enter_container(message, SD_BUS_TYPE_DICT_ENTRY, "sv")) > 0) { const char *property_name = NULL; - int status __attribute__((unused)) - = sd_bus_message_read_basic(message, SD_BUS_TYPE_STRING, &property_name); + int status __attribute__((unused)) = sd_bus_message_read_basic(message, SD_BUS_TYPE_STRING, &property_name); assert(status > 0); if (!property_parse(&client->property, property_name, message)) { @@ -1059,7 +1044,6 @@ run(struct module *mod) bar->refresh(bar); } - LOG_DBG("exiting"); return ret; } @@ -1071,11 +1055,11 @@ description(const struct module *mod) } static struct module * -mpris_new(size_t timeout, struct particle *label) +mpris_new(size_t timeout_ms, struct particle *label) { struct private *priv = calloc(1, sizeof(*priv)); priv->label = label; - priv->timeout_ms = timeout; + priv->timeout_ms = timeout_ms; struct module *mod = module_common_new(); mod->private = priv; @@ -1094,8 +1078,8 @@ from_conf(const struct yml_node *node, struct conf_inherit inherited) const struct yml_node *query_timeout = yml_get_value(node, "query_timeout"); const struct yml_node *c = yml_get_value(node, "content"); - size_t timeout_ms = DEFAULT_QUERY_TIMEOUT * 1000; - if(query_timeout != NULL) + size_t timeout_ms = DEFAULT_QUERY_TIMEOUT_MS; + if (query_timeout != NULL) timeout_ms = yml_value_as_int(query_timeout) * 1000; // FIXME: This is a redundant copy From dfa0970b75297cf2047d6cf45af98cf093c07e6a Mon Sep 17 00:00:00 2001 From: haruInDisguise Date: Mon, 10 Mar 2025 12:03:17 +0100 Subject: [PATCH 6/7] module_mpris: Fixed multi threading issues regarding 'identity_list' This addresses changes requested by @dnkl --- modules/mpris.c | 38 ++++++++++++++++---------------------- 1 file changed, 16 insertions(+), 22 deletions(-) diff --git a/modules/mpris.c b/modules/mpris.c index 79f1980..6a03e8c 100644 --- a/modules/mpris.c +++ b/modules/mpris.c @@ -86,6 +86,7 @@ struct context { tll(struct client *) clients; struct client *current_client; + const string_array *identity_list_ref; bool has_update; }; @@ -96,13 +97,12 @@ struct private int refresh_abort_fd; size_t timeout_ms; + string_array identity_list; struct particle *label; struct context context; }; -static tll(const char *) identity_list; - #if defined(LOG_ENABLE_DBG) && LOG_ENABLE_DBG static void __attribute__((unused)) debug_print_argument_type(sd_bus_message *message) @@ -166,12 +166,6 @@ clients_free_by_unique_name(struct context *context, const char *unique_name) } } -static void -client_free_all(struct context *context) -{ - tll_free_and_free(context->clients, client_free); -} - static void client_add(struct context *context, const char *name, const char *unique_name) { @@ -211,9 +205,9 @@ client_change_unique_name(struct client *client, const char *new_name) } static bool -verify_bus_name(const char *name) +verify_bus_name(const string_array *identity_list, const char *name) { - tll_foreach(identity_list, it) + tll_foreach(*identity_list, it) { const char *ident = it->item; @@ -438,11 +432,10 @@ destroy(struct module *mod) struct private *m = mod->private; struct context *context = &m->context; - client_free_all(context); - + tll_free_and_free(context->clients, client_free); sd_bus_close(context->monitor_connection); - tll_foreach(identity_list, it) { free((char *)it->item); } + tll_free_and_free(m->identity_list, free); m->label->destroy(m->label); free(m); @@ -509,7 +502,7 @@ context_event_handle_name_acquired(sd_bus_message *message, struct context *cont return; } - if (verify_bus_name(name)) { + if (verify_bus_name(context->identity_list_ref, name)) { const char *unique_name = sd_bus_message_get_destination(message); LOG_DBG("'NameAcquired': Acquired new client: %s unique: %s", name, unique_name); client_add(context, name, unique_name); @@ -652,6 +645,7 @@ context_new(struct private *m, struct context *context) sd_bus_message_unref(reply); (*context) = (struct context){ + .identity_list_ref = &m->identity_list, .monitor_connection = connection, .clients = tll_init(), }; @@ -1055,12 +1049,18 @@ description(const struct module *mod) } static struct module * -mpris_new(size_t timeout_ms, struct particle *label) +mpris_new(const struct yml_node *ident_list, size_t timeout_ms, struct particle *label) { struct private *priv = calloc(1, sizeof(*priv)); priv->label = label; priv->timeout_ms = timeout_ms; + size_t i = 0; + for (struct yml_list_iter iter = yml_list_iter(ident_list); iter.node != NULL; yml_list_next(&iter), i++) { + char *string = strdup(yml_value_as_string(iter.node)); + tll_push_back(priv->identity_list, string); + } + struct module *mod = module_common_new(); mod->private = priv; mod->run = &run; @@ -1082,14 +1082,8 @@ from_conf(const struct yml_node *node, struct conf_inherit inherited) if (query_timeout != NULL) timeout_ms = yml_value_as_int(query_timeout) * 1000; - // FIXME: This is a redundant copy - size_t i = 0; - for (struct yml_list_iter iter = yml_list_iter(ident_list); iter.node != NULL; yml_list_next(&iter), i++) { - const char *string = strdup(yml_value_as_string(iter.node)); - tll_push_back(identity_list, string); - } - return mpris_new(timeout_ms, conf_to_particle(c, inherited)); + return mpris_new(ident_list, timeout_ms, conf_to_particle(c, inherited)); } static bool From ca0f565237656dd5bf3af8b5e2d429357545b614 Mon Sep 17 00:00:00 2001 From: haruInDisguise Date: Tue, 18 Mar 2025 12:05:02 +0100 Subject: [PATCH 7/7] module_mpris: Refactoring + Fixed mutex usage - Addressed inconsistens variable naming and removed redundant code. - Mutex locks are now used correctly. - The context struct now references the modules config, making config access less awkward --- modules/mpris.c | 60 +++++++++++++++++++------------------------------ 1 file changed, 23 insertions(+), 37 deletions(-) diff --git a/modules/mpris.c b/modules/mpris.c index 6a03e8c..5ddf6e0 100644 --- a/modules/mpris.c +++ b/modules/mpris.c @@ -81,12 +81,13 @@ struct client { }; struct context { + const struct private *mpd_config; + sd_bus *monitor_connection; sd_bus_message *update_message; tll(struct client *) clients; struct client *current_client; - const string_array *identity_list_ref; bool has_update; }; @@ -98,9 +99,8 @@ struct private size_t timeout_ms; string_array identity_list; - struct particle *label; - struct context context; + struct particle *label; }; #if defined(LOG_ENABLE_DBG) && LOG_ENABLE_DBG @@ -135,31 +135,23 @@ metadata_clear(struct metadata *metadata) } } -static void -property_clear(struct property *property) -{ - metadata_clear(&property->metadata); - memset(property, 0, sizeof(*property)); -} - static void client_free(struct client *client) { - property_clear(&client->property); - free((void *)client->bus_name); free((void *)client->bus_unique_name); free(client); } static void -clients_free_by_unique_name(struct context *context, const char *unique_name) +client_free_by_unique_name(struct context *context, const char *unique_name) { tll_foreach(context->clients, it) { struct client *client = it->item; if (strcmp(client->bus_unique_name, unique_name) == 0) { LOG_DBG("client_remove: Removing client %s", client->bus_name); + client_free(client); tll_remove(context->clients, it); } @@ -430,10 +422,9 @@ static void destroy(struct module *mod) { struct private *m = mod->private; - struct context *context = &m->context; - tll_free_and_free(context->clients, client_free); - sd_bus_close(context->monitor_connection); + tll_free_and_free(m->context.clients, client_free); + sd_bus_close(m->context.monitor_connection); tll_free_and_free(m->identity_list, free); m->label->destroy(m->label); @@ -464,7 +455,7 @@ context_event_handle_name_owner_changed(sd_bus_message *message, struct context return; LOG_DBG("event_handler: 'NameOwnerChanged': Target bus disappeared: %s", client->bus_name); - clients_free_by_unique_name(context, client->bus_unique_name); + client_free_by_unique_name(context, client->bus_unique_name); if (context->current_client == client) context->current_client = NULL; @@ -502,7 +493,7 @@ context_event_handle_name_acquired(sd_bus_message *message, struct context *cont return; } - if (verify_bus_name(context->identity_list_ref, name)) { + if (verify_bus_name(&context->mpd_config->identity_list, name)) { const char *unique_name = sd_bus_message_get_destination(message); LOG_DBG("'NameAcquired': Acquired new client: %s unique: %s", name, unique_name); client_add(context, name, unique_name); @@ -591,15 +582,17 @@ context_process_events(struct context *context, uint32_t timeout_ms) } static bool -context_new(struct private *m, struct context *context) +context_setup(struct context *context) { int status = true; sd_bus *connection; if ((status = sd_bus_default_user(&connection)) < 0) { LOG_ERR("Failed to connect to the desktop bus. errno: %d", status); - return -1; + return false; } + context->monitor_connection = connection; + /* Turn this connection into a monitor */ sd_bus_message *message; status = sd_bus_message_new_method_call(connection, &message, DBUS_SERVICE, DBUS_PATH, DBUS_INTERFACE_MONITORING, @@ -633,23 +626,16 @@ context_new(struct private *m, struct context *context) sd_bus_message *reply = NULL; sd_bus_error error = {}; - status = sd_bus_call(NULL, message, m->timeout_ms, &error, &reply); + status = sd_bus_call(NULL, message, context->mpd_config->timeout_ms, &error, &reply); if (status < 0 && sd_bus_error_is_set(&error)) { - LOG_ERR("context_new: got error response: %s: %s (%d)", error.name, error.message, - sd_bus_error_get_errno(&error)); + LOG_ERR("context_setup: got error: %s: %s (%d)", error.name, error.message, sd_bus_error_get_errno(&error)); return false; } sd_bus_message_unref(message); sd_bus_message_unref(reply); - (*context) = (struct context){ - .identity_list_ref = &m->identity_list, - .monitor_connection = connection, - .clients = tll_init(), - }; - sd_bus_add_filter(connection, NULL, context_event_handler, context); return status >= 0; @@ -765,6 +751,7 @@ static struct exposable * content_empty(struct module *mod) { struct private *m = mod->private; + mtx_lock(&mod->lock); struct tag_set tags = { .tags = (struct tag *[]){ @@ -784,11 +771,10 @@ content_empty(struct module *mod) .count = 10, }; + struct exposable *exposable = m->label->instantiate(m->label, &tags); + tag_set_destroy(&tags); mtx_unlock(&mod->lock); - struct exposable *exposable = m->label->instantiate(m->label, &tags); - - tag_set_destroy(&tags); return exposable; } @@ -862,6 +848,7 @@ content(struct module *mod) const enum tag_realtime_unit realtime_unit = (client->has_seeked_support && client->status == STATUS_PLAYING) ? TAG_REALTIME_MSECS : TAG_REALTIME_NONE; + mtx_lock(&mod->lock); struct tag_set tags = { .tags = (struct tag *[]){ tag_new_bool(mod, "has_seeked_support", client->has_seeked_support), @@ -880,11 +867,10 @@ content(struct module *mod) .count = 11, }; + struct exposable *exposable = m->label->instantiate(m->label, &tags); + tag_set_destroy(&tags); mtx_unlock(&mod->lock); - struct exposable *exposable = m->label->instantiate(m->label, &tags); - - tag_set_destroy(&tags); return exposable; } @@ -990,7 +976,7 @@ run(struct module *mod) const struct bar *bar = mod->bar; struct private *m = mod->private; - if (!context_new(m, &m->context)) { + if (!context_setup(&m->context)) { LOG_ERR("Failed to setup context"); return -1; } @@ -1054,6 +1040,7 @@ mpris_new(const struct yml_node *ident_list, size_t timeout_ms, struct particle struct private *priv = calloc(1, sizeof(*priv)); priv->label = label; priv->timeout_ms = timeout_ms; + priv->context.mpd_config = priv; size_t i = 0; for (struct yml_list_iter iter = yml_list_iter(ident_list); iter.node != NULL; yml_list_next(&iter), i++) { @@ -1082,7 +1069,6 @@ from_conf(const struct yml_node *node, struct conf_inherit inherited) if (query_timeout != NULL) timeout_ms = yml_value_as_int(query_timeout) * 1000; - return mpris_new(ident_list, timeout_ms, conf_to_particle(c, inherited)); }