[svn-r18374] Purpose: Fix bugs involving external links

Description:
Previously, the library would reopen the source file when traversing an external
link if unable to find the target file otherwise.  This has been corrected.
Also moved the call to H5F_flush from H5F_try_close to H5F_dest, so the file is
only flushed when the last identifier for the file is closed.  This prevernts
situations where the library could attempt to flush a file with protected
metadata.

Tested: jam, amani, linew (h5committest); Fedora

3 files changed, 50 insertions, 28 deletions

diff --git a/src/H5F.c b/src/H5F.c
index 8e4a11e..d21b31c 100644
--- a/src/H5F.c
+++ b/src/H5F.c
@@ -71,7 +71,7 @@ static H5F_t *H5F_new(H5F_file_t *shared, hid_t fcpl_id, hid_t fapl_id,
                       H5FD_t *lf);
 static herr_t H5F_build_actual_name(const H5F_t *f, const H5P_genplist_t *fapl,
     const char *name, char ** /*out*/ actual_name);
-static herr_t H5F_dest(H5F_t *f, hid_t dxpl_id);
+static herr_t H5F_dest(H5F_t *f, hid_t dxpl_id, hbool_t flush);
 static herr_t H5F_close(H5F_t *f);
 
 /* Declare a free list to manage the H5F_t struct */
@@ -967,7 +967,7 @@ done:
  *-------------------------------------------------------------------------
  */
 static herr_t
-H5F_dest(H5F_t *f, hid_t dxpl_id)
+H5F_dest(H5F_t *f, hid_t dxpl_id, hbool_t flush)
 {
     herr_t	   ret_value = SUCCEED;         /* Return value */
 
@@ -978,6 +978,14 @@ H5F_dest(H5F_t *f, hid_t dxpl_id)
     HDassert(f->shared);
 
     if(1 == f->shared->nrefs) {
+        /* Flush at this point since the file will be closed.
+         * Only try to flush the file if it was opened with write access, and if
+         * the caller requested a flush.
+         */
+        if((f->shared->flags & H5F_ACC_RDWR) && flush)
+            if(H5F_flush(f, dxpl_id) < 0)
+                HDONE_ERROR(H5E_CACHE, H5E_CANTFLUSH, FAIL, "unable to flush cache")
+
         /* Release objects that depend on the superblock being initialized */
         if(f->shared->sblock) {
             /* Shutdown file free space manager(s) */
@@ -1314,7 +1322,7 @@ H5F_open(const char *name, unsigned flags, hid_t fcpl_id, hid_t fapl_id,
 
 done:
     if(!ret_value && file)
-        if(H5F_dest(file, dxpl_id) < 0)
+        if(H5F_dest(file, dxpl_id, FALSE) < 0)
             HDONE_ERROR(H5E_FILE, H5E_CANTCLOSEFILE, NULL, "problems closing file")
 
     FUNC_LEAVE_NOAPI(ret_value)
@@ -1857,24 +1865,16 @@ H5F_try_close(H5F_t *f)
     if(H5F_close_mounts(f) < 0)
         HGOTO_ERROR(H5E_FILE, H5E_CANTCLOSEFILE, FAIL, "can't unmount child files")
 
-    /* Flush at this point since the file will be closed.  Don't invalidate
-     * the cache, since this file might still be open using another handle.
-     * However, make sure we flush in case that handle is read-only; its
-     * copy of the cache needs to be clean.
-     * Only try to flush the file if it was opened with write access.
-     */
-    if(f->intent & H5F_ACC_RDWR) {
-        /* Flush all caches */
-        if(H5F_flush(f, H5AC_dxpl_id) < 0)
-            HGOTO_ERROR(H5E_CACHE, H5E_CANTFLUSH, FAIL, "unable to flush cache")
-    } /* end if */
+    /* Delay flush until the shared file struct is closed, in H5F_dest.  If the
+     * application called H5Fclose, it would have been flushed in that function
+     * (unless it will have been flushed in H5F_dest anyways). */
 
     /*
      * Destroy the H5F_t struct and decrement the reference count for the
      * shared H5F_file_t struct. If the reference count for the H5F_file_t
      * struct reaches zero then destroy it also.
      */
-    if(H5F_dest(f, H5AC_dxpl_id) < 0)
+    if(H5F_dest(f, H5AC_dxpl_id, TRUE) < 0)
         HGOTO_ERROR(H5E_FILE, H5E_CANTCLOSEFILE, FAIL, "problems closing file")
 
 done:
@@ -1906,6 +1906,8 @@ done:
 herr_t
 H5Fclose(hid_t file_id)
 {
+    H5F_t       *f = NULL;
+    int         nref;
     herr_t	ret_value = SUCCEED;
 
     FUNC_ENTER_API(H5Fclose, FAIL)
@@ -1915,6 +1917,20 @@ H5Fclose(hid_t file_id)
     if(H5I_FILE != H5I_get_type(file_id))
 	HGOTO_ERROR(H5E_ARGS, H5E_BADTYPE, FAIL, "not a file ID")
 
+    /* Flush file if this is the last reference to this id and we have write
+     * intent, unless it will be flushed by the "shared" file being closed.
+     * This is only necessary to replicate previous behaviour, and could be
+     * disabled by an option/property to improve performance. */
+    if(NULL == (f = (H5F_t *)H5I_object(file_id)))
+        HGOTO_ERROR(H5E_ARGS, H5E_BADTYPE, FAIL, "invalid file identifier")
+    if((f->shared->nrefs > 1) && (H5F_INTENT(f) & H5F_ACC_RDWR)) {
+        if((nref = H5I_get_ref(file_id, FALSE)) < 0)
+            HGOTO_ERROR(H5E_ATOM, H5E_CANTGET, FAIL, "can't get ID ref count")
+        if(nref == 1)
+            if(H5F_flush(f, H5AC_dxpl_id) < 0)
+                HGOTO_ERROR(H5E_CACHE, H5E_CANTFLUSH, FAIL, "unable to flush cache")
+    } /* end if */
+
     /*
      * Decrement reference count on atom.  When it reaches zero the file will
      * be closed.
@@ -1982,7 +1998,7 @@ H5Freopen(hid_t file_id)
 
 done:
     if(ret_value < 0 && new_file)
-	if(H5F_dest(new_file, H5AC_dxpl_id) < 0)
+	if(H5F_dest(new_file, H5AC_dxpl_id, FALSE) < 0)
 	    HDONE_ERROR(H5E_FILE, H5E_CANTCLOSEFILE, FAIL, "can't close file")
 
     FUNC_LEAVE_API(ret_value)
diff --git a/src/H5Lexternal.c b/src/H5Lexternal.c
index db762cf..7ccdb68 100644
--- a/src/H5Lexternal.c
+++ b/src/H5Lexternal.c
@@ -412,19 +412,15 @@ H5L_extern_traverse(const char UNUSED *link_name, hid_t cur_group,
 
         /* get last component of file_name */
         GET_LAST_DELIMITER(actual_file_name, ptr)
-        if(ptr) {
-            /* Truncate filename portion from actual file name path */
-            *ptr = '\0';
+        if(!ptr)
+            HGOTO_ERROR(H5E_LINK, H5E_CANTOPENFILE, FAIL, "unable to open external file, external link file name = '%s', temp_file_name = '%s'", file_name, temp_file_name)
 
-            /* Build new file name for the external file */
-            if(H5L_build_name(actual_file_name, temp_file_name, &full_name/*out*/) < 0)
-                HGOTO_ERROR(H5E_LINK, H5E_CANTGET, FAIL, "can't prepend prefix to filename")
-        } /* end if */
-        else {
-            /* Transfer ownership of actual file name to 'full_name' variable */
-            full_name = actual_file_name;
-            actual_file_name = NULL;
-        } /* end else */
+        /* Truncate filename portion from actual file name path */
+        *ptr = '\0';
+
+        /* Build new file name for the external file */
+        if(H5L_build_name(actual_file_name, temp_file_name, &full_name/*out*/) < 0)
+            HGOTO_ERROR(H5E_LINK, H5E_CANTGET, FAIL, "can't prepend prefix to filename")
 
         /* Try opening with the resolved name */
         if(NULL == (ext_file = H5F_open(full_name, intent, H5P_FILE_CREATE_DEFAULT, fapl_id, H5AC_dxpl_id)))
diff --git a/src/H5Ocopy.c b/src/H5Ocopy.c
index 8bdbed5..1712706 100644
--- a/src/H5Ocopy.c
+++ b/src/H5Ocopy.c
@@ -946,6 +946,7 @@ H5O_copy_obj(H5G_loc_t *src_loc, H5G_loc_t *dst_loc, const char *dst_name,
     H5G_name_t      new_path;                   /* Copied object group hier. path */
     H5O_loc_t       new_oloc;                   /* Copied object object location */
     H5G_loc_t       new_loc;                    /* Group location of object copied */
+    H5F_t           *cached_dst_file;           /* Cached destination file */
     hbool_t         entry_inserted=FALSE;       /* Flag to indicate that the new entry was inserted into a group */
     unsigned        cpy_option = 0;             /* Copy options */
     herr_t          ret_value = SUCCEED;        /* Return value */
@@ -972,10 +973,19 @@ H5O_copy_obj(H5G_loc_t *src_loc, H5G_loc_t *dst_loc, const char *dst_name,
     H5G_loc_reset(&new_loc);
     new_oloc.file = dst_loc->oloc->file;
 
+    /* Make a copy of the destination file, in case the original is changed by
+     * H5O_copy_header.  If and when oloc's point to the shared file struct,
+     * this will no longer be necessary, so this code can be removed. */
+    cached_dst_file = dst_loc->oloc->file;
+
     /* Copy the object from the source file to the destination file */
     if(H5O_copy_header(src_loc->oloc, &new_oloc, dxpl_id, cpy_option) < 0)
         HGOTO_ERROR(H5E_OHDR, H5E_CANTCOPY, FAIL, "unable to copy object")
 
+    /* Patch dst_loc.  Again, this can be removed once oloc's point to shared
+     * file structs. */
+    dst_loc->oloc->file = cached_dst_file;
+
     /* Insert the new object in the destination file's group */
     if(H5L_link(dst_loc, dst_name, &new_loc, lcpl_id, H5P_DEFAULT, dxpl_id) < 0)
 	HGOTO_ERROR(H5E_DATATYPE, H5E_CANTINIT, FAIL, "unable to insert link")