[Merge] lp:~sergiusens/nuntium/message-api into lp:nuntium
Manuel de la Peña
manuel.delapena at canonical.com
Wed Jul 2 09:01:59 UTC 2014
Review: Needs Fixing
Diff comments:
> === modified file 'mediator.go'
> --- mediator.go 2014-07-01 12:24:38 +0000
> +++ mediator.go 2014-07-01 12:24:38 +0000
> @@ -41,7 +41,7 @@
> NewMSendReq chan *mms.MSendReq
> NewMRetrieveConfFile chan string
> NewMNotifyRespIndFile chan string
> - NewMSendReqFile chan string
> + NewMSendReqFile chan struct{ filePath, uuid string }
> outMessage chan *telepathy.OutgoingMessage
> terminate chan bool
> }
> @@ -62,7 +62,7 @@
> mediator.NewMNotifyRespInd = make(chan *mms.MNotifyRespInd)
> mediator.NewMNotifyRespIndFile = make(chan string)
> mediator.NewMSendReq = make(chan *mms.MSendReq)
> - mediator.NewMSendReqFile = make(chan string)
> + mediator.NewMSendReqFile = make(chan struct{ filePath, uuid string })
> mediator.outMessage = make(chan *telepathy.OutgoingMessage)
> mediator.terminate = make(chan bool)
> return mediator
> @@ -100,8 +100,8 @@
> go mediator.handleOutgoingMessage(msg)
> case mSendReq := <-mediator.NewMSendReq:
> go mediator.handleMSendReq(mSendReq)
> - case mSendReqFilePath := <-mediator.NewMSendReqFile:
> - go mediator.sendMSendReq(mSendReqFilePath)
> + case mSendReqFile := <-mediator.NewMSendReqFile:
> + go mediator.sendMSendReq(mSendReqFile.filePath, mSendReqFile.uuid)
> case id := <-mediator.modem.IdentityAdded:
> var err error
> mediator.telepathyService, err = mmsManager.AddService(id, mediator.outMessage, useDeliveryReports)
> @@ -256,8 +256,10 @@
> cts = append(cts, ct)
> }
> mSendReq := mms.NewMSendReq(msg.Recipients, cts)
> - //TODO
> - mediator.telepathyService.ReplySendMessage(msg.Reply, mSendReq.UUID)
> + if _, err := mediator.telepathyService.ReplySendMessage(msg.Reply, mSendReq.UUID); err != nil {
> + log.Print(err)
> + return
> + }
> mediator.NewMSendReq <- mSendReq
> }
>
> @@ -272,17 +274,28 @@
> enc := mms.NewEncoder(f)
> if err := enc.Encode(mSendReq); err != nil {
> log.Print("Unable to encode m-send.req for ", mSendReq.UUID)
> + if err := mediator.telepathyService.MessageStatusChanged(mSendReq.UUID, telepathy.PERMANENT_ERROR); err != nil {
> + log.Println(err)
> + }
> return
> }
> filePath := f.Name()
> log.Printf("Created %s to handle m-send.req for %s", filePath, mSendReq.UUID)
> - mediator.NewMSendReqFile <- filePath
> + mediator.sendMSendReq(filePath, mSendReq.UUID)
> }
>
> -func (mediator *Mediator) sendMSendReq(mSendReqFile string) {
> +func (mediator *Mediator) sendMSendReq(mSendReqFile, uuid string) {
> defer os.Remove(mSendReqFile)
> + defer mediator.telepathyService.MessageDestroy(uuid)
> if err := mediator.uploadFile(mSendReqFile); err != nil {
> + if err := mediator.telepathyService.MessageStatusChanged(uuid, telepathy.TRANSIENT_ERROR); err != nil {
> + log.Println(err)
> + }
> log.Printf("Cannot upload m-send.req encoded file %s to message center: %s", mSendReqFile, err)
> + return
> + }
> + if err := mediator.telepathyService.MessageStatusChanged(uuid, telepathy.SENT); err != nil {
> + log.Println(err)
> }
> }
>
>
> === modified file 'storage/storage.go'
> --- storage/storage.go 2014-07-01 12:24:38 +0000
> +++ storage/storage.go 2014-07-01 12:24:38 +0000
> @@ -44,6 +44,24 @@
> return writeState(state, storePath)
> }
>
> +func Destroy(uuid string) error {
> + if storePath, err := xdg.Data.Ensure(path.Join(SUBPATH, uuid+".db")); err == nil {
> + if err := os.Remove(storePath); err != nil {
> + return err
> + }
> + } else {
> + return err
> + }
> + if mmsPath, err := GetMMS(uuid); err == nil {
> + if err := os.Remove(mmsPath); err != nil {
> + return err
> + }
> + } else {
> + return err
> + }
> + return nil
> +}
> +
> func CreateResponseFile(uuid string) (*os.File, error) {
> filePath, err := xdg.Cache.Ensure(path.Join(SUBPATH, uuid+".m-notifyresp.ind"))
> if err != nil {
>
> === modified file 'telepathy/const.go'
> --- telepathy/const.go 2014-04-04 19:54:59 +0000
> +++ telepathy/const.go 2014-07-01 12:24:38 +0000
> @@ -24,6 +24,7 @@
> const (
> MMS_DBUS_NAME = "org.ofono.mms"
> MMS_DBUS_PATH = "/org/ofono/mms"
> + MMS_MESSAGE_DBUS_IFACE = "org.ofono.mms.Message"
> MMS_SERVICE_DBUS_IFACE = "org.ofono.mms.Service"
> MMS_MANAGER_DBUS_IFACE = "org.ofono.mms.Manager"
> )
> @@ -32,8 +33,17 @@
> IDENTITY = "Identity"
> USE_DELIVERY_REPORTS = "UseDeliveryReports"
> MESSAGE_ADDED = "MessageAdded"
> + MESSAGE_REMOVED = "MessageRemoved"
> SERVICE_ADDED = "ServiceAdded"
> SERVICE_REMOVED = "ServiceRemoved"
> + PROPERTY_CHANGED = "PropertyChanged"
> + STATUS = "Status"
> +)
> +
> +const (
> + PERMANENT_ERROR = "PermanentError"
> + SENT = "Sent"
> + TRANSIENT_ERROR = "TransientError"
> )
>
> const (
>
> === added file 'telepathy/message.go'
> --- telepathy/message.go 1970-01-01 00:00:00 +0000
> +++ telepathy/message.go 2014-07-01 12:24:38 +0000
> @@ -0,0 +1,104 @@
> +/*
> + * Copyright 2014 Canonical Ltd.
> + *
> + * Authors:
> + * Sergio Schvezov: sergio.schvezov at cannical.com
> + *
> + * This file is part of telepathy.
> + *
> + * mms is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 3.
> + *
> + * mms is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +package telepathy
> +
> +import (
> + "fmt"
> + "log"
> +
> + "launchpad.net/go-dbus/v1"
> +)
> +
> +type MessageInterface struct {
> + conn *dbus.Connection
> + objectPath dbus.ObjectPath
> + msgChan chan *dbus.Message
> + deleteChan chan dbus.ObjectPath
> + status string
> +}
> +
> +func NewMessageInterface(conn *dbus.Connection, objectPath dbus.ObjectPath, deleteChan chan dbus.ObjectPath) *MessageInterface {
> + msgInterface := MessageInterface{
> + conn: conn,
> + objectPath: objectPath,
> + deleteChan: deleteChan,
> + msgChan: make(chan *dbus.Message),
> + }
> + go msgInterface.watchDBusMethodCalls()
> + conn.RegisterObjectPath(msgInterface.objectPath, msgInterface.msgChan)
> + return &msgInterface
> +}
> +
> +func (msgInterface *MessageInterface) Close() {
> + close(msgInterface.msgChan)
> + msgInterface.msgChan = nil
> + msgInterface.conn.UnregisterObjectPath(msgInterface.objectPath)
> +}
> +
> +func (msgInterface *MessageInterface) watchDBusMethodCalls() {
> + var reply *dbus.Message
> +
> + for msg := range msgInterface.msgChan {
> + if msg.Interface != MMS_MESSAGE_DBUS_IFACE {
> + log.Println("Received unkown method call on", msg.Interface, msg.Member)
> + reply = dbus.NewErrorMessage(msg, "org.freedesktop.DBus.Error.UnknownMethod", "Unknown method")
> + continue
> + }
> + switch msg.Member {
> + case "Delete":
> + reply = dbus.NewMethodReturnMessage(msg)
> + //TODO implement store and forward
> + if err := msgInterface.conn.Send(reply); err != nil {
> + log.Println("Could not send reply:", err)
> + }
> + msgInterface.deleteChan <- msgInterface.objectPath
> + default:
> + log.Println("Received unkown method call on", msg.Interface, msg.Member)
> + reply = dbus.NewErrorMessage(msg, "org.freedesktop.DBus.Error.UnknownMethod", "Unknown method")
> + if err := msgInterface.conn.Send(reply); err != nil {
> + log.Println("Could not send reply:", err)
> + }
> + }
> + }
> +}
> +
> +func (msgInterface *MessageInterface) StatusChanged(status string) error {
> + var found bool
> + for _, validStatus := range []string{SENT, PERMANENT_ERROR, TRANSIENT_ERROR} {
> + if status == validStatus {
> + found = true
> + break
> + }
> + }
> + if !found {
> + return fmt.Errorf("status %s is not a valid status", status)
> + }
> + msgInterface.status = status
> + signal := dbus.NewSignalMessage(msgInterface.objectPath, MMS_MESSAGE_DBUS_IFACE, PROPERTY_CHANGED)
> + if err := signal.AppendArgs(STATUS, dbus.Variant{status}); err != nil {
> + return err
> + }
> + if err := msgInterface.conn.Send(signal); err != nil {
> + return err
> + }
> + return nil
> +}
The above can be rewritten in the following way:
for _, validStatus := range []string{SENT, PERMANENT_ERROR, TRANSIENT_ERROR} {
if status == validStatus {
msgInterface.status = status
signal := dbus.NewSignalMessage(msgInterface.objectPath, MMS_MESSAGE_DBUS_IFACE, PROPERTY_CHANGED)
if err := signal.AppendArgs(STATUS, dbus.Variant{status}); err != nil {
return err
}
if err := msgInterface.conn.Send(signal); err != nil {
return err
}
return nil
}
}
return fmt.Errorf("status %s is not a valid status", status)
In the above example we remove the use of the variable found but if you do not mind to use the sort package and you create you array already sorted(I don't knoe the string but we should check the constats):
i:= sort([]string{PERMANENT_ERROR, SENT, TRANSIENT_ERROR}, validStatus)
if i < len(data) && data[i] == x {
msgInterface.status = status
signal := dbus.NewSignalMessage(msgInterface.objectPath, MMS_MESSAGE_DBUS_IFACE, PROPERTY_CHANGED)
if err := signal.AppendArgs(STATUS, dbus.Variant{status}); err != nil {
return err
}
if err := msgInterface.conn.Send(signal); err != nil {
return err
}
return nil
}
return fmt.Errorf("status %s is not a valid status", status)
If we use sort is important to remember that the array most be sorted in ascending order, Removes the loop for a better search algorithm and reduces complexity. You can always leave it like it is :)
>
> === modified file 'telepathy/service.go'
> --- telepathy/service.go 2014-07-01 12:24:38 +0000
> +++ telepathy/service.go 2014-07-01 12:24:38 +0000
> @@ -24,6 +24,7 @@
> import (
> "fmt"
> "log"
> + "path/filepath"
> "strings"
> "time"
>
> @@ -40,12 +41,14 @@
> }
>
> type MMSService struct {
> - Payload ServicePayload
> - Properties map[string]dbus.Variant
> - conn *dbus.Connection
> - msgChan chan *dbus.Message
> - identity string
> - outMessage chan *OutgoingMessage
> + Payload ServicePayload
> + Properties map[string]dbus.Variant
> + conn *dbus.Connection
> + msgChan chan *dbus.Message
> + messageHandlers map[dbus.ObjectPath]*MessageInterface
> + msgDeleteChan chan dbus.ObjectPath
> + identity string
> + outMessage chan *OutgoingMessage
> }
>
> type Attachment struct {
> @@ -78,18 +81,29 @@
> Properties: properties,
> }
> service := MMSService{
> - Payload: payload,
> - Properties: serviceProperties,
> - conn: conn,
> - msgChan: make(chan *dbus.Message),
> - outMessage: outgoingChannel,
> - identity: identity,
> + Payload: payload,
> + Properties: serviceProperties,
> + conn: conn,
> + msgChan: make(chan *dbus.Message),
> + msgDeleteChan: make(chan dbus.ObjectPath),
> + messageHandlers: make(map[dbus.ObjectPath]*MessageInterface),
> + outMessage: outgoingChannel,
> + identity: identity,
> }
> go service.watchDBusMethodCalls()
> + go service.watchMessageDeleteCalls()
> conn.RegisterObjectPath(payload.Path, service.msgChan)
> return &service
> }
>
> +func (service *MMSService) watchMessageDeleteCalls() {
> + for msgObjectPath := range service.msgDeleteChan {
> + if err := service.MessageRemoved(msgObjectPath); err != nil {
> + log.Print("Failed to delete ", msgObjectPath, ": ", err)
> + }
> + }
> +}
> +
> func (service *MMSService) watchDBusMethodCalls() {
> for msg := range service.msgChan {
> var reply *dbus.Message
> @@ -147,6 +161,44 @@
> }
> }
>
> +func getUUIDFromObjectPath(objectPath dbus.ObjectPath) (string, error) {
> + str := string(objectPath)
> + defaultError := fmt.Errorf("%s is not a proper object path for a Message", str)
> + if str == "" {
> + return "", defaultError
> + }
> + uuid := filepath.Base(str)
> + if uuid == "" || uuid == ".." || uuid == "." {
> + return "", defaultError
> + }
> + return uuid, nil
> +}
> +
> +//MessageRemoved emits the MessageRemoved signal with the path of the removed
> +//message.
> +//It also actually removes the message from storage.
> +func (service *MMSService) MessageRemoved(objectPath dbus.ObjectPath) error {
> + service.messageHandlers[objectPath].Close()
> + delete(service.messageHandlers, objectPath)
> +
> + uuid, err := getUUIDFromObjectPath(objectPath)
> + if err != nil {
> + return err
> + }
> + if err := storage.Destroy(uuid); err != nil {
> + return err
> + }
> +
> + signal := dbus.NewSignalMessage(service.Payload.Path, MMS_SERVICE_DBUS_IFACE, MESSAGE_REMOVED)
> + if err := signal.AppendArgs(objectPath); err != nil {
> + return err
> + }
> + if err := service.conn.Send(signal); err != nil {
> + return err
> + }
> + return nil
> +}
> +
> //MessageAdded emits a MessageAdded with the path to the added message which
> //is taken as a parameter
> func (service *MMSService) MessageAdded(mRetConf *mms.MRetrieveConf) error {
> @@ -154,6 +206,7 @@
> if err != nil {
> return err
> }
> + service.messageHandlers[payload.Path] = NewMessageInterface(service.conn, payload.Path, service.msgDeleteChan)
> signal := dbus.NewSignalMessage(service.Payload.Path, MMS_SERVICE_DBUS_IFACE, MESSAGE_ADDED)
> if err := signal.AppendArgs(payload.Path, payload.Properties); err != nil {
> return err
> @@ -175,6 +228,7 @@
> func (service *MMSService) Close() {
> service.conn.UnregisterObjectPath(service.Payload.Path)
> close(service.msgChan)
> + close(service.msgDeleteChan)
> }
>
> func (service *MMSService) parseMessage(mRetConf *mms.MRetrieveConf) (ServicePayload, error) {
> @@ -236,9 +290,31 @@
> return recipients
> }
>
> -func (service *MMSService) ReplySendMessage(reply *dbus.Message, uuid string) error {
> - reply.AppendArgs(service.genMessagePath(uuid))
> - return service.conn.Send(reply)
> +func (service *MMSService) MessageDestroy(uuid string) error {
> + msgObjectPath := service.genMessagePath(uuid)
> + if msgInterface, ok := service.messageHandlers[msgObjectPath]; ok {
> + msgInterface.Close()
> + delete(service.messageHandlers, msgObjectPath)
> + }
> + return fmt.Errorf("no message interface handler for object path %s", msgObjectPath)
> +}
> +
> +func (service *MMSService) MessageStatusChanged(uuid, status string) error {
> + msgObjectPath := service.genMessagePath(uuid)
> + if msgInterface, ok := service.messageHandlers[msgObjectPath]; ok {
> + return msgInterface.StatusChanged(status)
> + }
> + return fmt.Errorf("no message interface handler for object path %s", msgObjectPath)
> +}
> +
> +func (service *MMSService) ReplySendMessage(reply *dbus.Message, uuid string) (dbus.ObjectPath, error) {
> + msgObjectPath := service.genMessagePath(uuid)
> + reply.AppendArgs(msgObjectPath)
> + if err := service.conn.Send(reply); err != nil {
> + return "", err
> + }
> + service.messageHandlers[msgObjectPath] = NewMessageInterface(service.conn, msgObjectPath, service.msgDeleteChan)
> + return msgObjectPath, nil
> }
>
> //TODO randomly creating a uuid until the download manager does this for us
>
--
https://code.launchpad.net/~sergiusens/nuntium/message-api/+merge/225147
Your team Ubuntu Phablet Team is subscribed to branch lp:nuntium.
More information about the Ubuntu-reviews
mailing list