On 05/18/2015 01:53 AM, Alfonso Sanchez-Beato wrote:
> Hi Denis,
>

<snip>

>         +
>         +       for (l = gprs->contexts; l; l = l->next) {
>         +               struct pri_context *ctx = l->data;
>         +
>         +               if (ctx->pending)
>         +                       return __ofono_error_busy(msg);
>         +       }
>         +
>         +       if (!dbus_message_get_args(msg, NULL, DBUS_TYPE_INVALID))
>         +               return __ofono_error_invalid_args(msg);
>         +
>         +       if (gprs->powered)
>         +               return __ofono_error_not_allowed(msg);
>         +
>         +       for (l = gprs->contexts; l; l = l->next) {
>         +               struct pri_context *ctx = l->data;
>         +
>         +               if (ctx->active)
>         +                       return __ofono_error_not_allowed(msg);
>
>
>     Is there a particular reason you're running through the list of
>     contexts again here?  Why not check for ctx->active at the same time
>     as ctx->pending?
>
>
> I wanted to make sure that __ofono_error_busy was returned before
> __ofono_error_not_allowed if one context was busy and another one was
> active, for instance. Not that I feel is really important and I can
> change this if the other way is preferred.
>

Makes sense.  However, since this is not obvious, we probably should add 
a comment as to why this is being done.  Want to submit a patch or do 
you want me to do this?

Regards,
-Denis