Hey!
This looks much better now :)
See comments below.
On Tue, Dec 15, 2015 at 3:44 PM, Carlo Lobrano <c.lobr...@gmail.com> wrote:
> From: Carlo Lobrano <c.lobr...@gmail.com>
>
> ---
> plugins/Makefile.am | 18 +-
> plugins/telit/mm-broadband-modem-telit.c | 192
> ++++++++++++++++++++++
> plugins/telit/mm-modem-helpers-telit.c | 77 +++++++++
> plugins/telit/mm-modem-helpers-telit.h | 25 +++
> plugins/telit/tests/test-mm-modem-helpers-telit.c | 101 ++++++++++++
> 5 files changed, 412 insertions(+), 1 deletion(-)
> create mode 100644 plugins/telit/mm-modem-helpers-telit.c
> create mode 100644 plugins/telit/mm-modem-helpers-telit.h
> create mode 100644 plugins/telit/tests/test-mm-modem-helpers-telit.c
>
> diff --git a/plugins/Makefile.am b/plugins/Makefile.am
> index b4a76fa..59b1bff 100644
> --- a/plugins/Makefile.am
> +++ b/plugins/Makefile.am
> @@ -550,11 +550,27 @@ libmm_plugin_telit_la_SOURCES = \
> telit/mm-plugin-telit.c \
> telit/mm-plugin-telit.h \
> telit/mm-broadband-modem-telit.c \
> - telit/mm-broadband-modem-telit.h
> + telit/mm-broadband-modem-telit.h \
> + telit/mm-modem-helpers-telit.c \
> + telit/mm-modem-helpers-telit.h
> libmm_plugin_telit_la_CPPFLAGS = $(PLUGIN_COMMON_COMPILER_FLAGS)
> libmm_plugin_telit_la_LDFLAGS = $(PLUGIN_COMMON_LINKER_FLAGS)
> udevrules_DATA += telit/77-mm-telit-port-types.rules
>
> +noinst_PROGRAMS += test-modem-helpers-telit
> +test_modem_helpers_telit_SOURCES = \
> + telit/mm-modem-helpers-telit.c \
> + telit/mm-modem-helpers-telit.h \
> + telit/tests/test-mm-modem-helpers-telit.c
> +test_modem_helpers_telit_CPPFLAGS = \
> + -I$(top_srcdir)/plugins/telit \
> + $(PLUGIN_COMMON_COMPILER_FLAGS)
> +test_modem_helpers_telit_LDADD = \
> + $(top_builddir)/libmm-glib/libmm-glib.la \
> + $(top_builddir)/src/libmodem-helpers.la
> +test_modem_helpers_telit_LDADD = $(top_builddir)/libmm-glib/libmm-glib.la
> +test_modem_helpers_telit_LDFLAGS = $(PLUGIN_COMMON_LINKER_FLAGS)
> +
> # MTK
> libmm_plugin_mtk_la_SOURCES = \
> mtk/mm-plugin-mtk.c \
> diff --git a/plugins/telit/mm-broadband-modem-telit.c
> b/plugins/telit/mm-broadband-modem-telit.c
> index 0f77231..69d6553 100644
> --- a/plugins/telit/mm-broadband-modem-telit.c
> +++ b/plugins/telit/mm-broadband-modem-telit.c
> @@ -31,6 +31,7 @@
> #include "mm-iface-modem.h"
> #include "mm-iface-modem-3gpp.h"
> #include "mm-broadband-modem-telit.h"
> +#include "mm-modem-helpers-telit.h"
>
> static void iface_modem_init (MMIfaceModem *iface);
> static void iface_modem_3gpp_init (MMIfaceModem3gpp *iface);
> @@ -40,6 +41,195 @@ G_DEFINE_TYPE_EXTENDED (MMBroadbandModemTelit,
> mm_broadband_modem_telit, MM_TYPE
> G_IMPLEMENT_INTERFACE (MM_TYPE_IFACE_MODEM_3GPP,
> iface_modem_3gpp_init));
>
>
> /*****************************************************************************/
> +/* Load unlock retries (Modem interface) */
> +
> +#define CSIM_QUERY_PIN_RETRIES_STR "+CSIM=10,0020000100"
> +#define CSIM_QUERY_PUK_RETRIES_STR "+CSIM=10,002C000100"
> +#define CSIM_QUERY_PIN2_RETRIES_STR "+CSIM=10,0020008100"
> +#define CSIM_QUERY_PUK2_RETRIES_STR "+CSIM=10,002C008100"
> +#define CSIM_QUERY_TIMEOUT 3
> +
> +typedef enum {
> + LOAD_UNLOCK_RETRIES_STEP_FIRST,
> + LOAD_UNLOCK_RETRIES_STEP_PIN,
> + LOAD_UNLOCK_RETRIES_STEP_PUK,
> + LOAD_UNLOCK_RETRIES_STEP_PIN2,
> + LOAD_UNLOCK_RETRIES_STEP_PUK2,
> + LOAD_UNLOCK_RETRIES_STEP_LAST
> +} LoadUnlockRetriesStep;
> +
> +typedef struct {
> + MMBroadbandModemTelit* self;
> + GSimpleAsyncResult* result;
> + MMUnlockRetries* retries;
> + LoadUnlockRetriesStep step;
> +} LoadUnlockRetriesContext;
> +
> +static void load_unlock_retries_step (LoadUnlockRetriesContext* ctx);
> +
> +static void
> +load_unlock_retries_context_complete_and_free (LoadUnlockRetriesContext* ctx)
> +{
Looks like the 'retries' object is not being freed properly in the
case of errors. What you need to do is to make sure that the ownership
of the retries object is always transferred properly between the
different logic parts.
I would start for making sure there is a g_object_unref
(ctx->retries); in this complete_and_free() method. No need to check
for retries being non-NULL as it is always created along with the
context. In that case, if there is any error and you call
complete_and_free(), that 'retries' will get properly disposed. See
later what to do to set it as result on successful operations.
> + g_simple_async_result_complete (ctx->result);
> + g_object_unref (ctx->result);
> + g_object_unref (ctx->self);
> + g_free (ctx);
How about using the GLib slice allocator for the context struct? i.e.
allocate with g_slice_new0() and dispose with g_slice_free().
> +}
> +
> +static MMUnlockRetries *
> +modem_load_unlock_retries_finish (MMIfaceModem *_self,
> + GAsyncResult *res,
> + GError **error)
These two previous lines need some whitespaces alignment to align the members.
> +{
> + if (g_simple_async_result_propagate_error (G_SIMPLE_ASYNC_RESULT (res),
> error))
> + return NULL;
> +
> + return (MMUnlockRetries*) g_simple_async_result_get_op_res_gpointer (
> + G_SIMPLE_ASYNC_RESULT (res));
Ok, so here you're returning the pointer that you had set as result.
What you did was to set the pointer here and avoid freeing it in the
context, as this method is returning a full valid reference always.
But, as you've seen, we were leaking the retries object in the error
cases and we actually added the always-unref in the
complete_and_free(). Let's assume that the gpointer set as result has
a FULL valid reference, like you had, but that it has been added with
g_object_unref() as GDestroyNotify in the
g_simple_async_result_set_op_res_gpointer() call later. Doing that
means that the ownership of the reference is fully maintained by the
GSimpleAsyncResult; the reference will be valid as long as the
GSimpleAsyncResult is valid. If we did so, then you would need to get
a NEW reference to rerturn in this finish() method, like this:
return (MMUnlockRetries *) g_object_ref
(g_simple_async_result_get_op_res_gpointer (G_SIMPLE_ASYNC_RESULT
(res));
> +}
> +
> +static void
> +csim_query_ready (MMBaseModem *self,
> + GAsyncResult* res,
> + LoadUnlockRetriesContext* ctx)
> +{
> + const gchar *response;
> + gint unlock_retries;
> + GError *error = NULL;
> +
> + /* If a particular query fails for some reason,
> + * still try and get the other SIM unlock retries values */
> + response = mm_base_modem_at_command_finish (self, res, &error);
> +
> + if (!response) {
> + mm_err ("No respose for step %d: %s",
> + ctx->step,
> + error->message);
mm_warn() better.
> + g_error_free (error);
> + goto next_step;
> + }
> +
> + if ( (unlock_retries = parse_csim_response (ctx->step, response,
> &error)) < 0) {
> + mm_err ("Parse error in step %d: %s",
> + ctx->step,
> + error->message);
mm_warn() better.
> + g_error_free (error);
> + goto next_step;
> + }
> +
> + switch (ctx->step) {
> + case LOAD_UNLOCK_RETRIES_STEP_PIN:
> + mm_dbg ("PIN unlock retries left: %d", unlock_retries);
> + mm_unlock_retries_set (ctx->retries, MM_MODEM_LOCK_SIM_PIN,
> unlock_retries);
> + break;
> + case LOAD_UNLOCK_RETRIES_STEP_PUK:
> + mm_dbg ("PUK unlock retries left: %d", unlock_retries);
> + mm_unlock_retries_set (ctx->retries, MM_MODEM_LOCK_SIM_PUK,
> unlock_retries);
> + break;
> + case LOAD_UNLOCK_RETRIES_STEP_PIN2:
> + mm_dbg ("PIN2 unlock retries left: %d", unlock_retries);
> + mm_unlock_retries_set (ctx->retries, MM_MODEM_LOCK_SIM_PIN2,
> unlock_retries);
> + break;
> + case LOAD_UNLOCK_RETRIES_STEP_PUK2:
> + mm_dbg ("PUK2 unlock retries left: %d", unlock_retries);
> + mm_unlock_retries_set (ctx->retries, MM_MODEM_LOCK_SIM_PUK2,
> unlock_retries);
> + break;
> + default:
> + break;
> + }
> +
> +next_step:
> + ctx->step++;
> + load_unlock_retries_step (ctx);
> +}
> +
> +static void
> +load_unlock_retries_step (LoadUnlockRetriesContext* ctx)
> +{
> + switch (ctx->step) {
> + case LOAD_UNLOCK_RETRIES_STEP_FIRST:
> + /* Fall back on next step */
> + ctx->step++;
> + case LOAD_UNLOCK_RETRIES_STEP_PIN:
> + mm_base_modem_at_command (MM_BASE_MODEM (ctx->self),
> + CSIM_QUERY_PIN_RETRIES_STR,
> + CSIM_QUERY_TIMEOUT,
> + FALSE,
> + (GAsyncReadyCallback) csim_query_ready,
> + ctx);
> + break;
> + case LOAD_UNLOCK_RETRIES_STEP_PUK:
> + mm_base_modem_at_command (MM_BASE_MODEM (ctx->self),
> + CSIM_QUERY_PUK_RETRIES_STR,
> + CSIM_QUERY_TIMEOUT,
> + FALSE,
> + (GAsyncReadyCallback) csim_query_ready,
> + ctx);
> + break;
> + case LOAD_UNLOCK_RETRIES_STEP_PIN2:
> + mm_base_modem_at_command (MM_BASE_MODEM (ctx->self),
> + CSIM_QUERY_PIN2_RETRIES_STR,
> + CSIM_QUERY_TIMEOUT,
> + FALSE,
> + (GAsyncReadyCallback) csim_query_ready,
> + ctx);
> + break;
> + case LOAD_UNLOCK_RETRIES_STEP_PUK2:
> + mm_base_modem_at_command (MM_BASE_MODEM (ctx->self),
> + CSIM_QUERY_PUK2_RETRIES_STR,
> + CSIM_QUERY_TIMEOUT,
> + FALSE,
> + (GAsyncReadyCallback) csim_query_ready,
> + ctx);
> + break;
> + case LOAD_UNLOCK_RETRIES_STEP_LAST:
> + /* Set result and complete */
> + g_simple_async_result_set_op_res_gpointer (ctx->result,
> + ctx->retries,
> + NULL);
Now, here's the last part needed in the 'retries' ownership logic.
Instead of that call we said we need to pass a full valid refence that
will be owned by the GSimpleAsyncResult, and for that we should do:
g_simple_async_result_set_op_res_gpointer (
ctx->result,
g_object_ref (ctx->retries),
(GDestroyNotify) g_object_unref);
Once we have done these changes, what we logically would have
* The original reference of the 'retries' object is in the context;
and is disposed when the context is disposed.
* When the result is set in the GSimpleAsyncResult, we set a NEW
reference, with a explicit GDestroyNotify method, which means that the
reference will be valid as long as the GSimpleAsyncResult is valid.
* Last, in the finish() function, we create yet another NEW
reference from the object set as gpointer result in the
GSimpleAsyncResult. This new reference is the one that the finish()
method returns.
Doing this, we make sure that all references are disposed when the
object owning them is also disposed.
> + load_unlock_retries_context_complete_and_free (ctx);
> + break;
> + default:
> + break;
> +
> +
> + }
> +}
> +
> +static void
> +run_unlock_retries_loading_sequence (MMIfaceModem* self,
> + LoadUnlockRetriesStep first,
> + GAsyncReadyCallback callback,
> + gpointer user_data)
> +{
> + LoadUnlockRetriesContext* ctx;
> +
> + ctx = g_new (LoadUnlockRetriesContext, 1);
> +
As said earlier, maybe use the g_slice_alloc() methods for the context?
> + ctx->self = g_object_ref (self);
> + ctx->result = g_simple_async_result_new (G_OBJECT (self),
> + callback,
> + user_data,
> +
> run_unlock_retries_loading_sequence);
> +
> + ctx->retries = mm_unlock_retries_new ();
> + ctx->step = first;
> +
> + load_unlock_retries_step (ctx);
> +}
> +
> +static void
> +modem_load_unlock_retries (MMIfaceModem *self,
> + GAsyncReadyCallback callback,
> + gpointer user_data)
> +{
> + run_unlock_retries_loading_sequence (self,
> + LOAD_UNLOCK_RETRIES_STEP_FIRST,
> + callback,
> + user_data);
Why not just totally avoid the run_unlock_retries_loading_sequence()
method and put its contents here? It's used nowhere else.
> +}
> +
> +/*****************************************************************************/
> /* Modem power down (Modem interface) */
>
> static gboolean
> @@ -336,6 +526,8 @@ mm_broadband_modem_telit_init (MMBroadbandModemTelit
> *self)
> static void
> iface_modem_init (MMIfaceModem *iface)
> {
> + iface->load_unlock_retries_finish = modem_load_unlock_retries_finish;
> + iface->load_unlock_retries = modem_load_unlock_retries;
> iface->reset = modem_reset;
> iface->reset_finish = modem_reset_finish;
> iface->modem_power_down = modem_power_down;
> diff --git a/plugins/telit/mm-modem-helpers-telit.c
> b/plugins/telit/mm-modem-helpers-telit.c
> new file mode 100644
> index 0000000..5e1cdf1
> --- /dev/null
> +++ b/plugins/telit/mm-modem-helpers-telit.c
> @@ -0,0 +1,77 @@
> +/* -*- Mode: C; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
> +/*
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * 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:
> + *
> + * Copyright (C) 2013 Google Inc.
How about your copyright here instead? :)
> + *
> + */
> +
> +#include <stdlib.h>
> +#include <string.h>
> +#include <stdio.h>
> +
> +#include <ModemManager.h>
> +#define _LIBMM_INSIDE_MMCLI
> +#include <libmm-glib.h>
> +
> +#include "mm-log.h"
> +#include "mm-modem-helpers.h"
> +#include "mm-modem-helpers-telit.h"
> +
> +/*****************************************************************************/
> +/* +CSIM response parser */
> +
> +gint parse_csim_response (const guint step, const gchar* response, GError**
> error)
> +{
> + GRegex *r = NULL;
> + GMatchInfo *match_info = NULL;
> + gchar* retries_hex_str;
> + guint retries;
> +
> +
> + r = g_regex_new ("\\+CSIM:\\s*[0-9]+,\\s*.*63C(.*)\"", G_REGEX_RAW, 0,
> NULL);
> + g_assert (NULL != r);
g_assert (r != NULL);
> +
> + if (!g_regex_match (r, response, 0, &match_info)) {
> + g_set_error (error, MM_CORE_ERROR, MM_CORE_ERROR_FAILED,
> + "Could not parse reponse '%s'", response);
> + g_match_info_free (match_info);
> + g_regex_unref (r);
> + return -1;
> + }
> +
> + if (!g_match_info_matches (match_info)) {
> + g_set_error (error, MM_CORE_ERROR, MM_CORE_ERROR_FAILED,
> + "Could not find matches in response '%s'", response);
> + g_match_info_free (match_info);
> + g_regex_unref (r);
> + return -1;
> + }
> +
> + retries_hex_str = mm_get_string_unquoted_from_match_info (match_info, 1);
> + g_assert (NULL != retries_hex_str);
Humm.. you sure you want to g_assert() here?
> +
> + /* convert hex value to uint */
> + if (1 != sscanf (retries_hex_str, "%x", &retries)) {
if (sscanf () != 1)
> + g_set_error (error, MM_CORE_ERROR, MM_CORE_ERROR_FAILED,
> + "Could not get retry value from match '%s'",
> + retries_hex_str);
> + g_match_info_free (match_info);
> + g_regex_unref (r);
> + return -1;
> + }
> +
> + g_free (retries_hex_str);
> + g_match_info_free (match_info);
> + g_regex_unref (r);
> +
> + return retries;
> +}
> diff --git a/plugins/telit/mm-modem-helpers-telit.h
> b/plugins/telit/mm-modem-helpers-telit.h
> new file mode 100644
> index 0000000..9854d74
> --- /dev/null
> +++ b/plugins/telit/mm-modem-helpers-telit.h
> @@ -0,0 +1,25 @@
> +/* -*- Mode: C; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
> +/*
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * 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:
> + *
> + * Copyright (C) 2013 Google Inc.
> + *
How about your copyright here instead? :)
> + */
> +#ifndef MM_MODEM_HELPERS_TELIT_H
> +#define MM_MODEM_HELPERS_TELIT_H
> +
> +#include <glib.h>
> +
> +/* +CSIM response parser */
> +gint parse_csim_response (const guint step, const gchar* response, GError**
> error);
> +
> +#endif /* MM_MODEM_HELPERS_TELIT_H */
> +
> diff --git a/plugins/telit/tests/test-mm-modem-helpers-telit.c
> b/plugins/telit/tests/test-mm-modem-helpers-telit.c
> new file mode 100644
> index 0000000..2db6d0c
> --- /dev/null
> +++ b/plugins/telit/tests/test-mm-modem-helpers-telit.c
> @@ -0,0 +1,101 @@
> +/* -*- Mode: C; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
> +/*
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * 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:
> + */
> +#include <stdio.h>
> +#include <glib.h>
> +#include <glib-object.h>
> +#include <locale.h>
> +
> +#include <ModemManager.h>
> +#define _LIBMM_INSIDE_MM
> +#include <libmm-glib.h>
> +
> +#include "mm-log.h"
> +#include "mm-modem-helpers.h"
> +#include "mm-modem-helpers-telit.h"
> +
> +typedef struct {
> + gchar* response;
> + gint result;
> +} CSIMResponseTest;
> +
> +static CSIMResponseTest valid_csim_response_test_list [] = {
> + /* The parser expects that 2nd arg contains
> + * substring "63Cx" where x is an HEX string
> + * representing the retry value */
> + {"+CSIM:8,\"xxxx63C1\"", 1},
> + {"+CSIM:8,\"xxxx63CA\"", 10},
> + {"+CSIM:8,\"xxxx63CF\"", 15},
> + /* The parser accepts spaces */
> + {"+CSIM:8,\"xxxx63C1\"", 1},
> + {"+CSIM:8, \"xxxx63C1\"", 1},
> + {"+CSIM: 8, \"xxxx63C1\"", 1},
> + {"+CSIM: 8, \"xxxx63C1\"", 1},
> + /* the parser expects an int as first argument (2nd arg's length),
> + * but does not check if it is correct */
> + {"+CSIM: 10, \"63CF\"", 15},
> + /* the parser expect a quotation mark at the end
> + * of the response, but not at the begin */
> + {"+CSIM: 10, 63CF\"", 15},
> + { NULL, -1}
> +};
> +
> +static CSIMResponseTest invalid_csim_response_test_list [] = {
> + /* Test missing final quotation mark */
> + {"+CSIM: 8, xxxx63CF", -1},
> + /* Negative test for substring "63Cx" */
> + {"+CSIM: 4, xxxx73CF\"", -1},
> + /* Test missing first argument */
> + {"+CSIM:xxxx63CF\"", -1},
> + { NULL, -1}
> +};
> +
> +static void
> +test_parse_csim_response (void)
> +{
> + const gint step = 1;
> + guint i;
> + gint res;
> + GError* error = NULL;
> +
> + /* Test valid responses */
> + for (i = 0; NULL != valid_csim_response_test_list[i].response; i++) {
valid_csim_response_test_list[i].response != NULL
> + res = parse_csim_response (step,
> valid_csim_response_test_list[i].response, &error);
> +
> + g_assert_no_error (error);
> + g_assert_cmpint (res, ==, valid_csim_response_test_list[i].result);
> + }
> +
> + /* Test invalid responses */
> + for (i = 0; NULL != invalid_csim_response_test_list[i].response; i++) {
> + res = parse_csim_response (step,
> invalid_csim_response_test_list[i].response, &error);
> +
> + g_assert_error (error, MM_CORE_ERROR, MM_CORE_ERROR_FAILED);
> + g_assert_cmpint (res, ==, invalid_csim_response_test_list[i].result);
> +
> + if (NULL != error) {
> + g_error_free (error);
> + error = NULL;
> + }
> + }
> +}
> +
> +int main (int argc, char **argv)
> +{
> + setlocale (LC_ALL, "");
> +
> + g_type_init ();
> + g_test_init (&argc, &argv, NULL);
> +
> + g_test_add_func ("/MM/telit/csim", test_parse_csim_response);
> + return g_test_run ();
> +}
> --
> 2.1.4
--
Aleksander
https://aleksander.es
_______________________________________________
ModemManager-devel mailing list
ModemManager-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
