date:20120321

Re: [PATCHv3 03/12] call-forwarding: Refactor cf_condition_find_with_cls() slightly

2012-03-21 Thread Oleg Zhurakivskyy


Hello Denis,

On 03/21/2012 01:50 AM, Denis Kenzior wrote:

+   return CFC(a)-cls - CFC(b)-cls;


You're breaking const-correctness here.


Thanks for pointing this. The intention was to save a few instructions here, but 
just missed that.



Also, why do you rename the function?  I do think we should have
'condition' or at least 'cond' in the name to make it clear what it
does.  My vote is for cf_cond_compare...


Thanks, I agree, cf_cond_compare() would be more consistent and clear.


+static struct ofono_call_forwarding_condition *cf_find(GSList *l, int cls)


Same as above, at least name it cf_cond_find.  Also, I'd prefer changes
to this function to be in a separate patch.


OK.


-static int cf_find_timeout(GSList *cf_list, int cls)
+static int cf_find_timeout(GSList *l, int cls)


Same here, lets name this cf_cond_find_timeout


OK.


-static void cf_cond_list_print(GSList *list)
+static void cf_cond_list_print(GSList *l)
  {
-   GSList *l;
-   struct ofono_call_forwarding_condition *cond;
-
-   for (l = list; l; l = l-next) {
-   cond = l-data;
+   struct ofono_call_forwarding_condition *c;

+   for (; l  (c = l-data); l = l-next)


After going back and forth on this one, I'm actually against this style.
  Mainly it is too hard to notice the assignment, inside the continuation
check expression.  I'd really prefer that we assign c inside the for
loop block.  Also, l-data should never be NULL, so in this case I'd
rather crash and find out early than inadvertently covering it up.


OK, thanks. I agree on both points here.


+   number = new_cfu ?  : phone_number_to_string(
lc-phone_number);


This part really belongs in a separate patch.


OK.

Thanks for the help, I will correct all these.

Regards,
Oleg
--
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki
Business Identity Code: 0357606 - 4
Domiciled in Helsinki

___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono

Re: [PATCHv3 04/12] call-forwarding: Refactor cf_find_unconditional()

2012-03-21 Thread Oleg Zhurakivskyy


Hello Denis,

On 03/21/2012 01:51 AM, Denis Kenzior wrote:

+#define cf_find_unconditional(_cf) \
+({ \
+   cf_find((_cf)-cf_conditions[CALL_FORWARDING_TYPE_UNCONDITIONAL],\
+   BEARER_CLASS_VOICE);\
+})
+


Actually I'd prefer if we made this an inline function rather than a
macro.  A bit easier to read that way.


Sure, strongly agree. I was going to do this myself, it just didn't make it into 
these patches.


Regards,
Oleg
--
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki
Business Identity Code: 0357606 - 4
Domiciled in Helsinki
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono

Re: [PATCHv3 05/12] call-forwarding: Minor cleanup of set_query_cf_callback

2012-03-21 Thread Oleg Zhurakivskyy


Hello Denis,

On 03/21/2012 01:54 AM, Denis Kenzior wrote:

-   if (cf-query_next == cf-query_end) {
-   reply = dbus_message_new_method_return(cf-pending);
-   __ofono_dbus_pending_reply(cf-pending, reply);
-   }
-


Actually this part is here on purpose, we want to generate a reply
before we generate a signal for PropertyChanged.


Thanks for the help here, it really missed my attention.

Regards,
Oleg
--
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki
Business Identity Code: 0357606 - 4
Domiciled in Helsinki
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono

Re: [PATCHv3 06/12] call-forwarding: Don't run conditional queries if cfu is active

2012-03-21 Thread Oleg Zhurakivskyy


Hello Denis,

On 03/21/2012 02:09 AM, Denis Kenzior wrote:

  static inline void get_query_next_cf_cond(struct ofono_call_forwarding *cf)
@@ -565,7 +564,9 @@ static DBusMessage *cf_get_properties(DBusConnection *conn, 
DBusMessage *msg,

cf-pending = dbus_message_ref(msg);
cf-query_next = 0;
-
+   cf-query_end = is_cfu_enabled(cf) ?
+   CALL_FORWARDING_TYPE_UNCONDITIONAL :
+   CALL_FORWARDING_TYPE_NOT_REACHABLE;
get_query_next_cf_cond(cf);

return NULL;


What if we're running GetProperties for the first time and CFU is active
at the network, but for some reason (e.g. driver doesn't implement it)
the SIM call forwarding rules are absent.  Won't we query conditional
CFs needlessly here?


Good point and thanks for pointing this. I will correct so, that once the CFU is 
discovered and active, the conditional queries will be skipped.


Regards,
Oleg
--
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki
Business Identity Code: 0357606 - 4
Domiciled in Helsinki
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono

[PATCH] sim: Use quoted string with AT+CRSM data parameter

2012-03-21 Thread Nicolas Bertrand

Fix issue with some modems preventing to update elementary files
(speedup, ZTE, huawei, MBM)
---
 drivers/atmodem/sim.c |   52 ++--
 1 files changed, 45 insertions(+), 7 deletions(-)

diff --git a/drivers/atmodem/sim.c b/drivers/atmodem/sim.c
index 8ee9822..f307708 100644
--- a/drivers/atmodem/sim.c
+++ b/drivers/atmodem/sim.c
@@ -305,18 +305,38 @@ static void at_sim_update_binary(struct ofono_sim *sim, 
int fileid,
 {
struct sim_data *sd = ofono_sim_get_data(sim);
struct cb_data *cbd = cb_data_new(cb, data);
-   char *buf = g_try_new(char, 36 + length * 2);
+   char *buf;
int len, ret;
+   int size = 36 + length * 2;
 
+   if (sd-vendor == OFONO_VENDOR_MBM ||
+   sd-vendor == OFONO_VENDOR_ZTE ||
+   sd-vendor == OFONO_VENDOR_HUAWEI ||
+   sd-vendor == OFONO_VENDOR_SPEEDUP)
+   size += 2; /*Add quotes*/
+
+   buf = g_try_new(char, size);
if (buf == NULL)
goto error;
 
len = sprintf(buf, AT+CRSM=214,%i,%i,%i,%i,, fileid,
start  8, start  0xff, length);
 
+   if (sd-vendor == OFONO_VENDOR_MBM ||
+   sd-vendor == OFONO_VENDOR_ZTE ||
+   sd-vendor == OFONO_VENDOR_HUAWEI ||
+   sd-vendor == OFONO_VENDOR_SPEEDUP)
+   sprintf(buf + len, \);
+
for (; length; length--)
len += sprintf(buf + len, %02hhX, *value++);
 
+   if (sd-vendor == OFONO_VENDOR_MBM ||
+   sd-vendor == OFONO_VENDOR_ZTE ||
+   sd-vendor == OFONO_VENDOR_HUAWEI ||
+   sd-vendor == OFONO_VENDOR_SPEEDUP)
+   sprintf(buf + len, \);
+
ret = g_at_chat_send(sd-chat, buf, crsm_prefix,
at_crsm_update_cb, cbd, g_free);
 
@@ -342,7 +362,10 @@ static void at_sim_update_record(struct ofono_sim *sim, 
int fileid,
int len, ret;
int size = 36 + length * 2;
 
-   if (sd-vendor == OFONO_VENDOR_MBM)
+   if (sd-vendor == OFONO_VENDOR_MBM ||
+   sd-vendor == OFONO_VENDOR_ZTE ||
+   sd-vendor == OFONO_VENDOR_HUAWEI ||
+   sd-vendor == OFONO_VENDOR_SPEEDUP)
size += 2; /*Add quotes*/
 
buf = g_try_new(char, size);
@@ -352,13 +375,19 @@ static void at_sim_update_record(struct ofono_sim *sim, 
int fileid,
len = sprintf(buf, AT+CRSM=220,%i,%i,4,%i,, fileid,
record, length);
 
-   if (sd-vendor == OFONO_VENDOR_MBM)
+   if (sd-vendor == OFONO_VENDOR_MBM ||
+   sd-vendor == OFONO_VENDOR_ZTE ||
+   sd-vendor == OFONO_VENDOR_HUAWEI ||
+   sd-vendor == OFONO_VENDOR_SPEEDUP)
len += sprintf(buf + len, \);
 
for (; length; length--)
len += sprintf(buf + len, %02hhX, *value++);
 
-   if (sd-vendor == OFONO_VENDOR_MBM)
+   if (sd-vendor == OFONO_VENDOR_MBM ||
+   sd-vendor == OFONO_VENDOR_ZTE ||
+   sd-vendor == OFONO_VENDOR_HUAWEI ||
+   sd-vendor == OFONO_VENDOR_SPEEDUP)
sprintf(buf + len, \);
 
ret = g_at_chat_send(sd-chat, buf, crsm_prefix,
@@ -385,7 +414,10 @@ static void at_sim_update_cyclic(struct ofono_sim *sim, 
int fileid,
int len, ret;
int size = 36 + length * 2;
 
-   if (sd-vendor == OFONO_VENDOR_MBM)
+   if (sd-vendor == OFONO_VENDOR_MBM ||
+   sd-vendor == OFONO_VENDOR_ZTE ||
+   sd-vendor == OFONO_VENDOR_HUAWEI ||
+   sd-vendor == OFONO_VENDOR_SPEEDUP)
size += 2; /* Add quotes */
 
buf = g_try_new(char, size);
@@ -394,13 +426,19 @@ static void at_sim_update_cyclic(struct ofono_sim *sim, 
int fileid,
 
len = sprintf(buf, AT+CRSM=220,%i,0,3,%i,, fileid, length);
 
-   if (sd-vendor == OFONO_VENDOR_MBM)
+   if (sd-vendor == OFONO_VENDOR_MBM ||
+   sd-vendor == OFONO_VENDOR_ZTE ||
+   sd-vendor == OFONO_VENDOR_HUAWEI ||
+   sd-vendor == OFONO_VENDOR_SPEEDUP)
len += sprintf(buf + len, \);
 
for (; length; length--)
len += sprintf(buf + len, %02hhX, *value++);
 
-   if (sd-vendor == OFONO_VENDOR_MBM)
+   if (sd-vendor == OFONO_VENDOR_MBM ||
+   sd-vendor == OFONO_VENDOR_ZTE ||
+   sd-vendor == OFONO_VENDOR_HUAWEI ||
+   sd-vendor == OFONO_VENDOR_SPEEDUP)
sprintf(buf + len, \);
 
ret = g_at_chat_send(sd-chat, buf, crsm_prefix,
-- 
1.7.5.4

___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono

Re: [PATCH] sim: Use quoted string with AT+CRSM data parameter

2012-03-21 Thread Marcel Holtmann

Hi Nicolas,

 Fix issue with some modems preventing to update elementary files
 (speedup, ZTE, huawei, MBM)
 ---
  drivers/atmodem/sim.c |   52 ++--
  1 files changed, 45 insertions(+), 7 deletions(-)
 
 diff --git a/drivers/atmodem/sim.c b/drivers/atmodem/sim.c
 index 8ee9822..f307708 100644
 --- a/drivers/atmodem/sim.c
 +++ b/drivers/atmodem/sim.c
 @@ -305,18 +305,38 @@ static void at_sim_update_binary(struct ofono_sim *sim, 
 int fileid,
  {
   struct sim_data *sd = ofono_sim_get_data(sim);
   struct cb_data *cbd = cb_data_new(cb, data);
 - char *buf = g_try_new(char, 36 + length * 2);
 + char *buf;
   int len, ret;
 + int size = 36 + length * 2;
  
 + if (sd-vendor == OFONO_VENDOR_MBM ||
 + sd-vendor == OFONO_VENDOR_ZTE ||
 + sd-vendor == OFONO_VENDOR_HUAWEI ||
 + sd-vendor == OFONO_VENDOR_SPEEDUP)
 + size += 2; /*Add quotes*/

this is better done with a switch statement. And please fix the comment
style to adhere to the coding style.

 +
 + buf = g_try_new(char, size);
   if (buf == NULL)
   goto error;
  
   len = sprintf(buf, AT+CRSM=214,%i,%i,%i,%i,, fileid,
   start  8, start  0xff, length);
  
 + if (sd-vendor == OFONO_VENDOR_MBM ||
 + sd-vendor == OFONO_VENDOR_ZTE ||
 + sd-vendor == OFONO_VENDOR_HUAWEI ||
 + sd-vendor == OFONO_VENDOR_SPEEDUP)
 + sprintf(buf + len, \);
 +

I am getting a little bit worried about spreading sd-vendor checks at
many places here. Maybe this can be done smarter and cleaner.

Regards

Marcel


___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono

Re: [PATCHv3 03/12] call-forwarding: Refactor cf_condition_find_with_cls() slightly

Re: [PATCHv3 04/12] call-forwarding: Refactor cf_find_unconditional()

Re: [PATCHv3 05/12] call-forwarding: Minor cleanup of set_query_cf_callback

Re: [PATCHv3 06/12] call-forwarding: Don't run conditional queries if cfu is active

[PATCH] sim: Use quoted string with AT+CRSM data parameter

Re: [PATCH] sim: Use quoted string with AT+CRSM data parameter

6 matches

Site Navigation

Mail list logo

Footer information