Add io operation callbacks

[bwrsandman]

Implementation of #111

Add unshield_open2 and unshield_open2_force_version that allows overriding
fopen, fseek, ftell, fread, fwrite, fclose, opendir, closedir,
readdir which lets the user pipe data from code without doing a round
trip to the hard drive.

[bwrsandman]

Example use in https://github.com/bwrsandman/unshield/blob/io_callbacks_example/src/isocab.c#L80-L366 for reference.

[twogood]

Awesome, looks really great at first glance! Need to review it properly of course.

[bwrsandman]

In my tests I never had to implement the callback to fwrite. I was only testing listing files.
I'm not too sure if that callback is needed as I think the only write is to a temp file.

Also, I wasn't too sure about desired code style so I tired to match it, but let me know if some lines are too long or there's a better form for struct/global names.

[twogood]

Sorry @bwrsandman for the lack of feedback. It's just that this change is not much of a priority for myself :)

[bwrsandman]

I'm still open for feedback when you have time.

[bwrsandman]

@twogood any chance you can have a look at this?

[twogood]

@twogood any chance you can have a look at this?

Thanks for your patience and kind reminder. I'd love to spend more time with unshield but it always comes too far down my list of priorities. I'm running a company and must prioritize paid work and sales activities there.

[bwrsandman]

Rebased and fixed the WIN32 compile error which was de to #define fseek and #define ftell

[bwrsandman]

Not too sure why linking with inflate fails on appveys. I'm tempted to say it's unrelated.

[twogood]

appveyor is broken, also due to same constraints on my time

[bwrsandman]

@twogood can you approve the workflows.

[twogood]

Well, well, well... hard to argue with those test results...

[bwrsandman]

I've rebased to trigger the new tests.
Any chance this can be reviewed and merged?

[kratz00]

I started reviewing this PR and have some remarks:

1. Please rebase with main branch
2. Fix the compilation errors, e.g.:

diff --git a/lib/libunshield.c b/lib/libunshield.c
index 58c56d2..994a3ea 100644
--- a/lib/libunshield.c
+++ b/lib/libunshield.c
@@ -310,7 +310,7 @@ static bool unshield_read_headers(Unshield* unshield, int version)/*{{{*/
       if (header->size < 4)
       {
         unshield_error("Header file %i too small", i);
-        FCLOSE(file);
+        FCLOSE(unshield, file);
         goto error;
       }
 
@@ -318,7 +318,7 @@ static bool unshield_read_headers(Unshield* unshield, int version)/*{{{*/
       if (!header->data)
       {
         unshield_error("Failed to allocate memory for header file %i", i);
-        FCLOSE(file);
+        FCLOSE(unshield, file);
         goto error;
       }

3. Fix the compiler warnings Unused parameter 'userdata' in libunshield.c, e.g.:

diff --git a/lib/libunshield.c b/lib/libunshield.c
index 58c56d2..9a3a98d 100644
--- a/lib/libunshield.c
+++ b/lib/libunshield.c
@@ -19,46 +19,55 @@
 
 void *unshield_default_fopen(const char *filename, const char *modes, void *userdata)
 {
+  (void)userdata;
   return fopen(filename, modes);
 }
 
 int unshield_default_fseek(void *file, long int offset, int whence, void *userdata)
 {
+  (void)userdata;
   return unshield_native_fseek(file, offset, whence);
 }
 
 long int unshield_default_ftell(void *file, void *userdata)
 {
+  (void)userdata;
   return unshield_native_ftell(file);
 }
 
 size_t unshield_default_fread(void *ptr, size_t size, size_t n, void *file, void *userdata)
 {
+  (void)userdata;
   return fread(ptr, size, n, file);
 }
 
 size_t unshield_default_fwrite(const void *ptr, size_t size, size_t n, void *file, void *userdata)
 {
+  (void)userdata;
   return fwrite(ptr, size, n, file);
 }
 
 int unshield_default_fclose(void *ptr, void *userdata)
 {
+  (void)userdata;
   return fclose(ptr);
 }
 
 void *unshield_default_opendir(const char *name, void *userdata)
 {
+  (void)userdata;
   return opendir(name);
 }
 
 int unshield_default_closedir(void *dir, void *userdata)
 {
+  (void)userdata;
   return closedir(dir);
 }
 
 struct dirent* unshield_default_readdir(void *dir, void *userdata)
 {
+  (void)userdata;
   return readdir(dir);
 }

Thanks in advance @bwrsandman

[bwrsandman]

@kratz00 Thanks for looking into this. I have rebased and fixed the warnings+errors.

[twogood]

I guess I can merge it with the tests passing and everything. I think what holds me back is that a "simple" function call like this:

fread(&tmp, 1, COMMON_HEADER_SIZE, reader->volume_file)

Turns into this "monster":

reader->unshield->io_callbacks->fread(&tmp, 1, COMMON_HEADER_SIZE, reader->volume_file, reader->unshield->io_userdata)

What if it looked something like this instead:

unshield_fread(reader->unshield, &tmp, 1, COMMON_HEADER_SIZE, reader->volume_file)

Implemented as:

size_t unshield_fread(Unshield* unshield, void *ptr, size_t size, size_t n, void *file)
{
  return unshield->io_callbacks->fread(ptr, size, n, file, unshield->io_userdata);
}

And so on...

Note: dry-coding the above!

[kratz00]

Thanks @bwrsandman looks good now.

@twogood
Your example could be simplified even more, e.g.:
`

unshield_fread(reader, &tmp, 1, COMMON_HEADER_SIZE)

size_t unshield_fread(UnshieldReader* reader, void *ptr, size_t size, size_t n, void *file)
{
  return reader->unshield->io_callbacks->fread(ptr, size, n, reader->volume_file, reader->unshield->io_userdata);
}

It might make sense to make all those wrapper functions - inline function for performance reasons.
What do you think?

[twogood]

Yes possible when we have an UnshieldReader struct, but for example fread in unshield_read_headers only has Unshield struct so I thought primarily on the more generic case but my example code had UnshieldReader as you observed!

[bwrsandman]

I decided to go with the full UnshieldReader version because it removes the passing of userdata and file in the most call sites.
Changed my mind...

[bwrsandman]

I went with using the Unshield struct which allows simpler calls for each of the UnshieldIoCallbacks functions.
The only downside of not having the volume_file is offset by the number of replacements possible.

[bwrsandman]

• Before you merge, I would like to add some stack memory file callback tests for read and write.

[twogood]

Looks so much better, thanks!

[bwrsandman]

I have added a validation test which extracts a file with the contents "TEST" all in stack memory. I'm open to feedback to improve it. Perhaps you can help me make an actual valid test.cab, test1.hdr, test1.cab...

I found a few missed callback calls mainly when calling unshield_file_save.

[bwrsandman]

Switched to using BUILD_TESTING and took out the prefixing changes. The prefix stuff can be done separately and is out of scope now.

One thing to note is that BUILD_TESTING is on by default per the docs.

[kratz00]

One thing to note is that BUILD_TESTING is on by default per the docs.

Yup, we can define what should be the default and add something like

option(BUILD_TESTING "Build libunshield tests for CTest" OFF)

I am good with the current state, thanks for your contribution @bwrsandman 👍

[twogood]

Thank you @bwrsandman and @kratz00 !