On Thu, 2010-07-29 at 15:17 -0700, Denis Kenzior wrote:
> Hi Inaky,
>
> On 07/29/2010 04:47 PM, Inaky Perez-Gonzalez wrote:
> > On Tue, 2010-07-27 at 10:08 -0700, Denis Kenzior wrote:
> >> Hi Inaky,
> >>
> >>> +/* + * Encapsulate information needed to export an SMS message
> >>> over D-Bus. + * + * @dbus_path: path of the object in the D-Bus
> >>> + * @dbus_msg: message this originated at + */ +struct
> >>> sms_msg_dbus_data { + char *dbus_path; + DBusMessage *dbus_msg;
> >>> +}; +
> >>
> >> I don't really see a point for this structure. When you send an
> >> SMS, assuming the arguments and format were valid, you return
> >> straight away. Thus SendMessage should be marked as not being
> >> ASYNC and storing of DBusMessage is unnecessary. Rest can be
> >> handled by txq_submit and the dbus path can be easily generated
> >> based on the UUID.
> >
> > First it serves as transitional storage -- later in the commit
> > sequences the dbus_msg is removed as it is, as you say, unneeded
> > once all the code is made sync. If at the end only one member is
> > left (dbus_msg)
>
> Don't get me wrong, if a structure is necessary in the end, so be it.
> However, during the review I only see a structure being introduced that
> is totally unnecessary. You can simply pass dbus_path around as
> userdata.
Agreed -- at the end, once everything is cleaned up only dbus_path is
necessary, and I agree it could be passed instead of having the struct.
However, for all the intermediate commits not to break (and thus break
bisectability), having that there is a must.
However, with so much reorganization in the code that has happened
during this review process, it is probably possible to do with out it.
I'll re-audit all that to see how it can be killed.
> If/when this structure does become necessary, feel free to
> introduce it.
>
> >
> > In our discussions to plan this stuff you said you wanted the D-Bus
> > unique name to contain the UUID and the number of PDUs
> > (/modemX/UUID_PDUs). That means I need to access the SMS object if I
> > want to generate the name on the fly. I need a backpointer for
> > that.
> >
> > Even if we remove the _PDUs requirement:
>
> There is no such requirement. If I gave you this idea, then I must have
> not been clear enough. I meant that PDUs would be used to generate the
> UUID.
That simplifies things further, thanks.
_______________________________________________
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
