Re: [PATCH 7/7] cdma-netreg: Add provider name support

[Denis Kenzior <denkenz@gmail.com>]

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

Hi Philippe,

On 11/24/2011 11:46 AM, Philippe Nunes wrote:
> ---
>  Makefile.am       |    3 ++-
>  src/cdma-netreg.c |   30 ++++++++++++++++++++++++++++++
>  2 files changed, 32 insertions(+), 1 deletions(-)
> 
> diff --git a/Makefile.am b/Makefile.am
> index 6002eb0..978ac9f 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -275,7 +275,8 @@ builtin_sources += drivers/atmodem/atutil.h \
>  			drivers/cdmamodem/voicecall.c \
>  			drivers/cdmamodem/devinfo.c \
>  			drivers/cdmamodem/connman.c \
> -			drivers/cdmamodem/network-registration.c
> +			drivers/cdmamodem/network-registration.c \
> +			plugins/mbpi.c plugins/mbpi.h
>  endif
>  
>  builtin_modules += g1
> diff --git a/src/cdma-netreg.c b/src/cdma-netreg.c
> index 222c3b7..19947ab 100644
> --- a/src/cdma-netreg.c
> +++ b/src/cdma-netreg.c
> @@ -28,6 +28,7 @@
>  #include <gdbus.h>
>  
>  #include "ofono.h"
> +#include "plugins/mbpi.h"

Don't do this, see below

>  
>  static GSList *g_drivers;
>  
> @@ -38,6 +39,7 @@ struct ofono_cdma_netreg {
>  	const struct ofono_cdma_netreg_driver *driver;
>  	void *driver_data;
>  	struct ofono_atom *atom;
> +	char *provider_name;
>  };
>  
>  static const char *cdma_netreg_status_to_string(enum cdma_netreg_status status)
> @@ -90,6 +92,10 @@ static DBusMessage *network_get_properties(DBusConnection *conn,
>  					&strength);
>  	}
>  
> +	if (cdma_netreg->provider_name)
> +		ofono_dbus_dict_append(&dict, "Name", DBUS_TYPE_STRING,
> +						&cdma_netreg->provider_name);
> +

You might also want to export the SystemIdentifier property here

>  	dbus_message_iter_close_container(&iter, &dict);
>  
>  	return reply;
> @@ -108,6 +114,9 @@ static void serving_system_callback(const struct ofono_error *error,
>  					const char *sid, void *data)
>  {
>  	struct ofono_cdma_netreg *cdma_netreg = data;
> +	const char *path = __ofono_atom_get_path(cdma_netreg->atom);
> +	DBusConnection *conn = ofono_dbus_get_connection();
> +	GError *err = NULL;
>  
>  	if (cdma_netreg->status != CDMA_NETWORK_REGISTRATION_STATUS_REGISTERED
>  			&& cdma_netreg->status !=
> @@ -120,6 +129,20 @@ static void serving_system_callback(const struct ofono_error *error,
>  	}
>  
>  	DBG("Serving system Identifier: %s", sid);
> +
> +	g_free(cdma_netreg->provider_name);
> +	cdma_netreg->provider_name = NULL;
> +	cdma_netreg->provider_name = mbpi_lookup_cdma_provider_name(sid, &err);

Please don't do it this way, this part should be abstracted behind a
provider name lookup plugin, similar to how context provisioning for GSM
is done.  Mobile Broadband Provider Info database might not be the only
source of this information, in fact manufacturers might have their own
(better) databases.

> +
> +	if (cdma_netreg->provider_name)
> +		ofono_dbus_signal_property_changed(conn, path,
> +				OFONO_CDMA_NETWORK_REGISTRATION_INTERFACE,
> +				"Name", DBUS_TYPE_STRING,
> +				&cdma_netreg->provider_name);
> +	else if (err != NULL) {
> +		ofono_error("%s", err->message);
> +		g_error_free(err);
> +	}

Are you sure you want to perform the lookup every time? Why don't you
cache the last result and not perform the query if the SID is the same
as the last one obtained.  We're highly unlikely to get a different SID
under normal circumstances (e.g. signal loss, etc) so re-querying seems
wasteful.

>  }
>  
>  static void set_registration_status(struct ofono_cdma_netreg *cdma_netreg,
> @@ -136,6 +159,12 @@ static void set_registration_status(struct ofono_cdma_netreg *cdma_netreg,
>  				"Status", DBUS_TYPE_STRING,
>  				&str_status);
>  
> +	if (cdma_netreg->status ==
> +			CDMA_NETWORK_REGISTRATION_STATUS_NOT_REGISTERED) {
> +		g_free(cdma_netreg->provider_name);
> +		cdma_netreg->provider_name = NULL;

See comment above

> +	}
> +
>  	if (cdma_netreg->status == CDMA_NETWORK_REGISTRATION_STATUS_REGISTERED
>  			|| cdma_netreg->status ==
>  				CDMA_NETWORK_REGISTRATION_STATUS_ROAMING)
> @@ -251,6 +280,7 @@ static void cdma_netreg_remove(struct ofono_atom *atom)
>  	if (cdma_netreg->driver && cdma_netreg->driver->remove)
>  		cdma_netreg->driver->remove(cdma_netreg);
>  
> +	g_free(cdma_netreg->provider_name);
>  	g_free(cdma_netreg);
>  }
>  

Regards,
-Denis