request-server: implement Open method

[drakkan]

add an optional interface to FileWriter to allow to open a file for both
writing and reading

Fixes #374

I noticed that an Open method is documented but not implemented, see here. I implemented this method. The change is backward compatible and implementing the new optional Filewriter interface the request server can handle both reading and writing to the same file fixing the corruption issue reported in #374

I know that we want something different for v2 and so I'll not push this change myself.

However I cannot wait until we implement v2 to fix a file corruption issue in SFTPGo, so if we don't agree to merge this change I'll maintain it in my fork until we have an alternative, thank you

[puellanivis]

I don’t think there’s much if any recommendations for change. Just musings over possible alternatives. (i.e. soft approve; design and structure are fine)

[puellanivis]

Is this necessary for non-testing situations?

[puellanivis]

I think I would probably rather see an actual implementation rather than exploiting the bytes.NewReader type, but I doubt there ends up being any difference in speed really, and this is more readable.

Why can’t the memFile contain a bytes.Buffer and then we could get a bunch of the functionalit— oh, huh, bytes.Buffer doesn’t implement ReaderAt or WriterAt? 😬

[drakkan]

Hi,

after changing the code like this

func (fs *root) getFileForWrite(r *Request) (*memFile, error) {
	if fs.mockErr != nil {
		return nil, fs.mockErr
	}
	_ = r.WithContext(r.Context()) // initialize context for deadlock testing
	fs.filesLock.Lock()
	defer fs.filesLock.Unlock()
	file, err := fs.fetch(r.Filepath)
	if err == os.ErrNotExist {
		dir, err := fs.fetch(path.Dir(r.Filepath))
		if err != nil {
			return nil, err
		}
		if !dir.isdir {
			return nil, os.ErrInvalid
		}
		file = newMemFile(r.Filepath, false)
		fs.files[r.Filepath] = file
	}
	return file, nil
}

func (fs *root) Filewrite(r *Request) (io.WriterAt, error) {
	file, err := fs.getFileForWrite(r)
	if err != nil {
		return nil, err
	}
	return file.WriterAt()
}

func (fs *root) OpenFile(r *Request) (WriterAtReaderAt, error) {
	return fs.getFileForWrite(r)
}

I'm getting test failures like this one:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x58 pc=0x680b45]

goroutine 229 [running]:
github.com/pkg/sftp.(*memFile).TransferError(0x0, 0x7b8da0, 0xc000410140)
	/home/nicola/goprojects/sftp/request-example.go:345 +0x25
github.com/pkg/sftp.(*Request).transferError(0xc000720750, 0x7b8da0, 0xc000410140)
	/home/nicola/goprojects/sftp/request.go:195 +0x185
github.com/pkg/sftp.(*RequestServer).Serve(0xc000616a00, 0x0, 0x0)
	/home/nicola/goprojects/sftp/request-server.go:171 +0x259
github.com/pkg/sftp.clientRequestServerPair.func1(0xc000020ea0, 0xc000313380, 0xc000318020)
	/home/nicola/goprojects/sftp/request-server_test.go:55 +0x245
created by github.com/pkg/sftp.clientRequestServerPair
	/home/nicola/goprojects/sftp/request-server_test.go:40 +0xe5
exit status 2

for some reason now in code like this

if t, ok := wr.(TransferError); ok {
	t.TransferError(err)
}

wr implements the interface but is nil (probably because is a pointer to a struct, the struct implements the interface but it is nil). I got the same error in SFTPGo in the past when returning a pointer to a struct. What do you think to add something like this

if t, ok := wr.(TransferError); ok && t != nil && !reflect.ValueOf(t).IsNil() {
	t.TransferError(err)
}

to avoid this issue? Thank you

[puellanivis]

wr implements the interface but is nil (probably because is a pointer to a struct, the struct implements the interface but it is nil).

Yeah, this is a source of a lot of issues, where a (*ConcreteType)(nil) implements a method, but does not expect a nil-pointer.

[drakkan]

wr implements the interface but is nil (probably because is a pointer to a struct, the struct implements the interface but it is nil).

Yeah, this is a source of a lot of issues, where a (*ConcreteType)(nil) implements a method, but does not expect a nil-pointer.

OK, so is this PR ready to merge for you? Or should we document that the implementer should expect a null pointer?

Thank you

[puellanivis]

More preferably, the Request.open(…) handler should handle err != nil at each call site, and not assign any values into r.state if there is an error.

It shouldn’t be hard to copy-paste the if err != nil { return statusFromError(pkt, err) } into each of the funcitons, and implement them properly, rather than cheating with a “fallthrough” logic pattern…

[drakkan]

Great! I wish you could review all my code!

[puellanivis]

Great! I wish you could review all my code!

You say that. And then a month in, you’re just like, “but it works, right?” 😰

[drakkan]

Great! I wish you could review all my code!

You say that. And then a month in, you’re just like, “but it works, right?”

I don't think so :)

[puellanivis]

All good!