Closed drakkan closed 2 years ago
So, my principal concern here is avoiding another Attr []byte
field issue, where we’re not keeping a decoded value, we’re keeping the raw unparsed binary data. 😬 That caused a headache when I went in and did the marshal/unmarshal work, and made my first aborted pass on getting it rolled out. But I don’t think there’s any concern of that here.
I’m not opposed to this idea, but one thing, is maybe we could take the chance to reformat the struct. Right now, it’s all clumped together, no paragraphs showing the grouping of fields, and both the RawTarget
and the purposed RawFilepath
have their comment on the line above them, rather than on the same line to the right, like the other fields do. This causes a break of the formatting flow.
There are already unexported fields, so no one could be using the implicit field building of the struct except us, so even reordering fields won’t break anyone’s code. Then we can put the four paths together, Flags
and Attrs
together, and maybe handle should be at the top?
And we can shorten this up pretty tight: func (r *Request) copy() *Request { r2 := *r ; r2.state = r.state.copy(); return &r2 }
?
Thanks, do you mean reordering the struct like this?
// Request contains the data and state for the incoming service request.
type Request struct {
handle string
// Get, Put, Setstat, Stat, Rename, Remove
// Rmdir, Mkdir, List, Readlink, Link, Symlink
Method string
RawFilepath string // raw file path as sent by the SFTP client
Filepath string // POSIX absolute path, "/" is used as base if RawFilepath is relative
RawTarget string // raw target path for rename and sym-links as sent by the SFTP client
Target string // POSIX absolute path for renames and sym-links, "/" is used as base if RawTarget is relative
Flags uint32
Attrs []byte // convert to sub-struct
// reader/writer/readdir from handlers
state
// context lasts duration of request
ctx context.Context
cancelCtx context.CancelFunc
}
So, my principal concern here is avoiding another
Attr []byte
field issue, where we’re not keeping a decoded value, we’re keeping the raw unparsed binary data. grimacing That caused a headache when I went in and did the marshal/unmarshal work, and made my first aborted pass on getting it rolled out. But I don’t think there’s any concern of that here.I’m not opposed to this idea, but one thing, is maybe we could take the chance to reformat the struct. Right now, it’s all clumped together, no paragraphs showing the grouping of fields, and both the
RawTarget
and the purposedRawFilepath
have their comment on the line above them, rather than on the same line to the right, like the other fields do. This causes a break of the formatting flow.There are already unexported fields, so no one could be using the implicit field building of the struct except us, so even reordering fields won’t break anyone’s code. Then we can put the four paths together,
Flags
andAttrs
together, and maybe handle should be at the top?And we can shorten this up pretty tight:
func (r *Request) copy() *Request { r2 := *r ; r2.state = r.state.copy(); return &r2 }
?
r2 := *r
will produce the warning assignment copies lock value to r2
, we could ignore the warning since in the next statement we do r2.state = r.state.copy()
. I'm not sure
will produce the warning
assignment copies lock value to r2
Ah yes. OK, that’s why we were doing it that way. OK then, let’s ignore that improvement. Ideally, if we’re reimplementing, we would be able to avoid using any locks, the same as http.Request
.
will produce the warning
assignment copies lock value to r2
Ah yes. OK, that’s why we were doing it that way. OK then, let’s ignore that improvement. Ideally, if we’re reimplementing, we would be able to avoid using any locks, the same as
http.Request
.
The lock seems only used to protect the state in close method that could happen concurrently with other requests. It is a read/write lock and in the most of the cases the lock should not be held for write (if my quick look at the code is correct) so it should not be performance critical.
I marked this PR as draft because I wanted to hear your opinion on the approach used and wanted to test the implementation of the start directory in a separate sftpgo branch. I'm not sure I have enough time this weekend to do that. So my priority, for now is to test this patch in a real implementation. If it works as expected we can discuss about other library improvements :smile:
After real testing I changed my mind and I would prefer to merge #498
This allows to clean the paths externally to the library and allows to handle a start directory for example.
Another option is to add a start directory option to the request server and build the relative path using this directory as base instead of
/
. I would prefer to handle paths myself outside the library but I'm open to discussions. I can also live without this feature :smile: