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.

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

add support for discarded longname field #496

Closed
wants to merge 2 commits into from
Closed

Conversation

kexirong
Copy link

@kexirong
Copy link
Author

The longname field is equally important to the client @puellanivis #452

Copy link
Collaborator

@puellanivis puellanivis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’ve left some notes, but know that this PR is highly unlikely to ever reach a point where a merge is appropriate.

The format of the `longname' field is unspecified by this protocol.
It MUST be suitable for use in the output of a directory listing
command (in fact, the recommended operation for a directory listing
command is to simply display this data). However, clients SHOULD NOT
attempt to parse the longname field for file attributes; they SHOULD
use the attrs field instead.

All information contained in the longname field can be (or will be able to be) extracted from the FileInfo already, except the link count. As there is no reason for any API client to have cause to just dump the contents of this string out for user consumption, rather than just re-rendering the same info, there’s just no good reason to expose this to clients at the API level.

There’s a good reason why the very next version of the standard removed it. It is not actually all that useful let alone important.

@@ -19,15 +19,23 @@ const (
sshFileXferAttrACmodTime | sshFileXferAttrExtended
)

func GetLongName(fi os.FileInfo) string {
return fi.(*fileInfo).LongName()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will panic if passed a different type of os.FileInfo

// longname contains useful information and should not be discarded
// mode hardlink user group m_time name
// -rwxr-xr-x 1 mjos staff 348911 Mar 25 14:29 t-filexfer
longname, data = unmarshalString(data) // discard longname
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code no longer “discards longname”

filename, data = unmarshalString(data)
_, data = unmarshalString(data) // discard longname
// longname contains useful information and should not be discarded
// mode hardlink user group m_time name
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The second field is not hardlink it is linkcount there is a subtle difference.

@kexirong
Copy link
Author

kexirong commented Feb 25, 2022

Thank you for correcting my code irregularities.
I didn't parse the longname, add it to the Extended field, just no longer discard and implement the get function.
There is no other way to get the linkcount field, it is required for interaction, such as cli or gui.
My native language is not English, but Chinese.

@puellanivis
Copy link
Collaborator

While this is indeed the only way to get the linkcount field, but you are incorrect. It is not required for interactive interfaces like CLI or GUI. Linkcount is rarely useful, and as noted, the very next version of the standard removes access to it entirely. And the standard specifically says that no client should be parsing information from this field anyways.

And as noted, the longname format is not actually standardized. Implementations are technically free to format their longname text differently, and remove the linkcount field entirely. The bug you pointed to was a compatibility bug, caused by a client that was incorrectly parsing the longname. That client never should have been doing that in the first place, but there we were.

As a participant in the wider world of SFTP program compatibility, I do not see any value in exposing this opaque text, and giving any amount of encouragement to people to use it in any way.

@kexirong
Copy link
Author

openssh is the most widely used ssh. It doesn't appear to be planning to upgrade the protocol to the next version. And ssh is mainly used by Unix-like systems. Then in my eyes openssh is orthodox, and other protocol versions for compatibility with windows are not business, and other file transfer protocols should be used instead of sftp, so I think as long as the text is in a fixed format, it can be parsed. These are my thoughts on the value of the longname field. And you don't need to use dard link count, so it makes no sense to think it
I have no intention of changing your mind. I plan to copy this library in my spare time and implement a client library for sftp compatible with openssh.
The above is my translation from Chinese to English using Google.

@kexirong kexirong closed this Feb 26, 2022
@puellanivis
Copy link
Collaborator

This library is already compatible with openssh. So, I am confused by why you’re making this argument. If openssh is showing this field, it is because it is dumping the raw opaque text for ls, openssh is definitely not parsing this field, and is definitively not extracting the linkcount out of the text.

Right now, this library only targets the -02 version, but it would be poor form to shoot ourselves in the foot and cut off all ability to be interoperable. Note, that when we were not generating this link text properly, we only had one report of an SFTP client that was misbehaving. This demonstrates a fairly widespread consensus that this field is wholly unreliable.

And ignoring all other clients and OSes is unrealistic, and improper. We have to be interoperable, because people expect us to be interoperable. This is not openssh-sftp it is sftp.

@drakkan
Copy link
Collaborator

drakkan commented Feb 26, 2022

as already noted the specs say:

However, clients SHOULD NOT
attempt to parse the longname field for file attributes; they SHOULD
use the attrs field instead

so you are doing something not recommended in specs and we don't want to do it here.

Of course you are free to maintain a custom fork with your patch, good luck with your project!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants