Skip to contentSkip to conversation

voicecall: fix transfer

PATCH In review Andre Matos 2 participants 9 messages Feb 7, 2011

lore
Share & tools
lore

Andre Matos

author
#0 lore raw

PATCH 1/1 voicecall: fix transfer

From: Andre Carvalho de Matos <[email protected]>

transfer method should fail if you have a multiparty call, according to 22.091 section 8.11

---
 src/voicecall.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)
src/voicecall.c +4 -2
index f3f6a9b..f959f55 100644
@@ -1339,6 +1339,7 @@ static DBusMessage *manager_transfer(DBusConnection *conn,
 	struct ofono_voicecall *vc = data;
 	int numactive;
 	int numheld;
+	int numconn;
 
 	if (vc->pending)
 		return __ofono_error_busy(msg);

@@ -1350,11 +1351,12 @@ static DBusMessage *manager_transfer(DBusConnection *conn,
 	 * implementing the call transfer operation for a call that is
 	 * still dialing/alerting.
 	 */
-	numactive += voicecalls_num_connecting(vc);
+	numconn = voicecalls_num_connecting(vc);
 
 	numheld = voicecalls_num_held(vc);
 
-	if ((numactive != 1) && (numheld != 1))
+	if ((numactive > 1 || numheld > 1) ||
+		((numheld + numactive + numconn) != 2))
 		return __ofono_error_failed(msg);
 
 	if (vc->driver->transfer == NULL)
Signature · 1 line
1.7.0.4
#1 lore raw

Hi Andre,

src/voicecall.c static DBusMessage *manager_transfer(DBusConnection *conn, · line 1357

Quoted 10 lines
src/voicecall.c +1 -2
@@ -1350,11 +1351,12 @@ static DBusMessage *manager_transfer(DBusConnection *conn,
 	 * implementing the call transfer operation for a call that is
 	 * still dialing/alerting.
 	 */
-	numactive += voicecalls_num_connecting(vc);
+	numconn = voicecalls_num_connecting(vc);
 
 	numheld = voicecalls_num_held(vc);
 
-	if ((numactive != 1) && (numheld != 1))

I don't get it, isn't the above condition taking care of this already? Its been a while but the precondition of ECT is:

Held Call
Outgoing or an Active call. You cannot have an Outgoing and an Active call at the same time. Hence numactive == 1 && numheld == 1 should work just fine.

src/voicecall.c static DBusMessage *manager_transfer(DBusConnection *conn, · line 1362

Quoted 5 lines
src/voicecall.c +2 -0
+	if ((numactive > 1 || numheld > 1) ||
+		((numheld + numactive + numconn) != 2))
 		return __ofono_error_failed(msg);
 
 	if (vc->driver->transfer == NULL)

Regards,
-Denis

author
#2 lore raw

Hi Denis,

Quoted 14 lines · Denis Kenzior

Hi Andre,

Review static DBusMessage

Quoted 1 line
(quoted diff) +0 -0
@@ -1350,11 +1351,12 @@ static DBusMessage

*manager_transfer(DBusConnection *conn,

src/voicecall.c static DBusMessage *manager_transfer(DBusConnection *conn, · line 1357

Quoted 9 lines
src/voicecall.c +1 -2
       * implementing the call transfer operation for a call that is
       * still dialing/alerting.
       */
-     numactive += voicecalls_num_connecting(vc);
+     numconn = voicecalls_num_connecting(vc);
 
      numheld = voicecalls_num_held(vc);
 
-     if ((numactive != 1) && (numheld != 1))
Quoted 8 lines

I don't get it, isn't the above condition taking care of this already? Its been a while but the precondition of ECT is:

Held Call
Outgoing or an Active call. You cannot have an Outgoing and an Active call at the same time. Hence numactive == 1 && numheld == 1 should work just fine.

What you wrote is correct. transfer works when it supposed to work.

The changes i did takes care of the negative cases. This means calling transfer should fail if we have:
multiparty call active and one held call one active call and one held multiparty call only one active call
only one held call
etc...

Quoted 3 lines

src/voicecall.c static DBusMessage *manager_transfer(DBusConnection *conn, · line 1358

Quoted 2 lines
src/voicecall.c +1 -0
+     if ((numactive > 1 || numheld > 1) ||

this checks if there is multiparty

Quoted 2 lines

+ ((numheld + numactive + numconn) != 2))

this makes sure we have the the two call necessary for a transfer.

Quoted 7 lines
              return __ofono_error_failed(msg);

src/voicecall.c static DBusMessage *manager_transfer(DBusConnection *conn, · line 1362

Quoted 2 lines
src/voicecall.c +0 -0
      if (vc->driver->transfer == NULL)

Regards,
-Denis

With my change oFono doesn't send unnecessary queries to the modem.

Best regards,
André

#3 lore raw

Hi Andre,

Quoted 28 lines · andre matos

Hi Denis,

On Mon, Feb 7, 2011 at 8:46 PM, Denis Kenzior <[email protected] <mailto:[email protected]>> wrote:

    Hi Andre,

    > @@ -1350,11 +1351,12 @@ static DBusMessage
    *manager_transfer(DBusConnection *conn,
    >        * implementing the call transfer operation for a call that is
    >        * still dialing/alerting.
    >        */
    > -     numactive += voicecalls_num_connecting(vc);
    > +     numconn = voicecalls_num_connecting(vc);
    >
    >       numheld = voicecalls_num_held(vc);
    >
    > -     if ((numactive != 1) && (numheld != 1))

    I don't get it, isn't the above condition taking care of this already?
    Its been a while but the precondition of ECT is:

    Held Call
    Outgoing or an Active call.  You cannot have an Outgoing and an Active
    call at the same time.  Hence numactive == 1 && numheld == 1 should work
    just fine.
Quoted 5 lines

What you wrote is correct. transfer works when it supposed to work.

The changes i did takes care of the negative cases. This means calling transfer should fail if we have:
multiparty call active and one held call

if mpty is an active call, then numactive would be > 1 -> fail to invoke ECT

Quoted 1 line

one active call and one held multiparty call

If mpty is a held call, then numheld would be > 1 -> fail to invoke ECT

Quoted 1 line

only one active call

If only active then numheld == 0 -> fail to invoke ECT

Quoted 1 line

only one held call

if only held then numactive == 0 -> fail to invoke ECT

So what condition are we actually trying to solve?

Regards,
-Denis

author
#4 lore raw

Hi Denis,

Quoted 35 lines · Denis Kenzior

Hi Andre,

Quoted 10 lines · andre matos

Hi Denis,

On Mon, Feb 7, 2011 at 8:46 PM, Denis Kenzior <[email protected] <mailto:[email protected]>> wrote:

Hi Andre,

> @@ -1350,11 +1351,12 @@ static DBusMessage
*manager_transfer(DBusConnection *conn,
> * implementing the call transfer operation for a call that

is

Quoted 10 lines

src/voicecall.c static DBusMessage *manager_transfer(DBusConnection *conn, · line 1357

Quoted 8 lines

* still dialing/alerting.
*/

src/voicecall.c +1 -2
-     numactive += voicecalls_num_connecting(vc);
+     numconn = voicecalls_num_connecting(vc);
 
      numheld = voicecalls_num_held(vc);
 
-     if ((numactive != 1) && (numheld != 1))

I don't get it, isn't the above condition taking care of this

already?

Quoted 4 lines

Its been a while but the precondition of ECT is:

Held Call
Outgoing or an Active call. You cannot have an Outgoing and an

Active

Quoted 1 line
    call at the same time.  Hence numactive == 1 && numheld == 1 should

work

Quoted 3 lines

just fine.

Quoted 9 lines
Quoted 5 lines

What you wrote is correct. transfer works when it supposed to work.

The changes i did takes care of the negative cases. This means calling transfer should fail if we have:
multiparty call active and one held call

if mpty is an active call, then numactive would be > 1 -> fail to invoke ECT

Are we reading the same code?

for this case we have:
numactive != 1 ==> true
numheld != 1 ==> false

(numactive != 1) && (numheld != 1) ==> false

in this case "return __ofono_error_failed(msg);" is not called and the at command is sent to modem.

I know the correct conditions for ECT. I am avoiding sending to the modem unnecessary commands. Why should i ask the modem to invoke ECT if i already know it is going to fail for a multiparty call.

Best regards,
André

#5 lore raw

Hi Andre,

Quoted 8 lines

Are we reading the same code?

for this case we have:
numactive != 1 ==> true
numheld != 1 ==> false

(numactive != 1) && (numheld != 1) ==> false

Then shouldn't the patch simply be modified to if (numactive != 1 || numheld != 1)?

Regards,
-Denis

author
#6 lore raw

Hi Denis,

Quoted 8 lines · Denis Kenzior

Hi Andre,

Quoted 6 lines

Are we reading the same code?

for this case we have:
numactive != 1 ==> true
numheld != 1 ==> false

Quoted 6 lines
Quoted 2 lines

(numactive != 1) && (numheld != 1) ==> false

Then shouldn't the patch simply be modified to if (numactive != 1 || numheld != 1)?

Yes, if you wish to sacrifice readability for simplicity.

I find my version self explanatory.

Best regards,
André

#7 lore raw

Hi Andre,

Quoted 20 lines · andre matos

Hi Denis,

On Mon, Feb 7, 2011 at 10:30 PM, Denis Kenzior <[email protected] <mailto:[email protected]>> wrote:

    Hi Andre,

    > Are we reading the same code?
    >
    > for this case we have:
    > numactive != 1  ==> true
    > numheld != 1  ==> false
    >
    > (numactive != 1) && (numheld != 1)  ==>  false
    >

    Then shouldn't the patch simply be modified to if (numactive != 1 ||
    numheld != 1)?
Quoted 2 lines

Yes, if you wish to sacrifice readability for simplicity.

Don't get too offended, the || was intended instead of && in the first place. My brain just had wired crossed and I wasn't seeing why you needed a 6 line patch to fix this.

Quoted 2 lines

I find my version self explanatory.

Sorry, but I do not find your style more readable, less actually. Also, when submitting patches please follow our coding style guidelines. You had at least 1 style violation, namely rule M4.

Regards,
-Denis

#8 lore raw
In reply to Andre Matos

Hi Andre,

Quoted 7 lines · Andre Matos

From: Andre Carvalho de Matos <[email protected]>

transfer method should fail if you have a multiparty call, according to 22.091 section 8.11

---
 src/voicecall.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

I pushed my own version of the fix upstream. Thanks for catching this one.

Regards,
-Denis