Hi Marcel,

On 12/19/2011 06:48 AM, Marcel Holtmann wrote:
> Hi Philippe,
>
>>   plugins/udevng.c |   61 +++++++++++++++++++++++++++++++++--------------------
>>   1 files changed, 38 insertions(+), 23 deletions(-)
>>
>> diff --git a/plugins/udevng.c b/plugins/udevng.c
>> index be87320..6a3c970 100644
>> --- a/plugins/udevng.c
>> +++ b/plugins/udevng.c
>> @@ -235,6 +235,7 @@ static gboolean setup_sierra(struct modem_info *modem)
>>   static gboolean setup_huawei(struct modem_info *modem)
>>   {
>>   	const char *mdm = NULL, *pcui = NULL, *diag = NULL;
>> +	const char *default_pcui = NULL;
>>   	GSList *list;
>>
>>   	DBG("%s", modem->syspath);
>> @@ -264,19 +265,24 @@ static gboolean setup_huawei(struct modem_info *modem)
>>   			if (mdm != NULL&&  pcui != NULL)
>>   				break;
>>   		} else if (g_strcmp0(info->interface, "255/255/255") == 0) {
>> -			if (g_strcmp0(info->number, "00") == 0)
>> +			if (mdm == NULL&&  g_strcmp0(info->number, "00") == 0)
>>   				mdm = info->devnode;
>> -			else if (g_strcmp0(info->number, "01") == 0)
>> -				pcui = info->devnode;
>> -			else if (g_strcmp0(info->number, "02") == 0)
>> -				pcui = info->devnode;
>> -			else if (g_strcmp0(info->number, "03") == 0)
>> -				pcui = info->devnode;
>> -			else if (g_strcmp0(info->number, "04") == 0)
>> -				pcui = info->devnode;
>> +			else if (pcui == NULL) {
>> +				if (g_strcmp0(info->number, "01") == 0)
>> +					default_pcui = info->devnode;
>> +				else if (g_strcmp0(info->number, "02") == 0)
>> +					default_pcui = info->devnode;
>> +				else if (g_strcmp0(info->number, "03") == 0)
>> +					default_pcui = info->devnode;
>> +				else if (g_strcmp0(info->number, "04") == 0)
>> +					default_pcui = info->devnode;
>> +			}
>>   		}
>>   	}
>>
>> +	if (pcui == NULL&&  default_pcui != NULL)
>> +		pcui = default_pcui;
>> +
>
> what is this default_pcui doing here. I am failing to understand its
> usage.

We need to prevent the assignment done through the label, to be 
overridden by the 'default' assignment (the one done by the implicit rule).

That's why I introduced new conditions:

	if (mdm == NULL && g_strcmp0(info->number, "00") == 0)

	and

	else if (pcui == NULL) {

In practice, if the pcui is already assigned through the label, the 
'default' assignment' algorithm is not considered.
On the contrary, the 'default' assignment is done using a dedicated 
variable 'default_pcui'.
If we were still using the variable 'pcui', we couldn't update anymore 
this variable through the 'default' assignment due to the new condition 
'if (pcui == NULL)'.

>
>>   	if (mdm == NULL || pcui == NULL)
>>   		return FALSE;
>>
>> @@ -291,7 +297,7 @@ static gboolean setup_huawei(struct modem_info *modem)
>>
>>   static gboolean setup_speedup(struct modem_info *modem)
>>   {
>> -	const char *aux = NULL, *mdm = NULL;
>> +	const char *aux = NULL, *mdm = NULL, *default_mdm = NULL;
>>   	GSList *list;
>>
>>   	DBG("%s", modem->syspath);
>> @@ -311,15 +317,20 @@ static gboolean setup_speedup(struct modem_info *modem)
>>   			if (aux != NULL)
>>   				break;
>>   		} else if (g_strcmp0(info->interface, "255/255/255") == 0) {
>> -			if (g_strcmp0(info->number, "01") == 0)
>> +			if (aux == NULL&&  g_strcmp0(info->number, "01") == 0)
>>   				aux = info->devnode;
>> -			else if (g_strcmp0(info->number, "02") == 0)
>> -				mdm = info->devnode;
>> -			else if (g_strcmp0(info->number, "03") == 0)
>> -				mdm = info->devnode;
>> +			else if (mdm == NULL) {
>> +				if (g_strcmp0(info->number, "02") == 0)
>> +					default_mdm = info->devnode;
>> +				else if (g_strcmp0(info->number, "03") == 0)
>> +					default_mdm = info->devnode;
>> +			}
>>   		}
>>   	}
>>
>> +	if (mdm == NULL&&  default_mdm != NULL)
>> +		mdm = default_mdm;
>> +
>
> Same here. This makes the code pretty much unreadable.

This logic becomes however necessary as I introduced dedicated rules for 
Speedup and Huawei based on the label.
Without this patch, the 'label' assignment doesn't work necessarily (in 
fact, it depends of the order of the interfaces).


>
>>   	if (aux == NULL || mdm == NULL)
>>   		return FALSE;
>>
>> @@ -385,9 +396,10 @@ static gboolean setup_alcatel(struct modem_info *modem)
>>   			if (aux != NULL)
>>   				break;
>>   		} else if (g_strcmp0(info->interface, "255/255/255") == 0) {
>> -			if (g_strcmp0(info->number, "03") == 0)
>> +			if (aux == NULL&&  g_strcmp0(info->number, "03") == 0)
>>   				aux = info->devnode;
>> -			else if (g_strcmp0(info->number, "05") == 0)
>> +			else if (mdm == NULL&&
>> +					g_strcmp0(info->number, "05") == 0)
>>   				mdm = info->devnode;
>>   		}
>>   	}
>> @@ -425,9 +437,10 @@ static gboolean setup_novatel(struct modem_info *modem)
>>   			if (aux != NULL)
>>   				break;
>>   		} else if (g_strcmp0(info->interface, "255/255/255") == 0) {
>> -			if (g_strcmp0(info->number, "00") == 0)
>> +			if (aux == NULL&&  g_strcmp0(info->number, "00") == 0)
>>   				aux = info->devnode;
>> -			else if (g_strcmp0(info->number, "01") == 0)
>> +			else if (mdm == NULL&&
>> +					g_strcmp0(info->number, "01") == 0)
>>   				mdm = info->devnode;
>>   		}
>>   	}
>> @@ -465,9 +478,10 @@ static gboolean setup_nokia(struct modem_info *modem)
>>   			if (aux != NULL)
>>   				break;
>>   		} else if (g_strcmp0(info->interface, "10/0/0") == 0) {
>> -			if (g_strcmp0(info->number, "02") == 0)
>> +			if (mdm == NULL&&  g_strcmp0(info->number, "02") == 0)
>>   				mdm = info->devnode;
>> -			else if (g_strcmp0(info->number, "04") == 0)
>> +			else if (aux == NULL&&
>> +					g_strcmp0(info->number, "04") == 0)
>>   				aux = info->devnode;
>>   		}
>>   	}
>> @@ -505,13 +519,14 @@ static gboolean setup_telit(struct modem_info *modem)
>>   			if (aux != NULL)
>>   				break;
>>   		} else if (g_strcmp0(info->interface, "255/255/255") == 0) {
>> -			if (g_strcmp0(info->number, "00") == 0)
>> +			if (mdm == NULL&&  g_strcmp0(info->number, "00") == 0)
>>   				mdm = info->devnode;
>>   			else if (g_strcmp0(info->number, "01") == 0)
>>   				diag = info->devnode;
>>   			else if (g_strcmp0(info->number, "02") == 0)
>>   				gps = info->devnode;
>> -			else if (g_strcmp0(info->number, "03") == 0)
>> +			else if (aux == NULL
>> +					&&  g_strcmp0(info->number, "03") == 0)
>>   				aux = info->devnode;
>>   		}
>>   	}
>
> This becomes unreadable. What are we trying to achieve here.

For Alcatel, Novatel, Nokia, Telit, I don't have a bug stating that we 
could have an overriding issue.
I just propagated the logic explained above for more safety.

Regards,

Philippe.


>
> Regards
>
> Marcel
>
>
> _______________________________________________
> ofono mailing list
> ofono(a)ofono.org
> http://lists.ofono.org/listinfo/ofono
>