Closed bwrsandman closed 10 months ago
Example use in https://github.com/bwrsandman/unshield/blob/io_callbacks_example/src/isocab.c#L80-L366 for reference.
Awesome, looks really great at first glance! Need to review it properly of course.
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.
Sorry @bwrsandman for the lack of feedback. It's just that this change is not much of a priority for myself :)
I'm still open for feedback when you have time.
@twogood any chance you can have a look at this?
@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.
Rebased and fixed the WIN32 compile error which was de to #define fseek
and #define ftell
Not too sure why linking with inflate
fails on appveys. I'm tempted to say it's unrelated.
appveyor is broken, also due to same constraints on my time
@twogood can you approve the workflows.
Well, well, well... hard to argue with those test results...
I've rebased to trigger the new tests. Any chance this can be reviewed and merged?
I started reviewing this PR and have some remarks:
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(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) {
Thanks in advance @bwrsandman
@kratz00 Thanks for looking into this. I have rebased and fixed the warnings+errors.
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!
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?
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!
I decided to go with the full
Changed my mind...UnshieldReader
version because it removes the passing of userdata
and file
in the most call sites.
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.
Looks so much better, thanks!
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
.
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.
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 :+1:
Thank you @bwrsandman and @kratz00 !
Implementation of #111
Add
unshield_open2
andunshield_open2_force_version
that allows overridingfopen
,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.