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

Fixes for memory allocation and freeing #21

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

Conversation

MarkRivers
Copy link

This PR replaces calls to malloc() and free() with H5allocate_memory() and H5free_memory(). This is the correct mechanism according to HDF5 Group.

It also simplifies blosc_plugin.c, removes blosc_plugin.h, and does some other minor improvements.

@tacaswell
Copy link
Contributor

The linux travis failures look like real build failures:

/home/travis/build/Blosc/hdf5-blosc/src/blosc_filter.c:209:45: error: ‘false’ undeclared (first use in this function)
     outbuf = H5allocate_memory(outbuf_size, false);

@MarkRivers
Copy link
Author

The builds were failing because "false" was not defined. It built OK on my local system, but not on travis. I changed "false" to 0, and then the first 2 Travis jobs completed OK. The next 2 jobs still fail, but simply because apt-get is not defined:

0.04s$ sudo apt-get install libhdf5-serial-dev
sudo: apt-get: command not found
The command "sudo apt-get install libhdf5-serial-dev" failed and exited with 1 during .
Your build has been stopped.

I don't know what those 2 jobs are supposed to do.

Copy link
Contributor

@tacaswell tacaswell left a comment

Choose a reason for hiding this comment

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

This seems reasonable and correct to me, however I have not done c++ regularly in a while.

It looks like the mac and windows CI has never passed.

@@ -228,8 +227,6 @@ size_t blosc_filter(unsigned flags, size_t cd_nelmts,
/* declare dummy variables */
size_t cbytes, blocksize;

free(outbuf);
Copy link
Contributor

Choose a reason for hiding this comment

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

Just verifying that when we get here outbuf will always be NULL so free was a no-op?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, it appears not to be needed.

@MarkRivers
Copy link
Author

Please don't merge this yet. The HDF5 Group developers are in internal discussions about how filter plugins should be doing memory allocation. I was told that using H5allocate_memory() and H5free_memory() is the correct way to do it, but now it seems that they are not sure about this.

Copy link
Contributor

@tacaswell tacaswell left a comment

Choose a reason for hiding this comment

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

Changing me review to request-changes due to @MarkRivers 's comment about HDF5 group's internal discussions.

@FrancescAlted
Copy link
Member

Ping. I plan to do a release soon, and I am curious if this is still valid or not.

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