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

Zlib: add stream-oriented operations #212

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

f00b4r0
Copy link
Contributor

@f00b4r0 f00b4r0 commented Jul 17, 2024

This PR introduces stream-oriented primitives for dealing with zlib data.

The code is split into functional commits for easier review.

Memory pressure is directly dependent on how often the stream output buffer is flushed through read() calls, so this implementation makes it possible to continuously compress data within a memory cap.

Notes:

  • I'm note very familiar with the documentation syntax, I hope I didn't mess up there and provide sufficient information.
  • I didn't feel necessary to include an explicit stream destructor since I assume that once the stream goes out of scope, the implicit destructor will be called freeing all leftover memory (the zstrm_t struct). That's why I chose to use *new() naming suffix for the stream constructor

Only happy testing has been performed (successfully) so far, comments welcome

HTH

@f00b4r0 f00b4r0 force-pushed the zlib branch 3 times, most recently from 1249ed8 to 5ba94d1 Compare July 18, 2024 20:50
@jow-
Copy link
Owner

jow- commented Jul 25, 2024

On a first glance this looks good. Not overly happy with the constructor naming, "infnew" and "defnew" reminds me on infinity and default, not inflate and deflate for some reason. What about calling the constructors inflater() and deflater() or inflate_stream() and deflate_stream() ?

  • Rename the internal resource type names to zlib.inflate and zlib.deflate for clarity
  • Is Z_FINISH really a useful default flush mode for the deflate write() method? Should we maybe default it to Z_NO_FLUSH?
  • Dito for inflate write()
  • The documentation is a bit broken, you should not put @see text inline into text (to test documentation rendering locally, run npm install; npm run doc in the source dir, then browse docs/module-zlib.html)
  • The call stream.write('', Z_FINISH) looks a bit messy, maybe it makes sense to implement a stream.finish() convenience method?

@f00b4r0
Copy link
Contributor Author

f00b4r0 commented Jul 25, 2024

On a first glance this looks good. Not overly happy with the constructor naming, "infnew" and "defnew" reminds me on infinity and default, not inflate and deflate for some reason. What about calling the constructors inflater() and deflater() or inflate_stream() and deflate_stream() ?

Ok, I'll go with the former to keep things short.

  • Rename the internal resource type names to zlib.inflate and zlib.deflate for clarity

Won't that be confusing with the same-name functions ?

  • Is Z_FINISH really a useful default flush mode for the deflate write() method? Should we maybe default it to Z_NO_FLUSH?

  • Dito for inflate write()

I suppose you have a point. My initial idea was to have the stream functions default to the same behavior as the single-call oriented ones, and force the user to select a flush mode, but it's probably not ideal now that I think about it. Will switch to Z_NO_FLUSH instead.

  • The documentation is a bit broken, you should not put @see text inline into text (to test documentation rendering locally, run npm install; npm run doc in the source dir, then browse docs/module-zlib.html)

Sorry no can do. I don't have npm on my system and no intention of installing it just for this use given how it's been a proven attack vector multiple times already. Sorry. I'll fix the @see.

  • The call stream.write('', Z_FINISH) looks a bit messy, maybe it makes sense to implement a stream.finish() convenience method?

Actually the example is clumsy (I was lazy there). In real life you would of course use Z_FINISH with the last sent chunk, not with an empty one. The latter works and completes the stream to a valid archive but adds an unnecessary extra block. I'll improve the doc.

@jow-
Copy link
Owner

jow- commented Jul 25, 2024

Won't that be confusing with the same-name functions ?

No, that string literal for the resource type is just used to uniquely identify the resource type within the global VM context, it does not share the namespace with variables. The name is also used when converting resource values to strings, e.g. for printing. A deflate stream will appear as something like <zlib.strmd 0xdeadbeef>. To make that name more descriptive, I'd suggest zlib.deflate or zlib.dstream or zlib.deflateStream. Compare with the resource labels in other modules:

  • fs.proc (FILE * returned by popen())
  • fs.file (FILE * returned by fopen() et al)
  • fs.dir (DIR *)
  • socket (int)
  • ubus.connection (struct ubus_context *)
  • ubus.deferred (struct ubus_request *)
  • ubus.object (struct ubus_object *)
  • ubus.notify (struct ubus_notify_request *)
  • ubus.request (struct ubus_request_data *)
  • uloop.timer
  • uloop.handle
  • etc.

@f00b4r0 f00b4r0 force-pushed the zlib branch 2 times, most recently from a694e92 to 20a1fed Compare July 27, 2024 09:36
Prepare for stream-oriented operations

Signed-off-by: Thibaut VARÈNE <[email protected]>
No functional change

Signed-off-by: Thibaut VARÈNE <[email protected]>
@f00b4r0
Copy link
Contributor Author

f00b4r0 commented Oct 9, 2024

@jow- friendly ping, is there anything else you wanted changed here? :)

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