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