Re: [PATCH] sim: Don't submit parallel EFpl reads
Hi Slava, On 12/08/2017 08:57 AM, Slava Monich wrote: In addition to not doing unnecessary SIM I/O, this fixes memory leaks like this one: ==10096== 74 (56 direct, 18 indirect) bytes in 2 blocks are definitely lost in loss record 1,252 of 1,342 ==10096==at 0x4841BF0: calloc (vg_replace_malloc.c) ==10096==by 0x4B03117: g_malloc0 (gmem.c) ==10096==by 0xF83DF: concat_lang_prefs (sim.c) ==10096==by 0xF8697: sim_efpl_read_cb (sim.c) ==10096==by 0x12CBF7: sim_fs_op_read_block_cb (simfs.c) --- src/sim.c | 3 +++ 1 file changed, 3 insertions(+) Applied, thanks. Regards, -Denis ___ ofono mailing list ofono@ofono.org https://lists.ofono.org/mailman/listinfo/ofono
[PATCH] sim: Don't submit parallel EFpl reads
In addition to not doing unnecessary SIM I/O, this fixes memory leaks
like this one:
==10096== 74 (56 direct, 18 indirect) bytes in 2 blocks are definitely lost in
loss record 1,252 of 1,342
==10096==at 0x4841BF0: calloc (vg_replace_malloc.c)
==10096==by 0x4B03117: g_malloc0 (gmem.c)
==10096==by 0xF83DF: concat_lang_prefs (sim.c)
==10096==by 0xF8697: sim_efpl_read_cb (sim.c)
==10096==by 0x12CBF7: sim_fs_op_read_block_cb (simfs.c)
---
src/sim.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/sim.c b/src/sim.c
index 2538c77..31df636 100644
--- a/src/sim.c
+++ b/src/sim.c
@@ -2200,6 +2200,9 @@ static void sim_efli_efpl_changed(int id, void *userdata)
if (sim->efli != NULL) /* This shouldn't happen */
return;
+ if (sim->language_prefs_update)
+ return;
+
if (sim->language_prefs) {
g_strfreev(sim->language_prefs);
sim->language_prefs = NULL;
--
1.9.1
___
ofono mailing list
ofono@ofono.org
https://lists.ofono.org/mailman/listinfo/ofono
Re: [PATCH] sim: Handle multiple EFpl read completions
On 08/12/17 17:14, Denis Kenzior wrote: ... Can we do something like if (sim->language_prefs_update) return; ? Yes, of course! Forget this patch then, I'll send a new one. Cheers, -Slava ___ ofono mailing list ofono@ofono.org https://lists.ofono.org/mailman/listinfo/ofono
Re: [PATCH 5/7] voicecall.c : Add functionality to dial from a memory location.
Hi Philippe,
On 12/06/2017 09:36 AM, Philippe De Swert wrote:
Now we can also dial favourites/quick contacts over bluetooth.
---
src/voicecall.c | 72 +++--
1 file changed, 65 insertions(+), 7 deletions(-)
diff --git a/src/voicecall.c b/src/voicecall.c
index ed6ea83f..f35047f8 100644
--- a/src/voicecall.c
+++ b/src/voicecall.c
@@ -1607,10 +1607,11 @@ error:
__ofono_error_failed(vc->pending));
}
-static int voicecall_dial_shortcut(struct ofono_voicecall *vc, enum ofono_clir_option clir,
- ofono_voicecall_cb_t cb, void *data)
+static int voicecall_dial_shortcut(struct ofono_voicecall *vc, const char
*position,
+ enum ofono_clir_option clir,
ofono_voicecall_cb_t cb, void *data)
{
struct ofono_modem *modem = __ofono_atom_get_modem(vc->atom);
+ long int memory_position;
if (g_slist_length(vc->call_list) >= MAX_VOICE_CALLS)
return -EPERM;
@@ -1618,9 +1619,6 @@ static int voicecall_dial_shortcut(struct ofono_voicecall
*vc, enum ofono_clir_o
if (ofono_modem_get_online(modem) == FALSE)
return -ENETDOWN;
- if (vc->driver->dial_last == NULL)
- return -ENOTSUP;
-
if (voicecalls_have_incoming(vc))
return -EBUSY;
@@ -1631,7 +1629,19 @@ static int voicecall_dial_shortcut(struct ofono_voicecall *vc, enum ofono_clir_o
if (voicecalls_have_active(vc) && voicecalls_have_held(vc))
return -EBUSY;
- vc->driver->dial_last(vc, clir, cb, vc);
+ /* when position is not given we dial the last called number */
+ if(position == NULL) {
not our coding style
+ if (vc->driver->dial_last == NULL)
+ return -ENOTSUP;
+
+ vc->driver->dial_last(vc, clir, cb, vc);
+ } else {
+ memory_position = strtol(position, NULL, 10);
You need to make sure that strtol succeeds, it might be better to
perform this checking in manager_dial_memory
+ if(vc->driver->dial_memory == NULL )
+ return -ENOTSUP;
+
+ vc->driver->dial_memory(vc, memory_position, clir, cb, vc);
+ }
return 0;
}
@@ -1656,7 +1666,7 @@ static DBusMessage *manager_dial_last(DBusConnection
*conn,
vc->pending = dbus_message_ref(msg);
- err = voicecall_dial_shortcut(vc, clir, manager_dial_shortcut_callback, vc);
+ err = voicecall_dial_shortcut(vc, NULL, clir,
manager_dial_shortcut_callback, vc);
if (err >= 0)
return NULL;
@@ -1678,6 +1688,50 @@ static DBusMessage *manager_dial_last(DBusConnection
*conn,
return __ofono_error_failed(msg);
}
+static DBusMessage *manager_dial_memory(DBusConnection *conn,
+ DBusMessage *msg, void *data)
+{
+ struct ofono_voicecall *vc = data;
+ const char *memory_location;
+ const char *clirstr;
+ enum ofono_clir_option clir;
+ int err;
+
+ if (vc->pending || vc->dial_req || vc->pending_em)
+ return __ofono_error_busy(msg);
+
+ if (dbus_message_get_args(msg, NULL, DBUS_TYPE_STRING, _location,
+ DBUS_TYPE_STRING, ,
+ DBUS_TYPE_INVALID) == FALSE)
+ return __ofono_error_invalid_args(msg);
+
+ if (clir_string_to_clir(clirstr, ) == FALSE)
+ return __ofono_error_invalid_format(msg);
+
+ vc->pending = dbus_message_ref(msg);
+
+ err = voicecall_dial_shortcut(vc, memory_location, clir,
manager_dial_shortcut_callback, vc);
+
+ if (err >= 0)
+ return NULL;
+
+ vc->pending = NULL;
+ dbus_message_unref(msg);
+
+ switch (err) {
+ case -EINVAL:
+ return __ofono_error_invalid_format(msg);
+
+ case -ENETDOWN:
+ return __ofono_error_not_available(msg);
+
+ case -ENOTSUP:
+ return __ofono_error_not_implemented(msg);
+ }
+
+ return __ofono_error_failed(msg);
+}
+
static DBusMessage *manager_transfer(DBusConnection *conn,
DBusMessage *msg, void *data)
{
@@ -2236,6 +2290,10 @@ static const GDBusMethodTable manager_methods[] = {
manager_dial) },
{ GDBUS_ASYNC_METHOD("DialLast",
GDBUS_ARGS({ "hide_callerid", "s" }), NULL, manager_dial_last)},
+ { GDBUS_ASYNC_METHOD("DialMemory",
+ GDBUS_ARGS({ "memory_location", "s" },{ "hide_callerid", "s" }),
Why would memory location be a string? Why not just make it an unsigned
int32 and make it easy?
+ NULL,
+ manager_dial_memory) },
{ GDBUS_ASYNC_METHOD("Transfer", NULL, NULL, manager_transfer) },
{ GDBUS_ASYNC_METHOD("SwapCalls", NULL, NULL,
Re: [PATCH 4/7] voicecall: Fix issue with invalid dbus path
Hi Philippe, On 12/06/2017 09:36 AM, Philippe De Swert wrote: Fix an error message from dbus about the path supplied not being valid. Related to commit f58e7685b0078df470300b99cd89fb9b3c45d1c0 ofonod[19107]: src/voicecall.c:voicecall_dial_shortcut() check position ofonod[19107]: src/voicecall.c:synthesize_outgoing_call() Registering new call: 1 process 19107: arguments to dbus_message_iter_append_basic() were incorrect, assertion "_dbus_check_is_valid_path (*string_p)" failed in file ../../../dbus/dbus-message.c line 2759. This is normally a bug in some application using the D-Bus library. --- src/voicecall.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/voicecall.c b/src/voicecall.c index 67384cf2..ed6ea83f 100644 --- a/src/voicecall.c +++ b/src/voicecall.c @@ -28,6 +28,7 @@ #include #include #include +#include This seems unrelated. I dropped this chunk #include #include @@ -1592,6 +1593,7 @@ static void manager_dial_shortcut_callback(const struct ofono_error *error, if (!v) goto error; + path = voicecall_build_path(vc, v->call); reply = dbus_message_new_method_return(vc->pending); dbus_message_append_args(reply, DBUS_TYPE_OBJECT_PATH, , DBUS_TYPE_INVALID); and applied this one. Regards, -Denis ___ ofono mailing list ofono@ofono.org https://lists.ofono.org/mailman/listinfo/ofono
Re: [PATCH 3/7] include/voicecall.h : Add support for dialing number at a given memory location
Hi Philippe,
On 12/06/2017 09:36 AM, Philippe De Swert wrote:
Add a new function to be able to dial numbers from memory/favourites.
---
include/voicecall.h | 5 +
1 file changed, 5 insertions(+)
diff --git a/include/voicecall.h b/include/voicecall.h
index 79a64bb4..6c4f3f5f 100644
--- a/include/voicecall.h
+++ b/include/voicecall.h
@@ -61,6 +61,11 @@ struct ofono_voicecall_driver {
const struct ofono_phone_number *number,
enum ofono_clir_option clir, ofono_voicecall_cb_t cb,
void *data);
+ /* dials a number at a given memory location */
+ void (*dial_memory)(struct ofono_voicecall *vc,
+ long int memory_location,
Why a long int? Memory indexes are always positive, no?
Do note that size of long will be different between 32 bit and 64 bit
architectures.
I would think an unsigned int would be sufficient here
+ enum ofono_clir_option clir, ofono_voicecall_cb_t cb,
+ void *data);
/* Dials the last number again, this handles the hfp profile last number
* dialing with the +BLDN AT command
*/
Regards,
-Denis
___
ofono mailing list
ofono@ofono.org
https://lists.ofono.org/mailman/listinfo/ofono
Re: [PATCH 1/7] voicecall: Support clir for last call dialing
Hi Philippe,
On 12/06/2017 09:35 AM, Philippe De Swert wrote:
When dialing the last called number we also want to use the caller id (clir)
settings that are currently used.
---
doc/voicecallmanager-api.txt | 2 +-
drivers/hfpmodem/voicecall.c | 4 ++--
include/voicecall.h | 3 ++-
src/voicecall.c | 18 ++
4 files changed, 19 insertions(+), 8 deletions(-)
This patch really needs to be broken up into 4. See HACKING,
'Submitting patches'.
diff --git a/doc/voicecallmanager-api.txt b/doc/voicecallmanager-api.txt
index 7aeb81f7..f82a0a10 100644
--- a/doc/voicecallmanager-api.txt
+++ b/doc/voicecallmanager-api.txt
@@ -59,7 +59,7 @@ Methods dict GetProperties()
[service].Error.NotImplemented
[service].Error.Failed
- object DialLast()
+ object DialLast(string hide_callerid)
Initiates a new outgoing call to the last dialled number.
diff --git a/drivers/hfpmodem/voicecall.c b/drivers/hfpmodem/voicecall.c
index 1e0489c2..a32a3242 100644
--- a/drivers/hfpmodem/voicecall.c
+++ b/drivers/hfpmodem/voicecall.c
@@ -405,8 +405,8 @@ static void hfp_dial(struct ofono_voicecall *vc,
CALLBACK_WITH_FAILURE(cb, data);
}
-static void hfp_dial_last(struct ofono_voicecall *vc, ofono_voicecall_cb_t cb,
- void *data)
+static void hfp_dial_last(struct ofono_voicecall *vc, enum ofono_clir_option
clir,
+ ofono_voicecall_cb_t cb, void *data)
{
struct voicecall_data *vd = ofono_voicecall_get_data(vc);
struct cb_data *cbd = cb_data_new(cb, data);
It doesn't look like you actually implementing the CLIR modifier in the
driver? E.g. the 'I' or 'i' suffix business.
I think last time I played with this half the AG implementations didn't
support the CLIR parameter anyway.
diff --git a/include/voicecall.h b/include/voicecall.h
index 6871a6b5..79a64bb4 100644
--- a/include/voicecall.h
+++ b/include/voicecall.h
@@ -64,7 +64,8 @@ struct ofono_voicecall_driver {
/* Dials the last number again, this handles the hfp profile last number
* dialing with the +BLDN AT command
*/
- void (*dial_last)(struct ofono_voicecall *vc, ofono_voicecall_cb_t cb,
void *data);
+ void (*dial_last)(struct ofono_voicecall *vc, enum ofono_clir_option
clir,
+ ofono_voicecall_cb_t cb, void *data);
/* Answers an incoming call, this usually corresponds to ATA */
void (*answer)(struct ofono_voicecall *vc,
ofono_voicecall_cb_t cb, void *data);
diff --git a/src/voicecall.c b/src/voicecall.c
index cd9182a1..9f323c6e 100644
--- a/src/voicecall.c
+++ b/src/voicecall.c
@@ -1605,7 +1605,7 @@ error:
__ofono_error_failed(vc->pending));
}
-static int voicecall_dial_last(struct ofono_voicecall *vc,
+static int voicecall_dial_last(struct ofono_voicecall *vc, enum
ofono_clir_option clir,
ofono_voicecall_cb_t cb, void *data)
{
struct ofono_modem *modem = __ofono_atom_get_modem(vc->atom);
@@ -1629,7 +1629,7 @@ static int voicecall_dial_last(struct ofono_voicecall *vc,
if (voicecalls_have_active(vc) && voicecalls_have_held(vc))
return -EBUSY;
- vc->driver->dial_last(vc, cb, vc);
+ vc->driver->dial_last(vc, clir, cb, vc);
return 0;
}
@@ -1638,14 +1638,23 @@ static DBusMessage *manager_dial_last(DBusConnection
*conn,
DBusMessage *msg, void *data)
{
struct ofono_voicecall *vc = data;
+ const char *clirstr;
+ enum ofono_clir_option clir;
int err;
if (vc->pending || vc->dial_req || vc->pending_em)
return __ofono_error_busy(msg);
+ if (dbus_message_get_args(msg, NULL, DBUS_TYPE_STRING, ,
+ DBUS_TYPE_INVALID) == FALSE)
+ return __ofono_error_invalid_args(msg);
+
+ if (clir_string_to_clir(clirstr, ) == FALSE)
+ return __ofono_error_invalid_format(msg);
+
vc->pending = dbus_message_ref(msg);
- err = voicecall_dial_last(vc, manager_dial_last_callback, vc);
+ err = voicecall_dial_last(vc, clir, manager_dial_last_callback, vc);
if (err >= 0)
return NULL;
@@ -2223,7 +2232,8 @@ static const GDBusMethodTable manager_methods[] = {
GDBUS_ARGS({ "number", "s" }, { "hide_callerid", "s" }),
GDBUS_ARGS({ "path", "o" }),
manager_dial) },
- { GDBUS_ASYNC_METHOD("DialLast", NULL, NULL, manager_dial_last)},
+ { GDBUS_ASYNC_METHOD("DialLast",
+ GDBUS_ARGS({ "hide_callerid", "s" }), NULL, manager_dial_last)},
{ GDBUS_ASYNC_METHOD("Transfer", NULL, NULL, manager_transfer) },
{
Re: [PATCH] sim: Handle multiple EFpl read completions
Hi Slava,
On 12/07/2017 11:09 AM, Slava Monich wrote:
Since sim_efli_efpl_changed() can be invoked several times before
completion of SIM reads (which are not cancellable), followed by
multiple completions, it's not enough to free old sim->language_prefs
in sim_efli_efpl_changed(). The same thing has to be done by
sim_efpl_read_cb() to avoid leaking memory e.g. after this
sequence of calls:
1. sim_efli_efpl_changed()
2. sim_efli_efpl_changed()
3. sim_efpl_read_cb()
4. sim_efpl_read_cb()
So why don't we just set a flag and ignore the duplicate update if the
flag is set?
Alternatively, we can always add cancellation support to simfs.
One such scenario involves __ofono_sim_refresh() calling
sim_efli_efpl_changed() first with id=SIM_EFPL_FILEID,
like this:
#0 sim_efli_efpl_changed (id=12037)
#1 sim_fs_notify_file_watches (id=-1)
#2 __ofono_sim_refresh (file_list=0x0, full_file_change=1, naa_init=1)
#3 handle_command_refresh ()
#4 ofono_stk_proactive_command_handled_notify ()
...
and then with id=28421 (SIM_EFLI_FILEID), so both files are
re-read twice, the second result overwriting the first one
and the first one getting lost (leaked). Since SIM reads are
asynchronous and take considerable time, I'm quite sure that
there are other scenarios as well.
In fact it's even better to keep the old sim->language_prefs
around until EFpl read is finished, to avoid issuing unnecessary
PropertyChanged signal if the second read produces the same
result, which it normally does.
Okay, that sounds useful
---
src/sim.c | 30 --
1 file changed, 24 insertions(+), 6 deletions(-)
diff --git a/src/sim.c b/src/sim.c
index 2538c77..2585f06 100644
--- a/src/sim.c
+++ b/src/sim.c
@@ -2081,6 +2081,26 @@ static char **concat_lang_prefs(GSList *a, GSList *b)
return ret;
}
+static gboolean strv_equal(char **sv1, char **sv2)
+{
+ const guint len1 = sv1 ? g_strv_length(sv1) : 0;
+ const guint len2 = sv2 ? g_strv_length(sv2) : 0;
+
+ if (len1 == len2) {
+ guint i;
+
+ for (i = 0; i < len1; i++) {
+ if (strcmp(sv1[i], sv2[i])) {
+ return FALSE;
+ }
+ }
+
+ return TRUE;
+ }
+
+ return FALSE;
+}
+
This really belongs somewhere else, like maybe util.[ch]
static void sim_efpl_read_cb(int ok, int length, int record,
const unsigned char *data,
int record_length, void *userdata)
@@ -2091,6 +2111,7 @@ static void sim_efpl_read_cb(int ok, int length, int
record,
gboolean efli_format = TRUE;
GSList *efli = NULL;
GSList *efpl = NULL;
+ char **old_prefs = sim->language_prefs;
if (!ok || length < 2)
goto skip_efpl;
@@ -2142,13 +2163,15 @@ skip_efpl:
g_slist_free_full(efpl, g_free);
}
- if (sim->language_prefs != NULL)
+ if (!strv_equal(sim->language_prefs, old_prefs))
ofono_dbus_signal_array_property_changed(conn, path,
OFONO_SIM_MANAGER_INTERFACE,
"PreferredLanguages",
DBUS_TYPE_STRING,
>language_prefs);
+ g_strfreev(old_prefs);
+
/* Proceed with sim initialization if we're not merely updating */
if (!sim->language_prefs_update)
__ofono_sim_recheck_pin(sim);
@@ -2200,11 +2223,6 @@ static void sim_efli_efpl_changed(int id, void *userdata)
if (sim->efli != NULL) /* This shouldn't happen */
return;
- if (sim->language_prefs) {
- g_strfreev(sim->language_prefs);
- sim->language_prefs = NULL;
- }
-
Can we do something like
if (sim->language_prefs_update)
return;
?
sim->language_prefs_update = true;
ofono_sim_read(sim->early_context, SIM_EFLI_FILEID,
Regards,
-Denis
___
ofono mailing list
ofono@ofono.org
https://lists.ofono.org/mailman/listinfo/ofono
Re: [PATCH] sim: Handle multiple EFpl read completions
Since sim_efli_efpl_changed() can be invoked several times before completion of SIM reads (which are not cancellable), followed by multiple completions, it's not enough to free old sim->language_prefs in sim_efli_efpl_changed(). The same thing has to be done by sim_efpl_read_cb() to avoid leaking memory e.g. after this sequence of calls: ... There are more leaks like this in sim.c, you can expect a few more patches if this one gets applied. Cheers, -Slava ___ ofono mailing list ofono@ofono.org https://lists.ofono.org/mailman/listinfo/ofono
Print Backtrace on crash in Ofono
Hi All,
I am trying to run Ofono on a ARM based AP processor. The backtrace
function which is present in Ofono is not working on that Processor though
I have done following:
Enabled -- > enable-debug option
Enabled -funwind_tables option in CC_FLAGS within enable-debug option
itself (configure.ac)
Provided -rdynamic option in LD_FLAGS within enable-debug option itseld (
configure.ac)
src/log.c
static void print_backtrace(unsigned int offset)
{
written = write(outfd[1], addr, len); -->This line is failing all the time.
}
I modified the print_backtrace() to below:
static void print_backtrace(unsigned int offset)
{
void *frames[99];
size_t n_ptrs;
//unsigned int i;
//int outfd[2], infd[2];
int pathlen;
//pid_t pid;
if (program_exec == NULL)
return;
pathlen = strlen(program_path);
ofono_error(" backtrace +");
n_ptrs = backtrace(frames, G_N_ELEMENTS(frames));
fprintf(stderr, "Last %d frames:\n", n_ptrs);
fprintf(stderr, "Offset is %d:\n", offset);
backtrace_symbols_fd(frames, n_ptrs, STDERR_FILENO);
ofono_error("");
}
With above modification, I am able to get backtrace but it only prints
address not the function names. Result is as below:
ofonod[2835]: backtrace +
Last 8 frames:
Offset is 2:
ofonod(+0xd5f60)[0x558a1a3f60]
ofonod(+0xd5ff4)[0x558a1a3ff4]
linux-vdso.so.1(__kernel_rt_sigreturn+0x0)[0x7f9cfa36c0]
ofonod(+0x3b614)[0x558a109614]
ofonod(ofono_modem_set_powered+0xe8)[0x558a1a6b18]
ofonod(+0x38548)[0x558a106548]
ofonod(+0x39548)[0x558a107548]
/usr/lib64/libglib-2.0.so.0(g_main_context_dispatch+0x114)[0x7f9ce8eed4]
ofonod[2835]:
Can some one help me what is missing and how can I resolve the issue ?
-Best Regards,
Rohit Agrawal
___
ofono mailing list
ofono@ofono.org
https://lists.ofono.org/mailman/listinfo/ofono
