[lmi-commits] [4988] Fix defect introduced 20061110T1628Z

[Greg Chicares]

Revision: 4988
          http://svn.sv.gnu.org/viewvc/?view=rev&root=lmi&revision=4988
Author:   chicares
Date:     2010-06-12 21:57:38 +0000 (Sat, 12 Jun 2010)
Log Message:
-----------
Fix defect introduced 20061110T1628Z

Modified Paths:
--------------
    lmi/trunk/configurable_settings.cpp
    lmi/trunk/configurable_settings_test.cpp

Modified: lmi/trunk/configurable_settings.cpp
===================================================================
--- lmi/trunk/configurable_settings.cpp 2010-06-12 01:39:19 UTC (rev 4987)
+++ lmi/trunk/configurable_settings.cpp 2010-06-12 21:57:38 UTC (rev 4988)
@@ -33,17 +33,14 @@
 #include "contains.hpp"
 #include "data_directory.hpp"     // AddDataDir()
 #include "handle_exceptions.hpp"
-#include "miscellany.hpp"
-#include "path_utility.hpp"
+#include "miscellany.hpp"         // lmi_array_size()
+#include "path_utility.hpp"       // validate_directory(), validate_filepath()
 #include "platform_dependent.hpp" // access()
-#include "xml_lmi.hpp"
 
 #include <boost/filesystem/fstream.hpp>
 #include <boost/filesystem/operations.hpp>
 #include <boost/filesystem/path.hpp>
 
-#include <xmlwrapp/nodes_view.h>
-
 #include <algorithm> // std::copy()
 #include <iterator>
 #include <sstream>
@@ -62,12 +59,26 @@
 /// it's non-complete--as is typical msw usage.
 ///
 /// Look for the configuration file first where FHS would have it.
-/// To support non-FHS platforms, if it's not found there, then
+/// To support non-FHS platforms, if it's not readable there, then
 /// look in the data directory.
 ///
-/// TODO ?? CALCULATION_SUMMARY Should write access be checked
-/// here? What if the first file found is read-only, but the
-/// second is read-write?
+/// Throws if the file is not readable.
+///
+/// A warning is given at initialization if the file is readable but
+/// not writable. It could conceivably be readable in both locations,
+/// but writable only in the second:
+///   -r--r--r-- ... /etc/opt/lmi/configurable_settings.xml
+///   -rw-rw-rw- ... /opt/lmi/data/configurable_settings.xml
+/// In that particular case, it might at first seem better to choose
+/// the second file. However, in the most plausible case--an archival
+/// copy of the system stored on a read-only medium, including coeval
+/// data files--it would be better to mount that medium as the data
+/// directory, e.g.:
+///   -rw-rw-rw- ... /etc/opt/lmi/configurable_settings.xml
+///   -r--r--r-- ... /dev/cdrom/configurable_settings.xml
+/// and the file in /etc/opt/lmi/ would be chosen by default, as seems
+/// most appropriate. (A knowledgeable user could of course move it
+/// aside if it is desired to use the file on the read-only medium.)
 
 fs::path const& configuration_filepath()
 {
@@ -78,10 +89,10 @@
         }
 
     std::string filename = "/etc/opt/lmi/" + configuration_filename();
-    if(access(filename.c_str(), R_OK))
+    if(0 != access(filename.c_str(), R_OK))
         {
         filename = AddDataDir(configuration_filename());
-        if(access(filename.c_str(), R_OK))
+        if(0 != access(filename.c_str(), R_OK))
             {
             fatal_error()
                 << "No readable file '"
@@ -91,6 +102,18 @@
                 ;
             }
         }
+
+    if(0 != access(filename.c_str(), W_OK))
+        {
+        warning()
+            << "Configurable-settings file '"
+            << filename
+            << "' can be read but not written."
+            << " No configuration changes can be saved."
+            << LMI_FLUSH
+            ;
+        }
+
     validate_filepath(filename, "Configurable-settings file");
     complete_path = fs::system_complete(filename);
     return complete_path;

Modified: lmi/trunk/configurable_settings_test.cpp
===================================================================
--- lmi/trunk/configurable_settings_test.cpp    2010-06-12 01:39:19 UTC (rev 
4987)
+++ lmi/trunk/configurable_settings_test.cpp    2010-06-12 21:57:38 UTC (rev 
4988)
@@ -37,23 +37,30 @@
     static void test()
         {
         test_normal_usage();
-        test_something_else();
+        test_writability();
         }
 
   private:
     static void test_normal_usage();
-    static void test_something_else();
+    static void test_writability();
 };
 
+/// Test for gross failure upon instantiation.
+
 void configurable_settings_test::test_normal_usage()
 {
     configurable_settings const& c = configurable_settings::instance();
     BOOST_TEST(!c.skin_filename().empty());
 }
 
-void configurable_settings_test::test_something_else()
+/// Save a copy of the file multiple times (because users might).
+
+void configurable_settings_test::test_writability()
 {
-    // Nothing here yet.
+    configurable_settings const& c = configurable_settings::instance();
+    std::string const filename("eraseme");
+    c.xml_serializable<configurable_settings>::save(filename);
+    c.xml_serializable<configurable_settings>::save(filename);
 }
 
 int test_main(int, char*[])