2016-08-28 16:30 GMT+02:00 Tanu Kaskinen <tanuk at iki.fi>:
> 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.
>
Without this patch, module-zeroconf-publish sets the Avahi property
device.icon-name but this icon does not seem to be used by
module-zeroconf-discover. On my machine, the sinks end up having
the icon-name "audio-card" and "audio-card-input" that seem to be
the default icons for 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.
>
I added the missing pa_xfree(properties)
> > @@ -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.
>
>
Removed.
> > +#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...)
>
Sounds better indead.
>
> > +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.
>
>
Fixed
> > + 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().
>
Done
>
> > + pa_log("Failed to send: %s:%s\n", error.name, 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.
>
Done
Sylvain
> --
> Tanu
> _______________________________________________
> pulseaudio-discuss mailing list
> pulseaudio-discuss at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/pulseaudio-discuss/attachments/20160831/88c50cf2/attachment.html>
