Hi Luiz,
> -----Original Message-----
> From: Amisha Jain (QUIC)
> Sent: Tuesday, February 4, 2025 5:34 PM
> To: 'Luiz Augusto von Dentz' <luiz.dentz@xxxxxxxxx>
> Cc: linux-bluetooth@xxxxxxxxxxxxxxx; Mohammed Sameer Mulla (QUIC)
> <quic_mohamull@xxxxxxxxxxx>; Harish Bandi (QUIC)
> <quic_hbandi@xxxxxxxxxxx>; Anubhav Gupta (QUIC)
> <quic_anubhavg@xxxxxxxxxxx>
> Subject: RE: [PATCH v1] obex: Add supported features tag in MAP Client
> Connect Request
>
> Hi Luiz,
>
> > -----Original Message-----
> > From: Luiz Augusto von Dentz <luiz.dentz@xxxxxxxxx>
> > Sent: Friday, January 31, 2025 3:07 AM
> > To: Amisha Jain (QUIC) <quic_amisjain@xxxxxxxxxxx>
> > Cc: linux-bluetooth@xxxxxxxxxxxxxxx; Mohammed Sameer Mulla (QUIC)
> > <quic_mohamull@xxxxxxxxxxx>; Harish Bandi (QUIC)
> > <quic_hbandi@xxxxxxxxxxx>; Anubhav Gupta (QUIC)
> > <quic_anubhavg@xxxxxxxxxxx>
> > Subject: Re: [PATCH v1] obex: Add supported features tag in MAP Client
> > Connect Request
> >
> > Hi Amisha,
> >
> > On Thu, Jan 30, 2025 at 2:38 PM Amisha Jain (QUIC)
> > <quic_amisjain@xxxxxxxxxxx> wrote:
> > >
> > > Hi Luiz,
> > >
> > > > -----Original Message-----
> > > > From: Luiz Augusto von Dentz <luiz.dentz@xxxxxxxxx>
> > > > Sent: Tuesday, January 28, 2025 11:20 PM
> > > > To: Amisha Jain (QUIC) <quic_amisjain@xxxxxxxxxxx>
> > > > Cc: linux-bluetooth@xxxxxxxxxxxxxxx; Mohammed Sameer Mulla (QUIC)
> > > > <quic_mohamull@xxxxxxxxxxx>; Harish Bandi (QUIC)
> > > > <quic_hbandi@xxxxxxxxxxx>; Anubhav Gupta (QUIC)
> > > > <quic_anubhavg@xxxxxxxxxxx>
> > > > Subject: Re: [PATCH v1] obex: Add supported features tag in MAP
> > > > Client Connect Request
> > > >
> > > > Hi Amisha,
> > > >
> > > > On Tue, Jan 28, 2025 at 12:43 PM Amisha Jain
> > > > <quic_amisjain@xxxxxxxxxxx>
> > > > wrote:
> > > > >
> > > > > This change is required for below PTS testcase -
> > > > >
> > > > > 1. MAP/MCE/MFB/BV-06-C
> > > > > Verify that the MCE sends its MapSupportedFeatures in the OBEX
> > > > > Connect request if the MSE declares support for the feature
> > > > > MapSupportedFeatures in Connect Request in its SDP record.
> > > > >
> > > > > If Server's SDP record contains the field 'MapSupportedFeatures
> > > > > in Connect Request' as supported then include the supported
> > > > > features tag in obex connect request.
> > > >
> > > > Can you include the btmon output with and without?
> > > >
> > > btmon output does not captures obex packet so adding snoop packet -
> >
> > Hmm, seems you are right, we only had it for hcidump not btmon.
> >
> > > Obex Connect request with this change - OBEX
> > > Role: Central
> > > Address: 0
> > > Packet Status: Final Packet
> > > Opcode: Connect
> > > Length: 35
> > > OBEX Version Number: 0x10
> > > flags
> > > Maximum Packet Length: 1800
> > > Target
> > > Header Encoding: Byte Sequence
> > > Header ID: Target
> > > Length: 19
> > > Target: MAS
> > > Application Parameters
> > > Header Encoding: Byte Sequence
> > > Header ID: Application Parameters
> > > Length: 9
> > > Parameter
> > > Tag: Map Supported Features
> > > Length: 4
> > > Value
> > > : Messages-Listing Format Version 1.1
> > > : Extended Event Report 1.1
> > > : Instance Information Feature
> > > : Delete Feature
> > > : Uploading Feature
> > > : Browsing Feature
> > > : Notification Feature
> > > : Notification Registration Feature
> > >
> > > Obex Connect request without this change - OBEX
> > > Role: Central
> > > Address: 0
> > > Packet Status: Final Packet
> > > Opcode: Connect
> > > Length: 26
> > > OBEX Version Number: 0x10
> > > flags
> > > Maximum Packet Length: 1800
> > > Target
> > > Header Encoding: Byte Sequence
> > > Header ID: Target
> > > Length: 19
> > > Target: MAS
> >
> > Please include this info as part of the patch description.
> >
> > > > > ---
> > > > > obexd/client/map.c | 19 +++++++++++++++++++
> > > > > 1 file changed, 19 insertions(+)
> > > > >
> > > > > diff --git a/obexd/client/map.c b/obexd/client/map.c index
> > > > > b8820335b..2fd0f74ec 100644
> > > > > --- a/obexd/client/map.c
> > > > > +++ b/obexd/client/map.c
> > > > > @@ -39,6 +39,7 @@
> > > > > #define OBEX_MAS_UUID \
> > > > >
> > > >
> "\xBB\x58\x2B\x40\x42\x0C\x11\xDB\xB0\xDE\x08\x00\x20\x0C\x9A\x66"
> > > > > #define OBEX_MAS_UUID_LEN 16
> > > > > +#define SUPPORTED_FEATURES_TAG 0x29
> > > > >
> > > > > #define MAP_INTERFACE "org.bluez.obex.MessageAccess1"
> > > > > #define MAP_MSG_INTERFACE "org.bluez.obex.Message1"
> > > > > @@ -2179,6 +2180,23 @@ static void parse_service_record(struct
> > > > map_data *map)
> > > > > map->supported_features = 0x0000001f; }
> > > > >
> > > > > +static void *map_supported_features(struct obc_session *session) {
> > > > > + const void *data;
> > > > > +
> > > > > + /* Supported Feature Bits */
> > > > > + data = obc_session_get_attribute(session,
> > > > > + SDP_ATTR_MAP_SUPPORTED_FEATURES);
> > > > > + if (!data)
> > > > > + return NULL;
> > > > > +
> > > > > + if(*(uint32_t *)data & 0x00080000)
> > > > > + return g_obex_apparam_set_uint32(NULL,
> > > > SUPPORTED_FEATURES_TAG,
> > > > > + 0x0000027f);
> > > >
> > > > Don't think it is safe to check the data like above, we don't know
> > > > if field returned is really 32 bits, perhaps it would be a good
> > > > idea to introduce something like obc_session_get_attribute_le32
> > > > that would ensure the value is really 32 bits and also check its
> > > > little/big endian
> > in the process.
> > > >
> > >
> > > As per the BT Spec, 32 bits field is reserved for 'MapSupportedFeatures'
> > attribute in SDP record. So, it will be always 32 bits. Each bit
> > corresponds to each feature. If any feature is not supported then that bit
> will be zero.
> >
> > First 32 bits is not a proper format without us knowing in what byte
> > order it is, second we need to actually check the attribute is
> > correctly formatted and properly do it matches the expected size, now
> > perhaps you are saying that is our own SDP record we are talking
> > about, but that doesn't change the fact that there should be checks to
> > make sure we don't access invalid memory if something goes wrong.
> >
>
> Sure, we will add the byte order check in next patch.
> For checking the size of attribute whether it is 32 bits or not can be tricky as
> the obc_session_get_attribute() will internally call the
> bluetooth_getattribute() function which only returns the attribute value in
> void* format.
> Even if we introduce new function like obc_session_get_attribute_le32 , it will
> also eventually call the bluetooth_getattribute().
>
> One way to resolve this is to change the return type of
> bluetooth_getattribute() to 'struct sdp_data_t' which will have the 'unitSize'
> info. This function is common call for many profiles hence more changes will
> be involved.
>
> Other way to resolve this is to add new function like
> bluetooth_getattribute_32() which will internally check the data size and
> return it.
>
> Please suggest any other way to resolve this or if we can go ahead with one of
> the above approaches.
>
Let us know for any way to resolve it.
> > >
> > > > > +
> > > > > + return NULL;
> > > > > +}
> > > > > +
> > > > > static int map_probe(struct obc_session *session) {
> > > > > struct map_data *map;
> > > > > @@ -2224,6 +2242,7 @@ static struct obc_driver map = {
> > > > > .uuid = MAS_UUID,
> > > > > .target = OBEX_MAS_UUID,
> > > > > .target_len = OBEX_MAS_UUID_LEN,
> > > > > + .supported_features = map_supported_features,
> > > > > .probe = map_probe,
> > > > > .remove = map_remove
> > > > > };
> > > > > --
> > > > > 2.34.1
> > > > >
> > > > >
> > > >
> > > >
> > > > --
> > > > Luiz Augusto von Dentz
> > >
> > > Thanks,
> > > Amisha
> >
> >
> >
> > --
> > Luiz Augusto von Dentz
>
> Thanks,
> Amisha
