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 open_memory routine #15

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

Conversation

bigcrush
Copy link

Routine lets create zip archive in memory. Then we can send zip over network for example without saving it to disk.

Copy link
Contributor

@MisterDA MisterDA left a comment

Choose a reason for hiding this comment

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

Consider using NULL instead of 0. This is more consistent with the code.

lua_zip.c Outdated

if (!errmsg && path) {
FILE *fp = fopen(path, "rb");
if (!fp) errmsg = strerror(errno); //FIXME: strerror() is not thread-safe
Copy link
Contributor

Choose a reason for hiding this comment

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

It should be fine to use strerror_r with a buffer of len 1024.

lua_zip.c Outdated
fseek(fp, 0, SEEK_END);
buf_sz = ftell(fp);
fseek(fp, 0, SEEK_SET);
//char *data = (char*)malloc(buf_sz);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't leave commented code.

@bigcrush
Copy link
Author

MisterDA, I followed your advice. Now I need create new PR?

@MisterDA
Copy link
Contributor

No, but you can rebase and force push if you want to, but that's not a big deal to me.
Thank you for contributing!
@brimworks what do you think?

@brimworks
Copy link
Owner

Sorry for the delay, things are pretty busy for me, so I probably won't be able to give this a great deal of attention until the weekend.

With that said, I did give a quick look at the code and I wonder if we could improve the API a bit. In particular, calling a method with a string that is either "file" or "string" argument seems a bit unexpected. Perhaps we could only have an API that takes the string argument, and avoid doing a file read? Users of the API should be able to use their own code to "slurp" the file into memory.

So, the new API would only support this:

local zip_arc = zip.open_memory([str [, flags]])

By removing support for a filename, it will make it easier to sandbox (in case a sandbox implementer wants to disable filesystem access and/or used in a non-blocking async context). It also slightly reduces the code we need to maintain.

Also, instead of reusing the ARCHIVE_MT, perhaps we could have a new meta-table, perhaps named ARCHIVE_MEMORY_MT. This ARCHIVE_MEMORY_MT could have a single user data that encapsulates both the struct zip* and zip_source_t* pointers. As long as the struct zip* comes first, then all existing ARCHIVE_MT methods should work, but then we could override the close() method with a memory specific method that returns a string for the content of the zip file.

If you are open to this idea, I should be able to get some time over the weekend to flesh this idea out a bit more. Or, if you want to take a stab, that would be great!

Thanks!
-Brian

@bigcrush
Copy link
Author

bigcrush commented Oct 17, 2019

I like the idea about new metatable. While feeling some inconvenience with "__source_memory" your idea is nicer to me.

Open_memory prototype was made just like that to follow a pattern with :add, :replace methods which accept ...zip_source. But now it seems to me it makes no sense as we work fully in memory

Thanks!

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