GNOME Bugzilla – Bug 672998
Support Connman Network Manager in gUPnP Context Manager
Last modified: 2012-05-03 14:13:06 UTC
Add a new feature to support Connman in gUPnP Context. The goal is to update this feature to follow Connman new features when they will be available, like "Network Zone" or Network Type (Cellular, Wifi, ...) This context exists to support features that won't/can't be supported in the Linux Context Manager.
Created attachment 210783 [details] [review] Connman support. Patch V0 - Cover letter
Created attachment 210784 [details] [review] Connman support - Patch V0 - Initial release
In general don't use G_UNLIKELY unless you've demonstrated that it's a worthwhile optimisation.
(In reply to comment #3) > In general don't use G_UNLIKELY unless you've demonstrated that it's a > worthwhile optimisation. Ok. I will remove them all from next version. PS: I just follow the "style" from NM Context Manager
In that case it's wrong too. :)
(In reply to comment #5) > In that case it's wrong too. :) :D Doc on G_LIKELY & G_UNLIKELY is pretty.. hem... brief. Any hint on when or why to use/don't use these macro.
They inject instructions that tell the processor to make specific optimisations and manipulate the predictions. Unless you know that the changes actually make a real world difference, there isn't any gain in doing so and you can actually hurt performance by overriding the CPU.
(In reply to comment #7) > They inject instructions that tell the processor to make specific optimisations > and manipulate the predictions. Unless you know that the changes actually make > a real world difference, there isn't any gain in doing so and you can actually > hurt performance by overriding the CPU. ok. I will check all macros. Thanks.
Tests would be really awesome. We've neglected that way too long.
Created attachment 210789 [details] [review] Connman support. Patch V1 - Cover letter
Created attachment 210790 [details] [review] Connman support - Patch V1 - Initial release
(In reply to comment #9) > Tests would be really awesome. We've neglected that way too long. I'm not sure the saving time will be significant. These macros are use in loops that parse Connman services or dictionaries. Connman services is more often a very short list as dictionaries used in this code. If you are really interested, I could make some tests and post the results. But I don't think they will be amazing.
Sorry, I meant unit/functional tests. Not tests related to the G_(UN)LIKELY stuff.
Review of attachment 210790 [details] [review]: There's some weird newlines going on in gupnp-connman-manager.c ::: libgupnp/gupnp-connman-manager.h @@ +28,3 @@ + +GType +gupnp_connman_manager_get_type (void) G_GNUC_CONST; Please use G_GNUC_INTERNAL to not pollute the symbol space (and annoy the Debian packager). Yes, that's wrong in the other context managers as well. @@ +62,3 @@ + GUPnPContextManagerClass parent_class; + + /* future padding */ You probably don't need this, this is an internal class. (I know, you followed the lead) @@ +71,3 @@ + +gboolean +gupnp_connman_manager_is_available (void); Please use G_GNUC_INTERNAL (s.a.)
Created attachment 210868 [details] [review] Connman support. Patch V2 - Cover letter
Created attachment 210869 [details] [review] Connman support - Patch V2 - Initial release
Created attachment 211361 [details] [review] Connman support. Patch v3 - Cover letter v3: Fix assigment bug (== instead of =) Remove unused variable
Created attachment 211362 [details] [review] Connman support - Patch v3 - Initial release
Please do not use V3 patches. I will post a new version soon/today as soon as I finish some tests. In the move to Connman 1.0, some dbus api has changed. I've fixed the code and squash some bugs. Thanks.
Created attachment 212432 [details] [review] Connman support. Patch v4 - Cover letter v4: Use new Connman dbus API - Remove signal ServicesAdded & ServicesRemoved - Use ServicesChanged Factorize and rename some functions for consistency Call g_object_set when a context property has changed, so "notify" signal will be emitted. Always get service name, not only for wifi services. v3: Fix assigment bug (== instead of =) Remove unused variable v2: Integrate Jens comments: - Use G_GNUC_INTERNAL macro when needed. - Remove unused and unnecessary structure members. v1: Remove G_UNLIKELY & G_LIKELY macro. No need for such optimization. v0: Initial release. Add the capability to use Connman in Context Manager
Created attachment 212433 [details] [review] Connman support - Patch v4 - Initial release
This should be the last version of the patch unless you found something. Regards.
Review of attachment 212433 [details] [review]: Good log messages are not just nice but also good for history so I'll do a bit of nitpicking (bike-shedding if you may): You are not adding "Connman support in GUPNPContext" but rather "Add ConnMan based context manager". Also there is no release involved. Reader of commit history will get confused if you use the word 'release' here.
(In reply to comment #23) > Review of attachment 212433 [details] [review]: > > Good log messages are not just nice but also good for history so I'll do a bit > of nitpicking (bike-shedding if you may): You are not adding "Connman support > in GUPNPContext" but rather "Add ConnMan based context manager". Also there is > no release involved. Reader of commit history will get confused if you use the > word 'release' here. Do you want I upload new version of the patches or will you update the messages if you decide to accept the patch?
Review of attachment 212433 [details] [review]: ::: libgupnp/gupnp-connman-manager.c @@ +1,2 @@ +/* + * Copyright (C) 2012 Intel Corporation. All rights reserved. Are you certain that this code is not based on any other code? Ignore this comment if its not but please keep the original copyrights and author info otherwise. @@ +349,3 @@ + (void) g_variant_iter_init (&iter, data); + + while (g_variant_iter_loop (&iter, "(&o@a{sv})", &path, &dict)) { There is a really long block in this while leading into multiple levels of indentation. Could you please refactor this code somewhat so this function isn't that long? @@ +373,3 @@ + + if (eth != NULL) { + (void) g_variant_lookup (eth, "Interface", "s", &iface); Is this '(void)' really needed? You seem to have used it in other places as well, please remove all occurances unless I'm missing something and this is really needed? @@ +446,3 @@ + + while (g_variant_iter_next (&iter, "&o", &path)) { + remove redundant newlines like these (i-e first line of if/else/while/for/ blocks doesn't need to be empty). @@ +474,3 @@ + manager = GUPNP_CONNMAN_MANAGER (user_data); + + //~ { "ServicesChanged", "a(oa{sv})ao" }, Remove commented-out code. @@ +506,3 @@ + g_warning ("Error fetching service list: %s", error->message); + g_error_free (error); + return; "Add a newline before each return, break, throw, continue etc. if it is not the only statement in that block" from coding style doc: http://gupnp.org/coding-style @@ +660,3 @@ + object_class = G_OBJECT_CLASS (gupnp_connman_manager_parent_class); + + if (object_class->constructed != NULL) { No braces for single statement blocks. This is one multiple places in this file alone. @@ +696,3 @@ +gupnp_connman_manager_is_available (void) +{ + GDBusProxy *dbus_proxy; just one space between 'GDBusProxy' and '*dbus_proxy' and align the rest according to this. Same in a few other functions.
(In reply to comment #24) > (In reply to comment #23) > > Review of attachment 212433 [details] [review] [details]: > > > > Good log messages are not just nice but also good for history so I'll do a bit > > of nitpicking (bike-shedding if you may): You are not adding "Connman support > > in GUPNPContext" but rather "Add ConnMan based context manager". Also there is > > no release involved. Reader of commit history will get confused if you use the > > word 'release' here. > > Do you want I upload new version of the patches or will you update the messages > if you decide to accept the patch? I'm only doing the review and i'm probably not going to be the one pushing your patch in the end so you'll have to do it.
Created attachment 212937 [details] [review] Connman based Context Manager. Patch v5 - Cover letter v5: Integrate Zeeshan Ali comments Fix Copyright header Split long function Fix coding style issues v4: Use new Connman dbus API - Remove signal ServicesAdded & ServicesRemoved - Use ServicesChanged Factorize and rename some functions for consistency Call g_object_set when a context property has changed, so "notify" signal will be emitted. Always get service name, not only for wifi services. v3: Fix assigment bug (== instead of =) Remove unused variable v2: Integrate Jens Georg comments: - Use G_GNUC_INTERNAL macro when needed. - Remove unused and unnecessary structure members. v1: Remove G_UNLIKELY & G_LIKELY macro. No need for such optimization. v0: Initial implementation.
Created attachment 212938 [details] [review] Connman based Context Manager - Patch v5 - Initial implementation
Review of attachment 212938 [details] [review]: Otherwise it looks very good from non-functional POV at least. So if Jens has tested this, he can push it once you correct this tiny issues. Thanks for your patience on this BTW! ::: libgupnp/gupnp-connman-manager.c @@ +208,3 @@ + (g_strcmp0 (state_str, "ready") == 0)) + new_state = CM_SERVICE_STATE_ACTIVE; + else new_state = CM_SERVICE_STATE_INACTIVE; else block belong in a separate line @@ +743,3 @@ + g_warning ("Failed to connect to Connman: %s", error->message); + g_error_free (error); + return ret; new line before return in here too.
Ok, after some struggle (debian shipping a _really_ old version of connman) got it to work. Seems to be working.
Created attachment 213303 [details] [review] Connman based Context Manager. Patch v6 - Cover letter v6: Check Connman package version Fix coding style issues v5: Integrate Zeeshan Ali comments Fix Copyright header Split long function Fix coding style issues v4: Use new Connman dbus API - Remove signal ServicesAdded & ServicesRemoved - Use ServicesChanged Factorize and rename some functions for consistency Call g_object_set when a context property has changed, so "notify" signal will be emitted. Always get service name, not only for wifi services. v3: Fix assigment bug (== instead of =) Remove unused variable v2: Integrate Jens Georg comments: - Use G_GNUC_INTERNAL macro when needed. - Remove unused and unnecessary structure members. v1: Remove G_UNLIKELY & G_LIKELY macro. No need for such optimization. v0: Initial implementation.
Created attachment 213304 [details] [review] Connman based Context Manager - Patch v6 - Initial implementation