Re: [PATCH 7/8][RFC] Invalidate SIM manager properties when SIM extracted, add Present property.

[Denis Kenzior <denkenz@gmail.com>]

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

Hi Andrew,

> ---
>  src/sim.c |  107
>  ++++++++++++++++++++++++++++++++++++++++++++++++++++++------- 1 files
>  changed, 95 insertions(+), 12 deletions(-)
> 
> diff --git a/src/sim.c b/src/sim.c
> index cddb3e4..fe535bb 100644
> --- a/src/sim.c
> +++ b/src/sim.c
> @@ -285,6 +285,7 @@ static DBusMessage *sim_get_properties(DBusConnection
>  *conn, char **service_numbers;
>  	char **locked_pins;
>  	const char *pin_name;
> +	dbus_bool_t present = sim->inserted;
> 
>  	reply = dbus_message_new_method_return(msg);
>  	if (!reply)
> @@ -296,6 +297,8 @@ static DBusMessage *sim_get_properties(DBusConnection
>  *conn, OFONO_PROPERTIES_ARRAY_SIGNATURE,
>  					&dict);
> 
> +	ofono_dbus_dict_append(&dict, "Present", DBUS_TYPE_BOOLEAN, &present);
> +

I suggest you simply skip the rest of the properties if Present is false and 
skip checking each and every one afterwards.

> @@ -992,6 +1001,8 @@ static void sim_imsi_cb(const struct ofono_error
>  *error, const char *imsi, void *data)
>  {
>  	struct ofono_sim *sim = data;
> +	DBusConnection *conn = ofono_dbus_get_connection();
> +	const char *path = __ofono_atom_get_path(sim->atom);
> 
>  	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
>  		ofono_error("Unable to read IMSI, emergency calls only");
> @@ -1000,6 +1011,11 @@ static void sim_imsi_cb(const struct ofono_error
>  *error, const char *imsi,
> 
>  	sim->imsi = g_strdup(imsi);
> 
> +	ofono_dbus_signal_property_changed(conn, path,
> +					SIM_MANAGER_INTERFACE,
> +					"SubscriberIdentity",
> +					DBUS_TYPE_STRING, &sim->imsi);
> +
>  	/* Read CPHS-support bits, this is still part of the SIM
>  	 * initialisation but no order is specified for it.  */
>  	ofono_sim_read(sim, SIM_EF_CPHS_INFORMATION_FILEID,
> @@ -1755,9 +1771,21 @@ const unsigned char
>  *ofono_sim_get_cphs_service_table(struct ofono_sim *sim) return
>  sim->cphs_service_table;
>  }
> 
> +static void sim_inserted_update(struct ofono_sim *sim)
> +{
> +	dbus_bool_t present = sim->inserted;
> +	DBusConnection *conn = ofono_dbus_get_connection();
> +	const char *path = __ofono_atom_get_path(sim->atom);
> +
> +	ofono_dbus_signal_property_changed(conn, path,
> +			SIM_MANAGER_INTERFACE, "Present",
> +			DBUS_TYPE_BOOLEAN, &present);
> +}
> +
>  void ofono_sim_inserted(struct ofono_sim *sim)
>  {
>  	sim->inserted = TRUE;
> +	sim_inserted_update(sim);
> 
>  	/* Perform SIM initialization according to 3GPP 31.102 Section 5.1.1.2
>  	 * The assumption here is that if sim manager is being initialized,
> @@ -1781,6 +1809,18 @@ void ofono_sim_inserted(struct ofono_sim *sim)
>  	sim_determine_phase(sim);
>  }
> 
> +static void sim_file_op_fail(struct sim_file_op *op)
> +{
> +	if (op->is_read == TRUE)
> +		((ofono_sim_file_read_cb_t) op->cb)
> +			(0, 0, 0, 0, 0, op->userdata);
> +	else
> +		((ofono_sim_file_write_cb_t) op->cb)
> +			(0, op->userdata);
> +
> +	sim_file_op_free(op);
> +}
> +
>  void ofono_sim_removed(struct ofono_sim *sim)
>  {
>  	GSList *l;
> @@ -1790,18 +1830,61 @@ void ofono_sim_removed(struct ofono_sim *sim)
>  		return;
> 
>  	sim->inserted = FALSE;
> +	sim_inserted_update(sim);
> 
>  	if (sim->ready == FALSE)
>  		return;
> 
>  	sim->ready = FALSE;
> 
> +	if (sim->simop_source) {
> +		g_source_remove(sim->simop_source);
> +		sim->simop_source = 0;
> +	}
> +
> +	if (sim->simop_q) {
> +		/* NOTE: there's an assumption here that the op's callback
> +		 * does not queue new ops on failure... */
> +		g_queue_foreach(sim->simop_q, (GFunc) sim_file_op_fail, NULL);
> +		g_queue_free(sim->simop_q);
> +		sim->simop_q = NULL;
> +	}
> +

You have to be really careful here, it is possible that we have an operation 
pending with the driver, so the driver will call back for that one.  Also, 
since you're removing everything anyway I see no need to call the error 
callback.

>  	for (l = sim->removed_watches->items; l; l = l->next) {
>  		struct ofono_watchlist_item *item = l->data;
>  		notify = item->notify;
> 
>  		notify(item->notify_data);
>  	}
> +
> +	if (sim->imsi) {
> +		g_free(sim->imsi);
> +		sim->imsi = NULL;
> +	}
> +
> +	if (sim->own_numbers) {
> +		g_slist_foreach(sim->own_numbers, (GFunc)g_free, NULL);
> +		g_slist_free(sim->own_numbers);
> +		sim->own_numbers = NULL;
> +	}
> +
> +	if (sim->service_numbers) {
> +		g_slist_foreach(sim->service_numbers,
> +				(GFunc)service_number_free, NULL);
> +		g_slist_free(sim->service_numbers);
> +		sim->service_numbers = NULL;
> +	}
> +
> +	if (sim->efli) {
> +		g_free(sim->efli);
> +		sim->efli = NULL;
> +		sim->efli_length = 0;
> +	}
> +
> +	if (sim->language_prefs) {
> +		g_strfreev(sim->language_prefs);
> +		sim->language_prefs = NULL;
> +	}

This needs to be put into a common destructor instead of duplicating 
everything twice.

>  }
> 
>  static void sim_command_fetch_cb(const struct ofono_error *error,
> 

Regards,
-Denis