review approve
On 20 Nov 2014 09:01, "John Lenton" <email address hidden> wrote:

> Thank you.
>
> Review: approve
> On 19 Nov 2014 19:15, "Samuele Pedroni" <email address hidden>
> wrote:
>
>> Samuele Pedroni has proposed merging
>> lp:~pedronis/go-dbus/atomic-write-per-conn into lp:go-dbus/v1.
>>
>> Commit message:
>> make message writes on a connection atomic (not globally)
>>
>> Requested reviews:
>>   Go D-Bus Hackers (go-dbus)
>>
>> For more details, see:
>>
>> https://code.launchpad.net/~pedronis/go-dbus/atomic-write-per-conn/+merge/242257
>>
>> make message writes on a connection atomic (not globally)
>> --
>> Your team Go D-Bus Hackers is requested to review the proposed merge of
>> lp:~pedronis/go-dbus/atomic-write-per-conn into lp:go-dbus/v1.
>>
>> === modified file 'auth.go'
>> --- auth.go     2014-10-22 21:48:27 +0000
>> +++ auth.go     2014-11-19 19:15:16 +0000
>> @@ -124,7 +124,7 @@
>>         inStream := bufio.NewReader(conn)
>>         send := func(command ...[]byte) ([][]byte, error) {
>>                 msg := bytes.Join(command, []byte(" "))
>> -               // writing at this point doesn't not need to be synced as
>> the connection
>> +               // writing at this point does not need to be synced as
>> the connection
>>                 // is not shared at this point.
>>                 _, err := conn.Write(append(msg, []byte("\r\n")...))
>>                 if err != nil {
>> @@ -182,7 +182,7 @@
>>                 return errors.New("Could not authenticate with any
>> mechanism")
>>         }
>>         // XXX: UNIX FD negotiation would go here.
>> -       // writing at this point doesn't not need to be synced as the
>> connection
>> +       // writing at this point does not need to be synced as the
>> connection
>>         // is not shared at this point.
>>         if _, err := conn.Write([]byte("BEGIN\r\n")); err != nil {
>>                 return err
>>
>> === modified file 'dbus.go'
>> --- dbus.go     2014-11-13 10:14:09 +0000
>> +++ dbus.go     2014-11-19 19:15:16 +0000
>> @@ -37,6 +37,7 @@
>>         // The unique name of this connection on the message bus.
>>         UniqueName string
>>         conn       net.Conn
>> +       writeLock  sync.Mutex
>>         busProxy   BusDaemon
>>         lastSerial uint32
>>
>> @@ -251,12 +252,16 @@
>>         return atomic.AddUint32(&p.lastSerial, 1)
>>  }
>>
>> +func (p *Connection) atomicWriteMessage(msg *Message) error {
>> +       p.writeLock.Lock()
>> +       defer p.writeLock.Unlock()
>> +       _, err := msg.WriteTo(p.conn)
>> +       return err
>> +}
>> +
>>  func (p *Connection) Send(msg *Message) error {
>>         msg.setSerial(p.nextSerial())
>> -       if _, err := msg.WriteTo(p.conn); err != nil {
>> -               return err
>> -       }
>> -       return nil
>> +       return p.atomicWriteMessage(msg)
>>  }
>>
>>  func (p *Connection) SendWithReply(msg *Message) (*Message, error) {
>> @@ -272,7 +277,7 @@
>>         p.methodCallReplies[serial] = replyChan
>>         p.handlerMutex.Unlock()
>>
>> -       if _, err := msg.WriteTo(p.conn); err != nil {
>> +       if err := p.atomicWriteMessage(msg); err != nil {
>>                 p.handlerMutex.Lock()
>>                 delete(p.methodCallReplies, serial)
>>                 p.handlerMutex.Unlock()
>>
>> === modified file 'message.go'
>> --- message.go  2014-10-22 16:08:20 +0000
>> +++ message.go  2014-11-19 19:15:16 +0000
>> @@ -4,7 +4,6 @@
>>         "encoding/binary"
>>         "errors"
>>         "io"
>> -       "sync"
>>  )
>>
>>  // See the D-Bus tutorial for information about message types.
>> @@ -27,8 +26,6 @@
>>         TypeError:        "error",
>>  }
>>
>> -var writeMutex sync.Mutex
>> -
>>  func (t MessageType) String() string { return messageTypeString[t] }
>>
>>  type MessageFlag uint8
>> @@ -320,12 +317,8 @@
>>         return msg, nil
>>  }
>>
>> -// WriteTo serialises the message and writes it to the given writer.
>> +// WriteTo serialises the message and writes it to the given writer. Not
>> atomic!
>>  func (p *Message) WriteTo(w io.Writer) (int64, error) {
>> -       // message writing needs to be sequential
>> -       writeMutex.Lock()
>> -       defer writeMutex.Unlock()
>> -
>>         fields := make([]headerField, 0, 10)
>>         if p.Path != "" {
>>                 fields = append(fields, headerField{1, Variant{p.Path}})
>>
>>
>>