Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200427150153.17055-1-david@lechnology.com> <CABBYNZKhVUqLortqL60CNtu0MZBdtW+qqdtn6zfacOVKDezYxQ@mail.gmail.com>
 <35f37936-e737-ac35-c538-9447a00d1cbc@lechnology.com>
In-Reply-To: <35f37936-e737-ac35-c538-9447a00d1cbc@lechnology.com>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Tue, 28 Apr 2020 10:03:40 -0700
Message-ID: <CABBYNZKYXjKDb77988uFE6hQvW-R2es=Y4rZfNML9zFciOmuRg@mail.gmail.com>
Subject: Re: [PATCH BlueZ v2] src/gatt-client: always check properties in WriteValue
To:     David Lechner <david@lechnology.com>
Cc:     "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

Hi David,

On Tue, Apr 28, 2020 at 7:39 AM David Lechner <david@lechnology.com> wrote:
>
> On 4/27/20 11:28 AM, Luiz Augusto von Dentz wrote:
> > Hi David,
> >
> > On Mon, Apr 27, 2020 at 8:07 AM David Lechner <david@lechnology.com> wrote:
> >>
> >> This modifies the GATT client characteristic WriteValue D-Bus method to
> >> always check that the characteristic supports the requested type of
> >> write by checking for the corresponding property before attempting to
> >> write.
> >>
> >> Before this change, if the "type" option was used and it was set to
> >> "reliable" or "request", then BlueZ would attempt the write even if the
> >> characteristic does not support that write type. On the other hand, if
> >> "type" was set to "command" or was not specified, the method would
> >> return a org.bluez.Error.NotSupported error without attempting to write.
> >>
> >> After this change, the WriteValue method will consistently return
> >> org.bluez.Error.NotSupported if the corresponding property flag is not
> >> set for all types of writes.
> >> ---
> >>
> >> v2 changes:
> >> - remove extra check of test variable not NULL
> >> - fix one line over 80 chars
> >>
> >>   src/gatt-client.c | 8 ++++----
> >>   1 file changed, 4 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/src/gatt-client.c b/src/gatt-client.c
> >> index a9bfc2802..f80742fbb 100644
> >> --- a/src/gatt-client.c
> >> +++ b/src/gatt-client.c
> >> @@ -1016,8 +1016,8 @@ static DBusMessage *characteristic_write_value(DBusConnection *conn,
> >>           *     - If value is larger than MTU - 3: long-write
> >>           *   * "write-without-response" property set -> write command.
> >>           */
> >> -       if ((!type && (chrc->ext_props & BT_GATT_CHRC_EXT_PROP_RELIABLE_WRITE))
> >> -                       || (type && !strcasecmp(type, "reliable"))) {
> >> +       if ((!type || !strcasecmp(type, "reliable")) && chrc->ext_props &
> >> +                               BT_GATT_CHRC_EXT_PROP_RELIABLE_WRITE) {
> >>                  supported = true;
> >>                  chrc->write_op = start_long_write(msg, chrc->value_handle, gatt,
> >>                                                  true, value, value_len, offset,
> >> @@ -1026,8 +1026,8 @@ static DBusMessage *characteristic_write_value(DBusConnection *conn,
> >>                          return NULL;
> >>          }
> >>
> >> -       if ((!type && chrc->props & BT_GATT_CHRC_PROP_WRITE) ||
> >> -                       (type && !strcasecmp(type, "request"))) {
> >> +       if ((!type || !strcasecmp(type, "request")) && chrc->props &
> >> +                                               BT_GATT_CHRC_PROP_WRITE) {
> >>                  uint16_t mtu;
> >>
> >>                  supported = true;
> >> --
> >> 2.17.1
> >
> > As far I can remember this is on purpose so the application can
> > overwrite the type if it needs to disabling the checks on the client
> > side, though the kernel can still return not supported, so if we want
> > to change this it will now not be possible to overwrite it anymore.
> >
>
> If this is the case, does it make sense to make the following change
> instead so that write without response can also be forced?
>
> diff --git a/src/gatt-client.c b/src/gatt-client.c
> index f80742fbb..3153f571f 100644
> --- a/src/gatt-client.c
> +++ b/src/gatt-client.c
> @@ -1050,8 +1050,8 @@ static DBusMessage *characteristic_write_value(DBusConnection *conn,
>                          return NULL;
>          }
>
> -       if ((type && strcasecmp(type, "command")) || offset ||
> -                       !(chrc->props & BT_GATT_CHRC_PROP_WRITE_WITHOUT_RESP))
> +       if ((type && strcasecmp(type, "command")) || offset || (!type &&
> +                       !(chrc->props & BT_GATT_CHRC_PROP_WRITE_WITHOUT_RESP)))
>                  goto fail;
>
>          supported = true;

Yes, that is probably one of the use cases to use the type to force
sending a command if the client don't care about the response but the
server don't mark write without response as supported, is that what
you are after? That sounds like a bug since it appears the intention
was to allow commands all along.

-- 
Luiz Augusto von Dentz