Re: [PATCHv3 03/12] call-forwarding: Refactor cf_condition_find_with_cls() slightly

[Denis Kenzior <denkenz@gmail.com>]

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

Hi Oleg,

On 03/13/2012 08:46 AM, Oleg Zhurakivskyy wrote:
> Mainly to simplify the usage.
> ---
>  src/call-forwarding.c |   98 +++++++++++++-----------------------------------
>  1 files changed, 27 insertions(+), 71 deletions(-)
> 
> diff --git a/src/call-forwarding.c b/src/call-forwarding.c
> index 8b6659a..5a2ab28 100644
> --- a/src/call-forwarding.c
> +++ b/src/call-forwarding.c
> @@ -42,6 +42,8 @@
>  /* According to 27.007 Spec */
>  #define DEFAULT_NO_REPLY_TIMEOUT 20
>  
> +#define CFC(_x) ((struct ofono_call_forwarding_condition *)(_x))
> +

I'm okay with such helpers, but...

>  #define is_cfu_enabled(_cf)				\
>  ({							\
>  	cf_find_unconditional(_cf) ? TRUE : FALSE;	\
> @@ -86,63 +88,36 @@ static void get_query_next_cf_cond(struct ofono_call_forwarding *cf);
>  static void set_query_next_cf_cond(struct ofono_call_forwarding *cf);
>  static void ss_set_query_next_cf_cond(struct ofono_call_forwarding *cf);
>  
> -static gint cf_condition_compare(gconstpointer a, gconstpointer b)
> +static gint cf_compare(gconstpointer a, gconstpointer b)
>  {
> -	const struct ofono_call_forwarding_condition *ca = a;
> -	const struct ofono_call_forwarding_condition *cb = b;
> -
> -	if (ca->cls < cb->cls)
> -		return -1;
> -
> -	if (ca->cls > cb->cls)
> -		return 1;
> -
> -	return 0;
> +	return CFC(a)->cls - CFC(b)->cls;

You're breaking const-correctness here.

Also, why do you rename the function?  I do think we should have
'condition' or at least 'cond' in the name to make it clear what it
does.  My vote is for cf_cond_compare...

>  }
>  
> -static gint cf_condition_find_with_cls(gconstpointer a, gconstpointer b)
> +static struct ofono_call_forwarding_condition *cf_find(GSList *l, int cls)

Same as above, at least name it cf_cond_find.  Also, I'd prefer changes
to this function to be in a separate patch.

>  {
> -	const struct ofono_call_forwarding_condition *c = a;
> -	int cls = GPOINTER_TO_INT(b);
> -
> -	if (c->cls < cls)
> -		return -1;
> -
> -	if (c->cls > cls)
> -		return 1;
> +	for (; l; l = l->next)
> +		if (cls == CFC(l->data)->cls)
> +			return CFC(l->data);
>  
> -	return 0;
> +	return NULL;
>  }
>  
> -static int cf_find_timeout(GSList *cf_list, int cls)
> +static int cf_find_timeout(GSList *l, int cls)

Same here, lets name this cf_cond_find_timeout

>  {
> -	GSList *l;
> -	struct ofono_call_forwarding_condition *c;
> -
> -	l = g_slist_find_custom(cf_list, GINT_TO_POINTER(cls),
> -		cf_condition_find_with_cls);
> -
> -	if (l == NULL)
> -		return DEFAULT_NO_REPLY_TIMEOUT;
> -
> -	c = l->data;
> +	struct ofono_call_forwarding_condition *c = cf_find(l, cls);
>  
> -	return c->time;
> +	return c ? c->time : DEFAULT_NO_REPLY_TIMEOUT;
>  }
>  
> -static void cf_cond_list_print(GSList *list)
> +static void cf_cond_list_print(GSList *l)
>  {
> -	GSList *l;
> -	struct ofono_call_forwarding_condition *cond;
> -
> -	for (l = list; l; l = l->next) {
> -		cond = l->data;
> +	struct ofono_call_forwarding_condition *c;
>  
> +	for (; l && (c = l->data); l = l->next)

After going back and forth on this one, I'm actually against this style.
 Mainly it is too hard to notice the assignment, inside the continuation
check expression.  I'd really prefer that we assign c inside the for
loop block.  Also, l->data should never be NULL, so in this case I'd
rather crash and find out early than inadvertently covering it up.

>  		DBG("CF Condition status: %d, class: %d, number: %s,"
>  			" number_type: %d, time: %d",
> -			cond->status, cond->cls, cond->phone_number.number,
> -			cond->phone_number.type, cond->time);
> -	}
> +			c->status, c->cls, c->phone_number.number,
> +			c->phone_number.type, c->time);
>  }
>  
>  static GSList *cf_cond_list_create(int total,
> @@ -175,8 +150,7 @@ static GSList *cf_cond_list_create(int total,
>  				sizeof(struct ofono_call_forwarding_condition));
>  			cond->cls = j;
>  
> -			l = g_slist_insert_sorted(l, cond,
> -							cf_condition_compare);
> +			l = g_slist_insert_sorted(l, cond, cf_compare);
>  		}
>  	}
>  
> @@ -330,12 +304,8 @@ static void set_new_cond_list(struct ofono_call_forwarding *cf,
>  		if (type == CALL_FORWARDING_TYPE_NO_REPLY)
>  			snprintf(tattr, sizeof(tattr), "%sTimeout", attr);
>  
> -		o = g_slist_find_custom(old, GINT_TO_POINTER(lc->cls),
> -					cf_condition_find_with_cls);
> -
> -		if (o) { /* On the old list, must be active */
> -			oc = o->data;
> -
> +		oc = cf_find(old, lc->cls);
> +		if (oc) { /* On the old list, must be active */
>  			if (oc->phone_number.type != lc->phone_number.type ||
>  				strcmp(oc->phone_number.number,
>  					lc->phone_number.number)) {
> @@ -356,8 +326,8 @@ static void set_new_cond_list(struct ofono_call_forwarding *cf,
>  						&timeout);
>  
>  			/* Remove from the old list */
> -			g_free(o->data);
> -			old = g_slist_remove(old, o->data);
> +			old = g_slist_remove(old, oc);
> +			g_free(oc);
>  		} else {
>  			number = phone_number_to_string(&lc->phone_number);
>  
> @@ -435,21 +405,12 @@ static void set_new_cond_list(struct ofono_call_forwarding *cf,
>  			if (i == CALL_FORWARDING_TYPE_UNCONDITIONAL)
>  				continue;
>  
> -			l = g_slist_find_custom(cf->cf_conditions[i],
> -					GINT_TO_POINTER(BEARER_CLASS_VOICE),
> -					cf_condition_find_with_cls);
> -
> -			if (l == NULL)
> +			lc = cf_find(cf->cf_conditions[i], BEARER_CLASS_VOICE);
> +			if (lc == NULL)
>  				continue;
>  
> -			if (new_cfu)
> -				number = "";
> -			else {
> -				lc = l->data;
> -
> -				number = phone_number_to_string(
> +			number = new_cfu ? "" : phone_number_to_string(
>  							&lc->phone_number);

This part really belongs in a separate patch.

> -			}
>  
>  			ofono_dbus_signal_property_changed(conn, path,
>  						OFONO_CALL_FORWARDING_INTERFACE,
> @@ -796,7 +757,6 @@ static DBusMessage *cf_set_property(DBusConnection *conn, DBusMessage *msg,
>  
>  	if (cf_condition_timeout_property(property, &cls)) {
>  		dbus_uint16_t timeout;
> -		GSList *l;
>  		struct ofono_call_forwarding_condition *c;
>  
>  		type = CALL_FORWARDING_TYPE_NO_REPLY;
> @@ -809,15 +769,11 @@ static DBusMessage *cf_set_property(DBusConnection *conn, DBusMessage *msg,
>  		if (timeout < 1 || timeout > 30)
>  			return __ofono_error_invalid_format(msg);
>  
> -		l = g_slist_find_custom(cf->cf_conditions[type],
> -					GINT_TO_POINTER(cls),
> -					cf_condition_find_with_cls);
>  
> -		if (l == NULL)
> +		c = cf_find(cf->cf_conditions[type], cls);
> +		if (c == NULL)
>  			return __ofono_error_failed(msg);
>  
> -		c = l->data;
> -
>  		return set_property_request(cf, msg, type, cls,
>  						&c->phone_number, timeout);
>  	} else if (cf_condition_enabled_property(cf, property, &type, &cls)) {

Regards,
-Denis