On Thu, Apr 15, 2010 at 12:59 PM, Michael Blumenkrantz
<m...@zentific.com> wrote:
> Hi,
>
> Included are three patches for e_dbus. One implements the udisks/upower
> functionality that I've previously mentioned, one shuts up compiler
> warnings in ofono by casting, and the last fixes the spacing in all of
> e_dbus to comply with efl standards.
Sorry taking so long to reply, but did take some action today:
- ofono: the signature of the function was wrong, so I did not use
your patch, but the signature was fixed and the warning is now gone.
- spacing: not applying since it is just hard to review if just
whitespace changed, so not doing it... actually most of our code have
some cases of inconsistencies, we really lack some formal indent line
that we can use. For instance, the struct declaration is different in
most of places.
- udev: a quick look makes me bit worried about memory management.
I need to look deeper, but things like e_udev_property_string_get()
should return stringshared pointer, and not even increment its
reference. That would save some allocs, refs/unrefs... and API would
be more similar to the rest of EFL. For instance in that function you
are strdup() but the e_udev_property_strlist_get() you do not
duplicate the list... so it is inconsistent! In both cases you should
return "const TYPE *". This memory management is in other places as
well, for instance:
+e_upower_get_all_properties(E_DBus_Connection *conn, const char *udi,
E_DBus_Callback_Func cb_func, void *data)
+{
+ DBusMessage *msg;
+ DBusPendingCall *ret;
+ char *arg;
+
+ if (!(arg = strdup(E_UDEV_INTERFACE))) return NULL;
+ msg = e_udev_property_call_new(udi, "GetAll");
+ dbus_message_append_args(msg, DBUS_TYPE_STRING, &arg, DBUS_TYPE_INVALID);
+ ret = e_dbus_method_call_send(conn, msg,
unmarshal_device_get_all_properties, cb_func,
free_device_get_all_properties, -1, data);
+ dbus_message_unref(msg);
+ free(arg);
ouch! Why do you strdup, then free? Just define E_UDEV_INTERFACE as a
const char *E_UDEV_INTERFACE (E_UDEV_INTERFACE[] will not work with
GCC in this case).
And here:
+ case 's':
+ prop->type = E_UDEV_PROPERTY_TYPE_STRING;
+ dbus_message_iter_get_basic(&v_iter, &(prop->val.s));
+ break;
ouch! prop->val.s will be gone when the dbus message is gone! This is
just a pointer to the message internals, you should:
dbus_message_iter_get_basic(&v_iter, &tmp_str);
prop->val.s = eina_stringshare_add(tmp_str);
Same for STRLIST, each node should be eina_stringshare_add()... then
it should be, in e_udev_property_free()
if (prop->type = E_UDEV_PROPERTY_TYPE_STRLIST)
{
const char *s;
EINA_LIST_FREE(prop->val.strlist, s)
eina_stringshare_del(s);
}
Note that eina_hash_add() will already duplicate name, as it uses
eina_hash_string_small_new().
I'd also recommend using dbus typedefs instead of "ao". It will make
code bit longer, but will help people read it.
E_Udev_Bool_Return and other boolean, I'd make boolean unsigned char
or better: Eina_Bool. Also, I'm not 100% sure, but the DBus handling
of BOOL is a bit tricker and different from what one would expect... I
never remember such things from head, but I'd either check their docs
or read E_Connman where I did noticed and fixed it :-D
All in all, e_udev seems quite clean and just after fixing those it
can get in. Keep up the good work!
BR,
--
Gustavo Sverzut Barbieri
http://profusion.mobi embedded systems
--------------------------------------
MSN: barbi...@gmail.com
Skype: gsbarbieri
Mobile: +55 (19) 9225-2202
------------------------------------------------------------------------------
Download Intel® Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs
proactively, and fine-tune applications for parallel performance.
See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev
_______________________________________________
enlightenment-devel mailing list
enlightenment-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
