move "changed" status handling into library

* remove public status modifier as high-level operations no longer need it.
* add more information to clarify changed status handling in library.
* mark changed status at appropriate places in library.

6 files changed, 118 insertions, 125 deletions

diff --git a/src/cli_bin.cpp b/src/cli_bin.cpp
index 25a86ce..5823c02 100644
--- a/src/cli_bin.cpp
+++ b/src/cli_bin.cpp
@@ -165,8 +165,6 @@ doDiscard(Cstore& cstore, const vector<string>& args)
   if (!cstore.discardChanges()) {
     bye("discard failed\n");
   }
-  // special case for discard: don't return (don't want to mark changed)
-  exit(0);
 }
 
 static void
@@ -228,7 +226,6 @@ main(int argc, char **argv)
 
   // call the op function
   OpFunc[op_idx](cstore, path_comps);
-  cstore.markCfgPathChanged(path_comps);
   exit(0);
 }
 
diff --git a/src/common/unionfs.c b/src/common/unionfs.c
index 85ea9f1..da4cd7e 100644
--- a/src/common/unionfs.c
+++ b/src/common/unionfs.c
@@ -730,7 +730,7 @@ common_commit_copy_to_live_config(GNode *node, boolean suppress_piecewise_copy,
     /* note: return status is not checked and operation continues even if
      *       this fails. this follows the original logic.
      */
-    cstore_unmark_cfg_path_changed(cs, pcomps, ncomps);
+    cstore_unmark_cfg_path_changed(cs, (const char **) pcomps, ncomps);
     cstore_free_path_comps(pcomps, ncomps);
     cstore_free(cs);
   }
diff --git a/src/cstore/cstore.cpp b/src/cstore/cstore.cpp
index f146cb0..b7261fe 100644
--- a/src/cstore/cstore.cpp
+++ b/src/cstore/cstore.cpp
@@ -220,10 +220,10 @@ Cstore::deleteCfgPath(const vector<string>& path_comps, const vtw_def& def)
     /* assume default value is valid (parser should have validated).
      * also call unmark_deactivated() in case the node being deleted was
      * also deactivated. note that unmark_deactivated() succeeds if it's
-     * not marked deactivated.
+     * not marked deactivated. also mark "changed".
      */
     bool ret = (write_value(def.def_default) && mark_display_default()
-                && unmark_deactivated());
+                && unmark_deactivated() && mark_changed_with_ancestors());
     if (!ret) {
       output_user("Failed to set default value during delete\n");
     }
@@ -266,6 +266,10 @@ Cstore::deleteCfgPath(const vector<string>& path_comps, const vtw_def& def)
       ret = remove_node();
     }
   }
+  if (ret) {
+    // mark changed
+    ret = mark_changed_with_ancestors();
+  }
   RESTORE_PATHS;
   if (!ret) {
     output_user("Failed to delete specified config path\n");
@@ -893,7 +897,12 @@ Cstore::renameCfgPath(const vector<string>& args)
   string otagval = args[1];
   string ntagval = args[4];
   push_cfg_path(otagnode);
-  bool ret = rename_child_node(otagval, ntagval);
+  /* also mark changed. note that it's marking the "tag node" but not the
+   * "tag values" since one is being "deleted" and the other is being
+   * "added" anyway.
+   */
+  bool ret = (rename_child_node(otagval, ntagval)
+              && mark_changed_with_ancestors());
   pop_cfg_path();
   return ret;
 }
@@ -909,7 +918,11 @@ Cstore::copyCfgPath(const vector<string>& args)
   string otagval = args[1];
   string ntagval = args[4];
   push_cfg_path(otagnode);
-  bool ret = copy_child_node(otagval, ntagval);
+  /* also mark changed. note that it's marking the "tag node" but not the
+   * new "tag value" since it is being "added" anyway.
+   */
+  bool ret = (copy_child_node(otagval, ntagval)
+              && mark_changed_with_ancestors());
   pop_cfg_path();
   return ret;
 }
@@ -1038,7 +1051,7 @@ Cstore::cfgPathAdded(const vector<string>& path_comps)
  *     (remember this functions is NOT "deactivate-aware")
  *       (1) cfgPathDeleted()
  *       (2) cfgPathAdded()
- *       (3) marked_changed()
+ *       (3) cfg_node_changed()
  */
 bool
 Cstore::cfgPathChanged(const vector<string>& path_comps)
@@ -1048,7 +1061,7 @@ Cstore::cfgPathChanged(const vector<string>& path_comps)
   }
   SAVE_PATHS;
   append_cfg_path(path_comps);
-  bool ret = marked_changed();
+  bool ret = cfg_node_changed();
   RESTORE_PATHS;
   return ret;
 }
@@ -1208,7 +1221,7 @@ Cstore::cfgPathGetChildNodesStatusDA(const vector<string>& path_comps,
     } else {
       SAVE_PATHS;
       append_cfg_path(ppath);
-      if (marked_changed()) {
+      if (cfg_node_changed()) {
         cmap[work_nodes[i]] = C_NODE_STATUS_CHANGED;
       } else {
         cmap[work_nodes[i]] = C_NODE_STATUS_STATIC;
@@ -1738,7 +1751,9 @@ Cstore::markCfgPathDeactivated(const vector<string>& path_comps)
 
   SAVE_PATHS;
   append_cfg_path(path_comps);
-  bool ret = (mark_deactivated() && unmark_deactivated_descendants());
+  // note: also mark changed
+  bool ret = (mark_deactivated() && unmark_deactivated_descendants()
+              && mark_changed_with_ancestors());
   RESTORE_PATHS;
   return ret;
 }
@@ -1752,17 +1767,16 @@ Cstore::unmarkCfgPathDeactivated(const vector<string>& path_comps)
 {
   SAVE_PATHS;
   append_cfg_path(path_comps);
-  bool ret = unmark_deactivated();
+  // note: also mark changed
+  bool ret = (unmark_deactivated() && mark_changed_with_ancestors());
   RESTORE_PATHS;
   return ret;
 }
 
-/* mark specified path as changed in working config.
- * the marking is used during commit to check if a node has been changed.
- * this should be done after set/delete/activate/deactivate.
- * note: if a node is changed, all of its ancestors are also considered
- *       changed.
- * return true if successful. otherwise return false.
+/* "changed" status handling.
+ * the "changed" status is used during commit to check if a node has been
+ * changed. note that if a node is "changed", all of its ancestors are also
+ * considered changed (this follows the original logic).
  *
  * the original backend implementation only uses the "changed" marker at
  * "root" to indicate whether the whole config has changed. for the rest
@@ -1775,38 +1789,30 @@ Cstore::unmarkCfgPathDeactivated(const vector<string>& path_comps)
  * in "changes only", so they are _not_ treated as "changed". this creates
  * problems in various parts of the backend.
  *
- * the new CLI backend/library marks all changed nodes explicitly, and the
- * "changed" status depends on such markers. note that the actual marking
- * is done by the low-level implementation, so it does not have to be done
- * as a "file marker" as long as the low-level implementation can correctly
- * answer the "changed" query for a given path.
+ * the new CLI backend/library "marks" all changed nodes explicitly, and the
+ * "changed" status depends on such markers. the marking is done using the
+ * pure virtual mark_changed_with_ancestors() function, which is provided
+ * by the low-level implementation, so it does not have to be done as a
+ * "per-node file marker" as long as the low-level implementation can
+ * correctly answer the "changed" query for a given path.
+ *
+ * note that "changed" nodes does not include "added" and "deleted" nodes.
+ * for the convenience of implementation, the backend must always query
+ * for "changed" nodes *after* "added" and "deleted" nodes. in other
+ * words, the backend will only treat a node as "changed" if it is neither
+ * "added" nor "deleted". currently there are only two places that perform
+ * changed status query: cfgPathGetChildNodesStatus() and
+ * cfgPathGetChildNodesStatusDA(). see those two functions for the usage.
+ *
+ * what this means is that the backend can choose to either mark or not
+ * mark "added"/"deleted" nodes as "changed" at its convenience. for
+ * example, "set" and "delete" always do the marking, but "rename" and
+ * "copy" do not.
+ *
+ * changed status queries are provided by the cfg_node_changed() function,
+ * and changed markers can be removed by unmarkCfgPathChanged() below (used
+ * by "commit").
  */
-bool
-Cstore::markCfgPathChanged(const vector<string>& path_comps)
-{
-  // first mark the root changed
-  if (!mark_changed()) {
-    return false;
-  }
-
-  // now mark each level as changed
-  vector<string> ppath;
-  for (size_t i = 0; i < path_comps.size(); i++) {
-    ppath.push_back(path_comps[i]);
-    if (!cfg_path_exists(ppath, false, false)) {
-      // this level no longer in working. nothing further.
-      break;
-    }
-    SAVE_PATHS;
-    append_cfg_path(ppath);
-    bool ret = mark_changed();
-    RESTORE_PATHS;
-    if (!ret) {
-      return false;
-    }
-  }
-  return true;
-}
 
 /* unmark "changed" status of specified path in working config.
  * this is used, e.g., at the end of "commit" to reset a subtree.
@@ -2272,6 +2278,10 @@ Cstore::set_cfg_path(const vector<string>& path_comps, bool output)
         }
       }
     }
+    if (!mark_changed_with_ancestors()) {
+      ret = false;
+      break;
+    }
     RESTORE_PATHS;
   }
   RESTORE_PATHS; // if "break" was hit
@@ -2289,29 +2299,32 @@ Cstore::set_cfg_path(const vector<string>& path_comps, bool output)
     pop_cfg_path();
     // only do it if it's previously marked default
     if (marked_display_default(false)) {
-      ret = unmark_display_default();
-
-      /* XXX work around current commit's unionfs implementation problem.
-       * current commit's unionfs implementation looks at the "changes only"
-       * directory (i.e., the r/w portion of the union mount), which is wrong.
-       *
-       * all config information should be obtained from two directories:
-       * "active" and "working", e.g., instead of looking at whiteout files
-       * in "changes only" to find deleted nodes, nodes that are in "active"
-       * but not in "working" have been deleted.
-       *
-       * in this particular case, commit looks at "changes only" to read the
-       * node.val file. however, since the value didn't change (only the
-       * "default status" changed), node.val doesn't appear in "changes only".
-       * here we re-write the file to force it into "changes only" so that
-       * commit can work correctly.
-       */
-      vector<string> vvec;
-      read_value_vec(vvec, false);
-      write_value_vec(vvec);
-
-      // pretend it didn't exist since we changed the status
-      path_exists = false;
+      if ((ret = unmark_display_default())) {
+        /* XXX work around current commit's unionfs implementation problem.
+         * current commit's unionfs implementation looks at the "changes
+         * only" directory (i.e., the r/w portion of the union mount), which
+         * is wrong.
+         *
+         * all config information should be obtained from two directories:
+         * "active" and "working", e.g., instead of looking at whiteout
+         * files in "changes only" to find deleted nodes, nodes that are in
+         * "active" but not in "working" have been deleted.
+         *
+         * in this particular case, commit looks at "changes only" to read
+         * the node.val file. however, since the value didn't change (only
+         * the "default status" changed), node.val doesn't appear in
+         * "changes only". here we re-write the file to force it into
+         * "changes only" so that commit can work correctly.
+         */
+        vector<string> vvec;
+        read_value_vec(vvec, false);
+        write_value_vec(vvec);
+
+        // pretend it didn't exist since we changed the status
+        path_exists = false;
+        // also mark changed
+        ret = mark_changed_with_ancestors();
+      }
     }
     RESTORE_PATHS;
   }
diff --git a/src/cstore/cstore.hpp b/src/cstore/cstore.hpp
index 4b07fab..0c80ffd 100644
--- a/src/cstore/cstore.hpp
+++ b/src/cstore/cstore.hpp
@@ -142,8 +142,7 @@ public:
   virtual bool setupSession() = 0;
   virtual bool teardownSession() = 0;
   virtual bool inSession() = 0;
-  // common
-  bool markCfgPathChanged(const vector<string>& path_comps);
+  // commit
   bool unmarkCfgPathChanged(const vector<string>& path_comps);
   // XXX load
   //bool unmarkCfgPathDeactivatedDescendants(const vector<string>& path_comps);
@@ -307,14 +306,14 @@ private:
   virtual bool mark_deactivated() = 0;
   virtual bool unmark_deactivated() = 0;
   virtual bool unmark_deactivated_descendants() = 0;
+  virtual bool mark_changed_with_ancestors() = 0;
   virtual bool unmark_changed_with_descendants() = 0;
-  virtual bool mark_changed() = 0;
   virtual bool remove_comment() = 0;
   virtual bool set_comment(const string& comment) = 0;
   virtual bool discard_changes(unsigned long long& num_removed) = 0;
 
   // observers for current work path
-  virtual bool marked_changed() = 0;
+  virtual bool cfg_node_changed() = 0;
 
   // observers for current work path or active path
   virtual bool read_value_vec(vector<string>& vvec, bool active_cfg) = 0;
diff --git a/src/cstore/unionfs/cstore-unionfs.cpp b/src/cstore/unionfs/cstore-unionfs.cpp
index 7f14483..5cfd8ee 100644
--- a/src/cstore/unionfs/cstore-unionfs.cpp
+++ b/src/cstore/unionfs/cstore-unionfs.cpp
@@ -749,6 +749,32 @@ UnionfsCstore::unmark_deactivated_descendants()
   return ret;
 }
 
+// mark current work path and all ancestors as "changed"
+bool
+UnionfsCstore::mark_changed_with_ancestors()
+{
+  b_fs::path opath = mutable_cfg_path; // use a copy
+  while (opath.has_parent_path()) {
+    b_fs::path marker = (work_root / opath);
+    pop_path(opath);
+    if (!b_fs_exists(marker) || !b_fs_is_directory(marker)) {
+      // don't do anything if the node is not there
+      continue;
+    }
+    marker /= C_MARKER_CHANGED;
+    if (b_fs_exists(marker)) {
+      // reached a node already marked => done
+      break;
+    }
+    if (!create_file(marker.file_string())) {
+      output_internal("failed to mark changed [%s]\n",
+                      marker.file_string().c_str());
+      return false;
+    }
+  }
+  return true;
+}
+
 /* remove all "changed" markers under the current work path. this is used,
  * e.g., at the end of "commit" to reset a subtree.
  */
@@ -777,39 +803,6 @@ UnionfsCstore::unmark_changed_with_descendants()
   return true;
 }
 
-bool
-UnionfsCstore::mark_changed()
-{
-  if (!mutable_cfg_path.has_parent_path()) {
-    /* at root, mark changed. root marker is needed by the original
-     * implementation as an indication of whether the whole config
-     * has changed.
-     */
-    b_fs::path marker = get_work_path() / C_MARKER_CHANGED;
-    if (b_fs_exists(marker)) {
-      // already marked. treat as success.
-      return true;
-    }
-    if (!create_file(marker.file_string())) {
-      output_internal("failed to mark changed [%s]\n",
-                      get_work_path().file_string().c_str());
-      return false;
-    }
-    return true;
-  }
-
-  /* XXX not at root => nop for now.
-   *     we should be marking changed here. however, as commit is still
-   *     using its own unionfs implementation, it will not understand the
-   *     markers and therefore will not perform the necessary cleanup when
-   *     it's done.
-   *
-   *     for now, don't mark anything besides root. the query function
-   *     will use unionfs-specific implementation (changes-only dir).
-   */
-  return true;
-}
-
 // remove the comment at the current work path
 bool
 UnionfsCstore::remove_comment()
@@ -880,21 +873,12 @@ UnionfsCstore::get_comment(string& comment, bool active_cfg)
   return read_whole_file(cfile, comment);
 }
 
+// whether current work path is "changed"
 bool
-UnionfsCstore::marked_changed()
+UnionfsCstore::cfg_node_changed()
 {
-  /* this function is only called by cfgPathChanged() in base class.
-   *
-   * XXX currently just use the changes_only dir for this query.
-   *     see explanation in mark_changed().
-   *
-   *     this implementation relies on the fact that cfgPathChanged()
-   *     includes deleted/added nodes (including deactivated/activated
-   *     nodes since it's NOT deactivate-aware). if that is not the case,
-   *     result will be different between deleted nodes (NOT IN
-   *     changes_only) and deactivated nodes (IN changes_only).
-   */
-  return b_fs_exists(get_change_path());
+  b_fs::path marker = get_work_path() / C_MARKER_CHANGED;
+  return b_fs_exists(marker);
 }
 
 /* XXX currently "committed marking" is done inside commit.
diff --git a/src/cstore/unionfs/cstore-unionfs.hpp b/src/cstore/unionfs/cstore-unionfs.hpp
index bff4844..90f28c5 100644
--- a/src/cstore/unionfs/cstore-unionfs.hpp
+++ b/src/cstore/unionfs/cstore-unionfs.hpp
@@ -158,14 +158,14 @@ private:
   bool mark_deactivated();
   bool unmark_deactivated();
   bool unmark_deactivated_descendants();
+  bool mark_changed_with_ancestors();
   bool unmark_changed_with_descendants();
-  bool mark_changed();
   bool remove_comment();
   bool set_comment(const string& comment);
   bool discard_changes(unsigned long long& num_removed);
 
   // observers for work path
-  bool marked_changed();
+  bool cfg_node_changed();
 
   // observers for work path or active path
   bool cfg_node_exists(bool active_cfg);