Discussion:
[PATCH] Set exit value in connmanctl
(too old to reply)
Mikko Tuumanen
2015-10-27 07:58:16 UTC
Permalink
---
client/agent.c | 2 +-
client/commands.c | 27 +++++++++++++++++++++------
client/dbus_helpers.c | 5 +++--
client/input.c | 6 ++++--
client/input.h | 2 +-
5 files changed, 30 insertions(+), 12 deletions(-)

diff --git a/client/agent.c b/client/agent.c
index d020889..b87d5bb 100644
--- a/client/agent.c
+++ b/client/agent.c
@@ -235,7 +235,7 @@ static DBusMessage *agent_release(DBusConnection *connection,
"VPNd\n");

if (__connmanctl_is_interactive() == false)
- __connmanctl_quit();
+ __connmanctl_quit(0);

return dbus_message_new_method_return(message);
}
diff --git a/client/commands.c b/client/commands.c
index 3bfdfd7..ee56d46 100644
--- a/client/commands.c
+++ b/client/commands.c
@@ -153,14 +153,21 @@ static int enable_return(DBusMessageIter *iter, const char *error,
else
str = tech;

- if (!error)
+ int ret;
+ if (!error) {
+ ret = 0;
fprintf(stdout, "Enabled %s\n", str);
- else
+ } else {
+ if (!strncmp("Already ",error,8))
+ ret = -2;
+ else
+ ret = -1;
fprintf(stderr, "Error %s: %s\n", str, error);
+ }

g_free(user_data);

- return 0;
+ return ret;
}

static int cmd_enable(char *args[], int num, struct connman_option *options)
@@ -202,14 +209,22 @@ static int disable_return(DBusMessageIter *iter, const char *error,
else
str = tech;

- if (!error)
+ int ret;
+ if (!error) {
+ ret = 0;
fprintf(stdout, "Disabled %s\n", str);
- else
+ } else {
+ if (!strncmp("Already ", error, 8))
+ ret = -2;
+ else
+ ret = -1;
+
fprintf(stderr, "Error %s: %s\n", str, error);
+ }

g_free(user_data);

- return 0;
+ return ret;
}

static int cmd_disable(char *args[], int num, struct connman_option *options)
diff --git a/client/dbus_helpers.c b/client/dbus_helpers.c
index 9c4cfee..fe55abf 100644
--- a/client/dbus_helpers.c
+++ b/client/dbus_helpers.c
@@ -141,6 +141,7 @@ static void dbus_method_reply(DBusPendingCall *call, void *user_data)
int res = 0;
DBusMessage *reply;
DBusMessageIter iter;
+ int exit_value = 0;

__connmanctl_save_rl();

@@ -152,7 +153,7 @@ static void dbus_method_reply(DBusPendingCall *call, void *user_data)
dbus_error_init(&err);
dbus_set_error_from_message(&err, reply);

- callback->cb(NULL, err.message, callback->user_data);
+ exit_value = callback->cb(NULL, err.message, callback->user_data);

dbus_error_free(&err);
goto end;
@@ -164,7 +165,7 @@ static void dbus_method_reply(DBusPendingCall *call, void *user_data)
end:
__connmanctl_redraw_rl();
if (__connmanctl_is_interactive() == false && res != -EINPROGRESS)
- __connmanctl_quit();
+ __connmanctl_quit(exit_value);

g_free(callback);
dbus_message_unref(reply);
diff --git a/client/input.c b/client/input.c
index e59df8a..f310d15 100644
--- a/client/input.c
+++ b/client/input.c
@@ -42,9 +42,11 @@ static bool interactive = false;
static bool save_input;
static char *saved_line;
static int saved_point;
+static int exit_status = 0;

-void __connmanctl_quit(void)
+void __connmanctl_quit(int status)
{
+ exit_status = status;
if (main_loop)
g_main_loop_quit(main_loop);
}
@@ -264,7 +266,7 @@ int __connmanctl_input_init(int argc, char *argv[])
main_loop = g_main_loop_new(NULL, FALSE);
g_main_loop_run(main_loop);

- err = 0;
+ err = exit_status;
}

if (interactive) {
diff --git a/client/input.h b/client/input.h
index c7256a4..2136d13 100644
--- a/client/input.h
+++ b/client/input.h
@@ -29,7 +29,7 @@
extern "C" {
#endif

-void __connmanctl_quit(void);
+void __connmanctl_quit(int status);
bool __connmanctl_is_interactive(void);
void __connmanctl_save_rl(void);
void __connmanctl_redraw_rl(void);
--
2.1.4
Patrik Flykt
2015-10-28 08:46:40 UTC
Permalink
Hi,
Post by Mikko Tuumanen
---
client/agent.c | 2 +-
client/commands.c | 27 +++++++++++++++++++++------
client/dbus_helpers.c | 5 +++--
client/input.c | 6 ++++--
client/input.h | 2 +-
5 files changed, 30 insertions(+), 12 deletions(-)
diff --git a/client/agent.c b/client/agent.c
index d020889..b87d5bb 100644
--- a/client/agent.c
+++ b/client/agent.c
@@ -235,7 +235,7 @@ static DBusMessage *agent_release(DBusConnection *connection,
"VPNd\n");
if (__connmanctl_is_interactive() == false)
- __connmanctl_quit();
+ __connmanctl_quit(0);
return dbus_message_new_method_return(message);
}
diff --git a/client/commands.c b/client/commands.c
index 3bfdfd7..ee56d46 100644
--- a/client/commands.c
+++ b/client/commands.c
@@ -153,14 +153,21 @@ static int enable_return(DBusMessageIter *iter, const char *error,
else
str = tech;
- if (!error)
+ int ret;
int ret = 0; should be declared together with the other variables. As a
nitpick the convention has been that variables with values were declared
first, then uninitialized variables.
Post by Mikko Tuumanen
+ if (!error) {
+ ret = 0;
fprintf(stdout, "Enabled %s\n", str);
- else
+ } else {
+ if (!strncmp("Already ",error,8))
+ ret = -2;
+ else
+ ret = -1;
I don't think one can rely on the string here. It's a semi-descriptive
one that may be used for logging or other purposes, but not much else.

The code is looking for -EINPROGRESS in dbus_method_reply(), so "proper"
values from errno*.h shall be used. What if this just sets the return
value to -EOPNOTSUPP?
Post by Mikko Tuumanen
fprintf(stderr, "Error %s: %s\n", str, error);
+ }
g_free(user_data);
- return 0;
+ return ret;
}
static int cmd_enable(char *args[], int num, struct connman_option *options)
@@ -202,14 +209,22 @@ static int disable_return(DBusMessageIter *iter, const char *error,
else
str = tech;
- if (!error)
+ int ret;
+ if (!error) {
+ ret = 0;
fprintf(stdout, "Disabled %s\n", str);
- else
+ } else {
+ if (!strncmp("Already ", error, 8))
+ ret = -2;
+ else
+ ret = -1;
+
fprintf(stderr, "Error %s: %s\n", str, error);
+ }
Same here.
Post by Mikko Tuumanen
g_free(user_data);
- return 0;
+ return ret;
}
static int cmd_disable(char *args[], int num, struct connman_option *options)
diff --git a/client/dbus_helpers.c b/client/dbus_helpers.c
index 9c4cfee..fe55abf 100644
--- a/client/dbus_helpers.c
+++ b/client/dbus_helpers.c
@@ -141,6 +141,7 @@ static void dbus_method_reply(DBusPendingCall *call, void *user_data)
int res = 0;
DBusMessage *reply;
DBusMessageIter iter;
+ int exit_value = 0;
__connmanctl_save_rl();
@@ -152,7 +153,7 @@ static void dbus_method_reply(DBusPendingCall *call, void *user_data)
dbus_error_init(&err);
dbus_set_error_from_message(&err, reply);
- callback->cb(NULL, err.message, callback->user_data);
+ exit_value = callback->cb(NULL, err.message, callback->user_data);
dbus_error_free(&err);
goto end;
@@ -164,7 +165,7 @@ static void dbus_method_reply(DBusPendingCall *call, void *user_data)
__connmanctl_redraw_rl();
if (__connmanctl_is_interactive() == false && res != -EINPROGRESS)
- __connmanctl_quit();
+ __connmanctl_quit(exit_value);
g_free(callback);
dbus_message_unref(reply);
diff --git a/client/input.c b/client/input.c
index e59df8a..f310d15 100644
--- a/client/input.c
+++ b/client/input.c
@@ -42,9 +42,11 @@ static bool interactive = false;
static bool save_input;
static char *saved_line;
static int saved_point;
+static int exit_status = 0;
-void __connmanctl_quit(void)
+void __connmanctl_quit(int status)
{
+ exit_status = status;
if (main_loop)
g_main_loop_quit(main_loop);
}
@@ -264,7 +266,7 @@ int __connmanctl_input_init(int argc, char *argv[])
main_loop = g_main_loop_new(NULL, FALSE);
g_main_loop_run(main_loop);
- err = 0;
+ err = exit_status;
}
if (interactive) {
diff --git a/client/input.h b/client/input.h
index c7256a4..2136d13 100644
--- a/client/input.h
+++ b/client/input.h
@@ -29,7 +29,7 @@
extern "C" {
#endif
-void __connmanctl_quit(void);
+void __connmanctl_quit(int status);
bool __connmanctl_is_interactive(void);
void __connmanctl_save_rl(void);
void __connmanctl_redraw_rl(void);
Cheers,

Patrik
Mikko Tuumanen
2015-10-29 07:52:27 UTC
Permalink
Post by Patrik Flykt
Post by Mikko Tuumanen
+ if (!strncmp("Already ",error,8))
+ ret = -2;
+ else
+ ret = -1;
I don't think one can rely on the string here. It's a semi-descriptive
one that may be used for logging or other purposes, but not much else.
Yes. A better way would be to look at DBusError.name, but that is not
available without changing connmanctl_dbus_method_return_func_t
Post by Patrik Flykt
The code is looking for -EINPROGRESS in dbus_method_reply(), so "proper"
values from errno*.h shall be used. What if this just sets the return
value to -EOPNOTSUPP?
Ok, I'll make another patch some day.

Patrik Flykt
2015-10-28 08:49:08 UTC
Permalink
Post by Mikko Tuumanen
---
client/agent.c | 2 +-
client/commands.c | 27 +++++++++++++++++++++------
client/dbus_helpers.c | 5 +++--
client/input.c | 6 ++++--
client/input.h | 2 +-
5 files changed, 30 insertions(+), 12 deletions(-)
And a bit more description on the idea behind the change in the commit
message, if I may.

Cheers,

Patrik
Jaakko Hannikainen
2015-10-28 11:50:33 UTC
Permalink
Connmanctl returns the status of the last executed command. Update
documentation to reflect this.
---
doc/connmanctl.1.in | 6 ++++++
1 file changed, 6 insertions(+)

diff --git a/doc/connmanctl.1.in b/doc/connmanctl.1.in
index 0f891bd..07320bd 100644
--- a/doc/connmanctl.1.in
+++ b/doc/connmanctl.1.in
@@ -231,6 +231,12 @@ Listens for added or removed vpn connections.
Listens for the changes to vpn connections, for example connecting to a VPN.
.PP
.SH
+EXIT STATUS
+In single command mode, \fBconnmanctl\fR returns 0 on success and nonzero on
+failure, and in interactive mode \fBconnmanctl\fR returns the status of the last
+command. The nonzero exit codes correspond with standard errno values, see
+\fBerrno.h\fR(3).
+.SH
EXAMPLE
Listing available technologies:
.PP
--
2.6.1
Loading...