On Tue, 2008-08-05 at 16:10 -0400, Matt McCutchen wrote: > On Tue, 2008-08-05 at 15:26 +0100, Eduardo Suárez wrote: > > Is it possible to add a rule like > > > > dir-merge myconf/mybackuprules > > > > ? > > > > That is, the file to merge is in a subdirectory of directory where we > > want the rules to apply. > > Rsync doesn't currently support that, but it would be a very reasonable > addition. After the change, a dir-merge path beginning with / or ../ > would request a scan of parent directories, while any other dir-merge > path would be saved as-is and followed from each directory to obtain > rules for that directory. > > It looks like the place to implement this is in parse_merge_name in > exclude.c. Simply changing the "if (fn != buf)" to also test for an > initial "../" makes the "dir-merge myconf/mybackuprules" example work. > However, parse_merge_name is called in three places, and the change > might break other uses.
I've been working on this; my patch in progress is attached. After two hours, I finally nailed down what should happen for each of the three calls to parse_merge_name, and I added a badly needed comment explaining it all. (This is what I mean when I say rsync is tangled!) A dir-merge file in a subdirectory seems to work, and I removed the obsolete no-slashes case from the top of parse_merge_name to fix a problem where rsync looked in the wrong place for an ordinary merge file specified in a per-dir merge file. However, in testing, I ran into a separate bug: rsync crashes when popping from a dir that defined a dir-merge rule that scanned a parent that defined another dir-merge rule. To reproduce: mkdir -p src/a/b/c echo ': ../rulesb' >src/a/b/c/rulesc echo ': rulesa' >src/a/b/rulesb rsync -r --list-only --filter=': rulesc' src/ When rsync pushes into c, it loads the ": ../rulesb" rule and performs a parent dirscan, which loads the ": rulesa" rule. But when rsync pops from c, clearing the rulesb list doesn't free the ": rulesa" rule because it is in the inherited part of the list. Thus, rsync tries to restore state for 2 per-dir merge files when it has state only for 1, hence the crash. Wayne, you could probably do a better job than I could of fixing this. Matt
diff --git a/exclude.c b/exclude.c
index 6eb83cb..5a98053 100644
--- a/exclude.c
+++ b/exclude.c
@@ -105,6 +105,8 @@ static int mergelist_size = 0;
static void free_filter(struct filter_struct *ex)
{
if (ex->match_flags & MATCHFLG_PERDIR_MERGE) {
+ rprintf(FINFO, "Freeing mergelist %s, mergelist_cnt becoming %d\n",
+ ex->u.mergelist->debug_type, mergelist_cnt - 1);
free(ex->u.mergelist->debug_type);
free(ex->u.mergelist);
mergelist_cnt--;
@@ -113,6 +115,15 @@ static void free_filter(struct filter_struct *ex)
free(ex);
}
+/* Helper for identifying duplicate per-dir merge rules. */
+static const char *perdir_merge_relative_part(const char *merge_file)
+{
+ if (merge_file[0] == '/' || strncmp(merge_file, "../", 3) == 0)
+ return strrchr(merge_file, '/') + 1; /* There is a slash. */
+ else
+ return merge_file;
+}
+
/* Build a filter structure given a filter pattern. The value in "pat"
* is not null-terminated. */
static void add_rule(struct filter_list_struct *listp, const char *pat,
@@ -210,26 +221,17 @@ static void add_rule(struct filter_list_struct *listp, const char *pat,
if (mflags & MATCHFLG_PERDIR_MERGE) {
struct filter_list_struct *lp;
- unsigned int len;
int i;
- if ((cp = strrchr(ret->pattern, '/')) != NULL)
- cp++;
- else
- cp = ret->pattern;
-
- /* If the local merge file was already mentioned, don't
- * add it again. */
+ /* If this per-dir merge file was already mentioned, don't
+ * add it again. Cancel .. components first to accurately
+ * determine whether it activates a parent_dirscan, in which
+ * case only the basename is significant. */
+ clean_fname(ret->pattern, CFN_COLLAPSE_DOT_DOT_DIRS);
+ cp = perdir_merge_relative_part(ret->pattern);
for (i = 0; i < mergelist_cnt; i++) {
struct filter_struct *ex = mergelist_parents[i];
- const char *s = strrchr(ex->pattern, '/');
- if (s)
- s++;
- else
- s = ex->pattern;
- len = strlen(s);
- if (len == pat_len - (cp - ret->pattern)
- && memcmp(s, cp, len) == 0) {
+ if (strcmp(cp, perdir_merge_relative_part(ex->pattern)) == 0) {
free_filter(ret);
return;
}
@@ -251,6 +253,7 @@ static void add_rule(struct filter_list_struct *listp, const char *pat,
out_of_memory("add_rule");
}
mergelist_parents[mergelist_cnt++] = ret;
+ rprintf(FINFO, "Added mergelist for %s, mergelist_cnt now %d\n", cp, mergelist_cnt);
} else
ret->u.slash_cnt = slash_cnt;
@@ -282,32 +285,50 @@ static void clear_filter_list(struct filter_list_struct *listp)
listp->head = listp->tail = NULL;
}
-/* This returns an expanded (absolute) filename for the merge-file name if
- * the name has any slashes in it OR if the parent_dirscan var is True;
- * otherwise it returns the original merge_file name. If the len_ptr value
- * is non-NULL the merge_file name is limited by the referenced length
- * value and will be updated with the length of the resulting name. We
- * always return a name that is null terminated, even if the merge_file
- * name was not. */
+/* parse_merge_name is called to process merge-file names in three cases:
+ *
+ * 1. When an non-per-dir merge rule is added.
+ * - Sanitize path using NULL (module_dir) and dirbuf_depth (which is 0 as
+ * desired if the rule came from initial filter processing rather than a
+ * per-dir merge file). If non-sanitizing, just cancel .. components.
+ * - If the path is relative, prepend dirbuf. During initial filter
+ * processing, dirbuf is empty, so this step has no effect and we will
+ * follow the path relative to the current directory, as desired. From a
+ * per-dir merge file, this step makes the path absolute so that we
+ * interpret it correctly.
+ * 2. When a dir-merge rule is set up (setup_merge_file), with dirbuf containing
+ * the topmost dir to which it will apply.
+ * - Sanitize path using NULL (module_dir) and dirbuf_depth. If non-
+ * sanitizing, just cancel .. components.
+ * - If the path begins with ../, then prepend the dirbuf, yielding an
+ * absolute path.
+ * - A path that is absolute at this point will activate the parent_dirscan
+ * in setup_merge_file, while a relative path (which contains no .. due to
+ * sanitization/cleaning) will be stored for following from each dir.
+ * 3. When a dir-merge rule is added during another parent_dirscan.
+ * - We want to interpret the rule relative to the ancestor dir in which it
+ * was defined, even though we don't actually push into that dir to set up
+ * the rule. Thus, we apply the same processing as in #2 while we have the
+ * desired dirbuf value, leaving an absolute or ..-free relative path that
+ * #2 won't modify further, except...
+ * - #2 will prepend the module_dir to an absolute path, so we are passed
+ * prefix_skip == module_dirlen to avoid prepending a duplicate module_dir
+ * now.
+ *
+ * When this is called on a rule addition (#1 or #3), merge_file is not null-
+ * terminated, but len_ptr points to its length and is updated with the length
+ * of the result. In all cases, the result is null-terminated. */
static char *parse_merge_name(const char *merge_file, unsigned int *len_ptr,
- unsigned int prefix_skip)
+ unsigned int prefix_skip, BOOL is_perdir)
{
static char buf[MAXPATHLEN];
char *fn, tmpbuf[MAXPATHLEN];
unsigned int fn_len;
- if (!parent_dirscan && *merge_file != '/') {
- /* Return the name unchanged it doesn't have any slashes. */
- if (len_ptr) {
- const char *p = merge_file + *len_ptr;
- while (--p > merge_file && *p != '/') {}
- if (p == merge_file) {
- strlcpy(buf, merge_file, *len_ptr + 1);
- return buf;
- }
- } else if (strchr(merge_file, '/') == NULL)
- return (char *)merge_file;
- }
+ if (len_ptr)
+ rprintf(FINFO, "parse_merge_name: merge_file %.*s (length %d)\n", *len_ptr, merge_file, *len_ptr);
+ else
+ rprintf(FINFO, "parse_merge_name: merge_file %s\n", merge_file);
fn = *merge_file == '/' ? buf : tmpbuf;
if (sanitize_paths) {
@@ -329,8 +350,11 @@ static char *parse_merge_name(const char *merge_file, unsigned int *len_ptr,
fn_len = clean_fname(fn, CFN_COLLAPSE_DOT_DOT_DIRS);
}
- /* If the name isn't in buf yet, it's wasn't absolute. */
- if (fn != buf) {
+ /* If the name isn't in buf yet, it's relative. In case #1, we want to
+ * prepend the dirbuf unconditionally. Otherwise, we want to make
+ * parent_dirscan-activating paths absolute but leave others alone. */
+ if (fn != buf && (!is_perdir || strncmp(fn, "../", 3) == 0)) {
+ rprintf(FINFO, "Prepending dirbuf (len %d), skipping %d\n", dirbuf_len, prefix_skip);
int d_len = dirbuf_len - prefix_skip;
if (d_len + fn_len >= MAXPATHLEN) {
rprintf(FERROR, "merge-file name overflows: %s\n", fn);
@@ -338,12 +362,14 @@ static char *parse_merge_name(const char *merge_file, unsigned int *len_ptr,
}
memcpy(buf, dirbuf + prefix_skip, d_len);
memcpy(buf + d_len, fn, fn_len + 1);
+ fn = buf;
fn_len = clean_fname(buf, CFN_COLLAPSE_DOT_DOT_DIRS);
}
if (len_ptr)
*len_ptr = fn_len;
- return buf;
+ rprintf(FINFO, "parse_merge_name returning %s\n", fn);
+ return fn;
}
/* Sets the dirbuf and dirbuf_len values. */
@@ -383,7 +409,8 @@ static BOOL setup_merge_file(struct filter_struct *ex,
char *x, *y, *pat = ex->pattern;
unsigned int len;
- if (!(x = parse_merge_name(pat, NULL, 0)) || *x != '/')
+ /* Case #2 in parse_merge_name comment */
+ if (!(x = parse_merge_name(pat, NULL, 0, True)) || *x != '/')
return 0;
y = strrchr(x, '/');
@@ -443,6 +470,7 @@ void *push_local_filters(const char *dir, unsigned int dirlen)
if (!push)
out_of_memory("push_local_filters");
+ rprintf(FINFO, "saving per-dir merge state to %p\n", push);
for (i = 0, ap = push; i < mergelist_cnt; i++) {
memcpy(ap++, mergelist_parents[i]->u.mergelist,
sizeof (struct filter_list_struct));
@@ -455,8 +483,8 @@ void *push_local_filters(const char *dir, unsigned int dirlen)
struct filter_list_struct *lp = ex->u.mergelist;
if (DEBUG_GTE(FILTER, 2)) {
- rprintf(FINFO, "[%s] pushing filter list%s\n",
- who_am_i(), lp->debug_type);
+ rprintf(FINFO, "[%s] pushing filter list %p for %s%s\n",
+ who_am_i(), lp, dirbuf, lp->debug_type);
}
lp->tail = NULL; /* Switch any local rules to inherited. */
@@ -495,8 +523,8 @@ void pop_local_filters(void *mem)
struct filter_list_struct *lp = ex->u.mergelist;
if (DEBUG_GTE(FILTER, 2)) {
- rprintf(FINFO, "[%s] popping filter list%s\n",
- who_am_i(), lp->debug_type);
+ rprintf(FINFO, "[%s] popping filter list %p%s\n",
+ who_am_i(), lp, lp->debug_type);
}
clear_filter_list(lp);
@@ -505,6 +533,7 @@ void pop_local_filters(void *mem)
if (!pop)
return;
+ rprintf(FINFO, "restoring per-dir merge state from %p\n", pop);
for (i = 0, ap = pop; i < mergelist_cnt; i++) {
memcpy(mergelist_parents[i]->u.mergelist, ap++,
sizeof (struct filter_list_struct));
@@ -998,14 +1027,16 @@ void parse_rule(struct filter_list_struct *listp, const char *pattern,
}
if (new_mflags & MATCHFLG_PERDIR_MERGE) {
if (parent_dirscan) {
+ /* Case #3 in parse_merge_name comment */
if (!(p = parse_merge_name(cp, &len,
- module_dirlen)))
+ module_dirlen, True)))
continue;
add_rule(listp, p, len, new_mflags, 0);
continue;
}
} else {
- if (!(p = parse_merge_name(cp, &len, 0)))
+ /* Case #1 in parse_merge_name comment */
+ if (!(p = parse_merge_name(cp, &len, 0, False)))
continue;
parse_filter_file(listp, p, new_mflags,
XFLG_FATAL_ERRORS);
signature.asc
Description: This is a digitally signed message part
-- Please use reply-all for most replies to avoid omitting the mailing list. To unsubscribe or change options: https://lists.samba.org/mailman/listinfo/rsync Before posting, read: http://www.catb.org/~esr/faqs/smart-questions.html
