Re: [RFC 2/2] client: Add authorized property handling to characteristic attribute

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

 



Hi Luiz,
czw., 3 maj 2018 o 11:58 Luiz Augusto von Dentz <luiz.dentz@xxxxxxxxx>
napisał(a):

> Hi Grzegorz,
> On Wed, May 2, 2018 at 4:06 PM Grzegorz Kołodziejczyk <
> grzegorz.kolodziejczyk@xxxxxxxxxxx> wrote:

> > Hi Luiz,
> > śr., 2 maj 2018 o 12:42 Luiz Augusto von Dentz <luiz.dentz@xxxxxxxxx>
> > napisał(a):

> > > Hi Grzegorz,
> > > On Wed, May 2, 2018 at 1:09 PM Grzegorz Kolodziejczyk <
> > > grzegorz.kolodziejczyk@xxxxxxxxxxx> wrote:

> > > > This patch adds handling of authorized property to bluetoothctl.
> > > > ---
> > > >   client/gatt.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
> > > >   1 file changed, 46 insertions(+)

> > > > diff --git a/client/gatt.c b/client/gatt.c
> > > > index 6bf644c48..fffa86851 100644
> > > > --- a/client/gatt.c
> > > > +++ b/client/gatt.c
> > > > @@ -1432,6 +1432,30 @@ static gboolean
> chrc_notify_acquired_exists(const
> > > GDBusPropertyTable *property,
> > > >          return FALSE;
> > > >   }

> > > > +static gboolean chrc_get_authorized(const GDBusPropertyTable
> *property,
> > > > +                                       DBusMessageIter *iter, void
> > *data)
> > > > +{
> > > > +       struct chrc *chrc = data;
> > > > +       dbus_bool_t value;
> > > > +
> > > > +       value = chrc->authorized ? TRUE : FALSE;
> > > > +
> > > > +       dbus_message_iter_append_basic(iter, DBUS_TYPE_BOOLEAN,
> &value);
> > > > +
> > > > +       return TRUE;
> > > > +}
> > > > +
> > > > +static gboolean chrc_get_authorized_exists(const GDBusPropertyTable
> > > *property,
> > > > +                                                               void
> > > *data)
> > > > +{
> > > > +       struct chrc *chrc = data;
> > > > +
> > > > +       if (chrc->authorization_req)
> > > > +               return TRUE;
> > > > +
> > > > +       return FALSE;
> > > > +}
> > > > +
> > > >   static const GDBusPropertyTable chrc_properties[] = {
> > > >          { "UUID", "s", chrc_get_uuid, NULL, NULL },
> > > >          { "Service", "o", chrc_get_service, NULL, NULL },
> > > > @@ -1442,6 +1466,8 @@ static const GDBusPropertyTable
> chrc_properties[]
> > =
> > > {
> > > >                                          chrc_write_acquired_exists
},
> > > >          { "NotifyAcquired", "b", chrc_get_notify_acquired, NULL,
> > > >                                          chrc_notify_acquired_exists
> },
> > > > +       { "Authorized", "b", chrc_get_authorized, NULL,
> > > > +
> > > chrc_get_authorized_exists },
> > > >          { }
> > > >   };

> > > > @@ -1665,6 +1691,15 @@ static void authorize_write_response(const
char
> > > *input, void *user_data)

> > > >          chrc->authorized = true;

> > > > +       /* Authorization check of prepare writes */
> > > > +       if (!chrc->value_len) {
> > > > +               reply = g_dbus_create_reply(pending_message,
> > > DBUS_TYPE_INVALID);
> > > > +               g_dbus_send_message(aad->conn, reply);
> > > > +               g_free(aad);
> > > > +
> > > > +               return;
> > > > +       }
> > > > +
> > > >          errsv = parse_value_arg(&iter, &chrc->value,
> &chrc->value_len,

> > > chrc->max_val_len);
> > > >          if (errsv == -EINVAL) {
> > > > @@ -1701,8 +1736,16 @@ static DBusMessage
> > > *chrc_write_value(DBusConnection *conn, DBusMessage *msg,
> > > >          DBusMessageIter iter;
> > > >          char *str;
> > > >          int errsv;
> > > > +       uint16_t offset = 0;

> > > >          dbus_message_iter_init(msg, &iter);
> > > > +       /* Get offset only (second parameter) */
> > > > +       dbus_message_iter_next(&iter);
> > > > +
> > > > +       parse_options(&iter, &offset, NULL, NULL, NULL);
> > > > +
> > > > +       if (chrc->authorization_req && offset == 0)
> > > > +               chrc->authorized = false;

> > > >          if (chrc->authorization_req && !chrc->authorized) {
> > > >                  struct authorize_attribute_data *aad;
> > > > @@ -1722,6 +1765,9 @@ static DBusMessage
> > *chrc_write_value(DBusConnection
> > > *conn, DBusMessage *msg,
> > > >                  return NULL;
> > > >          }

> > > > +       /* Rewind to value parameter */
> > > > +       dbus_message_iter_init(msg, &iter);

> > > I would prefer to make parse_value_arg parse the offset as well so we
> > don't
> > > have to reinit the iter.
> > Current version of bluetoothctl already parsses offset in
parse_value_arg
> > for writing value purposes (patch
> 1dd33d584ce1e4bd0f1d87e88663350a80f37f01).

> > As the offset is second parameter for write method it's needed (afaik)
to
> > recurse over iterator to offset parameter then parse value (there is no
> > need to parse value at beginnig). Currently this implementation assumes
> > that every write request with offset = 0 is new write request which has
to
> > be authoriez/re-authorized.

> That fact that parse_value_arg is already a strong indication that we
> should reuse it, we just need to add the offset as a output parameter,
like
> it is done in parse_options, that way we will get both the value and the
> offset in the same call and don't have to reinit the iter.
For now the parse_value_arg function is not only parsing dbus message and
gets values, but also write attribute value.
I guess the idea of requesting authorization for write value with offset =
0 is not considered for now so this wouldn't be needed.

> > > >          errsv = parse_value_arg(&iter, &chrc->value,
> &chrc->value_len,

> > > chrc->max_val_len);
> > > >          if (errsv == -EINVAL) {
> > > > --
> > > > 2.13.6

> > > > --
> > > > To unsubscribe from this list: send the line "unsubscribe
> > > linux-bluetooth" in
> > > > the body of a message to majordomo@xxxxxxxxxxxxxxx
> > > > More majordomo info at  http://vger.kernel.org/majordomo-info.html



> > > --
> > > Luiz Augusto von Dentz
> > Grzegorz Kołodziejczyk



> --
> Luiz Augusto von Dentz
Grzegorz Kołodziejczyk
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[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