<div dir="ltr"><div>I had forgotten to remove one of the unrelated change. Submitted v3 </div><div class="gmail_extra"><div class="gmail_quote"> </div><div class="gmail_quote">Sylvain</div><div class="gmail_quote"> </div><div class="gmail_quote">2016-08-28 16:30 GMT+02:00 Tanu Kaskinen <<a href="mailto:tanuk@iki.fi" target="_blank">tanuk@iki.fi</a>>: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Fri, 2016-08-26 at 18:22 +0200, Sylvain Baubeau wrote: > systemd-hostnamed provides an icon for the machine it is running on. > With this patch, module-zeroconf-publish uses this icon for the > 'icon-name' attribute in the Avahi properties. module-zeroconf-discover > passes this icon to module-tunnel using the module parameter > {sink/source}_properties. > > This allows to display a portable, desktop or phone instead of > the generic sound card icon. This also overrides the device icon when it's *not* a generic sound card icon. But I guess this is in most cases better, since the good icon names tend to be associated with ports, not sinks. > @@ -243,6 +248,7 @@ static void resolver_cb( > pa_xfree(dname); > pa_xfree(args); > pa_xfree(if_suffix); > + pa_xfree(properties); There are code paths that don't free the properties, causing memory leaks. > @@ -393,7 +399,7 @@ int pa__init(pa_module*m) { > u->avahi_poll = pa_avahi_poll_new(m->core->mainloop); > > if (!(u->client = avahi_client_new(u->avahi_poll, AVAHI_CLIENT_NO_FAIL, client_callback, u, &error))) { > - pa_log("pa_avahi_client_new() failed: %s", avahi_strerror(error)); > + pa_log("avahi_client_new() failed: %s", avahi_strerror(error)); I'd prefer separate patches for unrelated fixes. > +#define DBUS_HOSTNAME_ID "org.freedesktop.hostname1" This is used as the interface name in D-Bus messages, so DBUS_HOSTNAME_INTERFACE would be a better name. Also, I would prefer "HOSTNAME_DBUS_" as the prefix, because these strings are defined by systemd-hostnamed, not by D-Bus. (Sorry for nitpicking...) > +static char *get_icon_name(pa_module*m) { > + const char *interface = DBUS_HOSTNAME_ID; > + const char *property = DBUS_HOSTNAME_ICON_PROPERTY; > + char *icon_name; > + pa_dbus_connection *bus; > + DBusError error; > + DBusMessageIter args; > + DBusMessage *msg = NULL; > + DBusMessage *reply = NULL; > + DBusConnection *conn = NULL; > + DBusMessageIter sub; > + > + if (!(bus = pa_dbus_bus_get(m->core, DBUS_BUS_SYSTEM, &error))) { > + pa_log("Failed to get session bus connection: %s", error.message); System/session mismatch between the code and the error message. > + goto out; > + } > + > + conn = pa_dbus_connection_get(bus); > + > + msg = dbus_message_new_method_call(DBUS_HOSTNAME_ID, > + DBUS_HOSTNAME_PATH, > + "org.freedesktop.DBus.Properties", > + "Get"); > + dbus_message_append_args(msg, DBUS_TYPE_STRING, &interface, DBUS_TYPE_STRING, &property, DBUS_TYPE_INVALID); > + > + dbus_error_init(&error); > + if ((reply = dbus_connection_send_with_reply_and_block(conn, msg, -1, &error)) == NULL) { I'd rather not do blocking IPC. If you move this to the Avahi thread, that would be more tolerable (I'd rather not do even that, but this module is a lost cause anyway, because the communication with Avahi looks like it's done in a blocking manner). It looks like the AVAHI_CLIENT_S_RUNNING handler in client_callback() would be a suitable place to call get_icon_name(). > + pa_log("Failed to send: %s:%s\n", <a href="http://error.name" rel="noreferrer" target="_blank">error.name</a>, error.message); > + dbus_error_free(&error); > + goto out; > + } > + > + dbus_message_iter_init(reply, &args); > + if (dbus_message_iter_get_arg_type(&args) != DBUS_TYPE_VARIANT) { > + pa_log("Incorrect reply type\n"); > + goto out; > + } > + > + dbus_message_iter_recurse(&args, &sub); > + dbus_message_iter_get_basic(&sub, &icon_name); We should check the variant type before assuming that it's a string. -- Tanu _______________________________________________ pulseaudio-discuss mailing list <a href="mailto:pulseaudio-discuss@lists.freedesktop.org">pulseaudio-discuss@lists.freedesktop.org</a> <a href="https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss</a> </blockquote></div> </div></div>