<div dir="ltr"><div class="gmail_extra"><br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div><div class="h5">> + if (imtu)<br>
> + *imtu = 48;<br>
> +<br>
> + if (omtu)<br>
> + *omtu = 48;<br>
<br>
</div></div>Out of curiosity, are these values fixed in the spec?<br></blockquote><div><br></div><div>They are not but the kernel does not want to give us the real MTU values so this is hardcoded for now.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div><div class="h5">> + if (events & PA_IO_EVENT_INPUT) {<br>
> + char buf[512];<br>
> + ssize_t len;<br>
> +<br>
> + len = read (fd, buf, 511);<br>
> + buf[len] = 0;<br>
> + pa_log("RFCOMM << %s", buf);<br>
> +<br>
> + pa_log("RFCOMM >> OK");<br>
> + len = write (fd, "\r\nOK\r\n", 5);<br>
> + if (len < 0)<br>
> + pa_log_error("RFCOMM write error: %s", pa_cstrerror(errno));<br>
<br>
</div></div>Should we go to fail if this happens?<br></blockquote><div><br></div><div>I'm ignoring this error for now, it's not critical and I expect real failures such as disconnection etc to be handled with the HANGUP and ERROR events.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div><div class="h5"> d = pa_bluetooth_discovery_get_device_by_path(b->discovery, path);<br>
> + if (d == NULL) {<br>
> + pa_log_error("Device doesnt exist for %s", path);<br>
> + goto fail;<br>
> + }<br>
> +<br>
> + if (pa_streq(handler, HSP_AG_PROFILE)) {<br>
> + p = PA_BLUETOOTH_PROFILE_HEADSET_HEAD_UNIT;<br>
> + } else<br>
> + goto refused;<br>
<br>
</div></div>Is the else possible at all?<br>
<div><div class="h5"><br></div></div></blockquote><div><br></div><div>It should not be, I'll make it assert.</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div><div class="h5">> + dbus_message_iter_recurse(&element_i, &dict_i);<br>
> +<br>
> + if (key == NULL)<br>
> + break;<br>
> + if (dbus_message_iter_get_arg_type(&dict_i) != DBUS_TYPE_STRING)<br>
> + break;<br>
> +<br>
> + dbus_message_iter_get_basic(&dict_i, &key);<br>
> +<br>
> + if (!dbus_message_iter_next(&dict_i))<br>
> + break;<br>
> +<br>
> + if (dbus_message_iter_get_arg_type(&dict_i) != DBUS_TYPE_VARIANT)<br>
> + break;<br>
> +<br>
> + pa_log_debug("key=%s", key);<br>
> +<br>
> + dbus_message_iter_next(&element_i);<br>
> + }<br>
<br>
</div></div>If we're not doing anything with the keys, maybe we don't need to<br>
parse any of this?<br></blockquote><div><br></div><div>I'll remove it. It should only contain a version number but I don't plan to do anything with that for now.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<span class=""><br>> + trfc = pa_xnew0(struct transport_rfcomm, 1);<br>
> + trfc->rfcomm_fd = fd;<br>
> + trfc->rfcomm_io = b->core->mainloop->io_new(b->core->mainloop, fd, PA_IO_EVENT_INPUT|PA_IO_EVENT_HANGUP,<br>
> + rfcomm_io_callback, t);<br>
<br>
</span>Can reading/writing on the RFCOMM fd block the mainloop for long periods?<br></blockquote><div><br></div><div>The socket flags are set G_IO_FLAG_NONBLOCK in bluez, AFAICS.</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<span class=""><br>
> + trfc->backend = b;<br>
<br>
</span>Can the backend be destroyed before the transport? Maybe we should<br>
only reference the mainloop instead of the backend here.<br></blockquote><div><br></div><div>I'll have a look if all cleanup scenarios work fine. We should probably just ref the mainloop, indeed.</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span class="">
> +static DBusMessage *profile_request_disconnection(DBusConnection *conn, DBusMessage *m, void *userdata) {<br>
> + DBusMessage *r;<br>
> +<br>
> + pa_assert_se(r = dbus_message_new_method_return(m));<br>
<br>
</span>How is the transport torn down in this case?<br></blockquote><div><br></div><div>Bluez closes the RFCOMM socket, that makes us stop. Another case is when the SCO socket is closed. We free the transport, which makes us remove the device. I'll see if I can close it myself instead.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div><div class="h5"><br>> +<br>
> + } else if (dbus_message_is_method_call(m, BLUEZ_PROFILE_INTERFACE, "Release")) {<br>
> + } else if (dbus_message_is_method_call(m, BLUEZ_PROFILE_INTERFACE, "Cancel")) {<br>
<br>
</div></div>There doesn't actually seem to be a 'Cancel' method.<br></blockquote><div><br></div><div>I will remove it. It looks like it is supposed to be part of the API here:</div><div><br></div><div><a href="http://git.kernel.org/cgit/bluetooth/bluez.git/tree/profiles/iap/main.c#n228">http://git.kernel.org/cgit/bluetooth/bluez.git/tree/profiles/iap/main.c#n228</a></div><div><br></div><div>and in some exampled, but it does not look like it is used here:</div><div><br></div><div>is <a href="http://git.kernel.org/cgit/bluetooth/bluez.git/tree/src/profile.c">http://git.kernel.org/cgit/bluetooth/bluez.git/tree/src/profile.c</a></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div><div class="h5">> + switch(profile) {<br>
> + case PA_BLUETOOTH_PROFILE_HEADSET_HEAD_UNIT:<br>
> + object_name = HSP_AG_PROFILE;<br>
> + uuid = PA_BLUETOOTH_UUID_HSP_AG;<br>
> + break;<br>
> + default:<br>
> + pa_assert_not_reached();<br>
> + break;<br>
> + }<br>
> + pa_assert_se(dbus_connection_register_object_path(pa_dbus_connection_get(b->connection),<br>
> + object_name, &vtable_profile, b));<br>
> + register_profile (b, object_name, uuid);<br>
<br>
</div></div>There doesn't seem to be a way to handle failure when registering the<br>
profile. Should we perhaps be unregistering the object path?<br>
<div><div class="h5"><br></div></div></blockquote><div>yes, will do. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div><div class="h5">> +<br>
> + profile_done(backend, PA_BLUETOOTH_PROFILE_HEADSET_HEAD_UNIT);<br>
<br>
</div></div>backend->connection needs to be unref'ed here.<br>
<span class=""><br></span></blockquote><div>Will do,</div><div> </div><div>Thanks for reviewing, will post new patches with all suggestions and fixes soon,</div><div><br></div><div>Wim</div></div></div></div>