From 29273c0ba8399d0b135384aec33d71fe28a93169 Mon Sep 17 00:00:00 2001 From: An-Cheng Huang Date: Thu, 19 Aug 2010 17:48:55 -0700 Subject: add API function --- src/cstore/cstore.cpp | 13 +++++++++++-- src/cstore/cstore.hpp | 3 +++ 2 files changed, 14 insertions(+), 2 deletions(-) (limited to 'src') diff --git a/src/cstore/cstore.cpp b/src/cstore/cstore.cpp index 3cd0649..33210dc 100644 --- a/src/cstore/cstore.cpp +++ b/src/cstore/cstore.cpp @@ -1095,11 +1095,20 @@ Cstore::cfgPathGetDeletedChildNodesDA(const vector& path_comps, void Cstore::cfgPathGetDeletedValues(const vector& path_comps, vector& dvals) +{ + cfgPathGetDeletedValuesDA(path_comps, dvals, false); +} + +// same as above but DA +void +Cstore::cfgPathGetDeletedValuesDA(const vector& path_comps, + vector& dvals, + bool include_deactivated) { vector ovals; vector nvals; - if (!cfgPathGetValues(path_comps, ovals, true) - || !cfgPathGetValues(path_comps, nvals, false)) { + if (!cfgPathGetValuesDA(path_comps, ovals, true, include_deactivated) + || !cfgPathGetValuesDA(path_comps, nvals, false, include_deactivated)) { return; } map dmap; diff --git a/src/cstore/cstore.hpp b/src/cstore/cstore.hpp index 492462e..37a3e06 100644 --- a/src/cstore/cstore.hpp +++ b/src/cstore/cstore.hpp @@ -240,6 +240,9 @@ public: void cfgPathGetDeletedChildNodesDA(const vector& path_comps, vector& cnodes, bool include_deactivated = true); + void cfgPathGetDeletedValuesDA(const vector& path_comps, + vector& dvals, + bool include_deactivated = true); void cfgPathGetChildNodesStatusDA(const vector& path_comps, map& cmap); -- cgit v1.2.3 From 3c6f899045ebc92ae24513b986363afe23be146a Mon Sep 17 00:00:00 2001 From: An-Cheng Huang Date: Thu, 19 Aug 2010 18:56:21 -0700 Subject: adjust deactivate-aware logic --- src/cstore/cstore.cpp | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) (limited to 'src') diff --git a/src/cstore/cstore.cpp b/src/cstore/cstore.cpp index 33210dc..ebf9585 100644 --- a/src/cstore/cstore.cpp +++ b/src/cstore/cstore.cpp @@ -1198,16 +1198,24 @@ Cstore::cfgPathGetChildNodesStatusDA(const vector& path_comps, * * for "added" state, can't use cfgPathAdded() since it's not DA. * + * for "changed" state, can't use cfgPathChanged() since it's not DA. + * * deleted ones already handled above. */ if (!cfg_path_exists(ppath, true, true) && cfg_path_exists(ppath, false, true)) { cmap[work_nodes[i]] = C_NODE_STATUS_ADDED; - } else if (cfgPathChanged(ppath)) { - cmap[work_nodes[i]] = C_NODE_STATUS_CHANGED; } else { - cmap[work_nodes[i]] = C_NODE_STATUS_STATIC; + SAVE_PATHS; + append_cfg_path(ppath); + if (marked_changed()) { + cmap[work_nodes[i]] = C_NODE_STATUS_CHANGED; + } else { + cmap[work_nodes[i]] = C_NODE_STATUS_STATIC; + } + RESTORE_PATHS; } + ppath.pop_back(); } } -- cgit v1.2.3 From cbd1770462b2325372f90ef9200110dc66245377 Mon Sep 17 00:00:00 2001 From: An-Cheng Huang Date: Fri, 20 Aug 2010 13:29:49 -0700 Subject: handle "changed" status properly * original backend implementation uses unionfs-specific "changes only" dir to determine "changed" status. this breaks when it involves deactivated nodes. * new library design uses explicit per-node "changed" marker. however, since previously "commit" only handles a root "changed" marker, the new library could not implement this scheme and used a workaround instead. * now add API functions for "commit" to properly clean up "changed" markers. * modify "commit" to use these API functions and remove the workaround from the new library. --- src/common/unionfs.c | 46 ++++++++++++++++++++--------------- src/cstore/cstore-c.cpp | 13 ++++++++++ src/cstore/cstore-c.h | 3 +++ src/cstore/cstore.cpp | 16 ++++++++++++ src/cstore/cstore.hpp | 2 ++ src/cstore/unionfs/cstore-unionfs.cpp | 28 +++++++++++++++++++++ src/cstore/unionfs/cstore-unionfs.hpp | 1 + 7 files changed, 89 insertions(+), 20 deletions(-) (limited to 'src') diff --git a/src/common/unionfs.c b/src/common/unionfs.c index e7157c0..85ea9f1 100644 --- a/src/common/unionfs.c +++ b/src/common/unionfs.c @@ -10,6 +10,8 @@ #include "common/defs.h" #include "common/unionfs.h" +#include + boolean g_debug; extern vtw_path m_path; @@ -715,7 +717,26 @@ common_commit_copy_to_live_config(GNode *node, boolean suppress_piecewise_copy, printf("common_commit_copy_to_live_config(): %s\n",path); syslog(LOG_DEBUG,"common_commit_copy_to_live_config(): %s\n",path); } + + /* this function is called for each "subtree" that has been successfully + * committed. before doing anything else, remove the "changed" status + * from any changed nodes in this subtree first (since this subtree is + * going into the active config). + */ + { + void *cs = cstore_init(); + int ncomps; + char **pcomps = cstore_path_string_to_path_comps(path, &ncomps); + /* 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_free_path_comps(pcomps, ncomps); + cstore_free(cs); + } + char *command = malloc(MAX_LENGTH_DIR_PATH); + /* XXX must ... remove ... this ... */ static const char format0[]="mkdir -p %s ; /bin/true"; static const char formatpoint5[]="rm -fr '%s'"; /*tmpp*/ static const char format1[]="cp -r -f %s/* %s"; /*mdirp, tmpp*/ @@ -854,8 +875,8 @@ common_commit_clean_temp_config(GNode *root_node, boolean test_mode) char *command; command = malloc(MAX_LENGTH_DIR_PATH); + /* XXX must ... remove ... this ... */ static const char format2[]="sudo umount %s"; //mdirp - static const char format3[]="rm -f %s/" MOD_NAME " >&/dev/null ; /bin/true"; /*tmpp*/ static const char format5[]="rm -fr '%s'/{.*,*} >&/dev/null ; /bin/true"; /*cdirp*/ static const char format7[]="sudo mount -t unionfs -o dirs=%s=rw:%s=ro unionfs %s"; //cdirp, adirp, mdirp @@ -902,26 +923,11 @@ common_commit_clean_temp_config(GNode *root_node, boolean test_mode) (GNodeTraverseFunc)delete_wh_func, (gpointer)&sd); } - - sprintf(command, format3, tbuf); - if (g_debug) { - printf("%s\n",command); - syslog(LOG_DEBUG,"%s\n",command); - fflush(NULL); - } - if (test_mode == FALSE) { - system(command); - } - sprintf(command, format3, cbuf); - if (g_debug) { - printf("%s\n",command); - syslog(LOG_DEBUG,"%s\n",command); - fflush(NULL); - } - if (test_mode == FALSE) { - system(command); - } + /* originally the root "changed" marker was being removed here. this is now + * handled in common_commit_copy_to_live_config() since we need to do it + * subtree-by-subtree (and also remove the markers from any descendants). + */ sprintf(command, format5, cbuf); if (g_debug) { diff --git a/src/cstore/cstore-c.cpp b/src/cstore/cstore-c.cpp index fd818a5..c835efe 100644 --- a/src/cstore/cstore-c.cpp +++ b/src/cstore/cstore-c.cpp @@ -143,6 +143,19 @@ cstore_cfg_path_get_effective_value(void *handle, const char *path_comps[], return NULL; } +int +cstore_unmark_cfg_path_changed(void *handle, const char *path_comps[], + int num_comps) +{ + if (handle) { + vector vs; + _get_str_vec(vs, path_comps, num_comps); + Cstore *cs = (Cstore *) handle; + return (cs->unmarkCfgPathChanged(vs) ? 1 : 0); + } + return 0; +} + char ** cstore_path_string_to_path_comps(const char *path_str, int *num_comps) { diff --git a/src/cstore/cstore-c.h b/src/cstore/cstore-c.h index a2ad844..4f33672 100644 --- a/src/cstore/cstore-c.h +++ b/src/cstore/cstore-c.h @@ -38,6 +38,9 @@ char *cstore_cfg_path_get_effective_value(void *handle, const char *path_comps[], int num_comps); +int cstore_unmark_cfg_path_changed(void *handle, const char *path_comps[], + int num_comps); + /* the following are internal APIs for the library. they can only be used * during cstore operations since they operate on "current" paths constructed * by the operations. diff --git a/src/cstore/cstore.cpp b/src/cstore/cstore.cpp index ebf9585..0634988 100644 --- a/src/cstore/cstore.cpp +++ b/src/cstore/cstore.cpp @@ -1791,6 +1791,22 @@ Cstore::markCfgPathChanged(const vector& path_comps) 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. + * note: unmarking a node means all of its descendants are also unmarked, + * i.e., they become "unchanged". + * return true if successful. otherwise return false. + */ +bool +Cstore::unmarkCfgPathChanged(const vector& path_comps) +{ + SAVE_PATHS; + append_cfg_path(path_comps); + bool ret = unmark_changed_with_descendants(); + RESTORE_PATHS; + return ret; +} + ////// protected functions void diff --git a/src/cstore/cstore.hpp b/src/cstore/cstore.hpp index 37a3e06..4b07fab 100644 --- a/src/cstore/cstore.hpp +++ b/src/cstore/cstore.hpp @@ -144,6 +144,7 @@ public: virtual bool inSession() = 0; // common bool markCfgPathChanged(const vector& path_comps); + bool unmarkCfgPathChanged(const vector& path_comps); // XXX load //bool unmarkCfgPathDeactivatedDescendants(const vector& path_comps); @@ -306,6 +307,7 @@ private: virtual bool mark_deactivated() = 0; virtual bool unmark_deactivated() = 0; virtual bool unmark_deactivated_descendants() = 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; diff --git a/src/cstore/unionfs/cstore-unionfs.cpp b/src/cstore/unionfs/cstore-unionfs.cpp index 860d553..7f14483 100644 --- a/src/cstore/unionfs/cstore-unionfs.cpp +++ b/src/cstore/unionfs/cstore-unionfs.cpp @@ -749,6 +749,34 @@ UnionfsCstore::unmark_deactivated_descendants() return ret; } +/* remove all "changed" markers under the current work path. this is used, + * e.g., at the end of "commit" to reset a subtree. + */ +bool +UnionfsCstore::unmark_changed_with_descendants() +{ + try { + vector markers; + b_fs::recursive_directory_iterator di(get_work_path()); + for (; di != b_fs::recursive_directory_iterator(); ++di) { + if (!b_fs_is_regular(di->path()) + || di->path().filename() != C_MARKER_CHANGED) { + // not marker + continue; + } + markers.push_back(di->path()); + } + for (size_t i = 0; i < markers.size(); i++) { + b_fs::remove(markers[i]); + } + } catch (...) { + output_internal("failed to unmark changed with descendants [%s]\n", + get_work_path().file_string().c_str()); + return false; + } + return true; +} + bool UnionfsCstore::mark_changed() { diff --git a/src/cstore/unionfs/cstore-unionfs.hpp b/src/cstore/unionfs/cstore-unionfs.hpp index 9e49064..bff4844 100644 --- a/src/cstore/unionfs/cstore-unionfs.hpp +++ b/src/cstore/unionfs/cstore-unionfs.hpp @@ -158,6 +158,7 @@ private: bool mark_deactivated(); bool unmark_deactivated(); bool unmark_deactivated_descendants(); + bool unmark_changed_with_descendants(); bool mark_changed(); bool remove_comment(); bool set_comment(const string& comment); -- cgit v1.2.3 From 64ad04d23107e97832ec81c0cc32a95214196d6d Mon Sep 17 00:00:00 2001 From: An-Cheng Huang Date: Fri, 20 Aug 2010 13:42:12 -0700 Subject: remove workaround for "changed" status handling --- src/cstore/cstore.cpp | 17 +++++++++++++ src/cstore/unionfs/cstore-unionfs.cpp | 47 ++++++++--------------------------- 2 files changed, 27 insertions(+), 37 deletions(-) (limited to 'src') diff --git a/src/cstore/cstore.cpp b/src/cstore/cstore.cpp index 0634988..f146cb0 100644 --- a/src/cstore/cstore.cpp +++ b/src/cstore/cstore.cpp @@ -1763,6 +1763,23 @@ Cstore::unmarkCfgPathDeactivated(const vector& path_comps) * note: if a node is changed, all of its ancestors are also considered * changed. * return true if successful. otherwise return false. + * + * the original backend implementation only uses the "changed" marker at + * "root" to indicate whether the whole config has changed. for the rest + * of the config hierarchy, the original implementation treated all nodes + * that are present in the unionfs "changes only" directory as "changed". + * + * this worked until the introduction of "deactivate". since deactivated + * nodes are also present in the "changes only" directory, the backend + * treat them as "changed". on the other hand, deleted nodes don't appear + * 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. */ bool Cstore::markCfgPathChanged(const vector& path_comps) diff --git a/src/cstore/unionfs/cstore-unionfs.cpp b/src/cstore/unionfs/cstore-unionfs.cpp index 7f14483..46611ed 100644 --- a/src/cstore/unionfs/cstore-unionfs.cpp +++ b/src/cstore/unionfs/cstore-unionfs.cpp @@ -780,33 +780,16 @@ UnionfsCstore::unmark_changed_with_descendants() 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; - } + b_fs::path marker = get_work_path() / C_MARKER_CHANGED; + if (b_fs_exists(marker)) { + // already marked. treat as success. 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). - */ + 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; } @@ -883,18 +866,8 @@ UnionfsCstore::get_comment(string& comment, bool active_cfg) bool UnionfsCstore::marked_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. -- cgit v1.2.3 From f03db62d2de2f91f79a08aa9da53e264a5e69ddc Mon Sep 17 00:00:00 2001 From: An-Cheng Huang Date: Fri, 20 Aug 2010 13:51:17 -0700 Subject: mark nodes created by "default" as "changed" --- src/cstore/cstore.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src') diff --git a/src/cstore/cstore.cpp b/src/cstore/cstore.cpp index f146cb0..86a9455 100644 --- a/src/cstore/cstore.cpp +++ b/src/cstore/cstore.cpp @@ -2552,7 +2552,7 @@ Cstore::create_default_children() // has default value. set it. push_cfg_path(tcnodes[i]); if (!add_node() || !write_value(def.def_default) - || !mark_display_default()) { + || !mark_display_default() || !mark_changed()) { ret = false; } pop_cfg_path(); -- cgit v1.2.3 From e29b420868b312a1e3eaf52300002d3ba548481c Mon Sep 17 00:00:00 2001 From: An-Cheng Huang Date: Fri, 20 Aug 2010 19:15:21 -0700 Subject: don't remove the workaround yet * need to move changed status handling into the library. --- src/cstore/cstore.cpp | 2 +- src/cstore/unionfs/cstore-unionfs.cpp | 47 +++++++++++++++++++++++++++-------- 2 files changed, 38 insertions(+), 11 deletions(-) (limited to 'src') diff --git a/src/cstore/cstore.cpp b/src/cstore/cstore.cpp index 86a9455..f146cb0 100644 --- a/src/cstore/cstore.cpp +++ b/src/cstore/cstore.cpp @@ -2552,7 +2552,7 @@ Cstore::create_default_children() // has default value. set it. push_cfg_path(tcnodes[i]); if (!add_node() || !write_value(def.def_default) - || !mark_display_default() || !mark_changed()) { + || !mark_display_default()) { ret = false; } pop_cfg_path(); diff --git a/src/cstore/unionfs/cstore-unionfs.cpp b/src/cstore/unionfs/cstore-unionfs.cpp index 46611ed..7f14483 100644 --- a/src/cstore/unionfs/cstore-unionfs.cpp +++ b/src/cstore/unionfs/cstore-unionfs.cpp @@ -780,16 +780,33 @@ UnionfsCstore::unmark_changed_with_descendants() bool UnionfsCstore::mark_changed() { - b_fs::path marker = get_work_path() / C_MARKER_CHANGED; - if (b_fs_exists(marker)) { - // already marked. treat as success. + 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; } - if (!create_file(marker.file_string())) { - output_internal("failed to mark changed [%s]\n", - get_work_path().file_string().c_str()); - return false; - } + + /* 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; } @@ -866,8 +883,18 @@ UnionfsCstore::get_comment(string& comment, bool active_cfg) bool UnionfsCstore::marked_changed() { - b_fs::path marker = get_work_path() / C_MARKER_CHANGED; - return b_fs_exists(marker); + /* 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()); } /* XXX currently "committed marking" is done inside commit. -- cgit v1.2.3