Re: [PATCH 3/3] message: Code independent of protocol stack

[Denis Kenzior <denkenz@gmail.com>]

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

Hi Faiyaz,

>> Please keep this as an internal API (e.g.
>> __ofono_message_path_from_uuid) and update ofono.h appropriately.
> 
> Based on a comment in Patch 2/3, message.h and message.c are internal to
> core and to me they look more of a utility/helper functions to sms.c
> file (and in future cdma-sms.c file).  So should I even include
> "message.h" in ofono.h file. I could only rename the function to be
> message_XXX.
> 
> The sms.h still maintains the API "__ofono_sms_message_path_from_uuid"
> which is shared with "smart-messaging.c" and is defined in ofono.h
> 
> 

That would be fine as well.

>>> -        if (message_dbus_register(sms, m) == FALSE)
>>> +        if (ofono_message_dbus_register(
>>> +                    __ofono_atom_get_path(sms->atom),
>>> +                                m) == FALSE)
>>
>> This looks a bit ugly, can you find a better way to do this?  Perhaps
>> message_create should simply take an atom as well.
> I think we can go with
> 1) Pass "sms->atom", add "struct ofono_atom *atom" in the message struct
> and initialize the reference of the same in message_create. But I am not
> sure if it will be okay to pass a reference to a private data member
> from struct ofono_sms.
> struct message {
>     struct ofono_uuid uuid;
>     enum message_state state;
>     void *data;
>         struct ofono_atom *atom;
> };
> 
> 2) Pass "__ofono_atom_get_path(sms->atom)" and use malloc in
> message_create to initialize "atompath". Just use atompath through the
> code. But a general observation I had was that in ofono we used "static
> char xxx[]" instead of doing malloc. Hence did not go this route in the
> initial patch.
> 
> struct message {
>     struct ofono_uuid uuid;
>     enum message_state state;
>     void *data;
>     char *atompath;
> };
> 
> Please let me know your thoughts on the same.
> 

I don't think there's a huge difference between the two.  Use the atom
member for now and I will have a closer look when you resubmit.

Regards,
-Denis