Improve the fix for #350 included in #1119 #1521
Conversation
1) We have to use H5Tequal() to compare HDF5 type IDs. 2) When checking if we can re-use an NC_CHAR attribute it is enough to compare data types (H5Tequal() takes care of the size comparison). 3) This commit adds missing code (reuse_att was set but not used). Now an attribute in a NetCDF-4 file can be modified as many times as necessary, as long its type and length remain the same. Modifications changing either type or length of an attribute require deleting and re-creating an attribute which increments the attribute order creation index. Once this index reaches 65535 all attribute modifications (for a particular group or variable) will fail. For reference: Issue 350 title: NetCDF-4 limits the number of times an attribute can be modified Pull request 1119 title: Fix checking for HDF5 max dims, no longer re-create atts if not needed, confirm behavior for HDF5 cyclical files, allow user to set mpiexec
|
|
|
Unfortunately #350 was not fixed by #1119: the issue is present in the current |
libhdf5/nc4hdf.c
Outdated
|
|
||
| /* Delete the attribute. */ | ||
| if (file_typeid != existing_att_typeid || npoints != att->len) | ||
| if (!H5Tequal(file_typeid, existing_att_typeid) || npoints != att->len) |
There was a problem hiding this comment.
I should modify this condition instead of setting npoints to att->len above. This has been a long day...
There was a problem hiding this comment.
OK, it's a new day! ;-) Make the modification if you think it should be made.
There was a problem hiding this comment.
Done. I think this version makes it easier to see what's going on.
|
OK, glad you took care of this, but we also must add the test back in. Can you insert this at the bottom of nc_test4/tst_atts.c and make sure it works: |
|
And since you have separate code for handling text atts, we need a text att test too. Thanks for this fix! |
This should make the code a bit cleaner.
This commit adds three new tests: - a test documenting the limitation originally reported in Unidata#350 (in general modifying an attribute about 2^16 times makes it impossible to modify this file in ways requiring nc_redef() and nc_enddef() calls). - a test ensuring that a scalar attribute can be modified 2^16 times as long as its type and size remain the same - a test ensuring that a text attribute can be modified 2^16 times as long as its size remains the same This version uses the nc_redef(), nc_put_att_...(), nc_enddef() sequence. One could also use nc_open(), nc_put_att_...(), nc_close() but that would make these tests significantly slower.
| @@ -424,5 +424,99 @@ main(int argc, char **argv) | |||
| free(data_in); | |||
| } | |||
| SUMMARIZE_ERR; | |||
| printf("*** testing modification of an attribute 2^16 times (changing size)..."); | |||
|
Thanks @ckhroulev , a very complete and well-crafted fix! Your contribution is very helpful. ;-) |
|
Thanks very much! |
H5Tequal()to compare HDF5 type IDs.NC_CHARattribute it is enough to compare data types (H5Tequal()takes care of the size comparison).reuse_attwas set but not used).Now an attribute in a NetCDF-4 file can be modified as many times as necessary, as long its type and length remain the same.
Modifications changing either type or length of an attribute require deleting and re-creating an attribute which increments the attribute order creation index. Once this index reaches 65535 all attribute modifications (for a particular group or variable) will fail.