From 5ee8163051be8214507c13c86171ac90ca7cb91f Mon Sep 17 00:00:00 2001 From: David Lutterkort Date: Tue, 29 Jun 2010 15:32:44 -0700 Subject: [PATCH 9/9] Avoid unnecessary file parsing when reloading the tree We used to reparse every file we knew about upon aug_load. Now, we only reparse files if the file has changed on disk. We test a few scenarios to make sure aug_load retains its behavior of obliterating the tree and filling it with the latest from disk. This includes throwing away unsaved changes or trees that have been deleted. --- src/augeas.c | 72 ++++++++++++++++++++++++++- src/transform.c | 78 +++++++++++++++++++++++++++-- tests/cutest.c | 17 ++++++ tests/cutest.h | 4 ++ tests/test-load.c | 142 +++++++++++++++++++++++++++++++++++++++++++++++++++++ tests/xpath.tests | 1 + 6 files changed, 307 insertions(+), 7 deletions(-) diff --git a/src/augeas.c b/src/augeas.c index 744b699..45c2207 100644 --- a/src/augeas.c +++ b/src/augeas.c @@ -451,6 +451,51 @@ void tree_unlink_children(struct augeas *aug, struct tree *tree) { tree_unlink(tree->children); } +static void tree_mark_files(struct tree *tree) { + if (tree_child(tree, "path") != NULL) { + tree_mark_dirty(tree); + } else { + list_for_each(c, tree->children) { + tree_mark_files(c); + } + } +} + +static void tree_rm_dirty_files(struct augeas *aug, struct tree *tree) { + struct tree *p; + + if (!tree->dirty) + return; + + if ((p = tree_child(tree, "path")) != NULL) { + aug_rm(aug, p->value); + tree_unlink(tree); + } else { + struct tree *c = tree->children; + while (c != NULL) { + struct tree *next = c->next; + tree_rm_dirty_files(aug, c); + c = next; + } + } +} + +static void tree_rm_dirty_leaves(struct augeas *aug, struct tree *tree, + struct tree *protect) { + if (! tree->dirty) + return; + + struct tree *c = tree->children; + while (c != NULL) { + struct tree *next = c->next; + tree_rm_dirty_leaves(aug, c, protect); + c = next; + } + + if (tree != protect && tree->children == NULL) + tree_unlink(tree); +} + int aug_load(struct augeas *aug) { struct tree *meta = tree_child_cr(aug->origin, s_augeas); struct tree *meta_files = tree_child_cr(meta, s_files); @@ -462,13 +507,36 @@ int aug_load(struct augeas *aug) { ERR_NOMEM(load == NULL, aug); - tree_unlink_children(aug, meta_files); - tree_unlink_children(aug, files); + /* To avoid unnecessary loads of files, we reload an existing file in + * several steps: + * (1) mark all file nodes under /augeas/files as dirty (and only those) + * (2) process all files matched by a lens; we check (in + * transform_load) if the file has been modified. If it has, we + * reparse it. Either way, we clear the dirty flag. We also need to + * reread the file if part or all of it has been modified in the + * tree but not been saved yet + * (3) remove all files from the tree that still have a dirty entry + * under /augeas/files. Those files are not processed by any lens + * anymore + * (4) Remove entries from /augeas/files and /files that correspond + * to directories without any files of interest + */ + tree_clean(meta_files); + tree_mark_files(meta_files); list_for_each(xfm, load->children) { if (transform_validate(aug, xfm) == 0) transform_load(aug, xfm); } + + /* This makes it possible to spot 'directories' that are now empty + * because we removed their file contents */ + tree_clean(files); + + tree_rm_dirty_files(aug, meta_files); + tree_rm_dirty_leaves(aug, meta_files, meta_files); + tree_rm_dirty_leaves(aug, files, files); + tree_clean(aug->origin); list_for_each(v, vars->children) { diff --git a/src/transform.c b/src/transform.c index 0c56034..00552da 100644 --- a/src/transform.c +++ b/src/transform.c @@ -49,6 +49,7 @@ static const int glob_flags = GLOB_NOSORT; /* Loaded files are tracked underneath METATREE. When a file with name * FNAME is loaded, certain entries are made under METATREE / FNAME: * path : path where tree for FNAME is put + * mtime : time of last modification of the file as reported by stat(2) * lens/info : information about where the applied lens was loaded from * lens/id : unique hexadecimal id of the lens * error : indication of errors during processing FNAME, or NULL @@ -60,6 +61,7 @@ static const int glob_flags = GLOB_NOSORT; static const char *const s_path = "path"; static const char *const s_lens = "lens"; static const char *const s_info = "info"; +static const char *const s_mtime = "mtime"; static const char *const s_error = "error"; /* These are all put underneath "error" */ @@ -111,6 +113,59 @@ static bool is_regular_file(const char *path) { return S_ISREG(st.st_mode); } +static char *mtime_as_string(struct augeas *aug, const char *fname) { + int r; + struct stat st; + char *result = NULL; + + r = stat(fname, &st); + if (r < 0) { + /* If we fail to stat, silently ignore the error + * and report an impossible mtime */ + result = strdup("0"); + ERR_NOMEM(result == NULL, aug); + } else { + r = xasprintf(&result, "%ld", (long) st.st_mtime); + ERR_NOMEM(r < 0, aug); + } + return result; + error: + FREE(result); + return NULL; +} + +static bool file_current(struct augeas *aug, const char *fname, + struct tree *finfo) { + struct tree *mtime = tree_child(finfo, s_mtime); + struct tree *file = NULL, *path = NULL; + int r; + struct stat st; + int64_t mtime_i; + + if (mtime == NULL || mtime->value == NULL) + return false; + + r = xstrtoint64(mtime->value, 10, &mtime_i); + if (r < 0) { + /* Ignore silently and err on the side of caution */ + return false; + } + + r = stat(fname, &st); + if (r < 0) + return false; + + if (mtime_i != (int64_t) st.st_mtime) + return false; + + path = tree_child(finfo, s_path); + if (path == NULL) + return false; + + file = tree_find(aug, path->value); + return (file != NULL && ! file->dirty); +} + static int filter_generate(struct tree *xfm, const char *root, int *nmatches, char ***matches) { glob_t globbuf; @@ -326,7 +381,8 @@ static int store_error(struct augeas *aug, * Returns 0 on success, -1 on error */ static int add_file_info(struct augeas *aug, const char *node, - struct lens *lens, const char *lens_name) { + struct lens *lens, const char *lens_name, + const char *filename) { struct tree *file, *tree; char *tmp = NULL; int r; @@ -348,6 +404,13 @@ static int add_file_info(struct augeas *aug, const char *node, r = tree_set_value(tree, node); ERR_NOMEM(r < 0, aug); + /* Set 'mtime' */ + tmp = mtime_as_string(aug, filename); + ERR_BAIL(aug); + tree = tree_child_cr(file, s_mtime); + ERR_NOMEM(tree == NULL, aug); + tree_store_value(tree, &tmp); + /* Set 'lens/info' */ tmp = format_info(lens->info); ERR_NOMEM(tmp == NULL, aug); @@ -362,6 +425,8 @@ static int add_file_info(struct augeas *aug, const char *node, r = tree_set_value(tree, lens_name); ERR_NOMEM(r < 0, aug); + tree_clean(file); + result = 0; error: free(path); @@ -404,7 +469,7 @@ static int load_file(struct augeas *aug, struct lens *lens, path = file_name_path(aug, filename); ERR_NOMEM(path == NULL, aug); - r = add_file_info(aug, path, lens, lens_name); + r = add_file_info(aug, path, lens, lens_name, filename); if (r < 0) goto done; @@ -602,7 +667,8 @@ int transform_load(struct augeas *aug, struct tree *xfm) { for (int i=0; i < nmatches; i++) { const char *filename = matches[i] + strlen(aug->root) - 1; struct tree *finfo = file_info(aug, filename); - if (finfo != NULL && tree_child(finfo, s_lens) != NULL) { + if (finfo != NULL && !finfo->dirty && + tree_child(finfo, s_lens) != NULL) { const char *s = xfm_lens_name(finfo); char *fpath = file_name_path(aug, matches[i]); transform_file_error(aug, "mxfm_load", filename, @@ -610,9 +676,11 @@ int transform_load(struct augeas *aug, struct tree *xfm) { s, lens_name); aug_rm(aug, fpath); free(fpath); - } else { + } else if (!file_current(aug, matches[i], finfo)) { load_file(aug, lens, lens_name, matches[i]); } + if (finfo != NULL) + finfo->dirty = 0; FREE(matches[i]); } lens_release(lens); @@ -945,7 +1013,7 @@ int transform_save(struct augeas *aug, struct tree *xfm, result = 1; done: - r = add_file_info(aug, path, lens, lens_name); + r = add_file_info(aug, path, lens, lens_name, filename); if (r < 0) { err_status = "file_info"; result = -1; diff --git a/tests/cutest.c b/tests/cutest.c index 120e31c..08dbfd7 100644 --- a/tests/cutest.c +++ b/tests/cutest.c @@ -138,6 +138,23 @@ void CuAssertStrEquals_LineMsg(CuTest* tc, const char* file, int line, CuFailInternal(tc, file, line, string); } +void CuAssertStrNotEqual_LineMsg(CuTest* tc, const char* file, int line, + const char* message, + const char* expected, const char* actual) { + char *string = NULL; + + if (expected != NULL && actual != NULL && strcmp(expected, actual) != 0) + return; + + if (message != NULL) { + asprintf_or_die(&string, "%s: expected <%s> but was <%s>", message, + expected, actual); + } else { + asprintf_or_die(&string, "expected <%s> but was <%s>", expected, actual); + } + CuFailInternal(tc, file, line, string); +} + void CuAssertIntEquals_LineMsg(CuTest* tc, const char* file, int line, const char* message, int expected, int actual) { diff --git a/tests/cutest.h b/tests/cutest.h index a667e50..616fb1f 100644 --- a/tests/cutest.h +++ b/tests/cutest.h @@ -61,6 +61,9 @@ void CuAssert_Line(CuTest* tc, const char* file, int line, const char* message, void CuAssertStrEquals_LineMsg(CuTest* tc, const char* file, int line, const char* message, const char* expected, const char* actual); +void CuAssertStrNotEqual_LineMsg(CuTest* tc, + const char* file, int line, const char* message, + const char* expected, const char* actual); void CuAssertIntEquals_LineMsg(CuTest* tc, const char* file, int line, const char* message, int expected, int actual); @@ -82,6 +85,7 @@ void CuAssertPtrNotEqual_LineMsg(CuTest* tc, #define CuAssertStrEquals(tc,ex,ac) CuAssertStrEquals_LineMsg((tc),__FILE__,__LINE__,NULL,(ex),(ac)) #define CuAssertStrEquals_Msg(tc,ms,ex,ac) CuAssertStrEquals_LineMsg((tc),__FILE__,__LINE__,(ms),(ex),(ac)) +#define CuAssertStrNotEqual(tc,ex,ac) CuAssertStrNotEqual_LineMsg((tc),__FILE__,__LINE__,NULL,(ex),(ac)) #define CuAssertIntEquals(tc,ex,ac) CuAssertIntEquals_LineMsg((tc),__FILE__,__LINE__,NULL,(ex),(ac)) #define CuAssertIntEquals_Msg(tc,ms,ex,ac) CuAssertIntEquals_LineMsg((tc),__FILE__,__LINE__,(ms),(ex),(ac)) #define CuAssertDblEquals(tc,ex,ac,dl) CuAssertDblEquals_LineMsg((tc),__FILE__,__LINE__,NULL,(ex),(ac),(dl)) diff --git a/tests/test-load.c b/tests/test-load.c index a8107a1..fe92305 100644 --- a/tests/test-load.c +++ b/tests/test-load.c @@ -70,6 +70,8 @@ static struct augeas *setup_writable_hosts(CuTest *tc) { r = aug_set(aug, "/augeas/load/Hosts/incl", "/etc/hosts"); CuAssertRetSuccess(tc, r); + free(build_root); + free(etcdir); return aug; } @@ -262,6 +264,142 @@ static void testDefvarExpr(CuTest *tc) { aug_close(aug); } +static void testReloadChanged(CuTest *tc) { + FILE *fp; + augeas *aug = NULL; + const char *build_root, *mtime2, *s; + char *mtime1; + char *hosts = NULL; + int r; + + aug = setup_writable_hosts(tc); + + r = aug_load(aug); + CuAssertRetSuccess(tc, r); + + r = aug_get(aug, "/augeas/root", &build_root); + CuAssertIntEquals(tc, 1, r); + + r = aug_get(aug, "/augeas/files/etc/hosts/mtime", &s); + CuAssertIntEquals(tc, 1, r); + mtime1 = strdup(s); + CuAssertPtrNotNull(tc, mtime1); + + /* Tickle /etc/hosts behind augeas' back */ + r = asprintf(&hosts, "%setc/hosts", build_root); + CuAssertPositive(tc, r); + + fp = fopen(hosts, "a"); + CuAssertPtrNotNull(tc, fp); + + r = fprintf(fp, "192.168.0.1 other.example.com\n"); + CuAssertTrue(tc, r > 0); + + r = fclose(fp); + CuAssertRetSuccess(tc, r); + + /* Unsaved changes are discarded */ + r = aug_set(aug, "/files/etc/hosts/1/ipaddr", "127.0.0.2"); + CuAssertRetSuccess(tc, r); + + /* Check that we really did load the right file*/ + r = aug_load(aug); + CuAssertRetSuccess(tc, r); + + r = aug_get(aug, "/augeas/files/etc/hosts/mtime", &mtime2); + CuAssertIntEquals(tc, 1, r); + CuAssertStrNotEqual(tc, mtime1, mtime2); + + r = aug_match(aug, "/files/etc/hosts/*[ipaddr = '192.168.0.1']", NULL); + CuAssertIntEquals(tc, 1, r); + + r = aug_match(aug, "/files/etc/hosts/1[ipaddr = '127.0.0.1']", NULL); + CuAssertIntEquals(tc, 1, r); + + free(mtime1); + free(hosts); + aug_close(aug); +} + +static void testReloadDirty(CuTest *tc) { + augeas *aug = NULL; + int r; + + aug = setup_writable_hosts(tc); + + r = aug_load(aug); + CuAssertRetSuccess(tc, r); + + /* Unsaved changes are discarded */ + r = aug_set(aug, "/files/etc/hosts/1/ipaddr", "127.0.0.2"); + CuAssertRetSuccess(tc, r); + + r = aug_load(aug); + CuAssertRetSuccess(tc, r); + + r = aug_match(aug, "/files/etc/hosts/1[ipaddr = '127.0.0.1']", NULL); + CuAssertIntEquals(tc, 1, r); + + aug_close(aug); +} + +static void testReloadDeleted(CuTest *tc) { + augeas *aug = NULL; + int r; + + aug = setup_writable_hosts(tc); + + r = aug_load(aug); + CuAssertRetSuccess(tc, r); + + /* A missing file causes a reload */ + r = aug_rm(aug, "/files/etc/hosts"); + CuAssertPositive(tc, r); + + r = aug_load(aug); + CuAssertRetSuccess(tc, r); + + r = aug_match(aug, "/files/etc/hosts/1[ipaddr = '127.0.0.1']", NULL); + CuAssertIntEquals(tc, 1, r); + + /* A missing entry in a file causes a reload */ + r = aug_rm(aug, "/files/etc/hosts/1/ipaddr"); + CuAssertPositive(tc, r); + + r = aug_load(aug); + CuAssertRetSuccess(tc, r); + + r = aug_match(aug, "/files/etc/hosts/1[ipaddr = '127.0.0.1']", NULL); + CuAssertIntEquals(tc, 1, r); + + aug_close(aug); +} + +static void testReloadDeletedMeta(CuTest *tc) { + augeas *aug = NULL; + int r; + + aug = setup_writable_hosts(tc); + + r = aug_load(aug); + CuAssertRetSuccess(tc, r); + + /* Unsaved changes are discarded */ + r = aug_rm(aug, "/augeas/files/etc/hosts"); + CuAssertPositive(tc, r); + + r = aug_set(aug, "/files/etc/hosts/1/ipaddr", "127.0.0.2"); + CuAssertRetSuccess(tc, r); + + r = aug_load(aug); + CuAssertRetSuccess(tc, r); + + r = aug_match(aug, "/files/etc/hosts/1[ipaddr = '127.0.0.1']", NULL); + CuAssertIntEquals(tc, 1, r); + + aug_close(aug); +} + int main(void) { char *output = NULL; CuSuite* suite = CuSuiteNew(); @@ -274,6 +412,10 @@ int main(void) { SUITE_ADD_TEST(suite, testLoadSave); SUITE_ADD_TEST(suite, testLoadDefined); SUITE_ADD_TEST(suite, testDefvarExpr); + SUITE_ADD_TEST(suite, testReloadChanged); + SUITE_ADD_TEST(suite, testReloadDirty); + SUITE_ADD_TEST(suite, testReloadDeleted); + SUITE_ADD_TEST(suite, testReloadDeletedMeta); abs_top_srcdir = getenv("abs_top_srcdir"); if (abs_top_srcdir == NULL) diff --git a/tests/xpath.tests b/tests/xpath.tests index f2575a3..b16792a 100644 --- a/tests/xpath.tests +++ b/tests/xpath.tests @@ -148,6 +148,7 @@ test ipaddr-sibling //*[../ipaddr] test lircd-ancestor //*[ancestor::kudzu][label() != '#comment'] /augeas/files/etc/sysconfig/kudzu/path = /files/etc/sysconfig/kudzu + /augeas/files/etc/sysconfig/kudzu/mtime = ... /augeas/files/etc/sysconfig/kudzu/lens = @Shellvars /augeas/files/etc/sysconfig/kudzu/lens/info = ... /files/etc/sysconfig/kudzu/SAFE = no -- 1.6.6.1