Marshal extended data if file info supports it #553
Conversation
attrs.go
Outdated
| flags |= sshFileXferAttrExtended | ||
| fileStat.Extended = fiExt.Extended() |
There was a problem hiding this comment.
Consider that if len(fileStat.Extended) == 0 we have a better encoding by not asserting the sshFileXferAttrExtended flag in the first place?
There was a problem hiding this comment.
True, this should obviously be checked.
attrs.go
Outdated
| @@ -69,6 +69,19 @@ func fileInfoFromStat(stat *FileStat, name string) os.FileInfo { | |||
| } | |||
| } | |||
|
|
|||
| // FileInfoUidGid extends os.FileInfo and adds callbacks for Uid and Gid retrieval. | |||
There was a problem hiding this comment.
Hm, I’m not sure what the value is of exporting this interface here. Yes, it’s used immediately below, but it’s not anywhere where a caller would make use of it to understand where it is relevant?
There was a problem hiding this comment.
I've added a comment to the code to specify where it can be required. I'm using the RequestServer code to implement an sftp server that looks like a unix filesystem but is actually stored in a kind of object store. There is no reason to have *syscall.Stat_t around for just including uid/gid support.
I guess it could be an unexported interface too, of course.
There was a problem hiding this comment.
Yeah, I think our long-term plan should be that users can just return a sshfx.Attributes or sshfx.NameEntry more or less directly, meaning we can avoid needing to do too many work arounds to allow access to the full feature set.
But this is still a bit of a ways off, waiting for v2.
peterverraedt
force-pushed
the
marshal-extended
branch
2 times, most recently
from
352aa99
to
361e7cf
Compare
peterverraedt
force-pushed
the
marshal-extended
branch
from
361e7cf
to
1fe811c
Compare
attrs.go
Outdated
| // The call above will include the sshFileXferAttrUIDGID in case | ||
| // the os.FileInfo can be casted to *syscall.Stat_t on unix. | ||
| // If implementations of RequestServers Handlers want to implement | ||
| // setting uids and gids while running on non-unix systems or | ||
| // without using *syscall.Stat_t underneath on unix, we offer | ||
| // an alternative way to support this through the FileInfoUidGid | ||
| // interface. |
There was a problem hiding this comment.
So, I think my only remaining criticism, as small as it is, is that I was less worried about us internally tracking the proper usage of these extension FileInfo interfaces. That’s no problem, we maintain the code, and can “just get” how this works.
My concern was for importing code, and users of our package to know how and when the FileInfo extension interfaces should be used and what they will do. This means the documentation probably has to go into that for ListerAt or FileLister right?
There was a problem hiding this comment.
Right. I'll try to move the documentation of this.
peterverraedt
force-pushed
the
marshal-extended
branch
2 times, most recently
from
551612f
to
c6fc1b2
Compare
request-interfaces.go
Outdated
| // ListerAt provides entries that satisfy the os.FileInfo interface. | ||
| // Uid and gid information will on unix systems be retrieved from | ||
| // [os.FileInfo.Sys] if this function returns a [syscall.Stat_t]. | ||
| // Alternatively, if the entry implements [FileInfoUidGid], it will be | ||
| // used for uid and gid information. | ||
| // If the entry implements [FileInfoExtendedData], extended attributes will | ||
| // also be returned to the client. |
There was a problem hiding this comment.
One last nitpick: paragraphs. It seems it was not well paragraphed already, but we can take this opportunity to clean up the text.
Also, if we could use semantic line breaks: https://sembr.org/ over arbitrarily wrapping at around 80 cols.
Broadly:
// InterfaceName does an important thing presented "as a headline".
//
// Important details about the InterfaceName that are tricky or notable in the implementation,
// but not so broadly applicable that they need to be in the headline.
//
// If a Special Case applies,
// then we note the details about that special case.
// It should start with the context of when the condition applies,
// so readers can skip the whole paragraph if the condition is irrelevant to them.
//
// If more Special Cases apply,
// then they should keep following on new paragraphs.
// Putting each Special Case in its own paragraph means readers can quickly skip to the next paragraph.
//
// If a set of special conditions exist,
// and each is fairly straight-forward enough to just list them, go ahead and do that directly:
//
// A function domain = a special cases
// A type name: and it’s short simple situation
//
// Note: Notes should show up at the bottom,
// so that readers can skim up from the end for quick access to gotchas, and other minutiae.
If the file information returned by ListAt supports the FileInfoExtendedData interface, use it to retrieve extended data and pass it back to the client. In similar fashion, we add the FileInfoUidGid interface to allow for implementations to return uid and gid data, even if the os lacks support in syscall.Stat_t. Signed-off-by: Peter Verraedt <peter@verraedt.be>
peterverraedt
force-pushed
the
marshal-extended
branch
from
c6fc1b2
to
c632ce6
Compare
If the file information returned by ListAt supports the FileInfoExtendedData interface, use it to retrieve extended data and pass it back to the client. In similar fashion, we add the FileInfoUidGid interface to allow for implementations to return uid and gid data, even if the os lacks support in syscall.Stat_t.