connman.lists.linux.dev archive mirror
 help / color / mirror / Atom feed
From: Daniel Wagner <wagi@monom.org>
To: Michael Trimarchi <michael@amarulasolutions.com>
Cc: connman@lists.linux.dev, Jan.Ryll@bshg.com, Simon.Holesch@bshg.com
Subject: Re: [PATCH V2] Add GetKnownServices api to connaman
Date: Thu, 28 Oct 2021 08:16:18 +0200	[thread overview]
Message-ID: <20211028061618.cgmgprc7fo2nlpf2@beryllium.lan> (raw)
In-Reply-To: <20211027140141.9267-1-michael@amarulasolutions.com>

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

      parent reply	other threads:[~2021-10-28  6:16 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-27 14:01 Michael Trimarchi
2021-10-27 17:25 ` Michael Nazzareno Trimarchi
2021-10-28  6:31   ` Daniel Wagner
2021-10-28  6:16 ` Daniel Wagner [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20211028061618.cgmgprc7fo2nlpf2@beryllium.lan \
    --to=wagi@monom.org \
    --cc=Jan.Ryll@bshg.com \
    --cc=Simon.Holesch@bshg.com \
    --cc=connman@lists.linux.dev \
    --cc=michael@amarulasolutions.com \
    --subject='Re: [PATCH V2] Add GetKnownServices api to connaman' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).