server.go: "/" for windows #571
Conversation
|
I suppose I want to add that changing from this |
Ok, but I'd think it would be good to make it opt-out by default - Because I'd guess most people have not yet thought about this edge case, and would not expect the old behavior. What do you think? |
|
The correct default state of the option should be the old behavior. Because it’s the current behavior, and we don’t know if people are depending on this current behavior. |
|
How to add the an additional server option? Can you please help out? Edit: Ok fixed in latest commit |
|
@powellnorma I'm not familiar with the old Server implementation and so I can't help you much here and I don't even have time to do so. The feature you propose makes sense and would be useful for Windows users (although you can use RequestServer as an alternative) but please:
Thanks for understanding and for trying to contribute to this package |
server.go
Outdated
| type dummyFile struct { | ||
| } | ||
|
|
||
| func (f *dummyFile) Stat() (os.FileInfo, error) { | ||
| return nil, os.ErrPermission | ||
| } | ||
| func (f *dummyFile) ReadAt(b []byte, off int64) (int, error) { | ||
| return 0, os.ErrPermission | ||
| } | ||
| func (f *dummyFile) WriteAt(b []byte, off int64) (int, error) { | ||
| return 0, os.ErrPermission | ||
| } | ||
| func (f *dummyFile) Readdir(int) ([]os.FileInfo, error) { | ||
| return nil, os.ErrPermission | ||
| } | ||
| func (f *dummyFile) Name() string { | ||
| return "dummyFile" | ||
| } | ||
| func (f *dummyFile) Truncate(int64) error { | ||
| return os.ErrPermission | ||
| } | ||
| func (f *dummyFile) Chmod(mode fs.FileMode) error { | ||
| return os.ErrPermission | ||
| } | ||
| func (f *dummyFile) Chown(uid, gid int) error { | ||
| return os.ErrPermission | ||
| } | ||
| func (f *dummyFile) Close() error { | ||
| return os.ErrPermission | ||
| } | ||
|
|
||
| var _ = dummyFile{} // ignore unused |
There was a problem hiding this comment.
Why
SSH_FX_PERMISSION_DENIED?
https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_03
[EPERM]
Operation not permitted. An attempt was made to perform an operation limited to processes with
appropriate privileges or to the owner of a file or other resource.
Consult https://elixir.bootlin.com/linux/latest/C/ident/EPERM and look for basically any use therein. There are far more uses of EPERM for “Operation not permitted” rather than simply “file permissions do not allow this operation”.
For example: https://man7.org/linux/man-pages/man2/mkdir.2.html
EPERM The filesystem containing pathname does not support the creation of directories.
Clients are far more likely to be operating under the assumption that os.ErrPermission is a “do not retry this operation, but handle is still open for further use”, while SSH_FX_FAILURE is so generic that a client cannot really recover from the failure programmatically at all, as it is completely undefined and unknown whether any further operations are possible at that point.
Who is "we"?
This package, and the users of this package. I am specifically excluding any people who are forking this package. They are not “we”—the users of this package—because by using a fork, they are necessarily using a different package.
I think this principle is useful for deciding if one wants to invest the time into implementing something.. But in this case it is already implemented, and IMO is cleaner (for the reason described above).
Part of software development is deleting unnecessary code.
The original form of Server has used a map[string]*os.File since it was first introduced in a merged in September 2015. We have only now 9 years later, seen good cause to convert this into an interface. And this is despite already rolling out a better dynamic server API in RequestServer, because of which we have restricted the focus of Server to be strictly a very simple, thin, and transparent layer over the underlying OS filesystems. And the only reason we’re picking this up, is because Windows is a unique OS among all of the OSes that Golang supports.
When I tell you, “we’re not going to need that”, I mean it. It’s been almost a decade until we even needed the interface for a second implementation. There is no reason to prepare for a future that we’re unlikely to ever need. And “yeah, but I already did the work” does not negate, “and now we have to maintaining it.”
Indeed, in the very PR that introduced the map[string]*os.File an earlier version did implement this as an interface. But this was removed in favor of a simple map[string]*os.File presumably after realizing that Sever can simply use the concrete type, and so we didn’t need the interface. So, that work was undone, because it was unnecessary.
But don't expect me to change this while not giving any clear reasons, thanks 🙏
You are now, as you have been this whole time, under no obligation to do the work of implementing this PR. You volunteered for this task. However, in opposition to your ability to work on this at whatever pace you want all the way up to and including at any time “taking your ball and going home”, the same expectations are not on us as the contributors.
In order for your PR to be effected, I, or another contributor to this package, must sacrifice time out of our lives in order to engage with you, and polish the PR to the point, where we are comfortable with the code enough to merge it into this widely used and adopted package, and failure to respond promptly would be disrespectful to the time and effort you’ve put in. Even that means it comes at the expense of our weekend, or evenings. (Since I certainly don’t have the spare time during my work day to do this volunteer work.)
You must certainly understand that while you might commit this code, and then move on to other projects, the contributors of this repo have volunteered to stick around and support all of the code in this repo long-term. It is not unreasonable for us to push back on overly complex code implementing features that we are not using, and see no immediate utility to in the future.
If this dummyFile type is so useful that it becomes necessary, then we can refactor our code at that point. Until then, we have no use for this code.
But don't expect me to change…
And we are under no obligation to merge your code. I reasonably suggested to you that you should remove it, and even before this more detailed and extensive post, I had provided sufficient explanation as to why. And now almost 2 hours of research in order to pedantically track down references to provide context, and dive through the commit history of this package in order to dig up points to justify my argument, I’m unsure what you ever expected my behavior to be.
I am a volunteer that no matter what else I do stand in representation for the group of all contributors who are volunteering their time. Being directly rude, actively aggressive, or abusive would be wholly inappropriate, and reflect poorly on the whole package’s contributors. So, I must be patient, but insistent. If you interpret this professional courtesy as “passive aggressive”, then I’m sorry.
If you have any feedback I’m happy to listen to it. But unfortunately, as a person who will be responsible for maintaining this code after your PR is merged, I continue to insist that this change be made, whether you find any rational reason behind it or not.
|
Not sure if I'm testing this PR in the correct way but I get this I'm testing using The sftp cli sends a lstat packet and the server errors out here |
|
Oh wow, yeah, that’s not translating the remote path into local path. 😮 P.S. wait, no, it is calling into |
|
OK, I think we don’t want to block the existing changes on implementing But right now, until we have full compatibility with the openssh CLI client, we just cannot merge this. Or, we merge this, and then fix the issues ourselves… there are a few nitpicks I’d like to address as well anyways. Either way, I’d like to release a patch version covering the zos changes first. |
fix #569