Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230113050604.71398-1-abhay.maheshbhai.maheta@intel.com>
In-Reply-To: <20230113050604.71398-1-abhay.maheshbhai.maheta@intel.com>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Fri, 13 Jan 2023 12:42:15 -0800
Message-ID: <CABBYNZ+_zBr_RcMUcXPyHzroakz4zRb3OvqaY6b+7mmA9+2ypA@mail.gmail.com>
Subject: Re: [PATCH BlueZ v3] shared/bap: Fixing Company ID, Vendor ID and
 Metadata handling
To:     Abhay Maheta <abhay.maheshbhai.maheta@intel.com>
Cc:     linux-bluetooth@vger.kernel.org, Abhay Maheta <mabhay125@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Abhay,

On Thu, Jan 12, 2023 at 9:11 PM Abhay Maheta
<abhay.maheshbhai.maheta@intel.com> wrote:
>
> This fixes Company ID, Vendor Codec ID and metadata storing
> PAC record.
> ---
>  src/shared/bap.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/src/shared/bap.c b/src/shared/bap.c
> index 0cafb75e6..463cdb7a6 100644
> --- a/src/shared/bap.c
> +++ b/src/shared/bap.c
> @@ -344,8 +344,10 @@ static void pac_foreach(void *data, void *user_data)
>
>         p = util_iov_push(iov, sizeof(*p));
>         p->codec.id = pac->codec.id;
> -       p->codec.cid = pac->codec.cid;
> -       p->codec.vid = pac->codec.vid;
> +       if (p->codec.id == 0xff) {
> +               p->codec.cid = cpu_to_le16(pac->codec.cid);
> +               p->codec.vid = cpu_to_le16(pac->codec.vid);
> +       }
>
>         if (pac->data) {
>                 p->cc_len = pac->data->iov_len;
> @@ -2773,7 +2775,7 @@ static void bap_parse_pacs(struct bt_bap *bap, uint8_t type,
>                 struct bt_pac *p;
>                 struct bt_ltv *cc;
>                 struct bt_pac_metadata *meta;
> -               struct iovec data, metadata;
> +               struct iovec data, *metadata = NULL;
>
>                 p = util_iov_pull_mem(&iov, sizeof(*p));
>                 if (!p) {
> @@ -2802,8 +2804,11 @@ static void bap_parse_pacs(struct bt_bap *bap, uint8_t type,
>                 data.iov_len = p->cc_len;
>                 data.iov_base = cc;
>
> -               metadata.iov_len = meta->len;
> -               metadata.iov_base = meta->data;
> +               if (meta->len) {
> +                       metadata = new0(struct iovec, 1);
> +                       metadata->iov_len = meta->len;
> +                       metadata->iov_base = meta->data;
> +               }

I'm trying to understand why you have done an extra copy of the
metadata if later it will be copied by the likes of util_iov_dup
inside bap_pac_new?

>
>                 util_iov_pull_mem(&iov, meta->len);
>
> @@ -2813,12 +2818,14 @@ static void bap_parse_pacs(struct bt_bap *bap, uint8_t type,
>                 /* Check if there is already a PAC record for the codec */
>                 pac = bap_pac_find(bap->rdb, type, &p->codec);
>                 if (pac) {
> -                       bap_pac_merge(pac, &data, &metadata);
> +                       bap_pac_merge(pac, &data, metadata);
> +                       free(metadata);
>                         continue;
>                 }
>
>                 pac = bap_pac_new(bap->rdb, NULL, type, &p->codec, NULL, &data,
> -                                                               &metadata);
> +                                                               metadata);
> +               free(metadata);
>                 if (!pac)
>                         continue;
>
> --
> 2.25.1
>


-- 
Luiz Augusto von Dentz