Purpose: Fixed Packet Table issues

Description:
    - Removed calls to H5Tget_native_type from PT APIs because it is up to the
      application, whether it wants the buffer to be read into memory in the
      machine’s native architecture.  Currently, however, the PT doesn't
      allow an application to specify memory datatype.  Perhaps, a new API can
      be added to provide that capability.
    - Added calls to H5Tcopy to H5PTcreate/H5PTcreate_fl/H5PTopen to save a
      copy of the application's datatype or the dataset's datatype.
    - Added various missing H5Tclose to the packet table tests, and various
      error checkings.
    Note: leave out changes to test_packet_vlen.c for 1.8 to wait on QAK about
          merging the commit ec2fbe0883f9e76df60bcfbebbd4b6f62d5a09e6
          [svn-r30158] first.
Platforms tested:
    Linux/32 2.6 (jam)
    Linux/64 (platypus)
    Darwin (osx1010test)

4 files changed, 164 insertions, 91 deletions

diff --git a/hl/c++/test/ptableTest.cpp b/hl/c++/test/ptableTest.cpp
index dd3d2ba..68b040e 100644
--- a/hl/c++/test/ptableTest.cpp
+++ b/hl/c++/test/ptableTest.cpp
@@ -38,23 +38,22 @@ int main(void)
     }
     else {
 
-            num_errors += BasicTest();
+	num_errors += BasicTest();
 
-            num_errors += TestCompoundDatatype();
+	num_errors += TestCompoundDatatype();
 
-            num_errors += TestGetPacket();
+	num_errors += TestGetPacket();
 
-            num_errors += TestGetNext();
+	num_errors += TestGetNext();
 
-            num_errors += TestCompress();
+	num_errors += TestCompress();
 
-            num_errors += TestErrors();
+	num_errors += TestErrors();
 
-            num_errors += SystemTest();
+	num_errors += SystemTest();
 
-#ifdef VLPT_REMOVED
-            num_errors += VariableLengthTest();
-#endif /* VLPT_REMOVED */
+	/* Test data corruption in packed structs */
+	num_errors += TestHDFFV_9758();
 
         /* Terminate access to the file. */
         err = H5Fclose(fileID);
@@ -562,73 +561,131 @@ error:
     return 1;
 }
 
-#ifdef VLPT_REMOVED
-int VariableLengthTest(void)
+/*-------------------------------------------------------------------------
+ * TestHDFFV_9758(): Test that a packet table with compound datatype which
+ *	contains string type can be created and written correctly. (HDFFV-9758)
+ *
+ * Notes:
+ *	Previously, data of the field that follows the string was read back
+ *	as garbage when #pragma pack(1) is used.
+ * 2016/10/20 -BMR
+ *-------------------------------------------------------------------------
+ */
+#pragma pack(1)  // no padding
+const char* ABHI_PT("/abhiTest");
+const hsize_t NUM_PACKETS = 5;
+const int STRING_LENGTH = 19; // including terminating NULL
+int TestHDFFV_9758()
 {
-    long test_long;
-    short test_short;
-    hvl_t read_buf;
-    VL_PacketTable* test_VLPT;
-    PacketTable* new_pt;
-
-    TESTING("variable-length packet tables")
-
-    /* Create a variable length table */
-    test_VLPT = new VL_PacketTable(fileID, "/VariableLengthTest", 1);
-
-    /* Verify that the creation succeeded */
-    if(! test_VLPT->IsValid())
-      goto error;
-
-    /* Append some packets */
-    test_short = 9;
-    test_VLPT->AppendPacket(&test_short, sizeof(short));
-    test_long = 16;
-    test_VLPT->AppendPacket(&test_long, sizeof(long));
-
-    /* Read them back and make sure they are correct */
-    test_VLPT->GetNextPackets(1, &read_buf);
-
-    if(read_buf.len != sizeof(short))
-      goto error;
-    if(*(short *)(read_buf.p) != test_short)
-      goto error;
-
-    /* Free the memory used by the read */
-    test_VLPT->FreeReadbuff(1, &read_buf);
-
-    /* Read the second record */
-    test_VLPT->GetNextPackets(1, &read_buf);
-
-    if(read_buf.len != sizeof(long))
-      goto error;
-    if(*(long *)(read_buf.p) != test_long)
-      goto error;
-
-    /* Free the memory used by the read */
-    test_VLPT->FreeReadbuff(1, &read_buf);
-
-    /* Close the packet table */
-    delete test_VLPT;
+    hid_t strtype;
+    hid_t compound_type;
+    herr_t err;
+    struct s1_t
+    {
+        int a;
+        float b;
+        double c;
+        char d[STRING_LENGTH]; // null terminated string
+        int e;
+    };
 
-    /* Reopen the packet table and verify that it is variable length */
-    new_pt = new PacketTable(fileID, "/VariableLengthTest");
+    s1_t s1[NUM_PACKETS];
+    
+    for (hsize_t i = 0; i < NUM_PACKETS; i++)
+    {
+        s1[i].a = i;
+        s1[i].b = 1.f * static_cast<float>(i * i);
+        s1[i].c = 1. / (i + 1);
+        sprintf(s1[i].d, "string%d", (int)i);
+        s1[i].e = 100+i;
+    }
 
-    /* Verify that the open succeeded */
-    if(! new_pt->IsValid())
-      goto error;
+    TESTING("data corruption in packed structs (HDFFV-9758)")
+
+    // Build a compound datatype
+    compound_type = H5Tcreate(H5T_COMPOUND, sizeof(s1_t));
+    if (compound_type < 0)
+	goto error;
+    
+    err = H5Tinsert(compound_type, "a_name", HOFFSET(s1_t, a), H5T_NATIVE_INT);
+    if (err < 0)
+	goto error;
+    err = H5Tinsert(compound_type, "b_name", HOFFSET(s1_t, b), H5T_NATIVE_FLOAT);
+    if (err < 0)
+	goto error;
+    err = H5Tinsert(compound_type, "c_name", HOFFSET(s1_t, c), H5T_NATIVE_DOUBLE);
+    if (err < 0)
+	goto error;
+
+    strtype = H5Tcopy (H5T_C_S1);
+    if (compound_type < 0)
+	goto error;
+    err = H5Tset_size (strtype, STRING_LENGTH); /* create string */
+    if (err < 0)
+	goto error;
+    err = H5Tinsert(compound_type, "d_name", HOFFSET(s1_t, d), strtype);
+    if (err < 0)
+	goto error;
+    err = H5Tinsert(compound_type, "e_name", HOFFSET(s1_t, e), H5T_NATIVE_INT);
+    if (err < 0)
+	goto error;
+
+    { // so ptable will go out of scope before PASSED
+
+    // Create a packet table
+    FL_PacketTable ptable(fileID, "/examplePacketTable", compound_type, 1);
+    if (not ptable.IsValid())
+	goto error;
+
+    // Add packets to the table
+    for (size_t i = 0; i < NUM_PACKETS; i++)
+    {
+	/* Appends one packet at the current position */
+        err = ptable.AppendPacket(s1 + i);
+        if (err < 0) goto error;
+    }
 
-    if(new_pt->IsVariableLength() != 1)
-      goto error;
+    // Check packet count
+    const hsize_t count = ptable.GetPacketCount(err);
+    if (err < 0)
+	goto error;
+  
+    if (count != NUM_PACKETS)
+    {
+	std::cerr
+        << "Number of packets in packet table should be " << NUM_PACKETS
+        << " but is " << count << endl;
+    }
 
-    /* Close the packet table */
-    delete new_pt;
+    // Read and verify the data
+    ptable.ResetIndex();
+    for (size_t i = 0; i < NUM_PACKETS; i++)
+    {
+	s1_t s2;
+	memset(&s2, 0, sizeof(s1_t));
+	err = ptable.GetNextPacket(&s2);
+	if (err < 0)
+	    goto error;
+
+	if (s2.a != s1[i].a || s2.e != s1[i].e)
+	    goto error;
+	else if (HDstrcmp(s2.d, s1[i].d))
+	    goto error;
+    }
+    } // end of ptable block
 
     PASSED();
     return 0;
 
 error:
+
+    H5E_BEGIN_TRY {
+        H5Tclose(strtype);
+        H5Tclose(compound_type);
+        H5Fclose(fileID);
+    } H5E_END_TRY;
+
     H5_FAILED();
     return 1;
 }
-#endif /* VLPT_REMOVED */
+
diff --git a/hl/c++/test/ptableTest.h b/hl/c++/test/ptableTest.h
index d351e34..60b55ca 100644
--- a/hl/c++/test/ptableTest.h
+++ b/hl/c++/test/ptableTest.h
@@ -52,7 +52,8 @@ int TestGetPacket(void);
    Test for unusual interactions between multiple packet tables. */
 int SystemTest(void);
 
-/* Test the variable length dataset functionality */
-int VariableLengthTest(void);
+/* Create a packet table with compound type, which has a string type.  Verify
+   that data was written and read correctly. */
+int TestHDFFV_9758(void);
 
 #endif /* PTABLETEST */
diff --git a/hl/src/H5PT.c b/hl/src/H5PT.c
index 5376086..5f0f94f 100644
--- a/hl/src/H5PT.c
+++ b/hl/src/H5PT.c
@@ -141,7 +141,9 @@ hid_t H5PTcreate(hid_t loc_id,
   if(H5Pclose(plistcopy_id) < 0)
     goto error;
 
-  if((table->type_id = H5Tget_native_type(dtype_id, H5T_DIR_DEFAULT)) < 0)
+  /* Make a copy of caller's datatype and save it in the table structure.
+     It will be closed when the table is closed */
+   if((table->type_id = H5Tcopy(dtype_id)) < 0)
     goto error;
 
   H5PT_create_index(table);
@@ -259,7 +261,9 @@ hid_t H5PTcreate_fl ( hid_t loc_id,
   if(H5Pclose(plist_id) < 0)
     goto error;
 
-  if((table->type_id = H5Tget_native_type(dtype_id, H5T_DIR_DEFAULT)) < 0)
+  /* Make a copy of caller's datatype and save it in the table structure.
+     It will be closed when the table is closed */
+  if((table->type_id = H5Tcopy(dtype_id)) < 0)
     goto error;
 
   H5PT_create_index(table);
@@ -349,8 +353,9 @@ hid_t H5PTopen( hid_t loc_id,
   if((type_id = H5Dget_type(table->dset_id)) < 0)
     goto error;
 
-  /* Get the table's native datatype */
-  if((table->type_id = H5Tget_native_type(type_id, H5T_DIR_ASCEND)) < 0)
+  /* Make a copy of the datatype obtained and save it in the table structure.
+     It will be closed when the table is closed */
+   if((table->type_id = H5Tcopy(type_id)) < 0)
     goto error;
 
   /* Close the disk datatype */
diff --git a/hl/test/test_packet.c b/hl/test/test_packet.c
index 0bd5292..f577947 100644
--- a/hl/test/test_packet.c
+++ b/hl/test/test_packet.c
@@ -72,8 +72,8 @@ static int cmp_par(size_t i, size_t j, particle_t *rbuf, particle_t *wbuf )
  if ( ( HDstrcmp( rbuf[i].name, wbuf[j].name ) != 0 ) ||
   rbuf[i].lati != wbuf[j].lati ||
   rbuf[i].longi != wbuf[j].longi ||
-  !FLT_ABS_EQUAL(rbuf[i].pressure,wbuf[j].pressure) ||
-  !DBL_ABS_EQUAL(rbuf[i].temperature,wbuf[j].temperature) ) {
+  !H5_FLT_ABS_EQUAL(rbuf[i].pressure,wbuf[j].pressure) ||
+  !H5_DBL_ABS_EQUAL(rbuf[i].temperature,wbuf[j].temperature) ) {
   return FAIL;
  }
  return SUCCEED;
@@ -95,8 +95,10 @@ make_particle_type(void)
   return FAIL;
 
  /* Insert fields. */
- string_type = H5Tcopy( H5T_C_S1 );
- H5Tset_size( string_type, (size_t)16 );
+ if ((string_type = H5Tcopy(H5T_C_S1)) < 0)
+     return FAIL;
+ if (H5Tset_size(string_type, (size_t)16) < 0)
+     return FAIL;
 
  if ( H5Tinsert(type_id, "Name", HOFFSET(particle_t, name) , string_type ) < 0 )
      return FAIL;
@@ -133,8 +135,10 @@ static int create_hl_table(hid_t fid)
     herr_t     status;
 
     /* Initialize the field field_type */
-    string_type = H5Tcopy( H5T_C_S1 );
-    H5Tset_size( string_type, (size_t)16 );
+    if ((string_type = H5Tcopy(H5T_C_S1)) < 0)
+        return FAIL;
+    if (H5Tset_size(string_type, (size_t)16) < 0)
+        return FAIL;
     field_type[0] = string_type;
     field_type[1] = H5T_NATIVE_INT;
     field_type[2] = H5T_NATIVE_INT;
@@ -152,12 +156,14 @@ static int create_hl_table(hid_t fid)
                         field_names, part_offset, field_type,
                         chunk_size, fill_data, compress, testPart  );
 
-if(status<0)
-  return FAIL;
-else
-  return SUCCEED;
-}
+  if (H5Tclose(string_type) < 0)
+    return FAIL;
 
+  if(status<0)
+    return FAIL;
+  else
+    return SUCCEED;
+}
 
 
 /*-------------------------------------------------------------------------
@@ -183,7 +189,8 @@ static int test_create_close(hid_t fid)
 
     /* Create the table */
     table = H5PTcreate_fl(fid, PT_NAME, part_t, (hsize_t)100, -1);
-    H5Tclose(part_t);
+    if (H5Tclose(part_t) < 0)
+	goto error;
     if( H5PTis_valid(table) < 0)
 	goto error;
     if( H5PTis_varlen(table) != 0)
@@ -248,7 +255,7 @@ static int test_append(hid_t fid)
 {
     herr_t err;
     hid_t table;
-    hsize_t count;
+    hsize_t count = 0;
 
     TESTING("H5PTappend");
 
@@ -458,7 +465,8 @@ static int    test_big_table(hid_t fid)
 
     /* Create a new table */
     table = H5PTcreate_fl(fid, "Packet Test Dataset2", part_t, (hsize_t)33, -1);
-    H5Tclose(part_t);
+    if (H5Tclose(part_t) < 0)
+        goto error;
     if( H5PTis_valid(table) < 0)
         goto error;
 
@@ -536,7 +544,8 @@ static int    test_opaque(hid_t fid)
 
     /* Create a new table */
     table = H5PTcreate_fl(fid, "Packet Test Dataset3", part_t, (hsize_t)100, -1);
-    H5Tclose(part_t);
+    if( H5Tclose(part_t) < 0)
+        goto error;
     if( H5PTis_valid(table) < 0)
         goto error;
 
@@ -743,9 +752,9 @@ static int test_rw_nonnative_dt(hid_t fid)
  /* Create a fixed-length packet table within the file */
  /* This table's "packets" will be simple integers and it will use no compression */
  if(H5Tget_order(H5T_NATIVE_INT) == H5T_ORDER_LE) {
-   ptable = H5PTcreate_fl(fid, "Packet Test Dataset, Non-native", H5T_STD_I32BE, (hsize_t)100, -1);
+   ptable = H5PTcreate(fid, "Packet Test Dataset, Non-native", H5T_STD_I32BE, (hsize_t)100, H5P_DEFAULT);
  } else {
-   ptable = H5PTcreate_fl(fid, "Packet Test Dataset, Non-native", H5T_STD_I32LE, (hsize_t)100, -1);
+   ptable = H5PTcreate(fid, "Packet Test Dataset, Non-native", H5T_STD_I32LE, (hsize_t)100, H5P_DEFAULT);
  }
  if(ptable == H5I_INVALID_HID)
         goto error;
@@ -973,7 +982,8 @@ int main(void)
         status = 1;
 
     /* Close the file */
-    H5Fclose(fid);
+    if (H5Fclose(fid) < 0)
+	status = 1;
 
     return status;
 }