Hi,
On Thu, 2015-10-15 at 15:07 +1100, Craig McQueen wrote:
> ---
>
> I'm not sure if you would like this use of commands. I'm interested not
> only in _setting_ clock settings, but also in reading them conveniently.
> The precedent in connmanctl is that there is one command to configure a
> service ('config'), and a separate command to see its properties
> ('services'). So for net.connman.Clock, the analagous commands are
> 'configclock' to configure, and 'clock' to read. But 'clock' also takes
> parameters which allow individual properties to be read.
Here I'd go for the simpler option of just having a 'clock' command that
shows all Clock properties. This way it is similar to 'technologies' and
'state' commands.
> My earlier e-mail--
> "connmanctl quitting before multiple return functions called" --
> can be seen with the 'clock' command, reading multiple items. E.g.:
>
> connmanctl clock time timeupdates timezone
>
> which on my ARM embedded Linux based system, sometimes gets all three
> responses, but often only two.
>
> client/commands.c | 273
> ++++++++++++++++++++++++++++++++++++++++++++++++++
> client/dbus_helpers.c | 6 ++
> 2 files changed, 279 insertions(+)
>
> diff --git a/client/commands.c b/client/commands.c
> index 3bfdfd7..8b56965 100644
> --- a/client/commands.c
> +++ b/client/commands.c
> @@ -291,6 +291,75 @@ static int peers_list(DBusMessageIter *iter,
> return 0;
> }
>
> +static int one_object_properties(DBusMessageIter *iter,
> + const char *error, void *user_data)
> +{
> + DBusMessageIter dict;
> +
> + if (!error) {
> + dbus_message_iter_recurse(iter, &dict);
> + __connmanctl_dbus_print(&dict, "", " = ", "\n");
> +
> + fprintf(stdout, "\n");
> +
> + } else {
> + fprintf(stderr, "Error: %s\n", error);
> + }
> +
> + return 0;
> +}
> +
> +static int one_object_one_property(DBusMessageIter *iter,
> + const char *error, void *user_data)
> +{
> + char *property = user_data;
> + int arg_type;
> + DBusMessageIter dict;
> + DBusMessageIter entry;
> + DBusMessageIter iter2;
> + char *str;
> +
> + if (error) {
> + fprintf(stderr, "Error: %s\n", error);
> + return 0;
> + }
> +
> + dbus_message_iter_recurse(iter, &dict);
> + while (dbus_message_iter_get_arg_type(&dict) == DBUS_TYPE_DICT_ENTRY) {
> + dbus_message_iter_recurse(&dict, &entry);
> + arg_type = dbus_message_iter_get_arg_type(&entry);
> + if (arg_type == DBUS_TYPE_STRING) {
> + dbus_message_iter_get_basic(&entry, &str);
> + } else {
> + str = "";
> + }
> + if (g_strcmp0(str, property) == 0) {
> + dbus_message_iter_next(&entry);
> + while (dbus_message_iter_get_arg_type(&entry) ==
> DBUS_TYPE_VARIANT) {
> + dbus_message_iter_recurse(&entry, &iter2);
> + entry = iter2;
> + }
> + switch (dbus_message_iter_get_arg_type(&entry)) {
> + case DBUS_TYPE_STRUCT:
> + case DBUS_TYPE_ARRAY:
> + case DBUS_TYPE_DICT_ENTRY:
> + dbus_message_iter_recurse(&entry, &iter2);
> + __connmanctl_dbus_print(&iter2, "", "=", " ");
> + break;
> + default:
> + __connmanctl_dbus_print(&entry, "", " = ", " =
> ");
> + break;
> + }
> + break;
> + }
> + dbus_message_iter_next(&dict);
> + }
> +
> + fprintf(stdout, "\n");
> +
> + return 0;
> +}
> +
> static int object_properties(DBusMessageIter *iter,
> const char *error, void *user_data)
> {
> @@ -1188,6 +1257,189 @@ static int cmd_config(char *args[], int num, struct
> connman_option *options)
> return result;
> }
>
> +static int configclock_return(DBusMessageIter *iter, const char *error,
> + void *user_data)
> +{
> + char *property = user_data;
> +
> + if (error)
> + fprintf(stderr, "Error %s: %s\n", property, error);
> +
> + g_free(user_data);
> +
> + return 0;
> +}
> +
> +static int cmd_clock(char *args[], int num, struct connman_option *options)
> +{
> + int result = 0, res = 0, index = 1, oldindex = 0;
> + int c;
> + char *path;
> + char **opt_start;
> + struct config_append append;
> +
> + if (num == 1) {
> + return __connmanctl_dbus_method_call(connection,
> CONNMAN_SERVICE, "/",
> + "net.connman.Clock", "GetProperties",
> + one_object_properties, NULL, NULL, NULL);
> + }
> +
> + while (index < num && args[index]) {
> + c = parse_args(args[index], options);
> + opt_start = &args[index + 1];
> + append.opts = opt_start;
> + append.values = 0;
> +
> + res = 0;
> +
> + oldindex = index;
> + path = g_strdup("/");
> +
> + switch (c) {
> + case 'T':
> + res = __connmanctl_dbus_method_call(connection,
> CONNMAN_SERVICE, "/",
> + "net.connman.Clock", "GetProperties",
> + one_object_one_property, "Time", NULL,
> NULL);
> + break;
> +
> + case 'u':
> + res = __connmanctl_dbus_method_call(connection,
> CONNMAN_SERVICE, "/",
> + "net.connman.Clock", "GetProperties",
> + one_object_one_property, "TimeUpdates",
> NULL, NULL);
> + break;
> +
> + case 'Z':
> + res = __connmanctl_dbus_method_call(connection,
> CONNMAN_SERVICE, "/",
> + "net.connman.Clock", "GetProperties",
> + one_object_one_property,
> "TimezoneUpdates", NULL, NULL);
> + break;
> +
> + case 'z':
> + res = __connmanctl_dbus_method_call(connection,
> CONNMAN_SERVICE, "/",
> + "net.connman.Clock", "GetProperties",
> + one_object_one_property, "Timezone",
> NULL, NULL);
> + break;
> +
> + case 't':
> + res = __connmanctl_dbus_method_call(connection,
> CONNMAN_SERVICE, "/",
> + "net.connman.Clock", "GetProperties",
> + one_object_one_property, "Timeservers",
> NULL, NULL);
> + break;
> +
> + default:
> + res = -EINVAL;
> + break;
> + }
> +
> + g_free(path);
> +
> + if (res < 0) {
> + if (res == -EINPROGRESS)
> + result = -EINPROGRESS;
> + else
> + printf("Error '%s': %s\n", args[oldindex],
> + strerror(-res));
> + } else
> + index += res;
> +
> + index++;
> + }
> +
> + return result;
> +}
> +
> +static int cmd_configclock(char *args[], int num, struct connman_option
> *options)
> +{
> + int result = 0, res = 0, index = 1, oldindex = 0;
> + int c;
> + char *path;
> + char **opt_start;
> + struct config_append append;
> +
> + while (index < num && args[index]) {
> + c = parse_args(args[index], options);
> + opt_start = &args[index + 1];
> + append.opts = opt_start;
> + append.values = 0;
> +
> + res = 0;
> +
> + oldindex = index;
> + path = g_strdup("/");
> +
> + switch (c) {
> + case 'u':
> + if (strcasecmp(*opt_start, "auto") != 0 &&
> strcasecmp(*opt_start, "manual") != 0) {
Try to keep lines < 80 chars in lenght. The convetion has been to just
use '(condition)' instead of comparing not equal to zero.
> + res = -EINVAL;
> + }
> + else {
else on the same line as }
> + res = __connmanctl_dbus_set_property(connection,
> + path, "net.connman.Clock",
> + configclock_return,
> + g_strdup("TimeUpdates"),
> + "TimeUpdates",
> + DBUS_TYPE_STRING, opt_start);
> + }
> + index++;
> + break;
> +
> + case 'Z':
> + if (strcasecmp(*opt_start, "auto") != 0 &&
> strcasecmp(*opt_start, "manual") != 0) {
> + res = -EINVAL;
> + }
> + else {
As above.
> + res = __connmanctl_dbus_set_property(connection,
> + path, "net.connman.Clock",
> + configclock_return,
> + g_strdup("TimezoneUpdates"),
> + "TimezoneUpdates",
> + DBUS_TYPE_STRING, opt_start);
> + }
> + index++;
> + break;
> +
> + case 'z':
> + res = __connmanctl_dbus_set_property(connection,
> + path, "net.connman.Clock",
> + configclock_return,
> + g_strdup("Timezone"),
> + "Timezone",
> + DBUS_TYPE_STRING, opt_start);
> + index++;
> + break;
> +
> + case 't':
> + res = __connmanctl_dbus_set_property_array(connection,
> + path, "net.connman.Clock",
> + configclock_return,
> g_strdup("Timeservers"),
> + "Timeservers",
> + DBUS_TYPE_STRING, config_append_str,
> + &append);
> + index += append.values;
> + break;
> +
> + default:
> + res = -EINVAL;
> + break;
> + }
> +
> + g_free(path);
> +
> + if (res < 0) {
> + if (res == -EINPROGRESS)
> + result = -EINPROGRESS;
> + else
> + printf("Error '%s': %s\n", args[oldindex],
> + strerror(-res));
> + } else
> + index += res;
> +
> + index++;
> + }
> +
> + return result;
> +}
> +
> static DBusHandlerResult monitor_changed(DBusConnection *connection,
> DBusMessage *message, void *user_data)
> {
> @@ -2145,6 +2397,23 @@ static struct connman_option service_options[] = {
> { NULL, }
> };
>
> +static struct connman_option clock_options[] = {
> + {"time", 'T', ""},
> + {"timeservers", 't', ""},
> + {"timeupdates", 'u', ""},
> + {"timezoneupdates", 'Z', ""},
> + {"timezone", 'z', ""},
> + { NULL, }
> +};
> +
Here I'd take the simpler option and have the 'clock' command print out
all properties without taking any arguments. Then the return function
looks identical(?) to 'state_print()'.
> +static struct connman_option configclock_options[] = {
> + {"timeservers", 't', "<ntp1> [<ntp2>] [...]"},
> + {"timeupdates", 'u', "auto|manual"},
> + {"timezoneupdates", 'Z', "auto|manual"},
> + {"timezone", 'z', "<timezone>"},
> + { NULL, }
> +};
> +
> static struct connman_option config_options[] = {
> {"nameservers", 'n', "<dns1> [<dns2>] [<dns3>]"},
> {"timeservers", 't', "<ntp1> [<ntp2>] [...]"},
> @@ -2535,6 +2804,10 @@ static const struct {
> { "move-after", "<service> <target service> ", NULL,
> cmd_service_move_after, "Move <service> after <target service>",
> lookup_service_arg },
> + { "clock", "[optons]", clock_options, cmd_clock,
> + "Show clock properties", NULL },
> + { "configclock", "[optons]", configclock_options,
> cmd_configclock,
> + "Set clock properties", NULL },
> { "config", "<service>", config_options, cmd_config,
> "Set service configuration options", lookup_config },
> { "monitor", "[off]", monitor_options, cmd_monitor,
> diff --git a/client/dbus_helpers.c b/client/dbus_helpers.c
> index 9c4cfee..f7ad7c2 100644
> --- a/client/dbus_helpers.c
> +++ b/client/dbus_helpers.c
> @@ -37,6 +37,7 @@ void __connmanctl_dbus_print(DBusMessageIter *iter, const
> char *pre,
> unsigned char c;
> dbus_uint16_t u16;
> dbus_uint32_t u;
> + dbus_uint64_t u64;
> dbus_int32_t i;
> double d;
>
> @@ -113,6 +114,11 @@ void __connmanctl_dbus_print(DBusMessageIter *iter,
> const char *pre,
> fprintf(stdout, "%d", i);
> break;
>
> + case DBUS_TYPE_UINT64:
> + dbus_message_iter_get_basic(iter, &u64);
> + fprintf(stdout, "%lu", u64);
To get this part to work correctly in all cases, please use "%"PRIu64
instead of "%lu" here.
> + break;
> +
> case DBUS_TYPE_DOUBLE:
> dbus_message_iter_get_basic(iter, &d);
> fprintf(stdout, "%f", d);
Sorry the review took this many calendar days to achieve. Thanks for
your efforts!
Patrik
_______________________________________________
connman mailing list
connman@connman.net
https://lists.connman.net/mailman/listinfo/connman
