From 7cb74b158f6aee35142ec19e91708ee803119656 Mon Sep 17 00:00:00 2001 From: Matt Joiner Date: Fri, 6 Nov 2020 10:36:49 +1100 Subject: [PATCH] Replace storage.IncompletePieceToWriter with io.Writer It was incorrect to assume piece hashing only operates on incomplete chunk data. This actually uncovered a bug where duplicate hash checks occurred, and the redundant checks would fail due to not reading the completed data. --- storage/interface.go | 9 ++------- storage/piece_resource.go | 34 +++++++++++++++++++++++--------- storage/sqlite/sqlite-storage.go | 5 +++-- storage/wrappers.go | 13 +++++++----- torrent.go | 4 ++-- 5 files changed, 40 insertions(+), 25 deletions(-) diff --git a/storage/interface.go b/storage/interface.go index a1c6fceb..869556f8 100644 --- a/storage/interface.go +++ b/storage/interface.go @@ -22,7 +22,8 @@ type TorrentImpl interface { Close() error } -// Interacts with torrent piece data. +// Interacts with torrent piece data. Optional interfaces to implement include io.WriterTo, such as +// when a piece supports a more efficient way to write out incomplete chunks type PieceImpl interface { // These interfaces are not as strict as normally required. They can // assume that the parameters are appropriate for the dimensions of the @@ -38,12 +39,6 @@ type PieceImpl interface { Completion() Completion } -// Optional PieceImpl interface when pieces support a more efficient way to write incomplete chunks -// out. -type IncompletePieceToWriter interface { - WriteIncompleteTo(w io.Writer) error -} - type Completion struct { Complete bool Ok bool diff --git a/storage/piece_resource.go b/storage/piece_resource.go index 7f875320..36088f32 100644 --- a/storage/piece_resource.go +++ b/storage/piece_resource.go @@ -46,7 +46,7 @@ type PieceProvider interface { } type ConsecutiveChunkWriter interface { - WriteConsecutiveChunks(prefix string, _ io.Writer) error + WriteConsecutiveChunks(prefix string, _ io.Writer) (int64, error) } type piecePerResourcePiece struct { @@ -54,14 +54,27 @@ type piecePerResourcePiece struct { rp resource.Provider } -var _ IncompletePieceToWriter = piecePerResourcePiece{} +var _ io.WriterTo = piecePerResourcePiece{} -func (s piecePerResourcePiece) WriteIncompleteTo(w io.Writer) error { +func (s piecePerResourcePiece) WriteTo(w io.Writer) (int64, error) { if ccw, ok := s.rp.(ConsecutiveChunkWriter); ok { - return ccw.WriteConsecutiveChunks(s.incompleteDirPath()+"/", w) + if s.mustIsComplete() { + return ccw.WriteConsecutiveChunks(s.completedInstancePath(), w) + } else { + return ccw.WriteConsecutiveChunks(s.incompleteDirPath()+"/", w) + } } - _, err := io.Copy(w, io.NewSectionReader(s.getChunks(), 0, s.mp.Length())) - return err + return io.Copy(w, io.NewSectionReader(s, 0, s.mp.Length())) +} + +// Returns if the piece is complete. Ok should be true, because we are the definitive source of +// truth here. +func (s piecePerResourcePiece) mustIsComplete() bool { + completion := s.Completion() + if !completion.Ok { + panic("must know complete definitively") + } + return completion.Complete } func (s piecePerResourcePiece) Completion() Completion { @@ -88,10 +101,9 @@ func (s piecePerResourcePiece) MarkNotComplete() error { } func (s piecePerResourcePiece) ReadAt(b []byte, off int64) (int, error) { - if s.Completion().Complete { + if s.mustIsComplete() { return s.completed().ReadAt(b, off) } - //panic("unexpected ReadAt of incomplete piece") return s.getChunks().ReadAt(b, off) } @@ -156,8 +168,12 @@ func (s piecePerResourcePiece) getChunks() (chunks chunks) { return } +func (s piecePerResourcePiece) completedInstancePath() string { + return path.Join("completed", s.mp.Hash().HexString()) +} + func (s piecePerResourcePiece) completed() resource.Instance { - i, err := s.rp.NewInstance(path.Join("completed", s.mp.Hash().HexString())) + i, err := s.rp.NewInstance(s.completedInstancePath()) if err != nil { panic(err) } diff --git a/storage/sqlite/sqlite-storage.go b/storage/sqlite/sqlite-storage.go index 7eeb8a10..9dded6b5 100644 --- a/storage/sqlite/sqlite-storage.go +++ b/storage/sqlite/sqlite-storage.go @@ -307,7 +307,7 @@ type provider struct { var _ storage.ConsecutiveChunkWriter = (*provider)(nil) -func (p *provider) WriteConsecutiveChunks(prefix string, w io.Writer) (err error) { +func (p *provider) WriteConsecutiveChunks(prefix string, w io.Writer) (written int64, err error) { p.withConn(func(conn conn) { err = io.EOF err = sqlitex.Exec(conn, ` @@ -321,7 +321,8 @@ func (p *provider) WriteConsecutiveChunks(prefix string, w io.Writer) (err error r := stmt.ColumnReader(0) //offset := stmt.ColumnInt64(1) //log.Printf("got %v bytes at offset %v", r.Len(), offset) - _, err := io.Copy(w, r) + w1, err := io.Copy(w, r) + written += w1 return err }, len(prefix), diff --git a/storage/wrappers.go b/storage/wrappers.go index aa6c609b..06cc12ad 100644 --- a/storage/wrappers.go +++ b/storage/wrappers.go @@ -38,14 +38,17 @@ type Piece struct { mip metainfo.Piece } -func (p Piece) WriteIncompleteTo(w io.Writer) error { - if i, ok := p.PieceImpl.(IncompletePieceToWriter); ok { - return i.WriteIncompleteTo(w) +var _ io.WriterTo = Piece{} + +// Why do we have this wrapper? Well PieceImpl doesn't implement io.Reader, so we can't let io.Copy +// and friends check for io.WriterTo and fallback for us since they expect an io.Reader. +func (p Piece) WriteTo(w io.Writer) (int64, error) { + if i, ok := p.PieceImpl.(io.WriterTo); ok { + return i.WriteTo(w) } n := p.mip.Length() r := io.NewSectionReader(p, 0, n) - _, err := io.CopyN(w, r, n) - return err + return io.CopyN(w, r, n) } func (p Piece) WriteAt(b []byte, off int64) (n int, err error) { diff --git a/torrent.go b/torrent.go index 412fad7d..ced35bb7 100644 --- a/torrent.go +++ b/torrent.go @@ -795,10 +795,10 @@ func (t *Torrent) hashPiece(piece pieceIndex) (ret metainfo.Hash, err error) { const logPieceContents = false if logPieceContents { var examineBuf bytes.Buffer - err = storagePiece.WriteIncompleteTo(io.MultiWriter(hash, &examineBuf)) + _, err = storagePiece.WriteTo(io.MultiWriter(hash, &examineBuf)) log.Printf("hashed %q with copy err %v", examineBuf.Bytes(), err) } else { - err = storagePiece.WriteIncompleteTo(hash) + _, err = storagePiece.WriteTo(hash) } missinggo.CopyExact(&ret, hash.Sum(nil)) return -- 2.48.1