Re: [PATCH] voicecall: fix transfer

Re: [PATCH] voicecall: fix transfer

[andre matos]

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

Hi,

On Mon, Feb 7, 2011 at 11:00 PM, Denis Kenzior <denkenz@gmail.com> wrote:

> Hi Andre,
>
> On 02/07/2011 03:48 PM, andre matos wrote:
> > Hi Denis,
> >
> > On Mon, Feb 7, 2011 at 10:30 PM, Denis Kenzior <denkenz@gmail.com
> > <mailto:denkenz@gmail.com>> 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)?
> >
> >
> > 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.
>

Not offended at all :) i will always have preference for my code :)
My code got more complicated because initially i thought that connect
counter was for incoming and outgoing calls.


>
> > 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.
>

That is strange i run checkpatch  on the code i did not get any warning. Is
there a  better script for the checking the patches before sending?

Best regards,
André

[-- Attachment #2: attachment.html --]
[-- Type: text/html, Size: 2357 bytes --]

Re: [PATCH] voicecall: fix transfer

[Denis Kenzior]

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

Hi Andre,

>     when submitting patches please follow our coding style guidelines.  You
>     had at least 1 style violation, namely rule M4.
> 
> 
> That is strange i run checkpatch  on the code i did not get any warning.
> Is there a  better script for the checking the patches before sending?

We follow Linux kernel coding style + a few extra rules.  These are not
covered by checkpatch.  See doc/coding-style.txt for a fairly
comprehensive list of them.

Nobody has yet volunteered to maintain a checkpatch style script for
oFono specifically, so today we rely on the review process to find most
of these violations.  Much of the codebase is consistent with these
rules, but you will find some inconsistencies in the code that was
written before some of these rules went into effect.  If you do, then
feel free to send a patch ;)

If in doubt check doc/coding-style.txt or the rest of the code.

Regards,
-Denis

[PATCH] voicecall: fix transfer

[Andre Matos]

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

From: Andre Carvalho de Matos <andre.carvalho.matos@stericsson.com>

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(-)

diff --git a/src/voicecall.c b/src/voicecall.c
index f3f6a9b..f959f55 100644
--- a/src/voicecall.c
+++ b/src/voicecall.c
@@ -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)
-- 
1.7.0.4

Re: [PATCH] voicecall: fix transfer

[Denis Kenzior]

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

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.

> +	if ((numactive > 1 || numheld > 1) ||
> +		((numheld + numactive + numconn) != 2))
>  		return __ofono_error_failed(msg);
>  
>  	if (vc->driver->transfer == NULL)

Regards,
-Denis

Re: [PATCH] voicecall: fix transfer

[andre matos]

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

Hi Denis,

On Mon, Feb 7, 2011 at 8:46 PM, Denis Kenzior <denkenz@gmail.com> 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.
>

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...


>
> > +     if ((numactive > 1 || numheld > 1) ||
>
this checks if there is multiparty

> +             ((numheld + numactive + numconn) != 2))
>
this makes sure we have the the two call necessary for a transfer.

>               return __ofono_error_failed(msg);
> >
> >       if (vc->driver->transfer == NULL)
>
> Regards,
> -Denis
>


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

Best regards,
André

[-- Attachment #2: attachment.html --]
[-- Type: text/html, Size: 2645 bytes --]

Re: [PATCH] voicecall: fix transfer

[Denis Kenzior]

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

Hi Andre,

On 02/07/2011 02:37 PM, andre matos wrote:
> Hi Denis,
> 
> On Mon, Feb 7, 2011 at 8:46 PM, Denis Kenzior <denkenz@gmail.com
> <mailto:denkenz@gmail.com>> 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.
> 
> 
> 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

> one active call and one held multiparty call

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

If only active then numheld == 0 -> fail to invoke ECT
> 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

Re: [PATCH] voicecall: fix transfer

[andre matos]

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

Hi Denis,


On Mon, Feb 7, 2011 at 10:05 PM, Denis Kenzior <denkenz@gmail.com> wrote:

> Hi Andre,
>
> On 02/07/2011 02:37 PM, andre matos wrote:
> > Hi Denis,
> >
> > On Mon, Feb 7, 2011 at 8:46 PM, Denis Kenzior <denkenz@gmail.com
> > <mailto:denkenz@gmail.com>> 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.
> >
> >
> > 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é

[-- Attachment #2: attachment.html --]
[-- Type: text/html, Size: 2796 bytes --]

Re: [PATCH] voicecall: fix transfer

[Denis Kenzior]

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

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)?

Regards,
-Denis

Re: [PATCH] voicecall: fix transfer

[andre matos]

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

Hi Denis,

On Mon, Feb 7, 2011 at 10:30 PM, Denis Kenzior <denkenz@gmail.com> 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)?
>

Yes, if you wish to sacrifice readability for simplicity.

I find my version self explanatory.

Best regards,
André

[-- Attachment #2: attachment.html --]
[-- Type: text/html, Size: 894 bytes --]

Re: [PATCH] voicecall: fix transfer

[Denis Kenzior]

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

Hi Andre,

On 02/07/2011 03:48 PM, andre matos wrote:
> Hi Denis,
> 
> On Mon, Feb 7, 2011 at 10:30 PM, Denis Kenzior <denkenz@gmail.com
> <mailto:denkenz@gmail.com>> 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)?
> 
> 
> 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.

> 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

Re: [PATCH] voicecall: fix transfer

[Denis Kenzior]

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

Hi Andre,

On 02/07/2011 05:56 AM, Andre Matos wrote:
> From: Andre Carvalho de Matos <andre.carvalho.matos@stericsson.com>
> 
> 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