Re: [PATCH 06/11] ubloxmodem: add Toby L2 gprs context driver

[Dragos Tatulea <dragos@endocode.com>]

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

Hi Denis,

Thanks for looking into this. Please see my 2 objections below.

On 03/17/2016 12:25 AM, Denis Kenzior wrote:
> Hi Dragos,
> 
> On 03/14/2016 10:50 AM, Dragos Tatulea wrote:
>> For now the driver works only with bridged mode for 2G/3G.
>>
>> Once it activates the context it reads the ip, netmask,
>> gw, dns and sets them in the context settings.
>> ---
>>   Makefile.am                       |   7 +
>>   drivers/ubloxmodem/gprs-context.c | 415 ++++++++++++++++++++++++++++++++++++++
>>   drivers/ubloxmodem/ubloxmodem.c   |  49 +++++
>>   drivers/ubloxmodem/ubloxmodem.h   |  25 +++
>>   4 files changed, 496 insertions(+)
>>   create mode 100644 drivers/ubloxmodem/gprs-context.c
>>   create mode 100644 drivers/ubloxmodem/ubloxmodem.c
>>   create mode 100644 drivers/ubloxmodem/ubloxmodem.h
>>
>> diff --git a/Makefile.am b/Makefile.am
>> index cde998d..7652cfe 100644
>> --- a/Makefile.am
>> +++ b/Makefile.am
>> @@ -374,6 +374,13 @@ builtin_sources += drivers/atmodem/atutil.h \
>>               drivers/speedupmodem/speedupmodem.c \
>>               drivers/speedupmodem/ussd.c
>>
>> +builtin_modules += ubloxmodem
>> +builtin_sources += drivers/atmodem/atutil.h \
>> +            drivers/ubloxmodem/ubloxmodem.h \
>> +            drivers/ubloxmodem/ubloxmodem.c \
>> +            drivers/ubloxmodem/gprs-context.c
>> +
>> +
>>   if PHONESIM
>>   builtin_modules += phonesim
>>   builtin_sources += plugins/phonesim.c
>> diff --git a/drivers/ubloxmodem/gprs-context.c b/drivers/ubloxmodem/gprs-context.c
>> new file mode 100644
>> index 0000000..4b8fb6c
>> --- /dev/null
>> +++ b/drivers/ubloxmodem/gprs-context.c
>> @@ -0,0 +1,415 @@
>> +/*
>> + *
>> + *  oFono - Open Source Telephony
>> + *
>> + *  Copyright (C) 2016  EndoCode AG. All rights reserved.
>> + *
>> + *  This program is free software; you can redistribute it and/or modify
>> + *  it under the terms of the GNU General Public License version 2 as
>> + *  published by the Free Software Foundation.
>> + *
>> + *  This program is distributed in the hope that it will be useful,
>> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + *  GNU General Public License for more details.
>> + *
>> + *  You should have received a copy of the GNU General Public License
>> + *  along with this program; if not, write to the Free Software
>> + *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
>> + *
>> + */
>> +
>> +#ifdef HAVE_CONFIG_H
>> +#include <config.h>
>> +#endif
>> +
>> +#define _GNU_SOURCE
>> +#include <string.h>
>> +#include <stdlib.h>
>> +#include <stdio.h>
>> +#include <stdbool.h>
>> +#include <errno.h>
>> +
>> +#include <glib.h>
>> +
>> +#include <ofono/log.h>
>> +#include <ofono/modem.h>
>> +#include <ofono/gprs-context.h>
>> +
>> +#include "gatchat.h"
>> +#include "gatresult.h"
>> +
>> +#include "ubloxmodem.h"
>> +
>> +static const char *none_prefix[] = { NULL };
>> +static const char *cgcontrdp_prefix[] = { "+CGCONTRDP:", NULL };
>> +
>> +struct gprs_context_data {
>> +    GAtChat *chat;
>> +    unsigned int active_context;
>> +    char apn[OFONO_GPRS_MAX_APN_LENGTH + 1];
>> +    ofono_gprs_context_cb_t cb;
>> +    void *cb_data;
>> +};
>> +
>> +#define UBLOX_MAX_DEF_CONTEXT 8
> 
> What does this do?  Doesn't seem to be used in this file.
> 
>> +
>> +/*
>> + * CGCONTRDP returns addr + netmask in the same string in the form
>> + * of "a.b.c.d.m.m.m.m" for IPv4. IPv6 is not supported so we ignore it.
>> + */
>> +static int read_addrnetmask(struct ofono_gprs_context *gc,
>> +                const char *addrnetmask)
> 
> Since this function sets both parameters, how about:
> 
> set_address_and_netmask()
> 
>> +{
>> +    char *dup = strdup(addrnetmask);
>> +    char *s = dup;
>> +
>> +    const char *addr = s;
>> +    const char *netmask = NULL;
>> +
>> +    int ret = -EINVAL;
>> +    int i;
>> +
>> +    /* Count 7 dots for ipv4, less or more means error. */
>> +    for (i = 0; i < 8; i++, s++) {
>> +        s = strchr(s, '.');
>> +        if (!s)
>> +            break;
>> +
>> +        if (i == 3) {
>> +            /* set netmask ptr and break the string */
>> +            netmask = s+1;
>> +            s[0] = 0;
>> +        }
>> +    }
>> +
>> +    if (i == 7) {
>> +        ofono_gprs_context_set_ipv4_address(gc, addr, 1);
>> +        ofono_gprs_context_set_ipv4_netmask(gc, netmask);
>> +
>> +        ret = 0;
>> +    }
>> +
>> +    free(dup);
>> +
>> +    return ret;
>> +}
>> +
>> +static void callback_with_error(struct gprs_context_data *gcd, GAtResult *res)
> 
> Please inline this function to be more consistent with the rest of the codebase.
> 
>> +{
>> +    struct ofono_error error;
>> +
>> +    decode_at_error(&error, g_at_result_final_response(res));
>> +    gcd->cb(&error, gcd->cb_data);
>> +}
>> +
>> +
> 
> 1 empty line is enough. We don't use more than 1 empty line in a row. Yes, we're that pedantic :)
> 
>> +static void set_gprs_context_interface(struct ofono_gprs_context *gc)
>> +{
>> +    struct ofono_modem *modem;
>> +    const char *interface;
>> +
>> +    /* read interface name read at detection time */
>> +    modem = ofono_gprs_context_get_modem(gc);
>> +    interface = ofono_modem_get_string(modem, "NetworkInterface");
>> +    ofono_gprs_context_set_interface(gc, interface);
>> +}
>> +
>> +static void cgcontrdp_bridge_cb(gboolean ok, GAtResult *result,
>> +                gpointer user_data)
>> +{
>> +    struct ofono_gprs_context *gc = user_data;
>> +    struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
>> +    GAtResultIter iter;
>> +    int cid = -1;
>> +
>> +    const char *laddrnetmask = NULL;
>> +    const char *gw = NULL;
>> +    const char *dns[2+1] = { NULL, NULL, NULL };
> 
> Do dns[3] here.
> 
>> +
>> +    DBG("ok %d", ok);
>> +
>> +    if (!ok) {
>> +        callback_with_error(gcd, result);
> 
> as mentioned above, lets inline the contents of callback_with_error here:
> e.g.
> decode_at_error(...);
> cb(...)
> 
>> +
>> +        return;
>> +    }
>> +
>> +    g_at_result_iter_init(&iter, result);
>> +
>> +    while (g_at_result_iter_next(&iter, "+CGCONTRDP:")) {
>> +        /* tmp vals for ignored fields */
>> +        int bearer_id;
>> +        const char *apn;
>> +
>> +        if (!g_at_result_iter_next_number(&iter, &cid))
>> +            break;
>> +
>> +        if (!g_at_result_iter_next_number(&iter, &bearer_id))
>> +            break;
>> +
>> +        if (!g_at_result_iter_next_string(&iter, &apn))
>> +            break;
>> +
>> +        if (!g_at_result_iter_next_string(&iter, &laddrnetmask))
>> +            break;
>> +
>> +        if (!g_at_result_iter_next_string(&iter, &gw))
>> +            break;
>> +
>> +        if (!g_at_result_iter_next_string(&iter, &dns[0]))
>> +            break;
>> +
>> +        if (!g_at_result_iter_next_string(&iter, &dns[1]))
>> +            break;
>> +    }
>> +
>> +    set_gprs_context_interface(gc);
>> +
>> +    if (!laddrnetmask || read_addrnetmask(gc, laddrnetmask) < 0) {
>> +        CALLBACK_WITH_FAILURE(gcd->cb, gcd->cb_data);
>> +        return;
>> +    }
>> +
>> +    if (gw)
>> +        ofono_gprs_context_set_ipv4_gateway(gc, gw);
>> +
>> +    if (dns[0])
>> +        ofono_gprs_context_set_ipv4_dns_servers(gc, dns);
>> +
>> +    CALLBACK_WITH_SUCCESS(gcd->cb, gcd->cb_data);
>> +}
>> +
>> +static int ublox_read_ip_config_bridge(struct ofono_gprs_context *gc)
>> +{
>> +    struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
>> +    char buf[64];
>> +
>> +    /* read ip configuration info */
>> +    snprintf(buf, sizeof(buf), "AT+CGCONTRDP=%u", gcd->active_context);
>> +    return g_at_chat_send(gcd->chat, buf, cgcontrdp_prefix,
>> +                cgcontrdp_bridge_cb, gc, NULL);
>> +
>> +}
>> +
>> +static void ublox_post_activation(struct ofono_gprs_context *gc)
>> +{
>> +    struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
>> +
>> +    if (ublox_read_ip_config_bridge(gc) < 0)
>> +        CALLBACK_WITH_FAILURE(gcd->cb, gcd->cb_data);
> 
> Since this is a two line function, it would be easier to read the code if it is moved into cgact_enable_cb
> 
> In fact, just sending CGCONTRDP inside cgact_enable_cb would be the simplest.

The next patches will need this as a standalone function. Another line for reading the router mode ip config will be introduced. And the function will be called from 3 places:
* directly in pri_context_activate (this patch)
* after authentication command (authentication support patch)
* from read_settings function (automatic context activation patch)

> 
>> +}
>> +
>> +static void cgact_enable_cb(gboolean ok, GAtResult *result, gpointer user_data)
>> +{
>> +    struct ofono_gprs_context *gc = user_data;
>> +    struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
>> +
>> +    DBG("ok %d", ok);
>> +    if (!ok) {
>> +        gcd->active_context = 0;
>> +        callback_with_error(gcd, result);
>> +
>> +        return;
>> +    }
>> +
>> +    ublox_post_activation(gc);
>> +}
>> +
>> +static void cgdcont_cb(gboolean ok, GAtResult *result, gpointer user_data)
>> +{
>> +    struct ofono_gprs_context *gc = user_data;
>> +    struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
>> +    char buf[64];
>> +
>> +    DBG("ok %d", ok);
>> +
>> +    if (!ok) {
>> +        gcd->active_context = 0;
>> +        callback_with_error(gcd, result);
>> +
>> +        return;
>> +    }
>> +
>> +    snprintf(buf, sizeof(buf), "AT+CGACT=1,%u", gcd->active_context);
>> +    if (g_at_chat_send(gcd->chat, buf, none_prefix,
>> +                cgact_enable_cb, gc, NULL))
>> +        return;
>> +
>> +    CALLBACK_WITH_FAILURE(gcd->cb, gcd->cb_data);
>> +}
>> +
>> +static void ublox_activate_ctx(struct ofono_gprs_context *gc)
> 
> Lets name this ublox_send_cgdcont, so it is clear what this helper function actually does
> 
>> +{
>> +    struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
>> +    char buf[OFONO_GPRS_MAX_APN_LENGTH + 128];
>> +    int len;
>> +
>> +    len = snprintf(buf, sizeof(buf), "AT+CGDCONT=%u,\"IP\"",
>> +                gcd->active_context);
>> +
>> +    if (gcd->apn)
>> +        snprintf(buf + len, sizeof(buf) - len - 3, ",\"%s\"",
>> +                    gcd->apn);
>> +
>> +    if (g_at_chat_send(gcd->chat, buf, none_prefix,
>> +                cgdcont_cb, gc, NULL) > 0)
>> +        return;
>> +
>> +    CALLBACK_WITH_FAILURE(gcd->cb, gcd->cb_data);
>> +}
>> +
>> +#define UBLOX_MAX_USER_LEN 50
>> +#define UBLOX_MAX_PASS_LEN 50
>> +
> 
> Why is this defined here?  Does it belong in a later patch?
> 
>> +static void ublox_gprs_activate_primary(struct ofono_gprs_context *gc,
>> +                const struct ofono_gprs_primary_context *ctx,
>> +                ofono_gprs_context_cb_t cb, void *data)
>> +{
>> +    struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
>> +
>> +    /* IPv6 support not implemented */
>> +    if (ctx->proto != OFONO_GPRS_PROTO_IP) {
>> +        CALLBACK_WITH_FAILURE(cb, data);
>> +        return;
>> +    }
>> +
>> +    DBG("cid %u", ctx->cid);
>> +
>> +    gcd->active_context = ctx->cid;
>> +    if (!gcd->active_context) {
>> +        ofono_error("can't activate more contexts");
>> +        CALLBACK_WITH_FAILURE(cb, data);
>> +        return;
>> +    }
>> +
>> +    gcd->cb = cb;
>> +    gcd->cb_data = data;
>> +    memcpy(gcd->apn, ctx->apn, sizeof(ctx->apn));
> 
> Why do we memcpy the APN into this structure?  If you don't want to make this function too long, then just pass the apn as a parameter to ublox_activate_ctx.
> 
The APN will be required by another patch that sends authentication command and then sends CGDCONT.

>> +
>> +    ublox_activate_ctx(gc);
>> +}
>> +
>> +static void cgact_disable_cb(gboolean ok, GAtResult *result, gpointer user_data)
>> +{
>> +    struct ofono_gprs_context *gc = user_data;
>> +    struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
>> +
>> +    DBG("ok %d", ok);
> 
> Empty line here after DBG, see item M1 in doc/coding-style.txt
> 
>> +    if (!ok) {
>> +        CALLBACK_WITH_FAILURE(gcd->cb, gcd->cb_data);
>> +        return;
>> +    }
>> +
>> +    gcd->active_context = 0;
>> +
>> +    CALLBACK_WITH_SUCCESS(gcd->cb, gcd->cb_data);
>> +}
>> +
>> +static void ublox_gprs_deactivate_primary(struct ofono_gprs_context *gc,
>> +                    unsigned int cid,
>> +                    ofono_gprs_context_cb_t cb, void *data)
>> +{
>> +    struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
>> +    char buf[64];
>> +
>> +    DBG("cid %u", cid);
>> +
>> +    gcd->cb = cb;
>> +    gcd->cb_data = data;
>> +
>> +    snprintf(buf, sizeof(buf), "AT+CGACT=0,%u", gcd->active_context);
>> +    g_at_chat_send(gcd->chat, buf, none_prefix,
>> +            cgact_disable_cb, gc, NULL);
>> +}
>> +
>> +static void cgev_notify(GAtResult *result, gpointer user_data)
>> +{
>> +    struct ofono_gprs_context *gc = user_data;
>> +    struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
>> +    GAtResultIter iter;
>> +    const char *event;
>> +    gint cid;
>> +
>> +    g_at_result_iter_init(&iter, result);
>> +
>> +    if (!g_at_result_iter_next(&iter, "+CGEV:"))
>> +        return;
>> +
>> +    if (!g_at_result_iter_next_unquoted_string(&iter, &event))
>> +        return;
>> +
>> +    if (g_str_has_prefix(event, "NW PDN DEACT")) {
>> +        if (!g_at_result_iter_skip_next(&iter))
>> +            return;
>> +    } else if (g_str_has_prefix(event, "NW DEACT") == FALSE)
>> +        return;
>> +
>> +    if (!g_at_result_iter_skip_next(&iter))
>> +        return;
>> +
>> +    if (!g_at_result_iter_next_number(&iter, &cid))
>> +        return;
>> +
>> +    DBG("cid %d", cid);
>> +
>> +    if ((unsigned int) cid != gcd->active_context)
>> +        return;
>> +
>> +    ofono_gprs_context_deactivated(gc, gcd->active_context);
>> +    gcd->active_context = 0;
>> +}
>> +
>> +
>> +static int ublox_gprs_context_probe(struct ofono_gprs_context *gc,
>> +                    unsigned int vendor, void *data)
>> +{
>> +    GAtChat *chat = data;
>> +    struct gprs_context_data *gcd;
>> +
>> +    DBG("");
>> +
>> +    gcd = g_try_new0(struct gprs_context_data, 1);
>> +    if (gcd == NULL)
>> +        return -ENOMEM;
>> +
>> +    gcd->chat = g_at_chat_clone(chat);
>> +
>> +    ofono_gprs_context_set_data(gc, gcd);
>> +
>> +    g_at_chat_register(chat, "+CGEV:", cgev_notify, FALSE, gc, NULL);
>> +
>> +    return 0;
>> +}
>> +
>> +static void ublox_gprs_context_remove(struct ofono_gprs_context *gc)
>> +{
>> +    struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
>> +
>> +    DBG("");
>> +
>> +    ofono_gprs_context_set_data(gc, NULL);
>> +
>> +    g_at_chat_unref(gcd->chat);
>> +
>> +    memset(gcd, 0, sizeof(*gcd));
>> +}
>> +
>> +static struct ofono_gprs_context_driver driver = {
>> +    .name            = "ubloxmodem",
>> +    .probe            = ublox_gprs_context_probe,
>> +    .remove            = ublox_gprs_context_remove,
>> +    .activate_primary    = ublox_gprs_activate_primary,
>> +    .deactivate_primary    = ublox_gprs_deactivate_primary,
>> +};
>> +
>> +
> 
> 
>> +void ublox_gprs_context_init(void)
>> +{
>> +    ofono_gprs_context_driver_register(&driver);
>> +}
>> +
>> +void ublox_gprs_context_exit(void)
>> +{
>> +    ofono_gprs_context_driver_unregister(&driver);
>> +}
> 
> <snip>
> 
> Looks great. Lets fix these minor nitpicks and I'll apply it.
> 
> Regards,
> -Denis
> _______________________________________________
> ofono mailing list
> ofono(a)ofono.org
> https://lists.ofono.org/mailman/listinfo/ofono

Thanks
-- 
Dragos Tatulea
Software Developer @ Endocode AG
dragos(a)endocode.com

Endocode AG, Brückenstraße 5A, 10179 Berlin
+49 30 1206 4472 | info(a)endocode.com | www.endocode.com

Vorstandsvorsitzender: Mirko Boehm
Vorstände: Dr. Thomas Fricke, Sebastian Sucker
Aufsichtsratsvorsitzende: Alexandra Boehm

Registergericht: Amtsgericht Charlottenburg - HRB 150748 B