Re: [PATCH v2] Bluetooth : Update the mas session structure

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

On Thu, Jun 27, 2024 at 12:24 PM Pauli Virtanen <pav@xxxxxx> 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@xxxxxxxxxxx>
> >  >> ---
> >  >>   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@xxxxxxxxxxx>
> > > > ---
> > > >   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





[Index of Archives]     [Bluez Devel]     [Linux Wireless Networking]     [Linux Wireless Personal Area Networking]     [Linux ATH6KL]     [Linux USB Devel]     [Linux Media Drivers]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Big List of Linux Books]

  Powered by Linux