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

Encoding SSH filexfer #430

Merged
merged 44 commits into from
Apr 20, 2021
Merged

Encoding SSH filexfer #430

merged 44 commits into from
Apr 20, 2021

Conversation

puellanivis
Copy link
Collaborator

@puellanivis puellanivis commented Apr 16, 2021

Let’s try this as round two.

Goals of this sub-package:

  • provide a common and safe wire encoding/decoding abstraction
  • provide a common packet-level abstraction (packets are the most natural abstraction layer to surface here)
  • the wire encoding/decoding abstraction should provide a easy and naïve use-case API (i.e. optimizing reallocations should not be necessary to even use the package)
  • packet abstractions should be cleanly separated from the wire encoding/decoding
  • filexfer-02 should be the default implementation (but not the only possible implementation)
  • the openssh extensions that we currently use should be implemented, in part as an example of how to write extensions
  • optimizing reallocations should be possible, but as noted above: way more importantly, it should not be required
  • when possible, input reusable buffers should be simple go types (i.e. []byte)

Non-goals:

  • thoroughly optimizing the filexfer-02 use case to the detriment of flexibility. (The reason for this subpackage is to build flexibility, not limit it.)
  • optimizations of avoiding reallocations should not be handled in package, but should be the responsibility of the caller.
  • avoiding nuances and gotchas of any particular type or strategy of reallocations are not this subpackage’s responsibility (e.g. []bytes in sync.Pools; the mechanics of why these issues exist could be fixed or resolved at any particular time. We do not want to be chasing the bleeding edge here.)

@drakkan
Copy link
Collaborator

drakkan commented Apr 20, 2021

Hi,

looks good to me, I propose to merge it without promising backward compatibility for now, so when we start using this package we will be free to make any changes we need. What do you think about it?

@puellanivis
Copy link
Collaborator Author

Seems reasonable, “this is a test run, and we might need to make changes as we convert our main package to use it” sort of thing.

We could maybe stuff it into an internal/ subdirectory until we’re ready to promise backwards compatibility?

@drakkan
Copy link
Collaborator

drakkan commented Apr 20, 2021

Seems reasonable, “this is a test run, and we might need to make changes as we convert our main package to use it” sort of thing.

We could maybe stuff it into an internal/ subdirectory until we’re ready to promise backwards compatibility?

internal is a good idea

@drakkan drakkan merged commit 197384b into master Apr 20, 2021
@drakkan
Copy link
Collaborator

drakkan commented Apr 20, 2021

Thank you

@puellanivis
Copy link
Collaborator Author

Now begins the fun part… 🙈 rewriting nearly everything.

@drakkan
Copy link
Collaborator

drakkan commented Apr 20, 2021

Maybe we should create a new branch for bugfixes, for example we could release 1.13.1 once #424 and #426 are fixed

@puellanivis
Copy link
Collaborator Author

Sounds good. Although, I actually started this PR based on a bunch of work I had essentially already done rewriting a lot of the client stuff.

Though, I’m still a bit at a loss for how to rresolve #424 . The PRs that I opened are way more defensive than anything else, and the changes I already started on would require both to be reworked anyways… but also since it seems that neither of the PRs actually solve the issue I’m absolutely stumped about where to go from there… and facing a big rewrite as well, I’m not sure anything we do now to fix the issue would matter much post-rewrite. 🤔

But if we have a lead on how to fix #424 before a rewrite, I’m all ears, but it’s just baffling honestly.

@puellanivis puellanivis deleted the refactor/encoding-ssh-filexfer branch April 20, 2021 12:11
@drakkan
Copy link
Collaborator

drakkan commented Apr 20, 2021

I tried to replicate # 424 several times with no luck. I have tried several things but I am unable to reproduce, it must be something really weird. If it gets fixed after rewriting, this will be fine too. We can create a new branch at any time

@drakkan drakkan mentioned this pull request Jun 12, 2021
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.

2 participants