Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Regroup marshling/unmarshaling and lint #456

Merged
merged 11 commits into from
Aug 20, 2021
Merged

Regroup marshling/unmarshaling and lint #456

merged 11 commits into from
Aug 20, 2021

Conversation

puellanivis
Copy link
Collaborator

This covers a few different things that were somewhat unrelated to starting to use filexfer but will make that transfer easier.

  • Collects and isolates marshal/unmarshal code into packets.go
  • Removing clamp and min functions that both did the same thing, and didn’t really contribute to more understandable code.
  • Fix corner cases in os.ModeType → uint32 where we treat the various types as bitfields that can be OR’ed together rather than mutually exclusive values.
  • a small bit of linting on request-server.go
  • fileInfo can use the values in the wrapped FileStat, which we also don’t need to treat as an any.
  • bring over the ls_formatting from PR-432, as this is an improvement beyond just bringing in filexfer as it extracts more UID/GID values for the various GOOS, supports setting UID/GID values from a FileStat, and also converts UIDs/GIDs to usernames and groupnames when possible. (Do we really want to use system-wide username/groupnames? Might be good to provide an ability to override this? Since request-server might be operating entirely disjoint from the underlying OS.)
  • removed a few cgo build conditions that were not actually necessary
  • verified that the code compiles for all GOOS supported in go1.16 (not verified that it works, just that it compiles)

@puellanivis
Copy link
Collaborator Author

I should run a regression test on this with FileZilla… since I’m touching the longname field…

@drakkan
Copy link
Collaborator

drakkan commented Aug 16, 2021 •
edited
Loading

Hi,

I just tested with FileZilla and it seems ok. SFTPGo test cases also passed.

I'm not sure about UIDs/GIDs to usernames and groupnames conversion, however we may allow you to override the conversion in the future if needed. Thanks!

@drakkan
Copy link
Collaborator

drakkan commented Aug 16, 2021 •
edited
Loading

The directory listing is now something like this

sftp> ls -la
drwxr-xr-x    1 0        0               0 Aug 16 07:19 .
drwxr-xr-x    1 0        0               0 Aug 16 07:19 local
-rw-r--r--    1 nicola   nicola       4034 Aug 16 07:19 file.txt
drwxr-xr-x    1 0        0               0 Aug 16 07:19 vdir1
drwxr-xr-x    1 0        0               0 Aug 16 07:19 vdir2
drwxr-xr-x    1 0        0               0 Aug 16 07:19 vdir3

., local, vdir1, vdir2, vdir3 are virtual directory and so UIDs/GIDs are not converted. The same happen for cloud storage based accounts. For non local files/dirs the type assertion fails and so no conversion happen. I think this is ok for now. Maybe if future we could allow to disable this conversion (it could slow down directory listing if we have a lot of entries) using a setting or provide an optional interface

@puellanivis
Copy link
Collaborator Author

Yeah, I was thinking of abstracting it away to an optional interface as well. It shouldn’t be too hard. 🤔

@puellanivis
Copy link
Collaborator Author

I’ve spun off the username and groupname lookup into an extension type on FileLister, default behavior for RequestServer should end up as “do not convert UID/GID from numbers”, and default unoverridable behavior for Server is to use os/user for username and groupname lookup.

@drakkan
Copy link
Collaborator

drakkan commented Aug 20, 2021

it looks good for me, thanks

@puellanivis puellanivis merged commit f525d18 into master Aug 20, 2021
@puellanivis puellanivis deleted the refactor/regroup-and-lint branch August 20, 2021 22:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@drakkan drakkan drakkan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@puellanivis @drakkan