Re: [PATCH BlueZ 2/2] src/profile.c: Add a GetProfileInfo method

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Mark,

On Mon, Aug 24, 2020 at 1:53 AM Mark Marshall
<mark.marshall@xxxxxxxxxxxxxxxxx> wrote:
>
> On Thu, 2020-08-20 at 09:33 -0700, Luiz Augusto von Dentz wrote:
> > Hi Mark,
> >
> > On Thu, Aug 20, 2020 at 3:10 AM Mark Marshall
> > <mark.marshall@xxxxxxxxxxxxxxxxx> wrote:
> > >
> > > On Wed, 2020-08-19 at 11:37 -0700, Luiz Augusto von Dentz wrote:
> > > > Hi Mark,
> > > >
> > > > On Wed, Aug 19, 2020 at 8:13 AM Mark Marshall
> > > > <mark.marshall@xxxxxxxxxxxxxxxxx> wrote:
> > > > >
> > > > > Add a GetProfileInfo method to org.bluez.ProfileManager1
> > > > > ---
> > > > >  doc/profile-api.txt | 13 +++++++
> > > > >  src/profile.c       | 93 +++++++++++++++++++++++++++++++++++++++++++++
> > > > >  2 files changed, 106 insertions(+)
> > > > >
> > > > > diff --git a/doc/profile-api.txt b/doc/profile-api.txt
> > > > > index 8c7d0a06d..d13703ab4 100644
> > > > > --- a/doc/profile-api.txt
> > > > > +++ b/doc/profile-api.txt
> > > > > @@ -133,6 +133,19 @@ Object path        /org/bluez
> > > > >
> > > > >                         Possible errors: org.bluez.Error.DoesNotExist
> > > > >
> > > > > +               options GetProfileInfo(object profile, object adapter)
> > > > > +
> > > > > +                       This returns a dictionary of options for the
> > > > > +                       profile.  Values returned are: UUID, Name,
> > > > > +                       Path, Service, Mode and AddressType.  The
> > > > > +                       adapter parameter is optional - if it is
> > > > > +                       non-empty, then two additional values might be
> > > > > +                       returned, if the profile is active on the
> > > > > +                       specified adapter: PSM and Channel.
> > > > > +
> > > > > +                       Possible errors: org.bluez.Error.InvalidArguments
> > > > > +                                        org.bluez.Error.DoesNotExist
> > > > > +
> > > >
> > > > If this is really required I would be willing to merge something like this:
> > > >
> > > > https://github.com/Vudentz/BlueZ/commit/9e196f8830511a4102e990d82d06c2e0487b3ad9
> > > >
> > > > It exposes service objects so you can control exactly what gets
> > > > connect, though now given a second look at this seem to return details
> > > > that the client can query directly on the socket itself like the
> > > > Channel, PSM, Mode, etc, also not sure what is the point on retrieving
> > > > things like UUID, Name, Path if the application is already in control
> > > > of these when registering.
> > > >
> > >
> > > I only returned a full dictionary of items here as I was trying to
> > > match RegisterProfile, I don't actually need all of this info.  (I
> > > also thought that the more verbose structure was more useful, but
> > > it is wasteful).
> > >
> > > The information that I really need is the PSM (or Channel) number.
> > > This information is needed on the server side, and the socket used
> > > is not exposed to anything outside of bluetoothd, as far as I can
> > > tell.  (This is the socket returned from bt_io_listen, in profile.c).
> >
> > The file descriptor passed on NewConnection is the same socket the
> > daemon uses so you can query things like PSM, etc, using getsockopt
> > just like the daemon does, or do you need it before a connection is
> > made? In which case it might be preferable to write the PSM back to
> > the Property.Set method, that said perhaps it is better to have a PSM
> > set instead of leaving that to be auto allocated since you may not
> > want to change if you need to expose over GATT.
>
> The problem is that NewConnection is too late.  This only gets called
> after the remote side has connected.  The reason that I don't think it
> can be a property, is that the PSM is technically a "per adapter"
> number, it's possible (I think) for BlueZ to auto-allocate a
> different PSM for each adapter.

Right, though I think we would allocate the same PSM and bind to BDADDR_ANY.

> If I do choose a fixed PSM, are there chances of a collision?
> Would such a collision matter?  I have "complete" control over all
> software on the server, but the client devices are not mine.
> What is the best advice about picking a fixed PSM?

If you have full control then assigning a fixed PSM is probably the
best thing you can do here as you can guarantee no one else will be
attempting to register a service for the same PSM.

> >
> > > I have my profile auto-select a free PSM, which I thought was the
> > > preferred method.  In the BR/EDR case, this number gets passed to the
> > > peer (I assume through SDP), but in the LE case, where I want to use
> > > LE-L2CAP, there is no defined mechanism to transfer the PSM.  My plan
> > > was to expose it as a GATT attribute, but this mechanism is not
> > > standardised, so I didn't think BlueZ would want to be involved?
> >
> > BlueZ don't need to get involved directly but you can have vendor
> > specific GATT service that expose the PSM so the remote side can learn
> > about it and connect.
> >
> > > Would a simpler interface that just returned the PSM or Channel number
> > > be better?  Is there another way to find out the PSM of a listening
> > > socket, on the server, before a connection is made?
> >
> > For auto allocation I would make actually make the properties
> > writable, though it seems we don't have any properties on Profile1 so
> > we would need to create then in order to reflect the options that can
> > be passed over on RegisterProfile.
> >
> > > (From my reading of the above patch, the service object is created
> > > as a link between a "profile" and a "device".  In my case, I think
> > > there is no service object yet, as I have no device connection?)
> >
> >
> >
> > >
> > > > >  Profile hierarchy
> > > > >  =================
> > > > > diff --git a/src/profile.c b/src/profile.c
> > > > > index 10850f305..e287a66d7 100644
> > > > > --- a/src/profile.c
> > > > > +++ b/src/profile.c
> > > > > @@ -2509,6 +2509,96 @@ static DBusMessage *unregister_profile(DBusConnection *conn,
> > > > >         return dbus_message_new_method_return(msg);
> > > > >  }
> > > > >
> > > > > +static DBusMessage *get_profile_info(DBusConnection *conn,
> > > > > +                                       DBusMessage *msg, void *user_data)
> > > > > +{
> > > > > +       DBusMessage *reply;
> > > > > +       DBusMessageIter iter, dict;
> > > > > +       const char *path, *adapter, *sender;
> > > > > +       struct ext_profile *ext;
> > > > > +       uint16_t u16;
> > > > > +       GSList *l, *next;
> > > > > +
> > > > > +       sender = dbus_message_get_sender(msg);
> > > > > +
> > > > > +       DBG("sender %s", sender);
> > > > > +
> > > > > +       if (!dbus_message_get_args(msg, NULL, DBUS_TYPE_OBJECT_PATH, &path,
> > > > > +                                  DBUS_TYPE_OBJECT_PATH, &adapter,
> > > > > +                                  DBUS_TYPE_INVALID)) {
> > > > > +               return btd_error_invalid_args(msg);
> > > > > +       }
> > > > > +
> > > > > +       if (adapter && !*adapter)
> > > > > +               adapter = NULL;
> > > > > +
> > > > > +       ext = find_ext_profile(sender, path);
> > > > > +       if (!ext)
> > > > > +               return btd_error_does_not_exist(msg);
> > > > > +
> > > > > +       reply = dbus_message_new_method_return(msg);
> > > > > +
> > > > > +       dbus_message_iter_init_append(reply, &iter);
> > > > > +
> > > > > +       dbus_message_iter_open_container(&iter, DBUS_TYPE_ARRAY,
> > > > > +                                        "{sv}", &dict);
> > > > > +
> > > > > +       g_dbus_dict_append_entry(&dict, "UUID", DBUS_TYPE_STRING,
> > > > > +                                &ext->uuid);
> > > > > +       if (ext->name) {
> > > > > +               g_dbus_dict_append_entry(&dict, "Name", DBUS_TYPE_STRING,
> > > > > +                                        &ext->name);
> > > > > +       }
> > > > > +       if (ext->path) {
> > > > > +               g_dbus_dict_append_entry(&dict, "Path", DBUS_TYPE_STRING,
> > > > > +                                        &ext->path);
> > > > > +       }
> > > > > +       if (ext->service) {
> > > > > +               g_dbus_dict_append_entry(&dict, "Service", DBUS_TYPE_STRING,
> > > > > +                                        &ext->service);
> > > > > +       }
> > > > > +
> > > > > +       u16 = ext->mode;
> > > > > +       g_dbus_dict_append_entry(&dict, "Mode", DBUS_TYPE_UINT16,
> > > > > +                                &u16);
> > > > > +
> > > > > +       u16 = ext->addr_type;
> > > > > +       g_dbus_dict_append_entry(&dict, "AddressType", DBUS_TYPE_UINT16,
> > > > > +                                &u16);
> > > > > +
> > > > > +       if (adapter) {
> > > > > +               for (l = ext->servers; l != NULL; l = next) {
> > > > > +                       struct ext_io *server = l->data;
> > > > > +                       const char *ctype;
> > > > > +
> > > > > +                       DBG("server:%p  %d %d psm:%d chan:%d",
> > > > > +                           server, server->resolving, server->connected,
> > > > > +                           server->psm, server->chan);
> > > > > +
> > > > > +                       next = g_slist_next(l);
> > > > > +
> > > > > +                       if (strcmp(adapter, adapter_get_path(server->adapter)))
> > > > > +                               continue;
> > > > > +
> > > > > +                       if (server->proto == BTPROTO_L2CAP) {
> > > > > +                               ctype = "PSM";
> > > > > +                               u16 = server->psm;
> > > > > +                       } else if (server->proto == BTPROTO_RFCOMM) {
> > > > > +                               ctype = "Channel";
> > > > > +                               u16 = server->chan;
> > > > > +                       } else {
> > > > > +                               continue;
> > > > > +                       }
> > > > > +                       g_dbus_dict_append_entry(
> > > > > +                               &dict, ctype, DBUS_TYPE_UINT16, &u16);
> > > > > +               }
> > > > > +       }
> > > > > +
> > > > > +       dbus_message_iter_close_container(&iter, &dict);
> > > > > +
> > > > > +       return reply;
> > > > > +}
> > > > > +
> > > > >  static const GDBusMethodTable methods[] = {
> > > > >         { GDBUS_METHOD("RegisterProfile",
> > > > >                         GDBUS_ARGS({ "profile", "o"}, { "UUID", "s" },
> > > > > @@ -2516,6 +2606,9 @@ static const GDBusMethodTable methods[] = {
> > > > >                         NULL, register_profile) },
> > > > >         { GDBUS_METHOD("UnregisterProfile", GDBUS_ARGS({ "profile", "o" }),
> > > > >                         NULL, unregister_profile) },
> > > > > +       { GDBUS_METHOD("GetProfileInfo",
> > > > > +                       GDBUS_ARGS({ "profile", "o" }, { "adapter", "o" }),
> > > > > +                       GDBUS_ARGS({ "options", "a{sv}" }), get_profile_info) },
> > > > >         { }
> > > > >  };
> > > > >
> > > > > --
> > > > > 2.17.1
> > > > >
> > > >
> > > >
> >
> >
>


-- 
Luiz Augusto von Dentz



[Index of Archives]     [Bluez Devel]     [Linux Wireless Networking]     [Linux Wireless Personal Area Networking]     [Linux ATH6KL]     [Linux USB Devel]     [Linux Media Drivers]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Big List of Linux Books]

  Powered by Linux