Re: [PATCH_v3 2/3] emulator: add drivers to request/release private network from ConnMan

[Denis Kenzior <denkenz@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2208 bytes --]

Hi Guillaume,

On 05/03/2011 09:44 AM, Guillaume Zajac wrote:
> ---
>  include/emulator.h |   18 +++++++++++
>  src/emulator.c     |   83 +++++++++++++++++++++++++++++++++++++++------------
>  2 files changed, 81 insertions(+), 20 deletions(-)

Please remember to separate patches according to directories whenever
possible.  See HACKING for more details.  This should be two patches,
one for include and one for emulator.

> 
> diff --git a/include/emulator.h b/include/emulator.h
> index 5cd894b..a7fc23b 100644
> --- a/include/emulator.h
> +++ b/include/emulator.h
> @@ -105,6 +105,24 @@ enum ofono_emulator_request_type ofono_emulator_request_get_type(
>  void ofono_emulator_set_indicator(struct ofono_emulator *em,
>  					const char *name, int value);
>  
> +ofono_bool_t ofono_emulator_setup_ppp(struct ofono_emulator *em, int fd,
> +				const char *server_ip, const char *peer_ip,
> +				const char *primary_dns,
> +				const char *secondary_dns);
> +
> +int ofono_emulator_get_uid(struct ofono_emulator *em);
> +

I'm not particularly happy with this, we should try to de-couple the
private network provider from the emulator.  We might end up with other
consumers later.

> +struct emulator_network_provision_driver {
> +	char *name;
> +	int (*get_settings)(struct ofono_emulator *em);
> +	void (*release)(int uid);
> +};
> +

I'd say you're on the right track here, but modeling this even closer to
gprs-provision might be a good idea.  Except you'll need to use a
callback, so something like:

struct ofono_private_network_settings {
	...
};

typedef (ofono_private_network_cb_t)(...);

struct ofono_private_network_driver {
	char *name;
	int (*request)(ofono_private_network_cb_t cb, void *data);
	void (*release)(int id);
};

> +int ofono_emulator_network_driver_register(
> +			const struct emulator_network_provision_driver *d);
> +void ofono_emulator_network_driver_unregister(
> +			const struct emulator_network_provision_driver *d);
> +

And then add __ofono_private_network_request / release to src/ofono.h
for now.  Similar to how gprs_provision does things.

>  #ifdef __cplusplus
>  }
>  #endif

Regards,
-Denis