[PATCH V2] Add GetKnownServices api to connaman

[PATCH V2] Add GetKnownServices api to connaman

[Michael Trimarchi]

Add a way to retrieve all the networks that were configured
using connman and not all the ones visible

---
Changes V1->V2:
	- remove SoB
	- Add documentation
	- remove LOG print
	- Avoid check null on functions that assert
	- adjust the function the look for service
	- clean up a bit variable name
	- remove initialization of variables that are not needed
---
 client/commands.c    | 14 +++++++++++
 doc/manager-api.txt  | 12 ++++++++++
 src/connman.h        |  1 +
 src/manager.c        | 23 ++++++++++++++++++
 src/service.c        | 55 ++++++++++++++++++++++++++++++++++++++++++++
 tools/manager-api.c  | 30 ++++++++++++++++++++++++
 tools/session-test.h |  1 +
 7 files changed, 136 insertions(+)

diff --git a/client/commands.c b/client/commands.c
index 53cc14c8..f4366e8c 100644
--- a/client/commands.c
+++ b/client/commands.c
@@ -420,6 +420,18 @@ static int cmd_services(char *args[], int num, struct connman_option *options)
 			object_properties, path, NULL, NULL);
 }
 
+static int cmd_known_services(char *args[], int num, struct connman_option *options)
+{
+	if (num > 1)
+		return -E2BIG;
+
+	return __connmanctl_dbus_method_call(connection,
+			 CONNMAN_SERVICE, CONNMAN_PATH,
+			 "net.connman.Manager",
+			 "GetKnownServices",
+			 services_list, NULL, NULL, NULL);
+}
+
 static int cmd_peers(char *args[], int num, struct connman_option *options)
 {
 	char *peer_name = NULL;
@@ -2808,6 +2820,8 @@ static const struct {
 	  "Display tethering clients", NULL },
 	{ "services",     "[<service>]",  service_options, cmd_services,
 	  "Display services", lookup_service_arg },
+	{ "known-services", NULL,  NULL, cmd_known_services,
+	  "Display known services", NULL },
 	{ "peers",        "[peer]",       NULL,            cmd_peers,
 	  "Display peers", lookup_peer_arg },
 	{ "scan",         "<technology>", NULL,            cmd_scan,
diff --git a/doc/manager-api.txt b/doc/manager-api.txt
index 6eaa0a38..093b25c1 100644
--- a/doc/manager-api.txt
+++ b/doc/manager-api.txt
@@ -39,6 +39,18 @@ Methods		dict GetProperties()
 
 			Possible Errors: [service].Error.InvalidArguments
 
+		array{object,dict} GetKnownServices()
+
+			Returns a sorted list of tuples with service
+			object path and dictionary of service propertiesa
+			Those are all the services including the one that
+			are registered but not visible or not in use.
+
+			This list will not contain sensitive information
+			like passphrases etc.
+
+			Possible Errors: [service].Error.InvalidArguments
+
 		array{object,dict} GetPeers() [experimental]
 
 			Returns a sorted list of tuples with peer object path
diff --git a/src/connman.h b/src/connman.h
index 68176086..cdc88532 100644
--- a/src/connman.h
+++ b/src/connman.h
@@ -682,6 +682,7 @@ int __connman_service_move(struct connman_service *service,
 				struct connman_service *target, bool before);
 int __connman_service_load_modifiable(struct connman_service *service);
 
+void __connman_known_service_list_struct(DBusMessageIter *iter);
 void __connman_service_list_struct(DBusMessageIter *iter);
 
 int __connman_service_compare(const struct connman_service *a,
diff --git a/src/manager.c b/src/manager.c
index 892d3a42..b196109a 100644
--- a/src/manager.c
+++ b/src/manager.c
@@ -175,6 +175,11 @@ static const struct connman_notifier technology_notifier = {
 	.idle_state	= idle_state,
 };
 
+static void append_known_service_structs(DBusMessageIter *iter, void *user_data)
+{
+	__connman_known_service_list_struct(iter);
+}
+
 static void append_service_structs(DBusMessageIter *iter, void *user_data)
 {
 	__connman_service_list_struct(iter);
@@ -195,6 +200,21 @@ static DBusMessage *get_services(DBusConnection *conn,
 	return reply;
 }
 
+static DBusMessage *get_known_services(DBusConnection *conn,
+				   DBusMessage *msg, void *data)
+{
+	DBusMessage *reply;
+
+	reply = dbus_message_new_method_return(msg);
+	if (!reply)
+		return NULL;
+
+	__connman_dbus_append_objpath_dict_array(reply,
+				 append_known_service_structs, NULL);
+
+	return reply;
+}
+
 static void append_peer_structs(DBusMessageIter *iter, void *user_data)
 {
 	__connman_peer_list_struct(iter);
@@ -534,6 +554,9 @@ static const GDBusMethodTable manager_methods[] = {
 	{ GDBUS_DEPRECATED_METHOD("RemoveProvider",
 			GDBUS_ARGS({ "provider", "o" }), NULL,
 			remove_provider) },
+	{ GDBUS_METHOD("GetKnownServices",
+			NULL, GDBUS_ARGS({"knownServices", "a(oa{sv})" }),
+			get_known_services) },
 	{ GDBUS_METHOD("GetServices",
 			NULL, GDBUS_ARGS({ "services", "a(oa{sv})" }),
 			get_services) },
diff --git a/src/service.c b/src/service.c
index 8d3c75a5..da8304cc 100644
--- a/src/service.c
+++ b/src/service.c
@@ -2705,6 +2705,61 @@ static void append_struct(gpointer value, gpointer user_data)
 	append_struct_service(iter, append_dict_properties, service);
 }
 
+void __connman_known_service_list_struct(DBusMessageIter *iter)
+{
+	gchar **services;
+	gchar **srv_id_parts;
+	guint i;
+	struct connman_service *service;
+
+	services = connman_storage_get_services();
+	if (!services)
+		return;
+
+	for (i = 0; i < g_strv_length(services); i++) {
+		struct connman_service *srv;
+
+		service = connman_service_create();
+		if (!service) {
+			connman_error("connman_service_create() allocation failed");
+			return;
+		}
+
+		service->identifier = g_strdup(services[i]);
+		srv_id_parts = g_strsplit(services[i], "_", -1);
+		if (srv_id_parts == NULL) {
+			g_free(service);
+			continue;
+		}
+
+		service->type = __connman_service_string2type(srv_id_parts[0]);
+		service->path = g_strdup_printf("%s/%s", STORAGEDIR, service->identifier);
+		if (service->type == CONNMAN_SERVICE_TYPE_WIFI)
+			service->security = __connman_service_string2security(srv_id_parts[g_strv_length(srv_id_parts) - 1]);
+
+		srv = find_service(service->path);
+		if (srv)
+			service->state = srv->state;
+
+		if (service->type == CONNMAN_SERVICE_TYPE_ETHERNET && srv)
+			service->name = srv->name;
+
+		if (service_load(service)) {
+			connman_error("service_load() returned error");
+			g_free(service);
+			g_strfreev(srv_id_parts);
+			g_strfreev(services);
+			return;
+		}
+
+		append_struct_service(iter, append_dict_properties, service);
+		g_strfreev(srv_id_parts);
+		g_free(service);
+	}
+
+	g_strfreev(services);
+}
+
 void __connman_service_list_struct(DBusMessageIter *iter)
 {
 	g_list_foreach(service_list, append_struct, iter);
diff --git a/tools/manager-api.c b/tools/manager-api.c
index e082962d..8668ca9e 100644
--- a/tools/manager-api.c
+++ b/tools/manager-api.c
@@ -64,6 +64,36 @@ static DBusMessage *set_property(DBusConnection *connection,
 	return reply;
 }
 
+DBusMessage *manager_get_known_services(DBusConnection *connection)
+{
+	DBusMessage *message, *reply;
+	DBusError error;
+
+	message = dbus_message_new_method_call(CONNMAN_SERVICE,
+				   CONNMAN_MANAGER_PATH,
+				   CONNMAN_MANAGER_INTERFACE,
+				   "GetKnownServices");
+
+	if (!message)
+		return NULL;
+
+	dbus_error_init(&error);
+
+	reply = dbus_connection_send_with_reply_and_block(connection,
+				  message, -1, &error);
+	if (!reply) {
+		if (dbus_error_is_set(&error))
+			dbus_error_free(&error);
+
+		dbus_message_unref(message);
+		return NULL;
+	}
+
+	dbus_message_unref(message);
+
+	return reply;
+}
+
 DBusMessage *manager_get_services(DBusConnection *connection)
 {
 	DBusMessage *message, *reply;
diff --git a/tools/session-test.h b/tools/session-test.h
index 2c068bd7..ab89f8c5 100644
--- a/tools/session-test.h
+++ b/tools/session-test.h
@@ -114,6 +114,7 @@ DBusMessage *session_disconnect(DBusConnection *connection,
 					struct test_session *session);
 
 /* manager-api.c */
+DBusMessage *manager_get_known_services(DBusConnection *connection);
 DBusMessage *manager_get_services(DBusConnection *connection);
 DBusMessage *manager_get_properties(DBusConnection *connection);
 DBusMessage *manager_create_session(DBusConnection *connection,
-- 
2.25.1

Re: [PATCH V2] Add GetKnownServices api to connaman

[Michael Nazzareno Trimarchi]

Hi Daniel

I even have the patch to remove known-networks. I have few question:

- what should be the behavior for an ethernet on?
- for wifi, it's removed and if you are connected it's disconnected is
that correct?

Apart of that:

I'm working on a patch that allow to store network configuration using
the config storage
but I have few questions:

- I have added autoconnect as a configuration option. I would like to
store networks but not have them
in auto connect mode. This should allow me to connect when they are
visible. (ethernet??)
- I have add an option to store if I want it immutable or not. The
stored configuration should be applied
if it's valid but I would like to connect using a different way from
connect call

--- a/src/config.c
+++ b/src/config.c
@@ -61,6 +61,8 @@ struct connman_config_service {
        GSList *service_identifiers;
        char *config_ident; /* file prefix */
        char *config_entry; /* entry name */
+       bool autoconnect;
+       bool immutable;
        bool hidden;
        bool virtual;
        char *virtual_file;
@@ -115,6 +117,8 @@ static bool cleanup = false;
 #define SERVICE_KEY_SECURITY           "Security"
 #define SERVICE_KEY_HIDDEN             "Hidden"
 #define SERVICE_KEY_MDNS               "mDNS"
+#define SERVICE_KEY_AUTOCONNECT        "Autoconnect"
+#define SERVICE_KEY_IMMUTABLE          "Immutable"

 #define SERVICE_KEY_IPv4               "IPv4"
 #define SERVICE_KEY_IPv6               "IPv6"
@@ -162,6 +166,8 @@ static const char *service_possible_keys[] = {
        SERVICE_KEY_SEARCH_DOMAINS,
        SERVICE_KEY_TIMESERVERS,
        SERVICE_KEY_DOMAIN,
+       SERVICE_KEY_AUTOCONNECT,
+       SERVICE_KEY_IMMUTABLE,
        NULL,
 };

@@ -229,7 +235,7 @@ static void unregister_service(gpointer data)
                        }
                }

-               if (!__connman_storage_remove_service(service_id))
+               if (config_service->autoconnect &&
!__connman_storage_remove_service(service_id))
                        DBG("Could not remove all files for service %s",
                                                                service_id);
        }
@@ -599,6 +605,12 @@ static bool load_service(GKeyFile *keyfile, const
char *group,
                return true;
        }

+       service->autoconnect = __connman_config_get_bool(keyfile, group,
+                                               SERVICE_KEY_AUTOCONNECT, NULL);
+
+       service->immutable = __connman_config_get_bool(keyfile, group,
+                                               SERVICE_KEY_IMMUTABLE, NULL);
+
        str = __connman_config_get_string(keyfile, group,
SERVICE_KEY_NAME, NULL);
        if (str) {
                g_free(service->name);
@@ -1180,6 +1192,8 @@ static void provision_service_wifi(struct
connman_config_service *config,

        if (config->hidden)
                __connman_service_set_hidden(service);
+
+       __connman_service_set_auto_connect(service, config->autoconnect);

Michael

On Wed, Oct 27, 2021 at 2:01 PM Michael Trimarchi
<michael@amarulasolutions.com> wrote:
>
> Add a way to retrieve all the networks that were configured
> using connman and not all the ones visible
>
> ---
> Changes V1->V2:
>         - remove SoB
>         - Add documentation
>         - remove LOG print
>         - Avoid check null on functions that assert
>         - adjust the function the look for service
>         - clean up a bit variable name
>         - remove initialization of variables that are not needed
> ---
>  client/commands.c    | 14 +++++++++++
>  doc/manager-api.txt  | 12 ++++++++++
>  src/connman.h        |  1 +
>  src/manager.c        | 23 ++++++++++++++++++
>  src/service.c        | 55 ++++++++++++++++++++++++++++++++++++++++++++
>  tools/manager-api.c  | 30 ++++++++++++++++++++++++
>  tools/session-test.h |  1 +
>  7 files changed, 136 insertions(+)
>
> diff --git a/client/commands.c b/client/commands.c
> index 53cc14c8..f4366e8c 100644
> --- a/client/commands.c
> +++ b/client/commands.c
> @@ -420,6 +420,18 @@ static int cmd_services(char *args[], int num, struct connman_option *options)
>                         object_properties, path, NULL, NULL);
>  }
>
> +static int cmd_known_services(char *args[], int num, struct connman_option *options)
> +{
> +       if (num > 1)
> +               return -E2BIG;
> +
> +       return __connmanctl_dbus_method_call(connection,
> +                        CONNMAN_SERVICE, CONNMAN_PATH,
> +                        "net.connman.Manager",
> +                        "GetKnownServices",
> +                        services_list, NULL, NULL, NULL);
> +}
> +
>  static int cmd_peers(char *args[], int num, struct connman_option *options)
>  {
>         char *peer_name = NULL;
> @@ -2808,6 +2820,8 @@ static const struct {
>           "Display tethering clients", NULL },
>         { "services",     "[<service>]",  service_options, cmd_services,
>           "Display services", lookup_service_arg },
> +       { "known-services", NULL,  NULL, cmd_known_services,
> +         "Display known services", NULL },
>         { "peers",        "[peer]",       NULL,            cmd_peers,
>           "Display peers", lookup_peer_arg },
>         { "scan",         "<technology>", NULL,            cmd_scan,
> diff --git a/doc/manager-api.txt b/doc/manager-api.txt
> index 6eaa0a38..093b25c1 100644
> --- a/doc/manager-api.txt
> +++ b/doc/manager-api.txt
> @@ -39,6 +39,18 @@ Methods              dict GetProperties()
>
>                         Possible Errors: [service].Error.InvalidArguments
>
> +               array{object,dict} GetKnownServices()
> +
> +                       Returns a sorted list of tuples with service
> +                       object path and dictionary of service propertiesa
> +                       Those are all the services including the one that
> +                       are registered but not visible or not in use.
> +
> +                       This list will not contain sensitive information
> +                       like passphrases etc.
> +
> +                       Possible Errors: [service].Error.InvalidArguments
> +
>                 array{object,dict} GetPeers() [experimental]
>
>                         Returns a sorted list of tuples with peer object path
> diff --git a/src/connman.h b/src/connman.h
> index 68176086..cdc88532 100644
> --- a/src/connman.h
> +++ b/src/connman.h
> @@ -682,6 +682,7 @@ int __connman_service_move(struct connman_service *service,
>                                 struct connman_service *target, bool before);
>  int __connman_service_load_modifiable(struct connman_service *service);
>
> +void __connman_known_service_list_struct(DBusMessageIter *iter);
>  void __connman_service_list_struct(DBusMessageIter *iter);
>
>  int __connman_service_compare(const struct connman_service *a,
> diff --git a/src/manager.c b/src/manager.c
> index 892d3a42..b196109a 100644
> --- a/src/manager.c
> +++ b/src/manager.c
> @@ -175,6 +175,11 @@ static const struct connman_notifier technology_notifier = {
>         .idle_state     = idle_state,
>  };
>
> +static void append_known_service_structs(DBusMessageIter *iter, void *user_data)
> +{
> +       __connman_known_service_list_struct(iter);
> +}
> +
>  static void append_service_structs(DBusMessageIter *iter, void *user_data)
>  {
>         __connman_service_list_struct(iter);
> @@ -195,6 +200,21 @@ static DBusMessage *get_services(DBusConnection *conn,
>         return reply;
>  }
>
> +static DBusMessage *get_known_services(DBusConnection *conn,
> +                                  DBusMessage *msg, void *data)
> +{
> +       DBusMessage *reply;
> +
> +       reply = dbus_message_new_method_return(msg);
> +       if (!reply)
> +               return NULL;
> +
> +       __connman_dbus_append_objpath_dict_array(reply,
> +                                append_known_service_structs, NULL);
> +
> +       return reply;
> +}
> +
>  static void append_peer_structs(DBusMessageIter *iter, void *user_data)
>  {
>         __connman_peer_list_struct(iter);
> @@ -534,6 +554,9 @@ static const GDBusMethodTable manager_methods[] = {
>         { GDBUS_DEPRECATED_METHOD("RemoveProvider",
>                         GDBUS_ARGS({ "provider", "o" }), NULL,
>                         remove_provider) },
> +       { GDBUS_METHOD("GetKnownServices",
> +                       NULL, GDBUS_ARGS({"knownServices", "a(oa{sv})" }),
> +                       get_known_services) },
>         { GDBUS_METHOD("GetServices",
>                         NULL, GDBUS_ARGS({ "services", "a(oa{sv})" }),
>                         get_services) },
> diff --git a/src/service.c b/src/service.c
> index 8d3c75a5..da8304cc 100644
> --- a/src/service.c
> +++ b/src/service.c
> @@ -2705,6 +2705,61 @@ static void append_struct(gpointer value, gpointer user_data)
>         append_struct_service(iter, append_dict_properties, service);
>  }
>
> +void __connman_known_service_list_struct(DBusMessageIter *iter)
> +{
> +       gchar **services;
> +       gchar **srv_id_parts;
> +       guint i;
> +       struct connman_service *service;
> +
> +       services = connman_storage_get_services();
> +       if (!services)
> +               return;
> +
> +       for (i = 0; i < g_strv_length(services); i++) {
> +               struct connman_service *srv;
> +
> +               service = connman_service_create();
> +               if (!service) {
> +                       connman_error("connman_service_create() allocation failed");
> +                       return;
> +               }
> +
> +               service->identifier = g_strdup(services[i]);
> +               srv_id_parts = g_strsplit(services[i], "_", -1);
> +               if (srv_id_parts == NULL) {
> +                       g_free(service);
> +                       continue;
> +               }
> +
> +               service->type = __connman_service_string2type(srv_id_parts[0]);
> +               service->path = g_strdup_printf("%s/%s", STORAGEDIR, service->identifier);
> +               if (service->type == CONNMAN_SERVICE_TYPE_WIFI)
> +                       service->security = __connman_service_string2security(srv_id_parts[g_strv_length(srv_id_parts) - 1]);
> +
> +               srv = find_service(service->path);
> +               if (srv)
> +                       service->state = srv->state;
> +
> +               if (service->type == CONNMAN_SERVICE_TYPE_ETHERNET && srv)
> +                       service->name = srv->name;
> +
> +               if (service_load(service)) {
> +                       connman_error("service_load() returned error");
> +                       g_free(service);
> +                       g_strfreev(srv_id_parts);
> +                       g_strfreev(services);
> +                       return;
> +               }
> +
> +               append_struct_service(iter, append_dict_properties, service);
> +               g_strfreev(srv_id_parts);
> +               g_free(service);
> +       }
> +
> +       g_strfreev(services);
> +}
> +
>  void __connman_service_list_struct(DBusMessageIter *iter)
>  {
>         g_list_foreach(service_list, append_struct, iter);
> diff --git a/tools/manager-api.c b/tools/manager-api.c
> index e082962d..8668ca9e 100644
> --- a/tools/manager-api.c
> +++ b/tools/manager-api.c
> @@ -64,6 +64,36 @@ static DBusMessage *set_property(DBusConnection *connection,
>         return reply;
>  }
>
> +DBusMessage *manager_get_known_services(DBusConnection *connection)
> +{
> +       DBusMessage *message, *reply;
> +       DBusError error;
> +
> +       message = dbus_message_new_method_call(CONNMAN_SERVICE,
> +                                  CONNMAN_MANAGER_PATH,
> +                                  CONNMAN_MANAGER_INTERFACE,
> +                                  "GetKnownServices");
> +
> +       if (!message)
> +               return NULL;
> +
> +       dbus_error_init(&error);
> +
> +       reply = dbus_connection_send_with_reply_and_block(connection,
> +                                 message, -1, &error);
> +       if (!reply) {
> +               if (dbus_error_is_set(&error))
> +                       dbus_error_free(&error);
> +
> +               dbus_message_unref(message);
> +               return NULL;
> +       }
> +
> +       dbus_message_unref(message);
> +
> +       return reply;
> +}
> +
>  DBusMessage *manager_get_services(DBusConnection *connection)
>  {
>         DBusMessage *message, *reply;
> diff --git a/tools/session-test.h b/tools/session-test.h
> index 2c068bd7..ab89f8c5 100644
> --- a/tools/session-test.h
> +++ b/tools/session-test.h
> @@ -114,6 +114,7 @@ DBusMessage *session_disconnect(DBusConnection *connection,
>                                         struct test_session *session);
>
>  /* manager-api.c */
> +DBusMessage *manager_get_known_services(DBusConnection *connection);
>  DBusMessage *manager_get_services(DBusConnection *connection);
>  DBusMessage *manager_get_properties(DBusConnection *connection);
>  DBusMessage *manager_create_session(DBusConnection *connection,
> --
> 2.25.1
>


-- 
Michael Nazzareno Trimarchi
Co-Founder & Chief Executive Officer
M. +39 347 913 2170
michael@amarulasolutions.com
__________________________________

Amarula Solutions BV
Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
T. +31 (0)85 111 9172
info@amarulasolutions.com
www.amarulasolutions.com

Re: [PATCH V2] Add GetKnownServices api to connaman

[Daniel Wagner]

Hi Michael,

On Wed, Oct 27, 2021 at 02:01:41PM +0000, Michael Trimarchi wrote:
> Add a way to retrieve all the networks that were configured
> using connman and not all the ones visible
> 
> ---
> Changes V1->V2:
> 	- remove SoB
> 	- Add documentation
> 	- remove LOG print
> 	- Avoid check null on functions that assert
> 	- adjust the function the look for service
> 	- clean up a bit variable name
> 	- remove initialization of variables that are not needed
> ---
>  client/commands.c    | 14 +++++++++++
>  doc/manager-api.txt  | 12 ++++++++++
>  src/connman.h        |  1 +
>  src/manager.c        | 23 ++++++++++++++++++
>  src/service.c        | 55 ++++++++++++++++++++++++++++++++++++++++++++
>  tools/manager-api.c  | 30 ++++++++++++++++++++++++
>  tools/session-test.h |  1 +
>  7 files changed, 136 insertions(+)
> 
> diff --git a/client/commands.c b/client/commands.c
> index 53cc14c8..f4366e8c 100644
> --- a/client/commands.c
> +++ b/client/commands.c
> @@ -420,6 +420,18 @@ static int cmd_services(char *args[], int num, struct connman_option *options)
>  			object_properties, path, NULL, NULL);
>  }
>  
> +static int cmd_known_services(char *args[], int num, struct connman_option *options)
> +{
> +	if (num > 1)
> +		return -E2BIG;
> +
> +	return __connmanctl_dbus_method_call(connection,
> +			 CONNMAN_SERVICE, CONNMAN_PATH,
> +			 "net.connman.Manager",
> +			 "GetKnownServices",
> +			 services_list, NULL, NULL, NULL);
> +}
> +
>  static int cmd_peers(char *args[], int num, struct connman_option *options)
>  {
>  	char *peer_name = NULL;
> @@ -2808,6 +2820,8 @@ static const struct {
>  	  "Display tethering clients", NULL },
>  	{ "services",     "[<service>]",  service_options, cmd_services,
>  	  "Display services", lookup_service_arg },
> +	{ "known-services", NULL,  NULL, cmd_known_services,
> +	  "Display known services", NULL },
>  	{ "peers",        "[peer]",       NULL,            cmd_peers,
>  	  "Display peers", lookup_peer_arg },
>  	{ "scan",         "<technology>", NULL,            cmd_scan,
> diff --git a/doc/manager-api.txt b/doc/manager-api.txt
> index 6eaa0a38..093b25c1 100644
> --- a/doc/manager-api.txt
> +++ b/doc/manager-api.txt
> @@ -39,6 +39,18 @@ Methods		dict GetProperties()
>  
>  			Possible Errors: [service].Error.InvalidArguments
>  
> +		array{object,dict} GetKnownServices()
> +
> +			Returns a sorted list of tuples with service
> +			object path and dictionary of service propertiesa

... properties. But the text is wrong anyway. 'object path' is not
correct, it is the storage path of the dictionary containing the
associated resources with this known service.

About those properties, e.g.

[('/var/lib/connman/ethernet_2cf05d0a93f0_cable',
  {'AutoConnect': False,
   'Domains': [],
   'Domains.Configuration': [],
   'Ethernet': {},
   'Favorite': False,
   'IPv4': {},
   'IPv4.Configuration': {},
   'IPv6': {},
   'IPv6.Configuration': {},
   'Immutable': False,
   'Nameservers': [],
   'Nameservers.Configuration': [],
   'Provider': {},
   'Proxy': {},
   'Proxy.Configuration': {},
   'Security': [],
   'Timeservers': [],
   'Timeservers.Configuration': [],
   'Type': 'ethernet',
   'mDNS': False,
   'mDNS.Configuration': False}),

I don't think this makes sense, especially that all services return
empty properties. GetKnownServices() should just return the object path
and then the dictionary containing the storage path.

> +void __connman_known_service_list_struct(DBusMessageIter *iter)
> +{
> +	gchar **services;
> +	gchar **srv_id_parts;
> +	guint i;
> +	struct connman_service *service;

Reverse Christmas tree, no glib types when not needed,

	struct connman_service *service;
	char **srv_id_parts;
	char **services;
	unsigned int i;


> +
> +	services = connman_storage_get_services();
> +	if (!services)
> +		return;
> +
> +	for (i = 0; i < g_strv_length(services); i++) {
> +		struct connman_service *srv;
> +
> +		service = connman_service_create();
> +		if (!service) {
> +			connman_error("connman_service_create() allocation failed");
> +			return;
> +		}
> +
> +		service->identifier = g_strdup(services[i]);
> +		srv_id_parts = g_strsplit(services[i], "_", -1);
> +		if (srv_id_parts == NULL) {

Does not make sense srv_id_parts is never NULL. See glib documentation.

> +			g_free(service);
> +			continue;
> +		}
> +
> +		service->type = __connman_service_string2type(srv_id_parts[0]);
> +		service->path = g_strdup_printf("%s/%s", STORAGEDIR, service->identifier);
> +		if (service->type == CONNMAN_SERVICE_TYPE_WIFI)
> +			service->security = __connman_service_string2security(srv_id_parts[g_strv_length(srv_id_parts) - 1]);
> +

The line is way too long. Do something like

		if (service->type == CONNMAN_SERVICE_TYPE_WIFI) {
			char *id = srv_id_parts[g_strv_length(srv_id_parts) - 1];
			service->security = __connman_service_string2security(id);
		}


> +		srv = find_service(service->path);
> +		if (srv)
> +			service->state = srv->state;
> +
> +		if (service->type == CONNMAN_SERVICE_TYPE_ETHERNET && srv)
> +			service->name = srv->name;
> +
> +		if (service_load(service)) {
> +			connman_error("service_load() returned error");
> +			g_free(service);
> +			g_strfreev(srv_id_parts);
> +			g_strfreev(services);
> +			return;
> +		}
> +
> +		append_struct_service(iter, append_dict_properties, service);
> +		g_strfreev(srv_id_parts);
> +		g_free(service);
> +	}
> +
> +	g_strfreev(services);
> +}
> +

Thanks,
Daniel

Re: [PATCH V2] Add GetKnownServices api to connaman

[Daniel Wagner]

Hi Michael,

On Wed, Oct 27, 2021 at 05:25:32PM +0000, Michael Nazzareno Trimarchi wrote:
> Hi Daniel
> 
> I even have the patch to remove known-networks. I have few question:
> 
> - what should be the behavior for an ethernet on?

I might not understand you question correctly, I assumed we don't
differentiate between types. Return the complete list and let the user
decide what to do with it.

> - for wifi, it's removed and if you are connected it's disconnected is
> that correct?

I haven't tested this but I would assume nothing will happen. These
files are all considered under control of ConnMan. So if you remove
those while ConnMan is running, it wont notice it. Hmm, to get this
working correctly we might need to add a RemoveKnownNetwork call. I want
to avoid to have ConnMan monitoring the storage directory (it's fragile
stuff and difficult to get right).

If we go down the road with RemoveKnownNetwork, then I suggest the
GetKnownNetwork call returns the object path, not the storage path.

Sorry for this late enlightenment from my side. I didn't realize all the
impacts when we started the discussion.

> Apart of that:
> 
> I'm working on a patch that allow to store network configuration using
> the config storage
> but I have few questions:
> 
> - I have added autoconnect as a configuration option. I would like to
> store networks but not have them
> in auto connect mode. This should allow me to connect when they are
> visible. (ethernet??)

I can't remember all the details around AutoConnect. It's a difficult
feature as it set on the first successful connect attempt. If
configuration files are involved there are other rules.

> - I have add an option to store if I want it immutable or not. The
> stored configuration should be applied

Immutable is derived from the fact if there is a config file, the
service immutable. It doesn't really make sense to make this
configurable.

HTH,
Daniel

Re: [PATCH V2] Add GetKnownServices api to connaman

[Michael Nazzareno Trimarchi]

Hi Daniel

I will resend those patches again because seems that people needs
them. We will try to combine our work on another
try

On Thu, Oct 28, 2021 at 8:31 AM Daniel Wagner <wagi@monom.org> wrote:
>
> Hi Michael,
>
> On Wed, Oct 27, 2021 at 05:25:32PM +0000, Michael Nazzareno Trimarchi wrote:
> > Hi Daniel
> >

Michael

> > I even have the patch to remove known-networks. I have few question:
> >
> > - what should be the behavior for an ethernet on?
>
> I might not understand you question correctly, I assumed we don't
> differentiate between types. Return the complete list and let the user
> decide what to do with it.
>
> > - for wifi, it's removed and if you are connected it's disconnected is
> > that correct?
>
> I haven't tested this but I would assume nothing will happen. These
> files are all considered under control of ConnMan. So if you remove
> those while ConnMan is running, it wont notice it. Hmm, to get this
> working correctly we might need to add a RemoveKnownNetwork call. I want
> to avoid to have ConnMan monitoring the storage directory (it's fragile
> stuff and difficult to get right).
>
> If we go down the road with RemoveKnownNetwork, then I suggest the
> GetKnownNetwork call returns the object path, not the storage path.
>
> Sorry for this late enlightenment from my side. I didn't realize all the
> impacts when we started the discussion.
>
> > Apart of that:
> >
> > I'm working on a patch that allow to store network configuration using
> > the config storage
> > but I have few questions:
> >
> > - I have added autoconnect as a configuration option. I would like to
> > store networks but not have them
> > in auto connect mode. This should allow me to connect when they are
> > visible. (ethernet??)
>
> I can't remember all the details around AutoConnect. It's a difficult
> feature as it set on the first successful connect attempt. If
> configuration files are involved there are other rules.
>
> > - I have add an option to store if I want it immutable or not. The
> > stored configuration should be applied
>
> Immutable is derived from the fact if there is a config file, the
> service immutable. It doesn't really make sense to make this
> configurable.
>
> HTH,
> Daniel



-- 
Michael Nazzareno Trimarchi
Co-Founder & Chief Executive Officer
M. +39 347 913 2170
michael@amarulasolutions.com
__________________________________

Amarula Solutions BV
Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
T. +31 (0)85 111 9172
info@amarulasolutions.com
www.amarulasolutions.com