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.

Sign up for GitHub

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

Improve the fix for #350 included in #1119 #1521

Merged
merged 4 commits into from
Nov 15, 2019
Merged

Improve the fix for #350 included in #1119 #1521

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
dd181de
Improve the fix for #350 included in #1119
ckhroulev Nov 12, 2019
098f2c1
Modify the condition used to check if an attribute can be re-used
ckhroulev Nov 13, 2019
6abbf8d
Whitespace changes
ckhroulev Nov 13, 2019
bb45db4
Add regression tests modifying attributes 2^16 times
ckhroulev Nov 13, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 25 additions & 30 deletions libhdf5/nc4hdf.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -611,58 +611,53 @@ put_att_grpa(NC_GRP_INFO_T *grp, int varid, NC_ATT_INFO_T *att)
BAIL(NC_EATTMETA);

/* How big is the attribute? */
if (att->nc_typeid == NC_CHAR) {
/* For text attributes the size is specified in the datatype
and it is enough to compare types using H5Tequal(). Here
we set npoints to disable the second part of the
comparison below. */
npoints = att->len;
}
else
{
if ((existing_spaceid = H5Aget_space(existing_attid)) < 0)
if ((existing_spaceid = H5Aget_space(existing_attid)) < 0)
BAIL(NC_EATTMETA);
if ((npoints = H5Sget_simple_extent_npoints(existing_spaceid)) < 0)
if ((npoints = H5Sget_simple_extent_npoints(existing_spaceid)) < 0)
BAIL(NC_EATTMETA);
}

if (!H5Tequal(file_typeid, existing_att_typeid) || npoints != att->len)
/* For text attributes the size is specified in the datatype
and it is enough to compare types using H5Tequal(). */
if (!H5Tequal(file_typeid, existing_att_typeid) ||
(att->nc_typeid != NC_CHAR && npoints != att->len))
{
/* The attribute exists but we cannot re-use it. */
/* The attribute exists but we cannot re-use it. */

/* Delete the attribute. */
/* Delete the attribute. */
if (H5Adelete(locid, att->hdr.name) < 0)
BAIL(NC_EHDFERR);

/* Re-create the attribute with the type and length
reflecting the new value (or values). */
if ((attid = H5Acreate(locid, att->hdr.name, file_typeid, spaceid,
H5P_DEFAULT)) < 0)
BAIL(NC_EATTMETA);
BAIL(NC_EATTMETA);

/* Write the values, (even if length is zero). */
if (H5Awrite(attid, file_typeid, data) < 0)
BAIL(NC_EATTMETA);
BAIL(NC_EATTMETA);
}
else
{
/* The attribute exists and we can re-use it. */
/* The attribute exists and we can re-use it. */

/* Write the values, re-using the existing attribute. */
if (H5Awrite(existing_attid, file_typeid, data) < 0)
BAIL(NC_EATTMETA);
/* Write the values, re-using the existing attribute. */
if (H5Awrite(existing_attid, file_typeid, data) < 0)
BAIL(NC_EATTMETA);
}
} else {
/* The attribute does not exist yet. */
}
else
{
/* The attribute does not exist yet. */

/* Create the attribute. */
if ((attid = H5Acreate(locid, att->hdr.name, file_typeid, spaceid,
H5P_DEFAULT)) < 0)
BAIL(NC_EATTMETA);
/* Create the attribute. */
if ((attid = H5Acreate(locid, att->hdr.name, file_typeid, spaceid,
H5P_DEFAULT)) < 0)
BAIL(NC_EATTMETA);

/* Write the values, (even if length is zero). */
if (H5Awrite(attid, file_typeid, data) < 0)
BAIL(NC_EATTMETA);
/* Write the values, (even if length is zero). */
if (H5Awrite(attid, file_typeid, data) < 0)
BAIL(NC_EATTMETA);
}

exit:
Expand Down
94 changes: 94 additions & 0 deletions nc_test4/tst_atts.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)...");
Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome tests, thanks!

{
/* This test documents that if the size of an attribute changes
* from one modification to the next we have to delete and
* re-create it, incrementing HDF5's attribute creation order
* index. In this case after about 2^16 modifications this
* index reaches its maximum, making it impossible to modify
* this file's metadata because nc_enddef() will fail. */
int ncid = -1, i = -1, error_code = NC_NOERR;
int data[2] = {1, 2};

/* Create a file. Note that we use a different file name here
* because we will not be able to close this file. */
if (nc_create("tst_att_modification.nc", NC_NETCDF4|NC_CLOBBER, &ncid)) ERR;
if (nc_enddef(ncid)) ERR;

/* Modify an attribute 2^16 times. */
for (i = 0; i < 65536; ++i)
{
if (nc_redef(ncid)) ERR;

/* Note that the attribute size changes every time. */
if (nc_put_att_int(ncid, NC_GLOBAL, "attribute", NC_INT,
1 + i % 2, data)) ERR;

/* nc_enddef() synchronizes attributes. */
error_code = nc_enddef(ncid);

/* After a number of iterations nc_enddef() will fail with
* NC_EATTMETA. */
if (error_code == NC_EATTMETA)
break;

/* Catch other errors. */
if (error_code != NC_NOERR) ERR;
}

/* nc_close() will fail too: just like nc_enddef() it tries to
* write dirty attributes to disc. */
if (nc_close(ncid) != NC_EATTMETA) ERR;

/* In this test reaching the end of the loop without encountering
* NC_EATTMETA is a failure. */
if (error_code != NC_EATTMETA) ERR;
}
SUMMARIZE_ERR;
printf("*** testing modification of a scalar attribute 2^16 times (same type and size)...");
{
/* This test ensures that a scalar attribute can be modified
* 2^16 times as long as its type and size remain the same. */
int ncid = -1, i = -1;

/* Create a file. */
if (nc_create(FILE_NAME, NC_NETCDF4|NC_CLOBBER, &ncid)) ERR;
if (nc_enddef(ncid)) ERR;

/* Modify an attribute 2^16 times. */
for (i = 0; i < 65536; ++i)
{
if (nc_redef(ncid)) ERR;
/* All built-in attribute types except for NC_CHAR (but
* including NC_STRING) use the same logic so it is enough
* to test nc_put_att_int(). */
if (nc_put_att_int(ncid, NC_GLOBAL, "attribute", NC_INT, 1, &i)) ERR;
/* nc_enddef() synchronizes attributes. */
if (nc_enddef(ncid)) ERR;
}

if (nc_close(ncid)) ERR;
}
SUMMARIZE_ERR;
printf("*** testing modification of a text attribute 2^16 times (same size)...");
{
/* This test ensures that a text attribute can be modified
* 2^16 times as long as its size remains the same. */
int ncid = -1, i = -1;
const char *string = "test string"; /* 11 characters */

/* Create a file. */
if (nc_create(FILE_NAME, NC_NETCDF4|NC_CLOBBER, &ncid)) ERR;
if (nc_enddef(ncid)) ERR;

/* Modify an attribute 2^16 times. */
for (i = 0; i < 65536; ++i)
{
if (nc_redef(ncid)) ERR;
if (nc_put_att_text(ncid, NC_GLOBAL, "attribute", 11, string)) ERR;
/* nc_enddef() synchronizes attributes */
if (nc_enddef(ncid)) ERR;
}

if (nc_close(ncid)) ERR;
}
SUMMARIZE_ERR;
FINAL_RESULTS;
}