Fwd: Cinterion plugin patch

[Aleksander Morgado]

Hey!

This looks much much better :)
See comments next, in no particular order.

On Mon, Oct 24, 2016 at 7:13 AM, matthew stanger <stangerm2 at gmail.com> wrote:
> Here is finally the update... to the above code reviewed patch. A big thank
> you Dan and Aleksander for the detail in the review it was really a great
> learning experience for me. Please don't hold back with this one either :).
> It's a pretty big rewrite so I'm sure there will still be some minor things
> to tidy up still but I think it should be more inline now. There is one
> thing in here for sure that needs fixing still but I wanted your options so
> please see the note at line 1192 and let me know what you think.

Regarding:

> +    /* TODO: Before upstream merge. This needs check_for_swwan_support to block
> +     * until it's finish. Otherwise there is race conidion with swwan_support assert?
> + * I'm really not sure how else to do this other than move it or use sleep :? */


The check_for_swwan_support() is calling
mm_base_modem_at_command_full() with a callback, so the whole
"enabling_modem_init" step will be running until the AT command is
finished and you can see whether SWWAN is supported or not. BTW; given
it's a "test" command (i.e. ^SWWAN=? should always reply the same
thing regardless of the modem state), you can use allow_cached=TRUE in
the mm_base_modem_at_command_full() call, so that if it ever gets
called again we don't ask the modem but use the cached response
instead.

More comments:

> +static void
> +disconnect_3gpp (MMBroadbandBearer *self,
> +                 MMBroadbandModem *modem,
> +                 MMPortSerialAt *primary,
> +                 MMPortSerialAt *secondary,
> +                 MMPort *data,
> +                 guint cid,
> +                 GAsyncReadyCallback callback,
> +                 gpointer user_data)
> +{
> +    Disconnect3gppContext *ctx;
> +    MMPort *port;
> +
> +    g_assert (primary != NULL);
> +
> +    /* Note: Not sure how else to get active data port? Can this be done without adding this
> +     * function to mm-base-modem.c?
> +     * TODO: Dual SIM how do we know which interface to grab/disconnect if two are active? */
> +    /* Get the Net port to be torn down */
> +    port = mm_base_modem_peek_current_data_port (MM_BASE_MODEM (modem), MM_PORT_TYPE_NET);

You don't need this peek_current_data_port() method; the "MMPort
*data" that you get as input is already the NET port that is being
disconnected.

> +static void
> +send_write_command_ctx_connect (Connect3gppContext *ctx,
> +                                gchar **command)

You can pass the command as a "const gchar *command" instead of gchar
**, which seems weird because the command string isn't being updated
inside the method.

> +    if (g_strcmp0 (bearer_interface, "0a") == 0)
> +        self->priv->pdp_cid = 3;
> +    else if (g_strcmp0 (bearer_interface, "0c") == 0)
> +        self->priv->pdp_cid = 1;

I know this works, but to me it looks saner to get the interface
number with mm_kernel_device_get_property_as_int() and then directly
do a numeric comparison with 0x0a or 0x0c.

>
> +static gint
> +cinterion_parse_auth_type (MMBearerAllowedAuth mm_auth)


This can return a BearerCinterionAuthType instead of an int, right?

> +static void
> +pdp_cid_map (MMBroadbandBearerCinterion *self, const gchar *bearer_interface)
> +{
> +    /* Map PDP context from the current Bearer. USB0 -> 3rd context, USB1 -> 1st context.
> +     * Note - Cinterion told me specifically to map the contexts to the interfaces this way, though
> +     * I've seen that SWWAN appear's to work fine with any PDP to any interface */

I would ask Cinterion for confirmation on why this mapping is needed.
Maybe those are just the defaults if the <WWAN adapter> parameter in
SWWAN isn't given?


> +    g_list_free(response_parsed);

Note we include a whitespace always before every parenthesis, even in
method calls. There are multiple places in the code where this is
still being done.

> +    /* Get the SWWAN read response */
> +    response = mm_base_modem_at_command_finish (modem, res, &error);
> +
> +    /* Error if parsing SWWAN read response fails */
> +    if (!mm_cinterion_parse_swwan_response (response, &response_parsed, &error)) {
> +        g_simple_async_result_take_error (ctx->result, error);
> +        connect_3gpp_context_complete_and_free (ctx);
> +        return;
> +    }

You shouldn't do this. Before calling
mm_cinterion_parse_swwan_response
(), first check whether you have a NULL response and therefore error
already set, e.g.:

response = mm_base_modem_at_command_finish (modem, res, &error);
if (!response || !mm_cinterion_parse_swwan_response (response,
&response_parsed, &error)) {
    g_simple_async_result_take_error (ctx->result, error);
    connect_3gpp_context_complete_and_free (ctx);
    return;
}

> +gboolean
> +mm_cinterion_parse_swwan_response (const gchar *response,
> +                                   GList **result,
> +                                   GError **error)
> +{
> +    if (*error)
> +        return FALSE;
> +
>
> +    if (!response) {
> +        g_set_error (error,
> +                     MM_CORE_ERROR,
> +                     MM_CORE_ERROR_FAILED,
> +                     "Recieved NULL from SWWAN response.");
> +        return FALSE;
> +    }

I would actually remove this two checks (the *error and the !response
one) all together. You don't need them if you're making sure response
is set in the caller. You can g_assert (response); if you want, but
just ignore the state of error, error should always be considered a
pointer to a GError which is set to NULL, that is the convention used
for all methods accepting a GError.

> +gboolean
> +mm_cinterion_parse_swwan_response (const gchar *response,
> +                                   GList **result,
> +                                   GError **error)
> ...
> +        *result = g_list_append (*result, GINT_TO_POINTER(cid));
> +        *result = g_list_append (*result, GINT_TO_POINTER(state));
> +        *result = g_list_append (*result, GINT_TO_POINTER(wwan_adapter));

Instead of returning a blind list of integers, let's do it in some
other way. You can define in the API of the helpers a struct like
this:
typedef struct {
    guint cid;
    guint state;
    guint wwan_adapter;
} SwwanResponseItem;

Your parse_swwan_response() could then be like you had it implemented,
but instead of a list of integers, you would return a list of
SwwanResponseItem structs. This will also support the SWWAN responses
with multiple lines, which is what the command would do:
  AT^SWWAN?
  [^SWWAN: <cid>, <state>[, <WWAN adapter>]]
  [^SWWAN: ...]

> +    /* TODO: It'd be nice to get 'OK' from response so we don't have to assume that
> +     * zero length response means 'OK' or am I doing it wrong?... */
> +    else if (!g_utf8_strlen (response, 100))
> +        *result = g_list_append (*result, GINT_TO_POINTER(0));

No need for that g_utf8_strlen(); if you get an empty string that was
because the response was just "OK". With the new API with the list of
SwwanResponseItem items, you would be returning TRUE in the method and
a NULL list (i.e. empty list).

Also, note that the command response format says that <WWAN adapter>
is optional. Does that mean that there are cases where SWWAN doesn't
return the WWAN adapter? If so, the sscanf() won't return a 3 and you
would be returning an error.

> +    /* 3rd context -> 1st wwan adapt / 1st context -> 2nd wwan adapt */
> +    gchar               *command;

Just one single whitespace in between the type and name is enough,
unless you're aligning things, which doesn't seem the case here. There
are other places in the patch where the same happens I think.


-- 
Aleksander
https://aleksander.es