@cryptotaxi247 / kubo / commits / 934b8b0dd

unixfs: integrate `pb.Data` into `FSNode`

To avoid duplicating fields and making the code easier to follow. Remove all of `FSNode` previous fields in favor on a single `pb.Data` structure that is not exported. Accessor methods are added only for the necessary internal fields. This takes up more memory, `pb.Data` is always created inside `FSNode` and it stays there instead of just being created and destroyed during the (un)marshal operations. The removed fields `Data`, `blocksizes` and `Type` had a direct counterpart in the embedded `pb.Data` structure, in contrast (only) the `subtotal` field doesn't have one, it was used as a temporary accumulator to track the `Filesize`, which is now being kept updated on every modification (to ensure the entire `FSNode` is always at a valid state), so `subtotal` could just be removed without the addition of any other field (this temporary accumulator was obscuring how `Filesize` was computed). To keep `Filesize` up to date a method was added (`UpdateFilesize()`) to adjust its value in the two places where the file size could be modified, when changing its data (in `SetData()`, accessor method added) and when adding or removing child nodes (in `AddBlockSize()` and `RemoveBlockSize()`). A constructor method was added (`NewFSNode()`) to initialize the required fields, like `Type` which is explicitly set, this deprecates the previous methodology of just calling `new(FSNode)` and relying in the default value of `pb.Data_DataType` (`Data_Raw`) to avoid an explicit assignment. Also, `Filesize` is initialized to avoid being left with a `nil` value before marshaling empty nodes, which would result in a different hash from previous versions, to be backwards compatible. Previous versions of `GetBytes()` always set the `Filesize` value, even though it is reflected as an `optional` field in the `.proto` file (this may be an inaccurate field rule). Without the duplicated fields the functions `GetBytes()` and `FSNodeFromBytes()` are now reduced to simple `Marshal()` and `Unmarshal()` operations respectively. License: MIT Signed-off-by: Lucas Molas <schomatis@gmail.com>

Lucas Molas committed Jun 8, 2018 at 12:21 UTC 934b8b0ddccda805eb99b7229032e9cd2f866e7c
7 files changed +72 -39
importer/helpers/dagbuilder.go
+2 -2
@@ -116,7 +116,7 @@ func (db *DagBuilderHelper) GetDagServ() ipld.DAGService {
116 func (db *DagBuilderHelper) NewUnixfsNode() *UnixfsNode {
117 n := &UnixfsNode{
118 node: new(dag.ProtoNode),
119 - ufmt: &ft.FSNode{Type: ft.TFile},
119 + ufmt: ft.NewFSNode(ft.TFile),
120 }
121 n.SetPrefix(db.prefix)
122 return n
@@ -161,7 +161,7 @@ func (db *DagBuilderHelper) NewLeaf(data []byte) (*UnixfsNode, error) {
161 func (db *DagBuilderHelper) newUnixfsBlock() *UnixfsNode {
162 n := &UnixfsNode{
163 node: new(dag.ProtoNode),
164 - ufmt: &ft.FSNode{Type: ft.TRaw},
164 + ufmt: ft.NewFSNode(ft.TRaw),
165 }
166 n.SetPrefix(db.prefix)
167 return n
importer/helpers/helpers.go
+2 -2
@@ -77,7 +77,7 @@ func (n *UnixfsNode) Set(other *UnixfsNode) {
77 n.raw = other.raw
78 n.rawnode = other.rawnode
79 if other.ufmt != nil {
80 - n.ufmt.Data = other.ufmt.Data
80 + n.ufmt.SetData(other.ufmt.GetData())
81 }
82 }
83
@@ -127,7 +127,7 @@ func (n *UnixfsNode) RemoveChild(index int, dbh *DagBuilderHelper) {
127
128 // SetData stores data in this node.
129 func (n *UnixfsNode) SetData(data []byte) {
130 - n.ufmt.Data = data
130 + n.ufmt.SetData(data)
131 }
132
133 // FileSize returns the total file size of this tree (including children)
mfs/file.go
+1 -1
@@ -60,7 +60,7 @@ func (fi *File) Open(flags int, sync bool) (FileDescriptor, error) {
60 return nil, err
61 }
62
63 - switch fsn.Type {
63 + switch fsn.GetType() {
64 default:
65 return nil, fmt.Errorf("unsupported fsnode type for 'file'")
66 case ft.TSymlink:
mfs/mfs_test.go
+1 -1
@@ -852,7 +852,7 @@ func TestFlushing(t *testing.T) {
852 t.Fatal(err)
853 }
854
855 - if fsnode.Type != ft.TDirectory {
855 + if fsnode.GetType() != ft.TDirectory {
856 t.Fatal("root wasnt a directory")
857 }
858
unixfs/mod/dagmodifier.go
+1 -1
@@ -526,7 +526,7 @@ func dagTruncate(ctx context.Context, n ipld.Node, size uint64, ds ipld.DAGServi
526 var cur uint64
527 end := 0
528 var modified ipld.Node
529 - ndata := new(ft.FSNode)
529 + ndata := ft.NewFSNode(ft.TRaw)
530 for i, lnk := range nd.Links() {
531 child, err := lnk.GetNode(ctx, ds)
532 if err != nil {
unixfs/unixfs.go
+63 -29
@@ -139,67 +139,101 @@ func DataSize(data []byte) (uint64, error) {
139 }
140 }
141
142 -// An FSNode represents a filesystem object.
142 +// An FSNode represents a filesystem object using the UnixFS specification.
143 +//
144 +// The `NewFSNode` constructor should be used instead of just calling `new(FSNode)`
145 +// to guarantee that the required (`Type` and `Filesize`) fields in the `format`
146 +// structure are initialized before marshaling (in `GetBytes()`).
147 type FSNode struct {
144 - Data []byte
148
146 - // total data size for each child
147 - blocksizes []uint64
148 -
149 - // running sum of blocksizes
150 - subtotal uint64
151 -
152 - // node type of this node
153 - Type pb.Data_DataType
149 + // UnixFS format defined as a protocol buffers message.
150 + format pb.Data
151 }
152
153 // FSNodeFromBytes unmarshal a protobuf message onto an FSNode.
154 func FSNodeFromBytes(b []byte) (*FSNode, error) {
158 - pbn := new(pb.Data)
159 - err := proto.Unmarshal(b, pbn)
155 + n := new(FSNode)
156 + err := proto.Unmarshal(b, &n.format)
157 if err != nil {
158 return nil, err
159 }
160
164 - n := new(FSNode)
165 - n.Data = pbn.Data
166 - n.blocksizes = pbn.Blocksizes
167 - n.subtotal = pbn.GetFilesize() - uint64(len(n.Data))
168 - n.Type = pbn.GetType()
161 return n, nil
162 }
163
164 +// NewFSNode creates a new FSNode structure with the given `dataType`.
165 +//
166 +// It initializes the (required) `Type` field (that doesn't have a `Set()`
167 +// accessor so it must be specified at creation), otherwise the `Marshal()`
168 +// method in `GetBytes()` would fail (`required field "Type" not set`).
169 +//
170 +// It also initializes the `Filesize` pointer field to ensure its value
171 +// is never nil before marshaling, this is not a required field but it is
172 +// done to be backwards compatible with previous `go-ipfs` versions hash.
173 +// (If it wasn't initialized there could be cases where `Filesize` could
174 +// have been left at nil, when the `FSNode` was created but no data or
175 +// child nodes were set to adjust it, as is the case in `NewLeaf()`.)
176 +func NewFSNode(dataType pb.Data_DataType) *FSNode {
177 + n := new(FSNode)
178 + n.format.Type = &dataType
179 +
180 + // Initialize by `Filesize` by updating it with a dummy (zero) value.
181 + n.UpdateFilesize(0)
182 +
183 + return n
184 +}
185 +
186 // AddBlockSize adds the size of the next child block of this node
187 func (n *FSNode) AddBlockSize(s uint64) {
174 - n.subtotal += s
175 - n.blocksizes = append(n.blocksizes, s)
188 + n.UpdateFilesize(int64(s))
189 + n.format.Blocksizes = append(n.format.Blocksizes, s)
190 }
191
192 // RemoveBlockSize removes the given child block's size.
193 func (n *FSNode) RemoveBlockSize(i int) {
180 - n.subtotal -= n.blocksizes[i]
181 - n.blocksizes = append(n.blocksizes[:i], n.blocksizes[i+1:]...)
194 + n.UpdateFilesize(-int64(n.format.Blocksizes[i]))
195 + n.format.Blocksizes = append(n.format.Blocksizes[:i], n.format.Blocksizes[i+1:]...)
196 }
197
198 // GetBytes marshals this node as a protobuf message.
199 func (n *FSNode) GetBytes() ([]byte, error) {
186 - pbn := new(pb.Data)
187 - pbn.Type = &n.Type
188 - pbn.Filesize = proto.Uint64(uint64(len(n.Data)) + n.subtotal)
189 - pbn.Blocksizes = n.blocksizes
190 - pbn.Data = n.Data
191 - return proto.Marshal(pbn)
200 + return proto.Marshal(&n.format)
201 }
202
203 // FileSize returns the total size of this tree. That is, the size of
204 // the data in this node plus the size of all its children.
205 func (n *FSNode) FileSize() uint64 {
197 - return uint64(len(n.Data)) + n.subtotal
206 + return n.format.GetFilesize()
207 }
208
209 // NumChildren returns the number of child blocks of this node
210 func (n *FSNode) NumChildren() int {
202 - return len(n.blocksizes)
211 + return len(n.format.Blocksizes)
212 +}
213 +
214 +// GetData retrieves the `Data` field from the internal `format`.
215 +func (n *FSNode) GetData() []byte {
216 + return n.format.GetData()
217 +}
218 +
219 +// SetData sets the `Data` field from the internal `format`
220 +// updating its `Filesize`.
221 +func (n *FSNode) SetData(newData []byte) {
222 + n.UpdateFilesize(int64(len(newData) - len(n.GetData())))
223 + n.format.Data = newData
224 +}
225 +
226 +// UpdateFilesize updates the `Filesize` field from the internal `format`
227 +// by a signed difference (`filesizeDiff`).
228 +// TODO: Add assert to check for `Filesize` > 0?
229 +func (n *FSNode) UpdateFilesize(filesizeDiff int64) {
230 + n.format.Filesize = proto.Uint64(uint64(
231 + int64(n.format.GetFilesize()) + filesizeDiff))
232 +}
233 +
234 +// GetType retrieves the `Type` field from the internal `format`.
235 +func (n *FSNode) GetType() pb.Data_DataType {
236 + return n.format.GetType()
237 }
238
239 // Metadata is used to store additional FSNode information.
unixfs/unixfs_test.go
+2 -3
@@ -10,14 +10,13 @@ import (
10 )
11
12 func TestFSNode(t *testing.T) {
13 - fsn := new(FSNode)
14 - fsn.Type = TFile
13 + fsn := NewFSNode(TFile)
14 for i := 0; i < 16; i++ {
15 fsn.AddBlockSize(100)
16 }
17 fsn.RemoveBlockSize(15)
18
20 - fsn.Data = make([]byte, 128)
19 + fsn.SetData(make([]byte, 128))
20
21 b, err := fsn.GetBytes()
22 if err != nil {