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.
> > > 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
--
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
