public inbox for linux-bluetooth@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] Bluetooth : Update the mas session structure
@ 2024-06-25  6:35 Amisha Jain
  2024-06-25  8:08 ` [v2] " bluez.test.bot
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Amisha Jain @ 2024-06-25  6:35 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: quic_mohamull, quic_hbandi, quic_anubhavg

Update the 'mas_session' structure such that
manager_emit_transfer_property(os->service_data, "Size")
will get the proper structure in arguments as
expected like structure 'obex_transfer' and transfer->path
won't be populated with inappropriate value.

As there is no new transfer registered during mas connect,
hence setting the path to NULL to avoid invoking the
g_dbus_emit_property_changed() property.

Signed-off-by: Amisha Jain <quic_amisjain@quicinc.com>
---
 obexd/plugins/mas.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/obexd/plugins/mas.c b/obexd/plugins/mas.c
index 10b972d65..71bf12ad3 100644
--- a/obexd/plugins/mas.c
+++ b/obexd/plugins/mas.c
@@ -51,6 +51,8 @@
 #define ML_BODY_END "</MAP-msg-listing>"
 
 struct mas_session {
+	uint8_t notification_status;
+	char *path;
 	struct mas_request *request;
 	void *backend_data;
 	gboolean finished;
@@ -59,7 +61,6 @@ struct mas_session {
 	GObexApparam *inparams;
 	GObexApparam *outparams;
 	gboolean ap_sent;
-	uint8_t notification_status;
 };
 
 static const uint8_t MAS_TARGET[TARGET_SIZE] = {
@@ -125,6 +126,7 @@ static void *mas_connect(struct obex_session *os, int *err)
 		goto failed;
 
 	manager_register_session(os);
+	mas->path = NULL;
 
 	return mas;
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* RE: [v2] Bluetooth : Update the mas session structure
  2024-06-25  6:35 [PATCH v2] Bluetooth : Update the mas session structure Amisha Jain
@ 2024-06-25  8:08 ` bluez.test.bot
  2024-06-25 13:49 ` [PATCH v2] " Luiz Augusto von Dentz
  2024-06-25 15:39 ` Pauli Virtanen
  2 siblings, 0 replies; 7+ messages in thread
From: bluez.test.bot @ 2024-06-25  8:08 UTC (permalink / raw)
  To: linux-bluetooth, quic_amisjain

[-- Attachment #1: Type: text/plain, Size: 949 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=865178

---Test result---

Test Summary:
CheckPatch                    PASS      0.51 seconds
GitLint                       PASS      0.35 seconds
BuildEll                      PASS      25.02 seconds
BluezMake                     PASS      1702.62 seconds
MakeCheck                     PASS      12.86 seconds
MakeDistcheck                 PASS      183.75 seconds
CheckValgrind                 PASS      258.36 seconds
CheckSmatch                   PASS      361.60 seconds
bluezmakeextell               PASS      123.30 seconds
IncrementalBuild              PASS      1518.10 seconds
ScanBuild                     PASS      1051.15 seconds



---
Regards,
Linux Bluetooth


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] Bluetooth : Update the mas session structure
  2024-06-25  6:35 [PATCH v2] Bluetooth : Update the mas session structure Amisha Jain
  2024-06-25  8:08 ` [v2] " bluez.test.bot
@ 2024-06-25 13:49 ` Luiz Augusto von Dentz
  2024-06-25 15:39 ` Pauli Virtanen
  2 siblings, 0 replies; 7+ messages in thread
From: Luiz Augusto von Dentz @ 2024-06-25 13:49 UTC (permalink / raw)
  To: Amisha Jain; +Cc: linux-bluetooth, quic_mohamull, quic_hbandi, quic_anubhavg

Hi Amisha,

On Tue, Jun 25, 2024 at 2:36 AM Amisha Jain <quic_amisjain@quicinc.com> wrote:
>
> Update the 'mas_session' structure such that
> manager_emit_transfer_property(os->service_data, "Size")
> will get the proper structure in arguments as
> expected like structure 'obex_transfer' and transfer->path
> won't be populated with inappropriate value.
>
> As there is no new transfer registered during mas connect,
> hence setting the path to NULL to avoid invoking the
> g_dbus_emit_property_changed() property.
>
> Signed-off-by: Amisha Jain <quic_amisjain@quicinc.com>
> ---
>  obexd/plugins/mas.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/obexd/plugins/mas.c b/obexd/plugins/mas.c
> index 10b972d65..71bf12ad3 100644
> --- a/obexd/plugins/mas.c
> +++ b/obexd/plugins/mas.c
> @@ -51,6 +51,8 @@
>  #define ML_BODY_END "</MAP-msg-listing>"
>
>  struct mas_session {
> +       uint8_t notification_status;
> +       char *path;
>         struct mas_request *request;
>         void *backend_data;
>         gboolean finished;
> @@ -59,7 +61,6 @@ struct mas_session {
>         GObexApparam *inparams;
>         GObexApparam *outparams;
>         gboolean ap_sent;
> -       uint8_t notification_status;
>  };
>
>  static const uint8_t MAS_TARGET[TARGET_SIZE] = {
> @@ -125,6 +126,7 @@ static void *mas_connect(struct obex_session *os, int *err)
>                 goto failed;
>
>         manager_register_session(os);
> +       mas->path = NULL;

There is something missing here, you introduce a new field and set it
to NULL but you never use it?

>         return mas;
>
> --
> 2.17.1
>
>


-- 
Luiz Augusto von Dentz

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] Bluetooth : Update the mas session structure
  2024-06-25  6:35 [PATCH v2] Bluetooth : Update the mas session structure Amisha Jain
  2024-06-25  8:08 ` [v2] " bluez.test.bot
  2024-06-25 13:49 ` [PATCH v2] " Luiz Augusto von Dentz
@ 2024-06-25 15:39 ` Pauli Virtanen
  2024-06-26  6:10   ` Amisha Jain
  2 siblings, 1 reply; 7+ messages in thread
From: Pauli Virtanen @ 2024-06-25 15:39 UTC (permalink / raw)
  To: Amisha Jain, linux-bluetooth; +Cc: quic_mohamull, quic_hbandi, quic_anubhavg

Hi,

ti, 2024-06-25 kello 12:05 +0530, Amisha Jain kirjoitti:
> Update the 'mas_session' structure such that
> manager_emit_transfer_property(os->service_data, "Size")
> will get the proper structure in arguments as
> expected like structure 'obex_transfer' and transfer->path
> won't be populated with inappropriate value.
> 
> As there is no new transfer registered during mas connect,
> hence setting the path to NULL to avoid invoking the
> g_dbus_emit_property_changed() property.
> 
> Signed-off-by: Amisha Jain <quic_amisjain@quicinc.com>
> ---
>  obexd/plugins/mas.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/obexd/plugins/mas.c b/obexd/plugins/mas.c
> index 10b972d65..71bf12ad3 100644
> --- a/obexd/plugins/mas.c
> +++ b/obexd/plugins/mas.c
> @@ -51,6 +51,8 @@
>  #define ML_BODY_END "</MAP-msg-listing>"
>  
>  struct mas_session {
> +	uint8_t notification_status;
> +	char *path;
>  	struct mas_request *request;
>  	void *backend_data;
>  	gboolean finished;
> @@ -59,7 +61,6 @@ struct mas_session {
>  	GObexApparam *inparams;
>  	GObexApparam *outparams;
>  	gboolean ap_sent;
> -	uint8_t notification_status;
>  };
>  
>  static const uint8_t MAS_TARGET[TARGET_SIZE] = {
> @@ -125,6 +126,7 @@ static void *mas_connect(struct obex_session *os, int *err)
>  		goto failed;
>  
>  	manager_register_session(os);
> +	mas->path = NULL;

Maybe the problem here is that the change in commit bb160515185e
("obexd: Emit Size property of transfer after open()") is not right?

diff --git a/obexd/src/obex.c b/obexd/src/obex.c
index a4bae857f..ed219d3e7 100644
--- a/obexd/src/obex.c
+++ b/obexd/src/obex.c
@@ -779,6 +779,9 @@ int obex_put_stream_start(struct obex_session *os, const char *filename)
                return err;
        }
 
+       if (os->size != OBJECT_SIZE_DELETE && os->size != OBJECT_SIZE_UNKNOWN)
+               manager_emit_transfer_property(os->service_data, "Size");
+
        os->path = g_strdup(filename);

This casts os->service_data to obex_transfer which IIUC does not work
for most the plugins, as it's the session struct.

Maybe plugins can emit the transfer property change in their open()
callback, for the plugins where it makes sense?

>  
>  	return mas;
>  

-- 
Pauli Virtanen

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] Bluetooth : Update the mas session structure
  2024-06-25 15:39 ` Pauli Virtanen
@ 2024-06-26  6:10   ` Amisha Jain
  2024-06-27 16:24     ` Pauli Virtanen
  0 siblings, 1 reply; 7+ messages in thread
From: Amisha Jain @ 2024-06-26  6:10 UTC (permalink / raw)
  To: Pauli Virtanen, linux-bluetooth; +Cc: quic_mohamull, quic_hbandi, quic_anubhavg

Hi,

Correct! The problem here is manager_emit_transfer_property() is 
expecting the structure of type 'obex_transfer' and we are passing the 
structure of session type which will be type mismatch and inappropriate 
values will be populated in further calls in code. Hence property "Size" 
will never emit on console(obexctl) as it is not set properly and might 
cause crash/disconnection.

 > diff --git a/obexd/src/obex.c b/obexd/src/obex.c
 > index a4bae857f..ed219d3e7 100644
 > --- a/obexd/src/obex.c
 > +++ b/obexd/src/obex.c
 > @@ -779,6 +779,9 @@ int obex_put_stream_start(struct obex_session 
*os, const char *filename)
 >                  return err;
 >          }
 >
 > +       if (os->size != OBJECT_SIZE_DELETE && os->size != 
OBJECT_SIZE_UNKNOWN)
 > +               manager_emit_transfer_property(os->service_data, "Size");
 > +
 >          os->path = g_strdup(filename);
 >


One way to resolve this issue is to add the additional field in 
'mas_session' so it can cast to struct 'obex_transfer'. We are adding 
new field 'char *path' as only transfer->path will be invoked and passed 
further.

void manager_emit_transfer_property(struct obex_transfer *transfer,
								char *name)
{
	if (!transfer->path)
		return;

	g_dbus_emit_property_changed(connection, transfer->path,
					TRANSFER_INTERFACE, name);
}

 >> Signed-off-by: Amisha Jain <quic_amisjain@quicinc.com>
 >> ---
 >>   obexd/plugins/mas.c | 4 +++-
 >>   1 file changed, 3 insertions(+), 1 deletion(-)
 >>
 >> diff --git a/obexd/plugins/mas.c b/obexd/plugins/mas.c
 >> index 10b972d65..71bf12ad3 100644
 >> --- a/obexd/plugins/mas.c
 >> +++ b/obexd/plugins/mas.c
 >> @@ -51,6 +51,8 @@
 >>   #define ML_BODY_END "</MAP-msg-listing>"
 >>
 >>   struct mas_session {
 >> +	uint8_t notification_status;
 >> +	char *path;
 >>   	struct mas_request *request;
 >>   	void *backend_data;
 >>   	gboolean finished;
 >> @@ -59,7 +61,6 @@ struct mas_session {
 >>   	GObexApparam *inparams;
 >>   	GObexApparam *outparams;
 >>   	gboolean ap_sent;
 >> -	uint8_t notification_status;
 >>   };
 >>
 >>   static const uint8_t MAS_TARGET[TARGET_SIZE] = {
 >> @@ -125,6 +126,7 @@ static void *mas_connect(struct obex_session 
*os, int *err)
 >>   		goto failed;
 >>
 >>   	manager_register_session(os);
 >> +	mas->path = NULL;
 >

There is no transfer already registered for mas during connection, so 
setting the path to NULL.

On 6/25/2024 9:09 PM, Pauli Virtanen wrote:
> Hi,
> 
> ti, 2024-06-25 kello 12:05 +0530, Amisha Jain kirjoitti:
>> Update the 'mas_session' structure such that
>> manager_emit_transfer_property(os->service_data, "Size")
>> will get the proper structure in arguments as
>> expected like structure 'obex_transfer' and transfer->path
>> won't be populated with inappropriate value.
>>
>> As there is no new transfer registered during mas connect,
>> hence setting the path to NULL to avoid invoking the
>> g_dbus_emit_property_changed() property.
>>
>> Signed-off-by: Amisha Jain <quic_amisjain@quicinc.com>
>> ---
>>   obexd/plugins/mas.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/obexd/plugins/mas.c b/obexd/plugins/mas.c
>> index 10b972d65..71bf12ad3 100644
>> --- a/obexd/plugins/mas.c
>> +++ b/obexd/plugins/mas.c
>> @@ -51,6 +51,8 @@
>>   #define ML_BODY_END "</MAP-msg-listing>"
>>   
>>   struct mas_session {
>> +	uint8_t notification_status;
>> +	char *path;
>>   	struct mas_request *request;
>>   	void *backend_data;
>>   	gboolean finished;
>> @@ -59,7 +61,6 @@ struct mas_session {
>>   	GObexApparam *inparams;
>>   	GObexApparam *outparams;
>>   	gboolean ap_sent;
>> -	uint8_t notification_status;
>>   };
>>   
>>   static const uint8_t MAS_TARGET[TARGET_SIZE] = {
>> @@ -125,6 +126,7 @@ static void *mas_connect(struct obex_session *os, int *err)
>>   		goto failed;
>>   
>>   	manager_register_session(os);
>> +	mas->path = NULL;
> 
> Maybe the problem here is that the change in commit bb160515185e
> ("obexd: Emit Size property of transfer after open()") is not right?
> 
> diff --git a/obexd/src/obex.c b/obexd/src/obex.c
> index a4bae857f..ed219d3e7 100644
> --- a/obexd/src/obex.c
> +++ b/obexd/src/obex.c
> @@ -779,6 +779,9 @@ int obex_put_stream_start(struct obex_session *os, const char *filename)
>                  return err;
>          }
>   
> +       if (os->size != OBJECT_SIZE_DELETE && os->size != OBJECT_SIZE_UNKNOWN)
> +               manager_emit_transfer_property(os->service_data, "Size");
> +
>          os->path = g_strdup(filename);
> 
> This casts os->service_data to obex_transfer which IIUC does not work
> for most the plugins, as it's the session struct.
> 
> Maybe plugins can emit the transfer property change in their open()
> callback, for the plugins where it makes sense?
> 
>>   
>>   	return mas;
>>   
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] Bluetooth : Update the mas session structure
  2024-06-26  6:10   ` Amisha Jain
@ 2024-06-27 16:24     ` Pauli Virtanen
  2024-06-27 17:40       ` Luiz Augusto von Dentz
  0 siblings, 1 reply; 7+ messages in thread
From: Pauli Virtanen @ 2024-06-27 16:24 UTC (permalink / raw)
  To: Amisha Jain, linux-bluetooth; +Cc: quic_mohamull, quic_hbandi, quic_anubhavg

Hi,

ke, 2024-06-26 kello 11:40 +0530, Amisha Jain kirjoitti:
> Hi,
> 
> Correct! The problem here is manager_emit_transfer_property() is 
> expecting the structure of type 'obex_transfer' and we are passing the 
> structure of session type which will be type mismatch and inappropriate 
> values will be populated in further calls in code. Hence property "Size" 
> will never emit on console(obexctl) as it is not set properly and might 
> cause crash/disconnection.
> 
>  > diff --git a/obexd/src/obex.c b/obexd/src/obex.c
>  > index a4bae857f..ed219d3e7 100644
>  > --- a/obexd/src/obex.c
>  > +++ b/obexd/src/obex.c
>  > @@ -779,6 +779,9 @@ int obex_put_stream_start(struct obex_session 
> *os, const char *filename)
>  >                  return err;
>  >          }
>  >
>  > +       if (os->size != OBJECT_SIZE_DELETE && os->size != 
> OBJECT_SIZE_UNKNOWN)
>  > +               manager_emit_transfer_property(os->service_data, "Size");
>  > +
>  >          os->path = g_strdup(filename);
>  >
> 
> 
> One way to resolve this issue is to add the additional field in 
> 'mas_session' so it can cast to struct 'obex_transfer'. We are adding 
> new field 'char *path' as only transfer->path will be invoked and passed 
> further.

Yes, thank you for clarification.

Although I'm not BlueZ maintainer, I think relying on type punning
mas_session and obex_transfer by adding fields so that their memory
representation is partly the same is not very good practice. Other
plugins like ftp.c seem also have similar issue.

One could consider some ways to avoid doing this, e.g. move the

        if (os->size != OBJECT_SIZE_DELETE && os->size != OBJECT_SIZE_UNKNOWN)
                manager_emit_transfer_property(os->service_data, "Size");

after each call to obex_put_stream_start in plugins/*.c
client/mns.c. For plugins where there's no transfer, it maybe can be
omitted.

> 
> void manager_emit_transfer_property(struct obex_transfer *transfer,
> 								char *name)
> {
> 	if (!transfer->path)
> 		return;
> 
> 	g_dbus_emit_property_changed(connection, transfer->path,
> 					TRANSFER_INTERFACE, name);
> }
> 
>  >> Signed-off-by: Amisha Jain <quic_amisjain@quicinc.com>
>  >> ---
>  >>   obexd/plugins/mas.c | 4 +++-
>  >>   1 file changed, 3 insertions(+), 1 deletion(-)
>  >>
>  >> diff --git a/obexd/plugins/mas.c b/obexd/plugins/mas.c
>  >> index 10b972d65..71bf12ad3 100644
>  >> --- a/obexd/plugins/mas.c
>  >> +++ b/obexd/plugins/mas.c
>  >> @@ -51,6 +51,8 @@
>  >>   #define ML_BODY_END "</MAP-msg-listing>"
>  >>
>  >>   struct mas_session {
>  >> +	uint8_t notification_status;
>  >> +	char *path;
>  >>   	struct mas_request *request;
>  >>   	void *backend_data;
>  >>   	gboolean finished;
>  >> @@ -59,7 +61,6 @@ struct mas_session {
>  >>   	GObexApparam *inparams;
>  >>   	GObexApparam *outparams;
>  >>   	gboolean ap_sent;
>  >> -	uint8_t notification_status;
>  >>   };
>  >>
>  >>   static const uint8_t MAS_TARGET[TARGET_SIZE] = {
>  >> @@ -125,6 +126,7 @@ static void *mas_connect(struct obex_session 
> *os, int *err)
>  >>   		goto failed;
>  >>
>  >>   	manager_register_session(os);
>  >> +	mas->path = NULL;
>  >
> 
> There is no transfer already registered for mas during connection, so 
> setting the path to NULL.
> 
> On 6/25/2024 9:09 PM, Pauli Virtanen wrote:
> > Hi,
> > 
> > ti, 2024-06-25 kello 12:05 +0530, Amisha Jain kirjoitti:
> > > Update the 'mas_session' structure such that
> > > manager_emit_transfer_property(os->service_data, "Size")
> > > will get the proper structure in arguments as
> > > expected like structure 'obex_transfer' and transfer->path
> > > won't be populated with inappropriate value.
> > > 
> > > As there is no new transfer registered during mas connect,
> > > hence setting the path to NULL to avoid invoking the
> > > g_dbus_emit_property_changed() property.
> > > 
> > > Signed-off-by: Amisha Jain <quic_amisjain@quicinc.com>
> > > ---
> > >   obexd/plugins/mas.c | 4 +++-
> > >   1 file changed, 3 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/obexd/plugins/mas.c b/obexd/plugins/mas.c
> > > index 10b972d65..71bf12ad3 100644
> > > --- a/obexd/plugins/mas.c
> > > +++ b/obexd/plugins/mas.c
> > > @@ -51,6 +51,8 @@
> > >   #define ML_BODY_END "</MAP-msg-listing>"
> > >   
> > >   struct mas_session {
> > > +	uint8_t notification_status;
> > > +	char *path;
> > >   	struct mas_request *request;
> > >   	void *backend_data;
> > >   	gboolean finished;
> > > @@ -59,7 +61,6 @@ struct mas_session {
> > >   	GObexApparam *inparams;
> > >   	GObexApparam *outparams;
> > >   	gboolean ap_sent;
> > > -	uint8_t notification_status;
> > >   };
> > >   
> > >   static const uint8_t MAS_TARGET[TARGET_SIZE] = {
> > > @@ -125,6 +126,7 @@ static void *mas_connect(struct obex_session *os, int *err)
> > >   		goto failed;
> > >   
> > >   	manager_register_session(os);
> > > +	mas->path = NULL;
> > 
> > Maybe the problem here is that the change in commit bb160515185e
> > ("obexd: Emit Size property of transfer after open()") is not right?
> > 
> > diff --git a/obexd/src/obex.c b/obexd/src/obex.c
> > index a4bae857f..ed219d3e7 100644
> > --- a/obexd/src/obex.c
> > +++ b/obexd/src/obex.c
> > @@ -779,6 +779,9 @@ int obex_put_stream_start(struct obex_session *os, const char *filename)
> >                  return err;
> >          }
> >   
> > +       if (os->size != OBJECT_SIZE_DELETE && os->size != OBJECT_SIZE_UNKNOWN)
> > +               manager_emit_transfer_property(os->service_data, "Size");
> > +
> >          os->path = g_strdup(filename);
> > 
> > This casts os->service_data to obex_transfer which IIUC does not work
> > for most the plugins, as it's the session struct.
> > 
> > Maybe plugins can emit the transfer property change in their open()
> > callback, for the plugins where it makes sense?
> > 
> > >   
> > >   	return mas;
> > >   
> > 

-- 
Pauli Virtanen

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] Bluetooth : Update the mas session structure
  2024-06-27 16:24     ` Pauli Virtanen
@ 2024-06-27 17:40       ` Luiz Augusto von Dentz
  0 siblings, 0 replies; 7+ messages in thread
From: Luiz Augusto von Dentz @ 2024-06-27 17:40 UTC (permalink / raw)
  To: Pauli Virtanen
  Cc: Amisha Jain, linux-bluetooth, quic_mohamull, quic_hbandi,
	quic_anubhavg

Hi,

On Thu, Jun 27, 2024 at 12:24 PM Pauli Virtanen <pav@iki.fi> wrote:
>
> Hi,
>
> ke, 2024-06-26 kello 11:40 +0530, Amisha Jain kirjoitti:
> > Hi,
> >
> > Correct! The problem here is manager_emit_transfer_property() is
> > expecting the structure of type 'obex_transfer' and we are passing the
> > structure of session type which will be type mismatch and inappropriate
> > values will be populated in further calls in code. Hence property "Size"
> > will never emit on console(obexctl) as it is not set properly and might
> > cause crash/disconnection.
> >
> >  > diff --git a/obexd/src/obex.c b/obexd/src/obex.c
> >  > index a4bae857f..ed219d3e7 100644
> >  > --- a/obexd/src/obex.c
> >  > +++ b/obexd/src/obex.c
> >  > @@ -779,6 +779,9 @@ int obex_put_stream_start(struct obex_session
> > *os, const char *filename)
> >  >                  return err;
> >  >          }
> >  >
> >  > +       if (os->size != OBJECT_SIZE_DELETE && os->size !=
> > OBJECT_SIZE_UNKNOWN)
> >  > +               manager_emit_transfer_property(os->service_data, "Size");
> >  > +
> >  >          os->path = g_strdup(filename);
> >  >
> >
> >
> > One way to resolve this issue is to add the additional field in
> > 'mas_session' so it can cast to struct 'obex_transfer'. We are adding
> > new field 'char *path' as only transfer->path will be invoked and passed
> > further.
>
> Yes, thank you for clarification.
>
> Although I'm not BlueZ maintainer, I think relying on type punning
> mas_session and obex_transfer by adding fields so that their memory
> representation is partly the same is not very good practice. Other
> plugins like ftp.c seem also have similar issue.

Yeah, it clearly was not intended to be that way.

> One could consider some ways to avoid doing this, e.g. move the
>
>         if (os->size != OBJECT_SIZE_DELETE && os->size != OBJECT_SIZE_UNKNOWN)
>                 manager_emit_transfer_property(os->service_data, "Size");
>
> after each call to obex_put_stream_start in plugins/*.c
> client/mns.c. For plugins where there's no transfer, it maybe can be
> omitted.

I think this was intended to be called directly by the plugins
otherwise we need to always create transfer objects for each object
being transferred and that be set as os->service_data which obviously
isn't the case here.

> >
> > void manager_emit_transfer_property(struct obex_transfer *transfer,
> >                                                               char *name)
> > {
> >       if (!transfer->path)
> >               return;
> >
> >       g_dbus_emit_property_changed(connection, transfer->path,
> >                                       TRANSFER_INTERFACE, name);
> > }
> >
> >  >> Signed-off-by: Amisha Jain <quic_amisjain@quicinc.com>
> >  >> ---
> >  >>   obexd/plugins/mas.c | 4 +++-
> >  >>   1 file changed, 3 insertions(+), 1 deletion(-)
> >  >>
> >  >> diff --git a/obexd/plugins/mas.c b/obexd/plugins/mas.c
> >  >> index 10b972d65..71bf12ad3 100644
> >  >> --- a/obexd/plugins/mas.c
> >  >> +++ b/obexd/plugins/mas.c
> >  >> @@ -51,6 +51,8 @@
> >  >>   #define ML_BODY_END "</MAP-msg-listing>"
> >  >>
> >  >>   struct mas_session {
> >  >> + uint8_t notification_status;
> >  >> + char *path;
> >  >>           struct mas_request *request;
> >  >>           void *backend_data;
> >  >>           gboolean finished;
> >  >> @@ -59,7 +61,6 @@ struct mas_session {
> >  >>           GObexApparam *inparams;
> >  >>           GObexApparam *outparams;
> >  >>           gboolean ap_sent;
> >  >> - uint8_t notification_status;
> >  >>   };
> >  >>
> >  >>   static const uint8_t MAS_TARGET[TARGET_SIZE] = {
> >  >> @@ -125,6 +126,7 @@ static void *mas_connect(struct obex_session
> > *os, int *err)
> >  >>                   goto failed;
> >  >>
> >  >>           manager_register_session(os);
> >  >> + mas->path = NULL;
> >  >
> >
> > There is no transfer already registered for mas during connection, so
> > setting the path to NULL.
> >
> > On 6/25/2024 9:09 PM, Pauli Virtanen wrote:
> > > Hi,
> > >
> > > ti, 2024-06-25 kello 12:05 +0530, Amisha Jain kirjoitti:
> > > > Update the 'mas_session' structure such that
> > > > manager_emit_transfer_property(os->service_data, "Size")
> > > > will get the proper structure in arguments as
> > > > expected like structure 'obex_transfer' and transfer->path
> > > > won't be populated with inappropriate value.
> > > >
> > > > As there is no new transfer registered during mas connect,
> > > > hence setting the path to NULL to avoid invoking the
> > > > g_dbus_emit_property_changed() property.
> > > >
> > > > Signed-off-by: Amisha Jain <quic_amisjain@quicinc.com>
> > > > ---
> > > >   obexd/plugins/mas.c | 4 +++-
> > > >   1 file changed, 3 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/obexd/plugins/mas.c b/obexd/plugins/mas.c
> > > > index 10b972d65..71bf12ad3 100644
> > > > --- a/obexd/plugins/mas.c
> > > > +++ b/obexd/plugins/mas.c
> > > > @@ -51,6 +51,8 @@
> > > >   #define ML_BODY_END "</MAP-msg-listing>"
> > > >
> > > >   struct mas_session {
> > > > + uint8_t notification_status;
> > > > + char *path;
> > > >           struct mas_request *request;
> > > >           void *backend_data;
> > > >           gboolean finished;
> > > > @@ -59,7 +61,6 @@ struct mas_session {
> > > >           GObexApparam *inparams;
> > > >           GObexApparam *outparams;
> > > >           gboolean ap_sent;
> > > > - uint8_t notification_status;
> > > >   };
> > > >
> > > >   static const uint8_t MAS_TARGET[TARGET_SIZE] = {
> > > > @@ -125,6 +126,7 @@ static void *mas_connect(struct obex_session *os, int *err)
> > > >                   goto failed;
> > > >
> > > >           manager_register_session(os);
> > > > + mas->path = NULL;
> > >
> > > Maybe the problem here is that the change in commit bb160515185e
> > > ("obexd: Emit Size property of transfer after open()") is not right?
> > >
> > > diff --git a/obexd/src/obex.c b/obexd/src/obex.c
> > > index a4bae857f..ed219d3e7 100644
> > > --- a/obexd/src/obex.c
> > > +++ b/obexd/src/obex.c
> > > @@ -779,6 +779,9 @@ int obex_put_stream_start(struct obex_session *os, const char *filename)
> > >                  return err;
> > >          }
> > >
> > > +       if (os->size != OBJECT_SIZE_DELETE && os->size != OBJECT_SIZE_UNKNOWN)
> > > +               manager_emit_transfer_property(os->service_data, "Size");
> > > +
> > >          os->path = g_strdup(filename);
> > >
> > > This casts os->service_data to obex_transfer which IIUC does not work
> > > for most the plugins, as it's the session struct.
> > >
> > > Maybe plugins can emit the transfer property change in their open()
> > > callback, for the plugins where it makes sense?
> > >
> > > >
> > > >           return mas;
> > > >
> > >
>
> --
> Pauli Virtanen
>


-- 
Luiz Augusto von Dentz

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2024-06-27 17:41 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-06-25  6:35 [PATCH v2] Bluetooth : Update the mas session structure Amisha Jain
2024-06-25  8:08 ` [v2] " bluez.test.bot
2024-06-25 13:49 ` [PATCH v2] " Luiz Augusto von Dentz
2024-06-25 15:39 ` Pauli Virtanen
2024-06-26  6:10   ` Amisha Jain
2024-06-27 16:24     ` Pauli Virtanen
2024-06-27 17:40       ` Luiz Augusto von Dentz

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox