Re: [PATCH v3 1/3] Add GetKnownServices api to connaman

Hi Roman,

On 6/26/25 2:27 AM, Roman Smrž wrote:
> From: Michael Trimarchi <[email protected]>
> 
> Add a way to retrieve all the networks that were configured
> using connman and not all the ones visible

Is this still about removing WiFi credentials?  Have you considered using iwd 
where this would be exceedingly trivial to do?

> ---
>   client/commands.c    | 14 ++++++++++++

Please separate the client part into its own commit

>   doc/manager-api.txt  | 11 +++++++++
>   src/connman.h        |  1 +
>   src/manager.c        | 23 +++++++++++++++++++
>   src/service.c        | 54 ++++++++++++++++++++++++++++++++++++++++++++
>   tools/manager-api.c  | 30 ++++++++++++++++++++++++
>   tools/session-test.h |  1 +

Ditto for 'tools/' bits.

>   7 files changed, 134 insertions(+)
> 

<snip>

> diff --git a/doc/manager-api.txt b/doc/manager-api.txt
> index 6eaa0a38..f36ff813 100644
> --- a/doc/manager-api.txt
> +++ b/doc/manager-api.txt
> @@ -39,6 +39,17 @@ Methods		dict GetProperties()
>   
>   			Possible Errors: [service].Error.InvalidArguments
>   
> +		array{object,dict} GetKnownServices()
> +
> +			Returns a sorted list of tuples with dictionary of service

Sorted how?

> +			properties. Those are all the services, including the ones
> +			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

<snip>

> diff --git a/src/service.c b/src/service.c
> index f73f42eb..bcda5076 100644
> --- a/src/service.c
> +++ b/src/service.c
> @@ -5441,6 +5441,60 @@ 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)
> +{
> +	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;

You can't just return here, you're leaking 'services'

> +		}
> +
> +		service->identifier = g_strdup(services[i]);
> +		service->path = g_strdup_printf("%s/service/%s", CONNMAN_PATH,
> +						service->identifier);
> +		srv_id_parts = g_strsplit(services[i], "_", -1);
 > +> +		service->type = __connman_service_string2type(srv_id_parts[0]);

This looks exceedingly horrible.  I don't see an equivalent type classification 
anywhere else.  Is there a way to use the information present in struct 
connman_config_service instead?

> +		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;

That looks dangerous since 'name' is dynamically allocated.

> +
> +		if (service_load(service)) {
> +			connman_error("service_load() returned error");
> +			g_free(service);

This looks completely insufficient given what connman_service_create() does.

> +			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);
> +}
> +

Regards,
-Denis