Merge lp:~sergiusens/nuntium/deactivate into lp:nuntium

[Sergio Schvezov]

Syncing upload/download operations with activation/deactivation per request.
Moving all the ofono context property checks and reflection logic to proper functions for easier reuse and readability.

[lp:~sergiusens/nuntium/deactivate]

82. By Sergio Schvezov on 2014-10-01

Log improv

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:81
http://jenkins.qa.ubuntu.com/job/nuntium-ci/16/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/nuntium-utopic-amd64-ci/16
    SUCCESS: http://jenkins.qa.ubuntu.com/job/nuntium-utopic-armhf-ci/16
        deb: http://jenkins.qa.ubuntu.com/job/nuntium-utopic-armhf-ci/16/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/nuntium-utopic-i386-ci/16

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/nuntium-ci/16/rebuild

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:82
http://jenkins.qa.ubuntu.com/job/nuntium-ci/17/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/nuntium-utopic-amd64-ci/17
    SUCCESS: http://jenkins.qa.ubuntu.com/job/nuntium-utopic-armhf-ci/17
        deb: http://jenkins.qa.ubuntu.com/job/nuntium-utopic-armhf-ci/17/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/nuntium-utopic-i386-ci/17

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/nuntium-ci/17/rebuild

review: Approve (continuous-integration)

[Tony Espy (awe)]

So this basically looks good, although I had a few comments that I'd like you to review before approving.

review: Needs Fixing

[Sergio Schvezov (sergiusens)]

address the comments inline, will proceed to fixing now

[lp:~sergiusens/nuntium/deactivate]

83. By Sergio Schvezov on 2014-10-02

Deactivating context if it was activated on scope exit

84. By Sergio Schvezov on 2014-10-02

active toggling improvements including new dbus error type checks and log handling

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:84
http://jenkins.qa.ubuntu.com/job/nuntium-ci/19/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/nuntium-utopic-amd64-ci/19
    SUCCESS: http://jenkins.qa.ubuntu.com/job/nuntium-utopic-armhf-ci/19
        deb: http://jenkins.qa.ubuntu.com/job/nuntium-utopic-armhf-ci/19/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/nuntium-utopic-i386-ci/19

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/nuntium-ci/19/rebuild

review: Approve (continuous-integration)

[Tony Espy (awe)]

Looks good!

review: Approve

[Alfonso Sanchez-Beato (alfonsosanchezbeato)]

LGTM, +1

[Alfonso Sanchez-Beato (alfonsosanchezbeato)]

review: Approve

=== modified file 'mediator.go'
--- mediator.go	2014-10-02 01:16:17 +0000
+++ mediator.go	2014-10-02 01:16:17 +0000
@@ -25,6 +25,7 @@
 	"io/ioutil"
 	"log"
 	"os"
+	"sync"
 
 	"launchpad.net/nuntium/mms"
 	"launchpad.net/nuntium/ofono"
@@ -44,6 +45,7 @@
 	NewMSendReqFile       chan struct{ filePath, uuid string }
 	outMessage            chan *telepathy.OutgoingMessage
 	terminate             chan bool
+	contextLock           sync.Mutex
 }
 
 //TODO these vars need a configuration location managed by system settings or
@@ -164,12 +166,21 @@
 }
 
 func (mediator *Mediator) getMRetrieveConf(mNotificationInd *mms.MNotificationInd) {
+	mediator.contextLock.Lock()
+	defer mediator.contextLock.Unlock()
+
 	preferredContext, _ := mediator.telepathyService.GetPreferredContext()
 	mmsContext, err := mediator.modem.ActivateMMSContext(preferredContext)
 	if err != nil {
 		log.Print("Cannot activate ofono context: ", err)
 		return
 	}
+	defer func() {
+		if err := mediator.modem.DeactivateMMSContext(mmsContext); err != nil {
+			log.Println("Issues while deactivating context:", err)
+		}
+	}()
+
 	if err := mediator.telepathyService.SetPreferredContext(mmsContext.ObjectPath); err != nil {
 		log.Println("Unable to store the preferred context for MMS:", err)
 	}
@@ -185,6 +196,7 @@
 	} else {
 		storage.UpdateDownloaded(mNotificationInd.UUID, filePath)
 	}
+
 	mediator.NewMRetrieveConfFile <- mNotificationInd.UUID
 }
 
@@ -360,6 +372,9 @@
 }
 
 func (mediator *Mediator) uploadFile(filePath string) (string, error) {
+	mediator.contextLock.Lock()
+	defer mediator.contextLock.Unlock()
+
 	preferredContext, _ := mediator.telepathyService.GetPreferredContext()
 	mmsContext, err := mediator.modem.ActivateMMSContext(preferredContext)
 	if err != nil {
@@ -368,6 +383,12 @@
 	if err := mediator.telepathyService.SetPreferredContext(mmsContext.ObjectPath); err != nil {
 		log.Println("Unable to store the preferred context for MMS:", err)
 	}
+	defer func() {
+		if err := mediator.modem.DeactivateMMSContext(mmsContext); err != nil {
+			log.Println("Issues while deactivating context:", err)
+		}
+	}()
+
 	proxy, err := mmsContext.GetProxy()
 	if err != nil {
 		return "", err
@@ -376,5 +397,7 @@
 	if err != nil {
 		return "", err
 	}
-	return mms.Upload(filePath, msc, proxy.Host, int32(proxy.Port))
+	mSendRespFile, uploadErr := mms.Upload(filePath, msc, proxy.Host, int32(proxy.Port))
+
+	return mSendRespFile, uploadErr
 }
 
=== modified file 'ofono/modem.go'
--- ofono/modem.go	2014-10-02 01:16:17 +0000
+++ ofono/modem.go	2014-10-02 01:16:17 +0000
@@ -29,6 +29,7 @@
 	"reflect"
 	"strconv"
 	"strings"
+	"time"
 
 	"launchpad.net/go-dbus/v1"
 )
@@ -38,6 +39,11 @@
 	contextTypeMMS      = "mms"
 )
 
+const (
+	ofonoAttachInProgressError = "org.ofono.AttachInProgress"
+	ofonoInProgressError       = "org.ofono.InProgress"
+)
+
 type OfonoContext struct {
 	ObjectPath dbus.ObjectPath
 	Properties PropertiesType
@@ -243,32 +249,101 @@
 		if context.isActive() {
 			return context, nil
 		}
-		log.Println("Trying to activate context on", context.ObjectPath)
-		obj := modem.conn.Object("org.ofono", context.ObjectPath)
-		_, err = obj.Call(CONNECTION_CONTEXT_INTERFACE, "SetProperty", "Active", dbus.Variant{true})
+		if err := context.toggleActive(true, modem.conn); err == nil {
+			return context, nil
+		} else {
+			log.Println("Failed to activate for", context.ObjectPath, ":", err)
+		}
+	}
+	return OfonoContext{}, errors.New("no context available to activate")
+}
+
+//DeactivateMMSContext deactivates the context if it is of type mms
+func (modem *Modem) DeactivateMMSContext(context OfonoContext) error {
+	if context.isTypeInternet() {
+		return nil
+	}
+
+	return context.toggleActive(false, modem.conn)
+}
+
+func activationErrorNeedsWait(err error) bool {
+	if dbusErr, ok := err.(*dbus.Error); ok {
+		return dbusErr.Name == ofonoInProgressError || dbusErr.Name == ofonoAttachInProgressError
+	}
+	return false
+}
+
+func (context OfonoContext) toggleActive(state bool, conn *dbus.Connection) error {
+	log.Println("Trying to set Active property to", state, "for context on", state, context.ObjectPath)
+	obj := conn.Object("org.ofono", context.ObjectPath)
+	for i := 0; i < 3; i++ {
+		_, err := obj.Call(CONNECTION_CONTEXT_INTERFACE, "SetProperty", "Active", dbus.Variant{state})
 		if err != nil {
-			log.Printf("Cannot Activate interface on %s: %s", context.ObjectPath, err)
+			log.Printf("Cannot set Activate to %t (try %d/3) interface on %s: %s", state, i+1, context.ObjectPath, err)
+			if activationErrorNeedsWait(err) {
+				time.Sleep(2 * time.Second)
+			}
 		} else {
-			return context, nil
+			return nil
 		}
 	}
-	return OfonoContext{}, errors.New("no context available to activate")
+	return errors.New("failed to activate context")
+}
+
+func (oContext OfonoContext) isTypeInternet() bool {
+	if v, ok := oContext.Properties["Type"]; ok {
+		return reflect.ValueOf(v.Value).String() == contextTypeInternet
+	}
+	return false
+}
+
+func (oContext OfonoContext) isTypeMMS() bool {
+	if v, ok := oContext.Properties["Type"]; ok {
+		return reflect.ValueOf(v.Value).String() == contextTypeMMS
+	}
+	return false
 }
 
 func (oContext OfonoContext) isActive() bool {
 	return reflect.ValueOf(oContext.Properties["Active"].Value).Bool()
 }
 
+func (oContext OfonoContext) hasMessageCenter() bool {
+	return oContext.messageCenter() != ""
+}
+
+func (oContext OfonoContext) messageCenter() string {
+	if v, ok := oContext.Properties["MessageCenter"]; ok {
+		return reflect.ValueOf(v.Value).String()
+	}
+	return ""
+}
+
+func (oContext OfonoContext) messageProxy() string {
+	if v, ok := oContext.Properties["MessageProxy"]; ok {
+		return reflect.ValueOf(v.Value).String()
+	}
+	return ""
+}
+
+func (oContext OfonoContext) name() string {
+	if v, ok := oContext.Properties["Name"]; ok {
+		return reflect.ValueOf(v.Value).String()
+	}
+	return ""
+}
+
 func (oContext OfonoContext) GetMessageCenter() (string, error) {
-	if msc := reflect.ValueOf(oContext.Properties["MessageCenter"].Value).String(); msc != "" {
-		return msc, nil
+	if oContext.hasMessageCenter() {
+		return oContext.messageCenter(), nil
 	} else {
 		return "", errors.New("context setting for the Message Center value is empty")
 	}
 }
 
 func (oContext OfonoContext) GetProxy() (proxyInfo ProxyInfo, err error) {
-	proxy := reflect.ValueOf(oContext.Properties["MessageProxy"].Value).String()
+	proxy := oContext.messageProxy()
 	// we need to support empty proxies
 	if proxy == "" {
 		return proxyInfo, nil
@@ -309,33 +384,8 @@
 	}
 
 	for _, context := range contexts {
-		var name, contextType, msgCenter, msgProxy string
-		var active bool
-		for k, v := range context.Properties {
-			if reflect.ValueOf(k).Kind() != reflect.String {
-				continue
-			}
-			k = reflect.ValueOf(k).String()
-			switch k {
-			case "Name":
-				name = reflect.ValueOf(v.Value).String()
-			case "Type":
-				contextType = reflect.ValueOf(v.Value).String()
-			case "MessageCenter":
-				msgCenter = reflect.ValueOf(v.Value).String()
-			case "MessageProxy":
-				msgProxy = reflect.ValueOf(v.Value).String()
-			case "Active":
-				active = reflect.ValueOf(v.Value).Bool()
-			}
-		}
-		log.Println("Check context valid for - Name", name,
-			"| Context type:", contextType,
-			"| MessageCenter:", msgCenter,
-			"| MessageProxy:", msgProxy,
-			"| Active:", active)
-		if (contextType == contextTypeInternet && active && msgCenter != "") || contextType == contextTypeMMS {
-			if context.ObjectPath == preferredContext || active {
+		if (context.isTypeInternet() && context.isActive() && context.hasMessageCenter()) || context.isTypeMMS() {
+			if context.ObjectPath == preferredContext || context.isActive() {
 				mmsContexts = append([]OfonoContext{context}, mmsContexts...)
 			} else {
 				mmsContexts = append(mmsContexts, context)
@@ -343,6 +393,7 @@
 		}
 	}
 	if len(mmsContexts) == 0 {
+		log.Printf("non matching contexts:\n %+v", contexts)
 		return mmsContexts, errors.New("No mms contexts found")
 	}
 	return mmsContexts, nil