lib/protocol: Refactor interface (#9375)

This is a refactor of the protocol/model interface to take the actual
message as the parameter, instead of the broken-out fields:

```diff
type Model interface {
        // An index was received from the peer device
-       Index(conn Connection, folder string, files []FileInfo) error
+       Index(conn Connection, idx *Index) error
        // An index update was received from the peer device
-       IndexUpdate(conn Connection, folder string, files []FileInfo) error
+       IndexUpdate(conn Connection, idxUp *IndexUpdate) error
        // A request was made by the peer device
-       Request(conn Connection, folder, name string, blockNo, size int32, offset int64, hash []byte, weakHash uint32, fromTemporary bool) (RequestResponse, error)
+       Request(conn Connection, req *Request) (RequestResponse, error)
        // A cluster configuration message was received
-       ClusterConfig(conn Connection, config ClusterConfig) error
+       ClusterConfig(conn Connection, config *ClusterConfig) error
        // The peer device closed the connection or an error occurred
        Closed(conn Connection, err error)
        // The peer device sent progress updates for the files it is currently downloading
-       DownloadProgress(conn Connection, folder string, updates []FileDownloadProgressUpdate) error
+       DownloadProgress(conn Connection, p *DownloadProgress) error
 }
```

(and changing the `ClusterConfig` to `*ClusterConfig` for symmetry;
we'll be forced to use all pointers everywhere at some point anyway...)

The reason for this is that I have another thing cooking which is a
small troubleshooting change to check index consistency during transfer.
This required adding a field or two to the index/indexupdate messages,
and plumbing the extra parameters in umpteen changes is almost as big a
diff as this is. I figured let's do it once and avoid having to do that
in the future again...

The rest of the diff falls out of the change above, much of it being in
test code where we run these methods manually...

lib/protocol/benchmark_test.go

@@ -167,30 +167,30 @@ func negotiateTLS(cert tls.Certificate, conn0, conn1 net.Conn) (net.Conn, net.Co
 
 type fakeModel struct{}
 
-func (*fakeModel) Index(Connection, string, []FileInfo) error {
+func (*fakeModel) Index(Connection, *Index) error {
 	return nil
 }
 
-func (*fakeModel) IndexUpdate(Connection, string, []FileInfo) error {
+func (*fakeModel) IndexUpdate(Connection, *IndexUpdate) error {
 	return nil
 }
 
-func (*fakeModel) Request(_ Connection, _, _ string, _, size int32, offset int64, _ []byte, _ uint32, _ bool) (RequestResponse, error) {
+func (*fakeModel) Request(_ Connection, req *Request) (RequestResponse, error) {
 	// We write the offset to the end of the buffer, so the receiver
 	// can verify that it did in fact get some data back over the
 	// connection.
-	buf := make([]byte, size)
-	binary.BigEndian.PutUint64(buf[len(buf)-8:], uint64(offset))
+	buf := make([]byte, req.Size)
+	binary.BigEndian.PutUint64(buf[len(buf)-8:], uint64(req.Offset))
 	return &fakeRequestResponse{buf}, nil
 }
 
-func (*fakeModel) ClusterConfig(Connection, ClusterConfig) error {
+func (*fakeModel) ClusterConfig(Connection, *ClusterConfig) error {
 	return nil
 }
 
 func (*fakeModel) Closed(Connection, error) {
 }
 
-func (*fakeModel) DownloadProgress(Connection, string, []FileDownloadProgressUpdate) error {
+func (*fakeModel) DownloadProgress(Connection, *DownloadProgress) error {
 	return nil
 }

lib/protocol/common_test.go

@@ -14,7 +14,7 @@ type TestModel struct {
 	weakHash      uint32
 	fromTemporary bool
 	indexFn       func(string, []FileInfo)
-	ccFn          func(ClusterConfig)
+	ccFn          func(*ClusterConfig)
 	closedCh      chan struct{}
 	closedErr     error
 }
@@ -25,25 +25,25 @@ func newTestModel() *TestModel {
 	}
 }
 
-func (t *TestModel) Index(_ Connection, folder string, files []FileInfo) error {
+func (t *TestModel) Index(_ Connection, idx *Index) error {
 	if t.indexFn != nil {
-		t.indexFn(folder, files)
+		t.indexFn(idx.Folder, idx.Files)
 	}
 	return nil
 }
 
-func (*TestModel) IndexUpdate(Connection, string, []FileInfo) error {
+func (*TestModel) IndexUpdate(Connection, *IndexUpdate) error {
 	return nil
 }
 
-func (t *TestModel) Request(_ Connection, folder, name string, _, size int32, offset int64, hash []byte, weakHash uint32, fromTemporary bool) (RequestResponse, error) {
-	t.folder = folder
-	t.name = name
-	t.offset = offset
-	t.size = size
-	t.hash = hash
-	t.weakHash = weakHash
-	t.fromTemporary = fromTemporary
+func (t *TestModel) Request(_ Connection, req *Request) (RequestResponse, error) {
+	t.folder = req.Folder
+	t.name = req.Name
+	t.offset = req.Offset
+	t.size = int32(req.Size)
+	t.hash = req.Hash
+	t.weakHash = req.WeakHash
+	t.fromTemporary = req.FromTemporary
 	buf := make([]byte, len(t.data))
 	copy(buf, t.data)
 	return &fakeRequestResponse{buf}, nil
@@ -54,14 +54,14 @@ func (t *TestModel) Closed(_ Connection, err error) {
 	close(t.closedCh)
 }
 
-func (t *TestModel) ClusterConfig(_ Connection, config ClusterConfig) error {
+func (t *TestModel) ClusterConfig(_ Connection, config *ClusterConfig) error {
 	if t.ccFn != nil {
 		t.ccFn(config)
 	}
 	return nil
 }
 
-func (*TestModel) DownloadProgress(Connection, string, []FileDownloadProgressUpdate) error {
+func (*TestModel) DownloadProgress(Connection, *DownloadProgress) error {
 	return nil
 }
 

lib/protocol/encryption.go

@@ -56,43 +56,43 @@ func newEncryptedModel(model rawModel, folderKeys *folderKeyRegistry, keyGen *Ke
 	}
 }
 
-func (e encryptedModel) Index(folder string, files []FileInfo) error {
-	if folderKey, ok := e.folderKeys.get(folder); ok {
+func (e encryptedModel) Index(idx *Index) error {
+	if folderKey, ok := e.folderKeys.get(idx.Folder); ok {
 		// incoming index data to be decrypted
-		if err := decryptFileInfos(e.keyGen, files, folderKey); err != nil {
+		if err := decryptFileInfos(e.keyGen, idx.Files, folderKey); err != nil {
 			return err
 		}
 	}
-	return e.model.Index(folder, files)
+	return e.model.Index(idx)
 }
 
-func (e encryptedModel) IndexUpdate(folder string, files []FileInfo) error {
-	if folderKey, ok := e.folderKeys.get(folder); ok {
+func (e encryptedModel) IndexUpdate(idxUp *IndexUpdate) error {
+	if folderKey, ok := e.folderKeys.get(idxUp.Folder); ok {
 		// incoming index data to be decrypted
-		if err := decryptFileInfos(e.keyGen, files, folderKey); err != nil {
+		if err := decryptFileInfos(e.keyGen, idxUp.Files, folderKey); err != nil {
 			return err
 		}
 	}
-	return e.model.IndexUpdate(folder, files)
+	return e.model.IndexUpdate(idxUp)
 }
 
-func (e encryptedModel) Request(folder, name string, blockNo, size int32, offset int64, hash []byte, weakHash uint32, fromTemporary bool) (RequestResponse, error) {
-	folderKey, ok := e.folderKeys.get(folder)
+func (e encryptedModel) Request(req *Request) (RequestResponse, error) {
+	folderKey, ok := e.folderKeys.get(req.Folder)
 	if !ok {
-		return e.model.Request(folder, name, blockNo, size, offset, hash, weakHash, fromTemporary)
+		return e.model.Request(req)
 	}
 
 	// Figure out the real file name, offset and size from the encrypted /
 	// tweaked values.
 
-	realName, err := decryptName(name, folderKey)
+	realName, err := decryptName(req.Name, folderKey)
 	if err != nil {
 		return nil, fmt.Errorf("decrypting name: %w", err)
 	}
-	realSize := size - blockOverhead
-	realOffset := offset - int64(blockNo*blockOverhead)
+	realSize := req.Size - blockOverhead
+	realOffset := req.Offset - int64(req.BlockNo*blockOverhead)
 
-	if size < minPaddedSize {
+	if req.Size < minPaddedSize {
 		return nil, errors.New("short request")
 	}
 
@@ -105,13 +105,13 @@ func (e encryptedModel) Request(folder, name string, blockNo, size int32, offset
 
 	var realHash []byte
 	fileKey := e.keyGen.FileKey(realName, folderKey)
-	if len(hash) > 0 {
+	if len(req.Hash) > 0 {
 		var additional [8]byte
 		binary.BigEndian.PutUint64(additional[:], uint64(realOffset))
-		realHash, err = decryptDeterministic(hash, fileKey, additional[:])
+		realHash, err = decryptDeterministic(req.Hash, fileKey, additional[:])
 		if err != nil {
 			// "Legacy", no offset additional data?
-			realHash, err = decryptDeterministic(hash, fileKey, nil)
+			realHash, err = decryptDeterministic(req.Hash, fileKey, nil)
 		}
 		if err != nil {
 			return nil, fmt.Errorf("decrypting block hash: %w", err)
@@ -120,7 +120,11 @@ func (e encryptedModel) Request(folder, name string, blockNo, size int32, offset
 
 	// Perform that request and grab the data.
 
-	resp, err := e.model.Request(folder, realName, blockNo, realSize, realOffset, realHash, 0, false)
+	req.Name = realName
+	req.Size = realSize
+	req.Offset = realOffset
+	req.Hash = realHash
+	resp, err := e.model.Request(req)
 	if err != nil {
 		return nil, err
 	}
@@ -142,16 +146,16 @@ func (e encryptedModel) Request(folder, name string, blockNo, size int32, offset
 	return rawResponse{enc}, nil
 }
 
-func (e encryptedModel) DownloadProgress(folder string, updates []FileDownloadProgressUpdate) error {
-	if _, ok := e.folderKeys.get(folder); !ok {
-		return e.model.DownloadProgress(folder, updates)
+func (e encryptedModel) DownloadProgress(p *DownloadProgress) error {
+	if _, ok := e.folderKeys.get(p.Folder); !ok {
+		return e.model.DownloadProgress(p)
 	}
 
 	// Encrypted devices shouldn't send these - ignore them.
 	return nil
 }
 
-func (e encryptedModel) ClusterConfig(config ClusterConfig) error {
+func (e encryptedModel) ClusterConfig(config *ClusterConfig) error {
 	return e.model.ClusterConfig(config)
 }
 

lib/protocol/nativemodel_darwin.go

@@ -15,21 +15,21 @@ type nativeModel struct {
 	rawModel
 }
 
-func (m nativeModel) Index(folder string, files []FileInfo) error {
-	for i := range files {
-		files[i].Name = norm.NFD.String(files[i].Name)
+func (m nativeModel) Index(idx *Index) error {
+	for i := range idx.Files {
+		idx.Files[i].Name = norm.NFD.String(idx.Files[i].Name)
 	}
-	return m.rawModel.Index(folder, files)
+	return m.rawModel.Index(idx)
 }
 
-func (m nativeModel) IndexUpdate(folder string, files []FileInfo) error {
-	for i := range files {
-		files[i].Name = norm.NFD.String(files[i].Name)
+func (m nativeModel) IndexUpdate(idxUp *IndexUpdate) error {
+	for i := range idxUp.Files {
+		idxUp.Files[i].Name = norm.NFD.String(idxUp.Files[i].Name)
 	}
-	return m.rawModel.IndexUpdate(folder, files)
+	return m.rawModel.IndexUpdate(idxUp)
 }
 
-func (m nativeModel) Request(folder, name string, blockNo, size int32, offset int64, hash []byte, weakHash uint32, fromTemporary bool) (RequestResponse, error) {
-	name = norm.NFD.String(name)
-	return m.rawModel.Request(folder, name, blockNo, size, offset, hash, weakHash, fromTemporary)
+func (m nativeModel) Request(req *Request) (RequestResponse, error) {
+	req.Name = norm.NFD.String(req.Name)
+	return m.rawModel.Request(req)
 }

lib/protocol/nativemodel_windows.go

@@ -19,24 +19,24 @@ type nativeModel struct {
 	rawModel
 }
 
-func (m nativeModel) Index(folder string, files []FileInfo) error {
-	files = fixupFiles(files)
-	return m.rawModel.Index(folder, files)
+func (m nativeModel) Index(idx *Index) error {
+	idx.Files = fixupFiles(idx.Files)
+	return m.rawModel.Index(idx)
 }
 
-func (m nativeModel) IndexUpdate(folder string, files []FileInfo) error {
-	files = fixupFiles(files)
-	return m.rawModel.IndexUpdate(folder, files)
+func (m nativeModel) IndexUpdate(idxUp *IndexUpdate) error {
+	idxUp.Files = fixupFiles(idxUp.Files)
+	return m.rawModel.IndexUpdate(idxUp)
 }
 
-func (m nativeModel) Request(folder, name string, blockNo, size int32, offset int64, hash []byte, weakHash uint32, fromTemporary bool) (RequestResponse, error) {
-	if strings.Contains(name, `\`) {
-		l.Warnf("Dropping request for %s, contains invalid path separator", name)
+func (m nativeModel) Request(req *Request) (RequestResponse, error) {
+	if strings.Contains(req.Name, `\`) {
+		l.Warnf("Dropping request for %s, contains invalid path separator", req.Name)
 		return nil, ErrNoSuchFile
 	}
 
-	name = filepath.FromSlash(name)
-	return m.rawModel.Request(folder, name, blockNo, size, offset, hash, weakHash, fromTemporary)
+	req.Name = filepath.FromSlash(req.Name)
+	return m.rawModel.Request(req)
 }
 
 func fixupFiles(files []FileInfo) []FileInfo {

lib/protocol/protocol.go

@@ -123,28 +123,28 @@ var (
 
 type Model interface {
 	// An index was received from the peer device
-	Index(conn Connection, folder string, files []FileInfo) error
+	Index(conn Connection, idx *Index) error
 	// An index update was received from the peer device
-	IndexUpdate(conn Connection, folder string, files []FileInfo) error
+	IndexUpdate(conn Connection, idxUp *IndexUpdate) error
 	// A request was made by the peer device
-	Request(conn Connection, folder, name string, blockNo, size int32, offset int64, hash []byte, weakHash uint32, fromTemporary bool) (RequestResponse, error)
+	Request(conn Connection, req *Request) (RequestResponse, error)
 	// A cluster configuration message was received
-	ClusterConfig(conn Connection, config ClusterConfig) error
+	ClusterConfig(conn Connection, config *ClusterConfig) error
 	// The peer device closed the connection or an error occurred
 	Closed(conn Connection, err error)
 	// The peer device sent progress updates for the files it is currently downloading
-	DownloadProgress(conn Connection, folder string, updates []FileDownloadProgressUpdate) error
+	DownloadProgress(conn Connection, p *DownloadProgress) error
 }
 
 // rawModel is the Model interface, but without the initial Connection
 // parameter. Internal use only.
 type rawModel interface {
-	Index(folder string, files []FileInfo) error
-	IndexUpdate(folder string, files []FileInfo) error
-	Request(folder, name string, blockNo, size int32, offset int64, hash []byte, weakHash uint32, fromTemporary bool) (RequestResponse, error)
-	ClusterConfig(config ClusterConfig) error
+	Index(*Index) error
+	IndexUpdate(*IndexUpdate) error
+	Request(*Request) (RequestResponse, error)
+	ClusterConfig(*ClusterConfig) error
 	Closed(err error)
-	DownloadProgress(folder string, updates []FileDownloadProgressUpdate) error
+	DownloadProgress(*DownloadProgress) error
 }
 
 type RequestResponse interface {
@@ -493,22 +493,22 @@ func (c *rawConnection) dispatcherLoop() (err error) {
 
 		switch msg := msg.(type) {
 		case *ClusterConfig:
-			err = c.model.ClusterConfig(*msg)
+			err = c.model.ClusterConfig(msg)
 
 		case *Index:
-			err = c.handleIndex(*msg)
+			err = c.handleIndex(msg)
 
 		case *IndexUpdate:
-			err = c.handleIndexUpdate(*msg)
+			err = c.handleIndexUpdate(msg)
 
 		case *Request:
-			go c.handleRequest(*msg)
+			go c.handleRequest(msg)
 
 		case *Response:
-			c.handleResponse(*msg)
+			c.handleResponse(msg)
 
 		case *DownloadProgress:
-			err = c.model.DownloadProgress(msg.Folder, msg.Updates)
+			err = c.model.DownloadProgress(msg)
 		}
 		if err != nil {
 			return newHandleError(err, msgContext)
@@ -613,14 +613,14 @@ func (c *rawConnection) readHeader(fourByteBuf []byte) (Header, error) {
 	return hdr, nil
 }
 
-func (c *rawConnection) handleIndex(im Index) error {
+func (c *rawConnection) handleIndex(im *Index) error {
 	l.Debugf("Index(%v, %v, %d file)", c.deviceID, im.Folder, len(im.Files))
-	return c.model.Index(im.Folder, im.Files)
+	return c.model.Index(im)
 }
 
-func (c *rawConnection) handleIndexUpdate(im IndexUpdate) error {
+func (c *rawConnection) handleIndexUpdate(im *IndexUpdate) error {
 	l.Debugf("queueing IndexUpdate(%v, %v, %d files)", c.deviceID, im.Folder, len(im.Files))
-	return c.model.IndexUpdate(im.Folder, im.Files)
+	return c.model.IndexUpdate(im)
 }
 
 // checkIndexConsistency verifies a number of invariants on FileInfos received in
@@ -685,8 +685,8 @@ func checkFilename(name string) error {
 	return nil
 }
 
-func (c *rawConnection) handleRequest(req Request) {
-	res, err := c.model.Request(req.Folder, req.Name, int32(req.BlockNo), int32(req.Size), req.Offset, req.Hash, req.WeakHash, req.FromTemporary)
+func (c *rawConnection) handleRequest(req *Request) {
+	res, err := c.model.Request(req)
 	if err != nil {
 		c.send(context.Background(), &Response{
 			ID:   req.ID,
@@ -704,7 +704,7 @@ func (c *rawConnection) handleRequest(req Request) {
 	res.Close()
 }
 
-func (c *rawConnection) handleResponse(resp Response) {
+func (c *rawConnection) handleResponse(resp *Response) {
 	c.awaitingMut.Lock()
 	if rc := c.awaiting[resp.ID]; rc != nil {
 		delete(c.awaiting, resp.ID)
@@ -1127,19 +1127,19 @@ type connectionWrappingModel struct {
 	model Model
 }
 
-func (c *connectionWrappingModel) Index(folder string, files []FileInfo) error {
-	return c.model.Index(c.conn, folder, files)
+func (c *connectionWrappingModel) Index(m *Index) error {
+	return c.model.Index(c.conn, m)
 }
 
-func (c *connectionWrappingModel) IndexUpdate(folder string, files []FileInfo) error {
-	return c.model.IndexUpdate(c.conn, folder, files)
+func (c *connectionWrappingModel) IndexUpdate(idxUp *IndexUpdate) error {
+	return c.model.IndexUpdate(c.conn, idxUp)
 }
 
-func (c *connectionWrappingModel) Request(folder, name string, blockNo, size int32, offset int64, hash []byte, weakHash uint32, fromTemporary bool) (RequestResponse, error) {
-	return c.model.Request(c.conn, folder, name, blockNo, size, offset, hash, weakHash, fromTemporary)
+func (c *connectionWrappingModel) Request(req *Request) (RequestResponse, error) {
+	return c.model.Request(c.conn, req)
 }
 
-func (c *connectionWrappingModel) ClusterConfig(config ClusterConfig) error {
+func (c *connectionWrappingModel) ClusterConfig(config *ClusterConfig) error {
 	return c.model.ClusterConfig(c.conn, config)
 }
 
@@ -1147,6 +1147,6 @@ func (c *connectionWrappingModel) Closed(err error) {
 	c.model.Closed(c.conn, err)
 }
 
-func (c *connectionWrappingModel) DownloadProgress(folder string, updates []FileDownloadProgressUpdate) error {
-	return c.model.DownloadProgress(c.conn, folder, updates)
+func (c *connectionWrappingModel) DownloadProgress(p *DownloadProgress) error {
+	return c.model.DownloadProgress(c.conn, p)
 }

lib/protocol/protocol_test.go

@@ -924,7 +924,7 @@ func TestDispatcherToCloseDeadlock(t *testing.T) {
 	m := newTestModel()
 	rw := testutil.NewBlockingRW()
 	c := getRawConnection(NewConnection(c0ID, rw, &testutil.NoopRW{}, testutil.NoopCloser{}, m, new(mockedConnectionInfo), CompressionAlways, nil, testKeyGen))
-	m.ccFn = func(ClusterConfig) {
+	m.ccFn = func(*ClusterConfig) {
 		c.Close(errManual)
 	}
 	c.Start()