Any comments or suggestions?
On 2020-27-11 10:00 -0600, Aaron Poffenberger <[email protected]> wrote:
> According to rdist(1), the special and cmdspecial commands accept an
> optional list of files. When the <name list> is omitted, the command
> should execute for all files, otherwise it should be executed only
> when one of the listed files is affected.
>
> The special command works as expected, but cmdspecial runs for all
> files regardless of <name list>.
>
> The diff fixes cmdspecial by adding "sc_updfilelist" to the subcmd
> structure to hold a list of affected files found in the <name list>
> (sc_args). For cmdspecial commands without a <name list>, the module
> level variable "updfilelist" is used as before.
>
> The diff also addresses a couple of related issues, which aren't
> technically necessary for the fix, but which helped with debugging and
> testing, and make rdist user messages clearer. I can break them out
> as separate patches if necessary.
>
> Changes:
> - Fix <name list> handling in cmdspecial commands
> - Modify user message to indicate whether special or cmdspecial
> commands apply to "any" file or a "list" of files
> - Show cmdspecial commands that would be executed when the "verify"
> option is set (rdist shows all other actions, including special
> commands, when "verify" is enabled)
>
>
> Test Script
> -----------
> I've included a shell script that demonstrates the <name list>
> problem. It's attached as a diff for ease of use. I'm not suggesting
> it be committed.
>
> The script creates a Distfile with special and cmdspecial commands
> with both a <name list> and no <name list> specified to highlight
> the difference in handling by rdist.
>
> The script runs the base-installed rdist twice, the first time with
> two changed files, the second with just one. In both cases it executes
> a simple sh script to log the env variable executed with the command
> (special or cmdspecial).
>
> If the script finds an executable rdist in /usr/src/usr.bin/rdist/, it
> then runs that version with the same two test cases.
>
> The script then displays a diff of the two logs showing that the
> patched rdist executes according to the documentation, otherwise it
> shows the output of the first execution to show the defective handling
> of cmdspecial.
>
> --Aaron
>
> cvs diff: Diffing .
> Index: client.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/rdist/client.c,v
> retrieving revision 1.37
> diff -u -p -r1.37 client.c
> --- client.c 28 Jun 2019 13:35:03 -0000 1.37
> +++ client.c 27 Nov 2020 14:45:16 -0000
> @@ -66,7 +66,7 @@ struct namelist *updfilelist = NULL; /*
>
> static void runspecial(char *, opt_t, char *, int);
> static void addcmdspecialfile(char *, char *, int);
> -static void freecmdspecialfiles(void);
> +static void freecmdspecialfiles(struct namelist **);
> static struct linkbuf *linkinfo(struct stat *);
> static int sendhardlink(opt_t, struct linkbuf *, char *, int);
> static int sendfile(char *, opt_t, struct stat *, char *, char *, int);
> @@ -172,7 +172,8 @@ runspecial(char *starget, opt_t opts, ch
> continue;
> if (sc->sc_args != NULL && !inlist(sc->sc_args, starget))
> continue;
> - message(MT_CHANGE, "special \"%s\"", sc->sc_name);
> + message(MT_CHANGE, "special <%s> \"%s\"",
> + sc->sc_args == NULL ? "any" : "list", sc->sc_name);
> if (IS_ON(opts, DO_VERIFY))
> continue;
> (void) sendcmd(C_SPECIAL,
> @@ -201,11 +202,19 @@ addcmdspecialfile(char *starget, char *r
>
> rfile = remfilename(source, Tdest, target, rname, destdir);
>
> - for (sc = subcmds; sc != NULL && !isokay; sc = sc->sc_next) {
> + for (sc = subcmds; sc != NULL; sc = sc->sc_next) {
> if (sc->sc_type != CMDSPECIAL)
> continue;
> - if (sc->sc_args != NULL && !inlist(sc->sc_args, starget))
> + if (sc->sc_args == NULL) {
> + isokay = TRUE;
> continue;
> + } else if (!inlist(sc->sc_args, starget))
> + continue;
> + new = xmalloc(sizeof *new);
> + new->n_name = xstrdup(rfile);
> + new->n_regex = NULL;
> + new->n_next = sc->sc_updfilelist;
> + sc->sc_updfilelist = new;
> isokay = TRUE;
> }
>
> @@ -222,11 +231,11 @@ addcmdspecialfile(char *starget, char *r
> * Free the file list
> */
> static void
> -freecmdspecialfiles(void)
> +freecmdspecialfiles(struct namelist **list)
> {
> struct namelist *ptr, *save;
>
> - for (ptr = updfilelist; ptr; ) {
> + for (ptr = *list; ptr; ) {
> if (ptr->n_name) (void) free(ptr->n_name);
> save = ptr->n_next;
> (void) free(ptr);
> @@ -235,7 +244,7 @@ freecmdspecialfiles(void)
> else
> ptr = NULL;
> }
> - updfilelist = NULL;
> + *list = NULL;
> }
>
> /*
> @@ -251,11 +260,15 @@ runcmdspecial(struct cmd *cmd, opt_t opt
> for (sc = cmd->c_cmds; sc != NULL; sc = sc->sc_next) {
> if (sc->sc_type != CMDSPECIAL)
> continue;
> - message(MT_CHANGE, "cmdspecial \"%s\"", sc->sc_name);
> + if (sc->sc_args != NULL && sc->sc_updfilelist == NULL)
> + continue;
> + message(MT_CHANGE, "cmdspecial <%s> \"%s\"",
> + sc->sc_args == NULL ? "any" : "list", sc->sc_name);
> if (IS_ON(opts, DO_VERIFY))
> continue;
> /* Send all the file names */
> - for (f = updfilelist; f != NULL; f = f->n_next) {
> + f = sc->sc_updfilelist != NULL ? sc->sc_updfilelist :
> updfilelist;
> + for (; f != NULL; f = f->n_next) {
> if (first) {
> (void) sendcmd(C_CMDSPECIAL, NULL);
> if (response() < 0)
> @@ -277,8 +290,9 @@ runcmdspecial(struct cmd *cmd, opt_t opt
> while (response() > 0)
> ;
> first = TRUE; /* Reset in case there are more CMDSPECIAL's */
> + freecmdspecialfiles(&sc->sc_updfilelist);
> }
> - freecmdspecialfiles();
> + freecmdspecialfiles(&updfilelist);
> }
>
> /*
> @@ -1050,6 +1064,7 @@ statupdate(int u, char *starget, opt_t o
> "%s: need to change to perm %#04o, owner %s,
> group %s",
> starget, lmode, user, group);
> runspecial(starget, opts, rname, destdir);
> + addcmdspecialfile(starget, rname, destdir);
> }
> else {
> message(MT_CHANGE, "%s: change to perm %#04o, owner %s,
> group %s",
> @@ -1079,6 +1094,7 @@ fullupdate(int u, char *starget, opt_t o
> if (IS_ON(opts, DO_VERIFY)) {
> message(MT_INFO, "%s: need to install", starget);
> runspecial(starget, opts, rname, destdir);
> + addcmdspecialfile(starget, rname, destdir);
> return(1);
> }
> if (!IS_ON(opts, DO_QUIET))
> @@ -1110,6 +1126,7 @@ fullupdate(int u, char *starget, opt_t o
> if (IS_ON(opts, DO_VERIFY)) {
> message(MT_INFO, "%s: need to update", starget);
> runspecial(starget, opts, rname, destdir);
> + addcmdspecialfile(starget, rname, destdir);
> return(1);
> }
> if (!IS_ON(opts, DO_QUIET))
> Index: client.h
> ===================================================================
> RCS file: /cvs/src/usr.bin/rdist/client.h,v
> retrieving revision 1.3
> diff -u -p -r1.3 client.h
> --- client.h 30 Aug 2017 07:42:52 -0000 1.3
> +++ client.h 26 Nov 2020 18:14:08 -0000
> @@ -84,6 +84,7 @@ struct subcmd {
> opt_t sc_options;
> char *sc_name;
> struct namelist *sc_args;
> + struct namelist *sc_updfilelist;
> struct subcmd *sc_next;
> };
>
> Index: gram.y
> ===================================================================
> RCS file: /cvs/src/usr.bin/rdist/gram.y,v
> retrieving revision 1.12
> diff -u -p -r1.12 gram.y
> --- gram.y 20 Jan 2015 09:00:16 -0000 1.12
> +++ gram.y 27 Nov 2020 15:21:31 -0000
> @@ -592,6 +592,7 @@ makesubcmd(int type)
> sc->sc_type = type;
> sc->sc_args = NULL;
> sc->sc_next = NULL;
> + sc->sc_updfilelist = NULL;
> sc->sc_name = NULL;
>
> return(sc);
> Index: test_cmdspecial
> ===================================================================
> RCS file: test_cmdspecial
> diff -N test_cmdspecial
> --- /dev/null 1 Jan 1970 00:00:00 -0000
> +++ test_cmdspecial 27 Nov 2020 15:44:43 -0000
> @@ -0,0 +1,128 @@
> +#!/bin/sh
> +
> +TEST_DIR=/tmp/rdist
> +rm -Rf $TEST_DIR*
> +mkdir -p "$TEST_DIR/src" "$TEST_DIR/tgt"
> +
> +if [ -x ./rdist ]; then
> + rdist_p="$(pwd)/rdist"
> +elif [ -x /usr/src/usr.bin/rdist/rdist ]; then
> + rdist_p="/usr/src/usr.bin/rdist/rdist"
> +fi
> +
> +cd "$TEST_DIR"
> +
> +# Create Distfile
> +cat <<- EOF > Distfile
> +test:
> +src -> localhost
> + install tgt ;
> + special src/file "echo special src/file env_REMFILE=\$REMFILE >> \
> +rdist.log" ;
> + special "echo special any env_REMFILE=\$REMFILE >> rdist.log" ;
> + cmdspecial src/file "echo cmdspecial src/file env_FILES=\$FILES >> \
> +rdist.log" ;
> + cmdspecial "echo cmdspecial any env_FILES=\$FILES >> rdist.log" ;
> +EOF
> +
> +# Run unpatched rdist with both files modified
> +# Both special and cmdspecial commands will execute
> +# NOTE: the FILES variable for cmdspecial *with* a <name list>
> +# will include the names of both files when it should
> +# only include the affected files in the <name list>
> +
> +echo ""
> +#echo "Unpatched rdist - update both files:" >> rdist.log
> +touch -d 2020-01-01T08:00:00 src/file{,2}
> +/usr/bin/rdist > /dev/null
> +mv rdist{,_orgnl_both_files}.log
> +
> +
> +# Run unpatched rdist with only file2 modified
> +# The special command with no <name list> will execute as it should
> +# The special command *with* a <name list> will not
> +# NOTE: Both cmdspecial commands will execute
> +# The cmdspecial command *with* a <name list> should
> +# not execute!
> +
> +#echo "Unpatched rdist - update file2 only:" >> rdist.log
> +touch -d 2020-01-01T09:00:00 src/file2
> +/usr/bin/rdist > /dev/null
> +mv rdist{,_orgnl_file2_only}.log
> +
> +
> +if [ -n "$rdist_p" ]; then
> + rm src/* tgt/*
> +
> + # Run unpatched rdist with both files modified
> + # Both special and cmdspecial commands will execute
> + # NOTE: the FILES variable for the cmdspecial *with* a
> + # <name list> will include only the specified file!
> +
> + touch -d 2020-01-01T08:00:00 src/file{,2}
> + "$rdist_p" > /dev/null
> + mv rdist{,_patch_both_files}.log
> +
> +
> + # Run unpatched rdist with only file2 modified
> + # The special command with no <name list> executes as expected
> + # The special command *with* a <name list> will not
> + # NOTE: Only the cmdspecial command *without* a <name list>
> + # will execute, as it should
> +
> + touch -d 2020-01-01T09:00:00 src/file2
> + "$rdist_p" > /dev/null
> + mv rdist{,_patch_file2_only}.log
> +
> + cat <<- EOF
> + Patched rdist: $rdist_p
> +
> + Diff logs when both files are affected
> + --------------------------------------
> + rdist_orgnl_both_files.log:
> + - The FILES variable for cmdspecial *with*
> + a specified <name list> shows both files!
> + rdist_patch_both_files.log:
> + - FILES shows only the expected file.
> +
> + EOF
> + diff -u "$TEST_DIR/rdist_orgnl_both_files.log" \
> + "$TEST_DIR/rdist_patch_both_files.log"
> +
> + cat <<- EOF
> +
> + Diff logs when only one file is affected
> + ----------------------------------------
> + rdist_orgnl_file2_only.log:
> + - The FILES variable for the cmdspecial *with*
> + "src/file" in the <name list> still runs!
> + rdist_patch_file2_only.log:
> + - The cmdspecial doesn't run, as expected.
> +
> + EOF
> + diff -u "$TEST_DIR/rdist_orgnl_file2_only.log" \
> + "$TEST_DIR/rdist_patch_file2_only.log"
> +else
> + cat <<- EOF
> + Patched rdist not found
> +
> + Log when both files are affected
> + --------------------------------------
> + rdist_orgnl_both_files.log:
> + - The FILES variable for cmdspecial *with*
> + a specified <name list> shows both files!
> +
> + EOF
> + cat rdist_orgnl_both_files.log
> +
> + cat <<- EOF
> +
> + Log when only one file is affected
> + ----------------------------------------
> + rdist_orgnl_file2_only.log:
> + - The FILES variable for the cmdspecial *with*
> + "src/file" in the <name list> still runs!
> +
> + EOF
> + cat rdist_orgnl_file2_only.log
> +fi
>
