Skip to content
This repository has been archived by the owner on Sep 1, 2022. It is now read-only.

Remove or improve I/O ByteStream? #811

Closed
anonimal opened this issue Feb 16, 2018 · 5 comments
Closed

Remove or improve I/O ByteStream? #811

anonimal opened this issue Feb 16, 2018 · 5 comments
Labels

Comments

@anonimal
Copy link
Collaborator


By submitting this issue, I confirm the following:

  • I have read and understood the contributor guide in kovri-docs.
  • I have checked that the issue I am reporting can be replicated or that the feature I am suggesting is not present.
  • I have checked opened or recently closed pull requests for existing solutions/implementations to my issue/suggestion.

From #810

In all, I'm becoming very irritated with the bytestream implementation / concept as a whole. While #140 has had the best of intentions, the bytestream is building on - and revolving around - previous spaghetti-code pointer concepts rather then a usable, modular, OOP design. The current bytestream design also prevents us from managing memory in a more guaranteed and interoperable way.

  1. We should wrap existing libraries rather then trying to re-invent the wheel
  2. Instead of enabling bad designs by "streaming", we should allocate contiguous memory when it's needed - as this is a majority of ByteStream use-cases! If we really need need sinks, then use a real streaming library that guarantees memory safety!
@coneiric
Copy link
Contributor

  1. Instead of enabling bad designs by "streaming", we should allocate contiguous memory when it's needed - as this is a majority of ByteStream use-cases! If we really need need sinks, then use a real streaming library that guarantees memory safety!

Cryptopp's pipeline looks really solid. Replacing stream instances with pipelines might also help with #784, no?

@anonimal
Copy link
Collaborator Author

Cryptopp's pipeline looks really solid.

Yes, I'm well aware of crypto++'s sources and sinks as I've implemented them elsewhere. The standard library also has streaming but, in both instances, the issue is overhead - especially when you start treating binary data as strings. That is something I want to avoid because our frequent use-case. Also, the point of this issue was to illustrate that we don't need streaming most of the time anyway if kovri is re-designed properly.

Replacing stream instances with pipelines might also help with #784, no?

No. SecBlock is for arrays of memory and SecureWipe is a free function. They don't require cryptopp sources or sinks.

@coneiric
Copy link
Contributor

Also, the point of this issue was to illustrate that we don't need streaming most of the time anyway if kovri is re-designed properly.

I misunderstood. Is there a tracking issue for the overall redesign?

SecBlock is for arrays of memory and SecureWipe is a free function.

I saw in the docs that SecBlock underlies some of crypto++'s sources and sinks, which made me think pipelining might be a good idea. Now that you pointed out the overhead issues, it makes sense why pipelining isn't needed.

Would using SecBlock and SecureWipe be part of the solution here, or is this issue independent from those use-cases?

@anonimal
Copy link
Collaborator Author

Is there a tracking issue for the overall redesign?

No, not yet, because many of the TODOs and open issues point out the various design flaws and can be addressed on a per-component basis. A single catch-all issue would basically rewrite the entire codebase from scratch. I've implied this intention countless times over IRC since we forked but not opened a catch-all issue. I have yet to even open all per-component issues nor mark them all with TODOs because the issues are too obvious.

Would using SecBlock and SecureWipe be part of the solution here, or is this issue independent from those use-cases?

Nope, unrelated.

@anonimal
Copy link
Collaborator Author

anonimal commented Sep 7, 2018

NOTICE: THIS ISSUE HAS BEEN MOVED TO GitLab. Please continue the discussion there. See #1013 for details.

@anonimal anonimal closed this as completed Sep 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

2 participants