-
Notifications
You must be signed in to change notification settings - Fork 380
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.
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
server.go: "/" for windows #571
Open
powellnorma
wants to merge
8
commits into
pkg:master
Choose a base branch
from
powellnorma:win-root
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
c7065ea
server.go: "/" for windows
powellnorma 931e169
review adjustments
powellnorma 2a3c7ed
review adjustments 2
powellnorma 3892818
review adjustments 3
powellnorma 35c82d7
review adjustments 4
powellnorma 0395c12
review adjustments 5
powellnorma 073d834
Merge branch 'master' into win-root
drakkan e52de81
Fix merge with master
drakkan File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
//go:build !windows | ||
// +build !windows | ||
|
||
package sftp | ||
|
||
import ( | ||
"io/fs" | ||
"os" | ||
) | ||
|
||
func (s *Server) openfile(path string, flag int, mode fs.FileMode) (file, error) { | ||
return os.OpenFile(path, flag, mode) | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you have made your life significantly harder than it needs to be.
We only have two real implementations:
os.File
and our internal pseudo-directory listing the drives.You have however, here implemented an abstraction layer to allow our internal pseudo-directory listing to just embed this and voila, it is now automagically a
FileLike
. Except, we don’t need that magic. We can just implement all of this directly onWinRoot
directly. Then we only have the one type rather than two.This also solves the problem you’ve created here where
dummyFile.Name()
always returns"dummyFile"
which is a valid filename, and quite an inappropriate value just be returning like everything should automatically know that it’s not a real filename.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I think this however makes it super easy to extend the current codebase for more "virtual paths", for which only "a small subset of file operations" is implemented. One can now just "inherit"
dummyFile
(via promoted fields) and "overwrite" the methods one needs to implement.Or one just removes "Name()" from the list of functions a default implementation is provided? What filename do you suggest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
YAGNI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you know? It doesn't add much complexity IMO. It allows one to do interesting stuff, for example one could create a /proc like "filesystem" that is completely implemented in the sftp server! I surely could see some interesting use cases for that, which I might even add later (in a private fork). I think one should consider the pros and cons, I don't see many cons in this case? But maybe I am missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want to add it to your private fork, I cannot stop you. But we don’t need this code in this package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mixing
dummyFile
withwinRoot
IMO mixes two separate issues.. The first is to implement a "dummy" that realizes thefile
interface, the second is to implement thewinRoot
functionality.When one looks at
winRoot
, one imediatly sees the gist of what it is doing. It is not obscurred by all the "dummy implementations", because those are contained indummyFile
.Who is "we"? Do you know who is going to fork it, and potentially want to add functionality? I don't think so.
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).
If this gets into "No you have to change X and Y and Z because I like it more" without mentioning real reasons behind that, then I must tell you that I don't have time for that. It is similar to the "No you need to use
permission denied
, because I say so", and disregarding the point I gave on why I thinkOperations cannot be performed on this handle
is more matching for a situation where not the privilege level of the user is the problem, but that the operation makes no sense for the referenced resource.This is your repository so you are free to do what you want. But don't expect me to change this while not giving any clear reasons, thanks 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_03
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
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”, whileSSH_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.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.
Part of software development is deleting unnecessary code.
The original form of
Server
has used amap[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 inRequestServer
, because of which we have restricted the focus ofServer
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 simplemap[string]*os.File
presumably after realizing thatSever
can simply use the concrete type, and so we didn’t need the interface. So, that work was undone, because it was unnecessary.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.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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the thorough explanation.
Okay, if that is the case I guess
SSH_FX_PERMISSION_DENIED
is practically the more appropriate choice.However, I want to note, that it feels like
SSH_FX_PERMISSION_DENIED
then takes the role of "generic error" thatSSH_FX_FAILURE
perhaps should have had.And I still think that
os.ErrPermission
is / can be misleading to the end-user who reads this error message.An error like
ENOSYS
would be ideal I guess. Hmm,SSH_FX_OP_UNSUPPORTED
comes to mind.Well, one could say that the server does not implement e.g. copying a directory to
/
(the virtual root). But it could be implemented to e.g. mean "create a new drive and copy the folder contents onto there".So I think it would be more similar to when e.g. Linux gives
ENOSYS
(orEOPNOTSUPP
) for deleting a file for a specific filesystem. It's not that linux is incapable of deleting files, but just the current implementation of the filesystem is (thus rendering linux unable to do that for that filesystem).If you think about it, this argument goes both ways. You originally said that implementing this would be a lot of work, and you don't see anyone having time for that. In effect, a useful feature thus might have never been implemented, because the code looks too "tedious" to be refactored/extended in such a way.
I see your point of keeping implementations "clean and lean". However, I'd say if it is not "a lot of complexity" that gets added, the advantages (easier to be extended in the future) outweighs the disadvantage.
And in this case, I personally even feel like separating
dummyFile
andwinRoot
is cleaner, even if one only usesdummyFile
once.But I guess it's a matter of taste, so I have changed the code according to your wishes.
No, what I perceived as "passive aggressive" was when you just repeated things you already said, which I had given an answer to, but you (thereby) ignored what I said.
It felt as if you were not willing to listen to the points I made, and instead just were half-annoyed that I dared to question some of your statements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had already addressed the appropriateness of this status value 5 days ago: #569 (comment)
Except that
ENOSYS
is not listed at all forunlink(2)
https://man7.org/linux/man-pages/man2/unlink.2.htmlIn fact, instead, it is noted specifically that for linux:
EOPNOTSUPP
is defined for sockets only, (https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_03)You might note that it allows for it to be conflated with
ENOTSUP
. However, even when conflated, only socket operations should be returning theEOPNOTSUPP
synonym, while other operations should still be returningENOTSUP
.As for the appropriateness of
ENOTSUP
not only does my argument againstSSH_FX_OP_UNSUPPORTED
still apply, so does my argument againstENOSYS
.You’ve mistaken entirely what my anticipation of the tedium would be.
Your originally proposed method of implementing this was to modify every single point in the code where we called
getHandle
and treat a magic handle token specially. I warned you against that course of action, because this would have indeed been horrendously tedious. I directed you instead, to the least tedious implementation.Instead, the tedium, which I anticipated, was never in constructing a pseudo-directory implementation. It was anticipating the process of ensuring everything worked properly, and that we covered all the “gotchas” that invariably result from implementing a parallel implementation—in this case to
*os.File
. Namely:Readdir(n)
with ann > 0
, you have to ensure to return a most that number of entries.Readdir(n)
with ann <= 0
, you have to ensure that it returns all entries.Readdir
a second time, you should not return any of the entries already returned.Readdir
is called and there are no remaining entries, it must returnio.EOF
, but not ever beforeReaddir
has to return filenames that actually reflect the usage that clients should be using them as (i.e.c:
not justc
)os.Stat("c:\")
actually returns, and how we need to change it in order to fit our use-case, so that we can be sure we’re returning the proper and appropriate values from all of thefs.FileInfo
values for the individual drivestoLocalPath
operations to ensure that we don’t allow accesses to\Windows
but instead require a drive nameIf I have repeated myself, it is because you refused to acknowledge or listen to the point I had already made, or brought up a point we had already moved from, while trying to get me to relitigate it. (see, above, where you again drag out
SSH_FX_OP_UNSUPPORTED
after I had already written a detailed essay about why it would be inappropriate.)Your arguments have repeatedly refused to actually listen to anything for which I was taking hours to research and compose, in order to patiently explain all this to you.
Even now, your response is not, “I understand. Thank you for taking so much time explaining these fine, minute, and subtle details, and helping to ensure that my code covers all of the gotcha corner cases involved in the standards. Let me know if this code addresses your concerns,” but instead you have answered, “I still refuse to acknowledge your arguments. I think I’m still right, but I have constructed a face-saving reason for me to comply. So, I will do it your way.”
You have mistaken “no matter how many times you repeat this argument, the standards and references continue to refute it,” for “I am ignoring your arguments.”
If I were ignoring your arguments, I would not be pulling up the POSIX documentation in order to evaluate the validity and soundness of your arguments. Meanwhile, in this single message alone, you’ve:
SSH_FX_OP_UNSUPPORTED
EOPNOTSUPP
without actually reading the POSIX standard to see that it is entirely constrained to sockets.To me, it really does seems like you are not only not reading my explanations; you’re not even reading your own references.
Under advisement from @drakkan , this matter has gotten quite a bit out of hand, so we’re parking everything involved for 2 weeks, so that all parties (including me) can cool off, and we can come back to this later, when emotions are not being run raw.
Since I am both: the person with an imbalance of control in this situation, and also the one closing down the discussion; I want to be clear that I’m not doing this to silence you. You may post a reasonable response, but no further actions will be taken from my side for two weeks.†
†: With the caveat that there is one linting comment to address a typo, which will follow this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like if we look at the bigger picture, we both mostly have the same goal of making this software more useful. It's not useful for either of us to spend much time discussing minor details, like whether to implement something in one way or a slightly different way.
Two things:
sshfs itself uses this error code when it gets an 'SSH_FX_OP_UNSUPPORTED':
https://github.com/libfuse/sshfs/blob/sshfs-3.7.3/sshfs.c#L1704
If we look at the linux source code, we see that it is actually used a lot in filesystem (fs/*) code:
https://elixir.bootlin.com/linux/v4.0/ident/EOPNOTSUPP
Your argument was that the server does generally support the operation (e.g. unlink), so it should not return this error.
However, my counter-argument was that "operation" might entail the subject (e.g. the path) that is operated on.
If we look at the docs for
SSH_FX_PERMISSION_DENIED
, they say:The user does not have sufficient permissions to perform the operation
.So it seems that here the term "operation" clearly entails the subject (path) that is operated on, because the user probably can e.g. unlink his own files.
I don't think you have addressed that "counter-argument" yet, but instead you say things like
repeated an already refuted SSH_FX_OP_UNSUPPORTED
.Maybe you have just missed/overlooked that, it's not (overall) a big deal. I think it will help us to assume that the other person had good intentions. Then probably it will be easier for us to see the point the other person is making, instead of going into some kind of "defensive mode".
You said:
just even adding support to use a pseudo-directory in Server would be a pain, because right now our handles are all mapping directly to an actual *os.File which—as noted above—cannot exist for this pseudo-directory.
This clearly references the restrictiveness of the old implementation, and that (alone) would be "a pain".
And true, making sure that everything is implemented correctly is quite some work too! And I am grateful that you took the time to review it that carefully.
I said:
Still, I have my own opinions. And while I might do X as requested because I agree with you on Y, it doesn't mean that this has to "invalidate" my previous opinion Z which may also be about X.
I don't think such accusations are helpful in any way.
Besides, I am not even sure what you mean by "own references". I mostly just said that I think for the enduser a specific error code could be confusing. And that another one seems more fitting IMO.
I don't think that there is a clear "right" or "wrong", which is probably why one could argue about this endlessly. But I don't think there is much value in that discussion, as both "perspectives" are somewhat valid, and in the end its IMO a minor detail of this feature.