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