Received-SPF: pass (google.com: best guess record for 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: <20200321195404.fvyku5hmcuqxt7sg@pali> <20200413162513.2221-1-pali@kernel.org>
 <20200413162513.2221-2-pali@kernel.org>
In-Reply-To: <20200413162513.2221-2-pali@kernel.org>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Mon, 13 Apr 2020 09:44:14 -0700
Message-ID: <CABBYNZKneybjWDEddrCXxtzJdGZSY__D7qtPuw25QMLDE9hg4Q@mail.gmail.com>
Subject: Re: [PATCH 1/3] src/profile: Distinguish between zero-set HFP AG
 features and unset HFP AG features
To:     =?UTF-8?Q?Pali_Roh=C3=A1r?= <pali@kernel.org>
Cc:     "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>,
        David Heidelberg <david@ixit.cz>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

Hi Pali,

On Mon, Apr 13, 2020 at 9:25 AM Pali Roh=C3=A1r <pali@kernel.org> wrote:
>
> When HFP AG features are not set then according to HFP 1.7.2 specificatio=
n
> it has value 0b001001. But when HFP AG features were explicitly set to
> zero, bluez did not propagated this value via NewConnection() DBus call, =
so
> callee used default value 0b001001 (according to HFP 1.7.2 specification)
> as bluez did not provided explicit zero value.
>
> To fix this problem add a new boolean variable have_features which
> indicates if SDP features were provided (with possible zero value) or not
> (when default value needs to be used). Code for generating SDP XML record=
s
> were also updated to handle this fact.
>
> Signed-off-by: Pali Roh=C3=A1r <pali@kernel.org>
> ---
>  src/profile.c | 32 ++++++++++++++++++++++++--------
>  1 file changed, 24 insertions(+), 8 deletions(-)
>
> diff --git a/src/profile.c b/src/profile.c
> index 192fd0245..884440408 100644
> --- a/src/profile.c
> +++ b/src/profile.c
> @@ -647,6 +647,7 @@ struct ext_profile {
>
>         uint16_t version;
>         uint16_t features;
> +       bool have_features;
>
>         GSList *records;
>         GSList *servers;
> @@ -669,6 +670,7 @@ struct ext_io {
>
>         uint16_t version;
>         uint16_t features;
> +       bool have_features;
>
>         uint16_t psm;
>         uint8_t chan;
> @@ -920,14 +922,18 @@ static void append_prop(gpointer a, gpointer b)
>         dbus_message_iter_close_container(dict, &entry);
>  }
>
> -static uint16_t get_supported_features(const sdp_record_t *rec)
> +static uint16_t get_supported_features(const sdp_record_t *rec,
> +                                       bool *have_features)
>  {
>         sdp_data_t *data;
>
>         data =3D sdp_data_get(rec, SDP_ATTR_SUPPORTED_FEATURES);
> -       if (!data || data->dtd !=3D SDP_UINT16)
> +       if (!data || data->dtd !=3D SDP_UINT16) {
> +               *have_features =3D false;
>                 return 0;
> +       }
>
> +       *have_features =3D true;
>         return data->val.uint16;
>  }
>
> @@ -972,7 +978,8 @@ static bool send_new_connection(struct ext_profile *e=
xt, struct ext_io *conn)
>         if (remote_uuid) {
>                 rec =3D btd_device_get_record(conn->device, remote_uuid);
>                 if (rec) {
> -                       conn->features =3D get_supported_features(rec);
> +                       conn->features =3D get_supported_features(rec,
> +                                                       &conn->have_featu=
res);
>                         conn->version =3D get_profile_version(rec);
>                 }
>         }
> @@ -991,7 +998,7 @@ static bool send_new_connection(struct ext_profile *e=
xt, struct ext_io *conn)
>                 dict_append_entry(&dict, "Version", DBUS_TYPE_UINT16,
>                                                         &conn->version);
>
> -       if (conn->features)
> +       if (conn->have_features)
>                 dict_append_entry(&dict, "Features", DBUS_TYPE_UINT16,
>                                                         &conn->features);
>
> @@ -1589,7 +1596,8 @@ static void record_cb(sdp_list_t *recs, int err, gp=
ointer user_data)
>                 if (conn->psm =3D=3D 0 && sdp_get_proto_desc(protos, OBEX=
_UUID))
>                         conn->psm =3D get_goep_l2cap_psm(rec);
>
> -               conn->features =3D get_supported_features(rec);
> +               conn->features =3D get_supported_features(rec,
> +                                                       &conn->have_featu=
res);
>                 conn->version =3D get_profile_version(rec);
>
>                 sdp_list_foreach(protos, (sdp_list_func_t) sdp_list_free,
> @@ -1750,15 +1758,19 @@ static int ext_disconnect_dev(struct btd_service =
*service)
>  static char *get_hfp_hf_record(struct ext_profile *ext, struct ext_io *l=
2cap,
>                                                         struct ext_io *rf=
comm)
>  {
> +       /* HFP 1.7.2: By default features bitfield is 0b000000 */
>         return g_strdup_printf(HFP_HF_RECORD, rfcomm->chan, ext->version,
> -                                               ext->name, ext->features)=
;
> +                               ext->name,
> +                               ext->have_features ? ext->features : 0x0)=
;
>  }
>
>  static char *get_hfp_ag_record(struct ext_profile *ext, struct ext_io *l=
2cap,
>                                                         struct ext_io *rf=
comm)
>  {
> +       /* HFP 1.7.2: By default features bitfield is 0b001001 */
>         return g_strdup_printf(HFP_AG_RECORD, rfcomm->chan, ext->version,
> -                                               ext->name, ext->features)=
;
> +                               ext->name,
> +                               ext->have_features ? ext->features : 0x9)=
;

I wonder why you didn't just initialize the features wiht 0x9 instead
of adding a flag to track it, btw add a define with value 0x09 like
HFP_DEFAULT_FEATURES that way it is clearer why we are doing this.

>  }
>
>  static char *get_hsp_ag_record(struct ext_profile *ext, struct ext_io *l=
2cap,
> @@ -1948,6 +1960,7 @@ static struct default_settings {
>                                         struct ext_io *rfcomm);
>         uint16_t        version;
>         uint16_t        features;
> +       bool            have_features;
>  } defaults[] =3D {
>         {
>                 .uuid           =3D SPP_UUID,
> @@ -2107,8 +2120,10 @@ static void ext_set_defaults(struct ext_profile *e=
xt)
>                 if (settings->version)
>                         ext->version =3D settings->version;
>
> -               if (settings->features)
> +               if (settings->have_features) {
>                         ext->features =3D settings->features;
> +                       ext->have_features =3D true;
> +               }
>
>                 if (settings->name)
>                         ext->name =3D g_strdup(settings->name);
> @@ -2198,6 +2213,7 @@ static int parse_ext_opt(struct ext_profile *ext, c=
onst char *key,
>
>                 dbus_message_iter_get_basic(value, &feat);
>                 ext->features =3D feat;
> +               ext->have_features =3D true;
>         } else if (strcasecmp(key, "Service") =3D=3D 0) {
>                 if (type !=3D DBUS_TYPE_STRING)
>                         return -EINVAL;
> --
> 2.20.1
>


--=20
Luiz Augusto von Dentz