Hi Denis:
On 01/13/2011 10:20 AM, ext Denis Kenzior wrote:
Hi Faiyaz,
+static const char *message_state_to_string(enum message_state s)
+{
+ switch (s) {
+ case MESSAGE_STATE_PENDING:
+ return "pending";
+ case MESSAGE_STATE_SENT:
+ return "sent";
+ case MESSAGE_STATE_FAILED:
+ return "failed";
+ }
+
+ return "invalid";
As mentioned previously, don't return "invalid" here.
Will remove "invalid"
<snip>
+gboolean ofono_message_dbus_register(const char *atompath, struct message *m)
+{
+ DBusConnection *conn = ofono_dbus_get_connection();
+ const char *path;
+
+ if ((atompath == NULL) || (m == NULL))
+ return FALSE;
Please get rid of these parentheses, they're not needed. Also, in
general we prefer not to use null checking in the core. The earlier we
crash the easier it is to find the bugs.
will fix all the occurrences.
+
+ path = ofono_message_path_from_uuid(atompath,&m->uuid);
+
+ if (!g_dbus_register_interface(conn, path, OFONO_MESSAGE_INTERFACE,
+ message_methods, message_signals,
+ NULL, m, message_destroy)) {
+ ofono_error("Could not register Message %s", path);
+ message_destroy(m);
+
+ return FALSE;
+ }
+
+ return TRUE;
+}
+
+void ofono_message_dbus_unregister(const char *atompath, struct message *m)
+{
+ DBusConnection *conn = ofono_dbus_get_connection();
+ const char *path;
+
+ if ((atompath == NULL) || (m == NULL))
+ return;
Same comment as above about the parentheses.
+
+ path = ofono_message_path_from_uuid(atompath,&m->uuid);
+
+ g_dbus_unregister_interface(conn, path, OFONO_MESSAGE_INTERFACE);
+
+ return;
+}
+
+const struct ofono_uuid *ofono_message_get_uuid(const struct message *m)
+{
+ if (m == NULL)
+ return NULL;
+
+ return&m->uuid;
+}
+
+void ofono_message_set_state(const char *atompath,
+ struct message *m, enum message_state new_state)
+{
+ DBusConnection *conn = ofono_dbus_get_connection();
+ const char *state;
+ const char *path;
+
+ if ((atompath == NULL) || (m == NULL))
+ return;
+
And again the comment about parentheses.
+ if (m->state == new_state)
+ return;
+
+ path = ofono_message_path_from_uuid(atompath,&m->uuid);
+
+ m->state = new_state;
+ state = message_state_to_string(m->state);
+
+ ofono_dbus_signal_property_changed(conn, path, OFONO_MESSAGE_INTERFACE,
+ "State", DBUS_TYPE_STRING,
+ &state);
+}
+
+void ofono_message_append_properties(struct message *m, DBusMessageIter *dict)
+{
+ const char *state;
+
+ if (m == NULL)
+ return;
+
+ state = message_state_to_string(m->state);
+ ofono_dbus_dict_append(dict, "State", DBUS_TYPE_STRING,&state);
+}
+
+void ofono_emit_message_added(const char *atompath, const char *interface,
+ struct message *m)
+{
+ DBusMessage *signal;
+ DBusMessageIter iter;
+ DBusMessageIter dict;
+ const char *path;
+
+ if ((atompath == NULL) || (m == NULL))
+ return;
+
And here
+ signal = dbus_message_new_signal(atompath, interface, "MessageAdded");
+ if (signal == NULL)
+ return;
+
+ path = ofono_message_path_from_uuid(atompath,&m->uuid);
+
+ dbus_message_iter_init_append(signal,&iter);
+
+ dbus_message_iter_append_basic(&iter, DBUS_TYPE_OBJECT_PATH,&path);
+
+ dbus_message_iter_open_container(&iter, DBUS_TYPE_ARRAY,
+ OFONO_PROPERTIES_ARRAY_SIGNATURE,
+ &dict);
+ ofono_message_append_properties(m,&dict);
+ dbus_message_iter_close_container(&iter,&dict);
+
+ g_dbus_send_message(ofono_dbus_get_connection(), signal);
+}
+
+void ofono_emit_message_removed(const char *atompath, const char *interface,
+ struct message *m)
+{
+ DBusConnection *conn = ofono_dbus_get_connection();
+ const char *path;
+
+ if ((atompath == NULL) || (m == NULL))
+ return;
+
And here ;)
+ path = ofono_message_path_from_uuid(atompath,&m->uuid);
+
+ g_dbus_emit_signal(conn, atompath, interface,
+ "MessageRemoved", DBUS_TYPE_OBJECT_PATH,&path,
+ DBUS_TYPE_INVALID);
+}
+
+const char *ofono_message_path_from_uuid(const char *atompath,
+ const struct ofono_uuid *uuid)
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
+{
+ static char path[256];
+
+ snprintf(path, sizeof(path), "%s/message_%s", atompath,
+ ofono_uuid_to_str(uuid));
+
+ return path;
+}
+
+void *ofono_message_get_data(struct message *m)
+{
+ return m->data;
+}
<snip>
@@ -1904,15 +1714,16 @@ int __ofono_sms_txq_submit(struct ofono_sms *sms,
GSList *list,
return -ENOMEM;
if (flags& OFONO_SMS_SUBMIT_FLAG_EXPOSE_DBUS) {
- m = message_create(&entry->uuid);
+ m = ofono_message_create(&entry->uuid, entry);
if (m == NULL)
goto err;
- 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.
goto err;
- g_hash_table_insert(sms->messages,&m->uuid, m);
- m->entry = entry;
+ g_hash_table_insert(sms->messages,&entry->uuid, m);
}
if (list->next != NULL) {
@@ -1934,7 +1745,9 @@ int __ofono_sms_txq_submit(struct ofono_sms *sms, GSList
*list,
cb(sms,&entry->uuid, data);
if (m&& (flags& OFONO_SMS_SUBMIT_FLAG_EXPOSE_DBUS))
- emit_message_added(sms, m);
+ ofono_emit_message_added(__ofono_atom_get_path(sms->atom),
+ OFONO_MESSAGE_MANAGER_INTERFACE,
+ m);
return 0;
@@ -1951,15 +1764,18 @@ int __ofono_sms_txq_set_submit_notify(struct ofono_sms
*sms,
ofono_destroy_func destroy)
{
struct message *m;
+ struct tx_queue_entry *entry;
m = g_hash_table_lookup(sms->messages, uuid);
if (m == NULL)
return -ENOENT;
- if (m->entry == NULL)
+ entry = (struct tx_queue_entry *)ofono_message_get_data(m);
Space after the cast, rule M6
Will fix it.
+ if (entry == NULL)
return -ENOTSUP;
- tx_queue_entry_set_submit_notify(m->entry, cb, data, destroy);
+ tx_queue_entry_set_submit_notify(entry, cb, data, destroy);
return 0;
}
+
What is this empty line for?
will fix it.
Regards,
-Denis
_______________________________________________
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
