[Saga-devel] saga SVN commit 3412: /trunk/adaptors/condor/job/

[jpabecasis at cct.lsu.edu]

User: jpabecasis
Date: 2009/01/31 03:25 PM

Modified:
 /trunk/adaptors/condor/job/
  temporary.cpp, temporary.hpp

Log:
 Some comments for the Windows port. Inlined is_handle_valid.

File Changes:

Directory: /trunk/adaptors/condor/job/
======================================

File [modified]: temporary.cpp
Delta lines: +18 -10
===================================================================
--- trunk/adaptors/condor/job/temporary.cpp	2009-01-31 21:23:48 UTC (rev 3411)
+++ trunk/adaptors/condor/job/temporary.cpp	2009-01-31 21:25:02 UTC (rev 3412)
@@ -12,6 +12,11 @@
 
 namespace saga { namespace adaptors { namespace condor {
 
+    // FIXME: On Windows, we're using both MAX_PATH and _MAX_PATH. Is this
+    // intentional? Also, I seem to remember that there were cases where a
+    // generated path could grow longer than either of those. Is there a more
+    // robust way to handle path lengths there?
+
     boost::filesystem::path get_temporary_dir()
     {
 #if defined(BOOST_WINDOWS)
@@ -30,6 +35,10 @@
     inline temporary_file::handle_type open_temp_file(char * tmp_name)
     {
 #if defined(BOOST_WINDOWS)
+        // FIXME: We're supposed to return the name of the temporary file in
+        // tmp_name. Here, the name is lost into oblivion. Also, what does the
+        // "tmp" do in the GetTempFileNameA call?
+
         // create file name
         char buffer[_MAX_PATH + 1];
         if (0 == GetTempFileNameA (tmp_name, "tmp", 0, buffer) )
@@ -42,15 +51,6 @@
 #endif
     }
 
-    bool is_handle_valid(temporary_file::handle_type fd)
-    {
-#if defined(BOOST_WINDOWS)
-        return INVALID_HANDLE_VALUE != fd;
-#else
-        return 0 <= fd;
-#endif
-    }
-
     std::auto_ptr<temporary_file> open_temporary_file(
             boost::filesystem::path template_)
     {
@@ -86,6 +86,12 @@
                 leaf = "temp";
 
 #if !defined(BOOST_WINDOWS)
+            // FIXME: Is windows able to provide the same semantics, i.e.,
+            // provide a temporary file with a user-provided prefix? Or should
+            // we instead create a directory with the prefix and generate
+            // random names in there? Having a common prefix (whether in the
+            // filename or as a directory helps in debugging and clean-up.
+
             // A random suffix is always appended to the filename.
             leaf += "-XXXXXX";
 #endif
@@ -106,8 +112,10 @@
         }
 
         std::auto_ptr<temporary_file> result;
-        if (is_handle_valid(fd = open_temp_file(buffer)))
+        if (detail::is_handle_valid(fd = open_temp_file(buffer)))
         {
+            // FIXME: On Windows, open_temp_file is not returning the name of
+            // the temporary file in buffer. We need it here.
             template_ = buffer;
             result.reset(new temporary_file(fd, template_));
         }

File [modified]: temporary.hpp
Delta lines: +24 -1
===================================================================
--- trunk/adaptors/condor/job/temporary.hpp	2009-01-31 21:23:48 UTC (rev 3411)
+++ trunk/adaptors/condor/job/temporary.hpp	2009-01-31 21:25:02 UTC (rev 3412)
@@ -27,6 +27,19 @@
 
 namespace saga { namespace adaptors { namespace condor {
 
+    namespace detail {
+
+        bool is_handle_valid(temporary_file::handle_type fd)
+        {
+    #if defined(BOOST_WINDOWS)
+            return INVALID_HANDLE_VALUE != fd;
+    #else
+            return 0 <= fd;
+    #endif
+        }
+
+    } // namespace detail
+
     // RAII for file descriptor returned from open_temporary_file.
     struct temporary_file
     {
@@ -44,7 +57,17 @@
 
         ~temporary_file()
         {
-            if (fd_ >= 0)
+            // TODO: Unlink on close should be optional and it should be
+            // possible to change dynamically. This is useful, e.g., to persist
+            // Condor log files while debugging.
+            // Out of curiosity, is the Windows FILE_FLAG_DELETE_ON_CLOSE more
+            // robust than calling unlink in the destructor on other systems?
+            // For instance, in case of a hard crash where the destructor isn't
+            // called, does Windows delete the file as well? What about after
+            // an OS crash?  Well, we're probably not concerned with those
+            // here...
+
+            if (detail::is_handle_valid(fd_))
             {
 #if defined(BOOST_WINDOWS)
                 CloseHandle(fd_);