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

Proposed by Sergio Schvezov
Status: Merged
Approved by: Tony Espy
Approved revision: 84
Merged at revision: 77
Proposed branch: lp:~sergiusens/nuntium/deactivate
Merge into: lp:nuntium
Prerequisite: lp:~sergiusens/nuntium/select_that
Diff against target: 260 lines (+111/-37)
2 files modified
mediator.go (+24/-1)
ofono/modem.go (+87/-36)
To merge this branch: bzr merge lp:~sergiusens/nuntium/deactivate
Reviewer Review Type Date Requested Status
Alfonso Sanchez-Beato Approve
Tony Espy Approve
PS Jenkins bot continuous-integration Approve
Review via email: mp+236724@code.launchpad.net

Commit message

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.

To post a comment you must log in.
lp:~sergiusens/nuntium/deactivate updated
82. By Sergio Schvezov

Log improv

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Tony Espy (awe) wrote :

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

review: Needs Fixing
Revision history for this message
Sergio Schvezov (sergiusens) wrote :

address the comments inline, will proceed to fixing now

lp:~sergiusens/nuntium/deactivate updated
83. By Sergio Schvezov

Deactivating context if it was activated on scope exit

84. By Sergio Schvezov

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

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Tony Espy (awe) wrote :

Looks good!

review: Approve
Revision history for this message
Alfonso Sanchez-Beato (alfonsosanchezbeato) wrote :

LGTM, +1

Revision history for this message
Alfonso Sanchez-Beato (alfonsosanchezbeato) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'mediator.go'
2--- mediator.go 2014-10-02 01:16:17 +0000
3+++ mediator.go 2014-10-02 01:16:17 +0000
4@@ -25,6 +25,7 @@
5 "io/ioutil"
6 "log"
7 "os"
8+ "sync"
9
10 "launchpad.net/nuntium/mms"
11 "launchpad.net/nuntium/ofono"
12@@ -44,6 +45,7 @@
13 NewMSendReqFile chan struct{ filePath, uuid string }
14 outMessage chan *telepathy.OutgoingMessage
15 terminate chan bool
16+ contextLock sync.Mutex
17 }
18
19 //TODO these vars need a configuration location managed by system settings or
20@@ -164,12 +166,21 @@
21 }
22
23 func (mediator *Mediator) getMRetrieveConf(mNotificationInd *mms.MNotificationInd) {
24+ mediator.contextLock.Lock()
25+ defer mediator.contextLock.Unlock()
26+
27 preferredContext, _ := mediator.telepathyService.GetPreferredContext()
28 mmsContext, err := mediator.modem.ActivateMMSContext(preferredContext)
29 if err != nil {
30 log.Print("Cannot activate ofono context: ", err)
31 return
32 }
33+ defer func() {
34+ if err := mediator.modem.DeactivateMMSContext(mmsContext); err != nil {
35+ log.Println("Issues while deactivating context:", err)
36+ }
37+ }()
38+
39 if err := mediator.telepathyService.SetPreferredContext(mmsContext.ObjectPath); err != nil {
40 log.Println("Unable to store the preferred context for MMS:", err)
41 }
42@@ -185,6 +196,7 @@
43 } else {
44 storage.UpdateDownloaded(mNotificationInd.UUID, filePath)
45 }
46+
47 mediator.NewMRetrieveConfFile <- mNotificationInd.UUID
48 }
49
50@@ -360,6 +372,9 @@
51 }
52
53 func (mediator *Mediator) uploadFile(filePath string) (string, error) {
54+ mediator.contextLock.Lock()
55+ defer mediator.contextLock.Unlock()
56+
57 preferredContext, _ := mediator.telepathyService.GetPreferredContext()
58 mmsContext, err := mediator.modem.ActivateMMSContext(preferredContext)
59 if err != nil {
60@@ -368,6 +383,12 @@
61 if err := mediator.telepathyService.SetPreferredContext(mmsContext.ObjectPath); err != nil {
62 log.Println("Unable to store the preferred context for MMS:", err)
63 }
64+ defer func() {
65+ if err := mediator.modem.DeactivateMMSContext(mmsContext); err != nil {
66+ log.Println("Issues while deactivating context:", err)
67+ }
68+ }()
69+
70 proxy, err := mmsContext.GetProxy()
71 if err != nil {
72 return "", err
73@@ -376,5 +397,7 @@
74 if err != nil {
75 return "", err
76 }
77- return mms.Upload(filePath, msc, proxy.Host, int32(proxy.Port))
78+ mSendRespFile, uploadErr := mms.Upload(filePath, msc, proxy.Host, int32(proxy.Port))
79+
80+ return mSendRespFile, uploadErr
81 }
82
83=== modified file 'ofono/modem.go'
84--- ofono/modem.go 2014-10-02 01:16:17 +0000
85+++ ofono/modem.go 2014-10-02 01:16:17 +0000
86@@ -29,6 +29,7 @@
87 "reflect"
88 "strconv"
89 "strings"
90+ "time"
91
92 "launchpad.net/go-dbus/v1"
93 )
94@@ -38,6 +39,11 @@
95 contextTypeMMS = "mms"
96 )
97
98+const (
99+ ofonoAttachInProgressError = "org.ofono.AttachInProgress"
100+ ofonoInProgressError = "org.ofono.InProgress"
101+)
102+
103 type OfonoContext struct {
104 ObjectPath dbus.ObjectPath
105 Properties PropertiesType
106@@ -243,32 +249,101 @@
107 if context.isActive() {
108 return context, nil
109 }
110- log.Println("Trying to activate context on", context.ObjectPath)
111- obj := modem.conn.Object("org.ofono", context.ObjectPath)
112- _, err = obj.Call(CONNECTION_CONTEXT_INTERFACE, "SetProperty", "Active", dbus.Variant{true})
113+ if err := context.toggleActive(true, modem.conn); err == nil {
114+ return context, nil
115+ } else {
116+ log.Println("Failed to activate for", context.ObjectPath, ":", err)
117+ }
118+ }
119+ return OfonoContext{}, errors.New("no context available to activate")
120+}
121+
122+//DeactivateMMSContext deactivates the context if it is of type mms
123+func (modem *Modem) DeactivateMMSContext(context OfonoContext) error {
124+ if context.isTypeInternet() {
125+ return nil
126+ }
127+
128+ return context.toggleActive(false, modem.conn)
129+}
130+
131+func activationErrorNeedsWait(err error) bool {
132+ if dbusErr, ok := err.(*dbus.Error); ok {
133+ return dbusErr.Name == ofonoInProgressError || dbusErr.Name == ofonoAttachInProgressError
134+ }
135+ return false
136+}
137+
138+func (context OfonoContext) toggleActive(state bool, conn *dbus.Connection) error {
139+ log.Println("Trying to set Active property to", state, "for context on", state, context.ObjectPath)
140+ obj := conn.Object("org.ofono", context.ObjectPath)
141+ for i := 0; i < 3; i++ {
142+ _, err := obj.Call(CONNECTION_CONTEXT_INTERFACE, "SetProperty", "Active", dbus.Variant{state})
143 if err != nil {
144- log.Printf("Cannot Activate interface on %s: %s", context.ObjectPath, err)
145+ log.Printf("Cannot set Activate to %t (try %d/3) interface on %s: %s", state, i+1, context.ObjectPath, err)
146+ if activationErrorNeedsWait(err) {
147+ time.Sleep(2 * time.Second)
148+ }
149 } else {
150- return context, nil
151+ return nil
152 }
153 }
154- return OfonoContext{}, errors.New("no context available to activate")
155+ return errors.New("failed to activate context")
156+}
157+
158+func (oContext OfonoContext) isTypeInternet() bool {
159+ if v, ok := oContext.Properties["Type"]; ok {
160+ return reflect.ValueOf(v.Value).String() == contextTypeInternet
161+ }
162+ return false
163+}
164+
165+func (oContext OfonoContext) isTypeMMS() bool {
166+ if v, ok := oContext.Properties["Type"]; ok {
167+ return reflect.ValueOf(v.Value).String() == contextTypeMMS
168+ }
169+ return false
170 }
171
172 func (oContext OfonoContext) isActive() bool {
173 return reflect.ValueOf(oContext.Properties["Active"].Value).Bool()
174 }
175
176+func (oContext OfonoContext) hasMessageCenter() bool {
177+ return oContext.messageCenter() != ""
178+}
179+
180+func (oContext OfonoContext) messageCenter() string {
181+ if v, ok := oContext.Properties["MessageCenter"]; ok {
182+ return reflect.ValueOf(v.Value).String()
183+ }
184+ return ""
185+}
186+
187+func (oContext OfonoContext) messageProxy() string {
188+ if v, ok := oContext.Properties["MessageProxy"]; ok {
189+ return reflect.ValueOf(v.Value).String()
190+ }
191+ return ""
192+}
193+
194+func (oContext OfonoContext) name() string {
195+ if v, ok := oContext.Properties["Name"]; ok {
196+ return reflect.ValueOf(v.Value).String()
197+ }
198+ return ""
199+}
200+
201 func (oContext OfonoContext) GetMessageCenter() (string, error) {
202- if msc := reflect.ValueOf(oContext.Properties["MessageCenter"].Value).String(); msc != "" {
203- return msc, nil
204+ if oContext.hasMessageCenter() {
205+ return oContext.messageCenter(), nil
206 } else {
207 return "", errors.New("context setting for the Message Center value is empty")
208 }
209 }
210
211 func (oContext OfonoContext) GetProxy() (proxyInfo ProxyInfo, err error) {
212- proxy := reflect.ValueOf(oContext.Properties["MessageProxy"].Value).String()
213+ proxy := oContext.messageProxy()
214 // we need to support empty proxies
215 if proxy == "" {
216 return proxyInfo, nil
217@@ -309,33 +384,8 @@
218 }
219
220 for _, context := range contexts {
221- var name, contextType, msgCenter, msgProxy string
222- var active bool
223- for k, v := range context.Properties {
224- if reflect.ValueOf(k).Kind() != reflect.String {
225- continue
226- }
227- k = reflect.ValueOf(k).String()
228- switch k {
229- case "Name":
230- name = reflect.ValueOf(v.Value).String()
231- case "Type":
232- contextType = reflect.ValueOf(v.Value).String()
233- case "MessageCenter":
234- msgCenter = reflect.ValueOf(v.Value).String()
235- case "MessageProxy":
236- msgProxy = reflect.ValueOf(v.Value).String()
237- case "Active":
238- active = reflect.ValueOf(v.Value).Bool()
239- }
240- }
241- log.Println("Check context valid for - Name", name,
242- "| Context type:", contextType,
243- "| MessageCenter:", msgCenter,
244- "| MessageProxy:", msgProxy,
245- "| Active:", active)
246- if (contextType == contextTypeInternet && active && msgCenter != "") || contextType == contextTypeMMS {
247- if context.ObjectPath == preferredContext || active {
248+ if (context.isTypeInternet() && context.isActive() && context.hasMessageCenter()) || context.isTypeMMS() {
249+ if context.ObjectPath == preferredContext || context.isActive() {
250 mmsContexts = append([]OfonoContext{context}, mmsContexts...)
251 } else {
252 mmsContexts = append(mmsContexts, context)
253@@ -343,6 +393,7 @@
254 }
255 }
256 if len(mmsContexts) == 0 {
257+ log.Printf("non matching contexts:\n %+v", contexts)
258 return mmsContexts, errors.New("No mms contexts found")
259 }
260 return mmsContexts, nil

Subscribers

People subscribed via source and target branches