Hi Tudor,
some general comments below
On 08/29/2012 03:05 AM, Tudor Marcu wrote:
This patch set introduces a new CLI for Connman to use with the current tree.
The new files in the client directory compile under a single executable
called "cmn", which provides a larger range of functions. It consists of
patches that add the new client and documentation, remove the old client,
and update the Makefile.am and other associated files.
The first 6 patches add the files containing the functions used in the
CLI, and the main CLI file and the files for interactive mode. The seventh
patch includes a new man page for cmn, and changes to TODO, AUTHORS, and
.gitignore. The last patch removes the old client and updates the Makefile.am
to compile the client and install the man page.
- git am complains about whitespace errors (blank lines at eof), just
check that you do not have extra newlines at the end of the files. Use
checkpatch.pl script from kernel sources to check the patches before
sending.
- when generating patch please do something like this
git format-patch -o patches --cover-letter \
--subject-prefix="PATCH vXX" HEAD~YY
where XX is the patchset version (your next one is 4)
And the patch sending should be done like this
git send-email patches/0*.patch
and press enter when asked what what is the message-id, this way you
will get nicely threaded mails where individual patches are under the
cover letter mail
- client still uses dbus-glib (includes dbus-glib-lowlevel.h) but does
not require that in configure, why do we need dbus-glib anyway? Connmand
works just fine without it. Something like this could be used instead
diff --git a/Makefile.am b/Makefile.am
index fee2e7a..68c0b96 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -146,7 +146,7 @@ client_cmn_SOURCES = $(gdbus_sources) src/log.c
include/log.h src/dbus.c \
client/interactive.h client/monitor.c \
client/monitor.h client/main.c
-client_cmn_LDADD = @DBUS_LIBS@ @GLIB_LIBS@ -ldbus-glib-1 -lreadline -ldl
+client_cmn_LDADD = @DBUS_LIBS@ @GLIB_LIBS@ -lreadline -ldl
endif
if WISPR
diff --git a/client/main.c b/client/main.c
index 45808c6..38aa4b8 100644
--- a/client/main.c
+++ b/client/main.c
@@ -29,7 +29,7 @@
#include <glib.h>
#include <dbus/dbus.h>
-#include <dbus/dbus-glib-lowlevel.h>
+#include <gdbus.h>
#include "client/data_manager.h"
#include "client/services.h"
@@ -110,7 +110,12 @@ static int monitor_connman(DBusConnection
*connection, char *interface,
DBusError err;
dbus_error_init(&err);
- dbus_connection_setup_with_g_main(connection, NULL);
+ g_dbus_setup_bus(DBUS_BUS_SYSTEM, NULL, &err);
+ if (dbus_error_is_set(&err)) {
+ fprintf(stderr, "Bus setup error: %s\n", err.message);
+ return -1;
+ }
+
dbus_bus_add_match(connection, rule, &err);
if (dbus_error_is_set(&err)) {
@@ -342,7 +347,6 @@ int main(int argc, char *argv[])
{0, 0, 0, 0}
};
- g_type_init();
main_loop = g_main_loop_new(NULL, TRUE);
dbus_error_init(&err);
diff --git a/client/monitor.c b/client/monitor.c
index 2cb5f76..757edf1 100644
--- a/client/monitor.c
+++ b/client/monitor.c
@@ -29,7 +29,6 @@
#include <glib.h>
#include <dbus/dbus.h>
-#include <dbus/dbus-glib-lowlevel.h>
#include "client/monitor.h"
#include "client/services.h"
- "cmn services" shows "* ARO foobar ....". The 'O'nline or 'R'eady
state chars are mutually exclusive so it is either R or O, not both at
the same time.
- "cmn tech" is a bit weird command, should we name it to "cmn
technology" and allow shorter version also. This is also true for other
commands meaning that we should match shorter command names also, like
"cmn conf" would match "cmn config", but "cmn con" would give error
because there are two options now "cmn connect" and "cmn config". So
work the same way as ip command from iproute package where you can say
"ip r g 1.2.3.4" instead of "ip route get 1.2.3.4". This is not top
level priority but nice to have.
- in interactive mode, the ctrl-d should exit the mode like in normal shell
- "cmn scan foobar" returns 0 even thou there was an error
- "cmn config foobar" returns 255 for errors but for example "cmn config
foobar --proxy manual servers 1.2.3.4 excludes 2.3.4.5" says "foobar is
not a valid service" but returns still 0. So as a general rule, if the
command fails, we need to return != 0 value.
- "cmn services --properties ethernet_001122334455_cable" returns the
properties like this
[Nameservers]
1.2.3.4 5.6.7.8 NULL
do we need the NULL here (especially at the end of the array)? Also
if the property is empty, we can just print empty line instead of lonely
NULL. This works ok for dicts which do not print any NULLs if the dict
is empty.
Cheers,
Jukka
_______________________________________________
connman mailing list
connman@connman.net
http://lists.connman.net/listinfo/connman
