Claudio Jeker([email protected]) on 2021.05.07 12:16:26 +0200:
> Currently our rsync does not follow the exit codes from rsync. Also the
> error handling is complex because ERR() and ERRX() are not terminating the
> process.
>
> This diff tries to start cleaning up the mess a bit. Introduce some exit
> codes to use and apply them in places where it is obvious.
>
> The rsync server process should normally communicate errors back via the
> rsync socket but there are some errors where this will most probably fail
> as well. A good example are memory failures.
>
> I used ERR_IPC as a kind of catchall for any system error that pops up
> (fork, socketpair but also pledge, unveil).
>
> The goal is to reduce the amount of ERR(), ERRX() and ERRX1() from rsync.
> This should simplify the code base.
>
> I also synced the mkpath() call with the one from mkdir and handle the
> error now outside of the call. Again I think the result is cleaner.
>
> OK?
ok benno
> --
> :wq Claudio
>
> Index: client.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/rsync/client.c,v
> retrieving revision 1.15
> diff -u -p -r1.15 client.c
> --- client.c 8 May 2019 20:00:25 -0000 1.15
> +++ client.c 7 May 2021 08:29:10 -0000
> @@ -43,7 +43,7 @@ rsync_client(const struct opts *opts, in
>
> if (pledge("stdio unix rpath wpath cpath dpath fattr chown getpw
> unveil",
> NULL) == -1)
> - err(1, "pledge");
> + err(ERR_IPC, "pledge");
>
> memset(&sess, 0, sizeof(struct sess));
> sess.opts = opts;
> Index: extern.h
> ===================================================================
> RCS file: /cvs/src/usr.bin/rsync/extern.h,v
> retrieving revision 1.36
> diff -u -p -r1.36 extern.h
> --- extern.h 31 Mar 2021 19:45:16 -0000 1.36
> +++ extern.h 7 May 2021 08:22:32 -0000
> @@ -42,6 +42,19 @@
> #define CSUM_LENGTH_PHASE2 (16)
>
> /*
> + * Rsync error codes.
> + */
> +#define ERR_SYNTAX 1
> +#define ERR_PROTOCOL 2
> +#define ERR_SOCK_IO 10
> +#define ERR_FILE_IO 11
> +#define ERR_WIREPROTO 12
> +#define ERR_IPC 14 /* catchall for any kind of syscall
> error */
> +#define ERR_TERMIMATED 16
> +#define ERR_WAITPID 21
> +#define ERR_NOMEM 22
> +
> +/*
> * Use this for --timeout.
> * All poll events will use it and catch time-outs.
> */
> Index: fargs.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/rsync/fargs.c,v
> retrieving revision 1.17
> diff -u -p -r1.17 fargs.c
> --- fargs.c 8 May 2019 20:00:25 -0000 1.17
> +++ fargs.c 7 May 2021 08:19:29 -0000
> @@ -17,6 +17,7 @@
> #include <sys/stat.h>
>
> #include <assert.h>
> +#include <err.h>
> #include <stdint.h>
> #include <stdlib.h>
> #include <string.h>
> @@ -51,7 +52,7 @@ fargs_cmdline(struct sess *sess, const s
> if (sess->opts->ssh_prog) {
> ap = strdup(sess->opts->ssh_prog);
> if (ap == NULL)
> - goto out;
> + err(ERR_NOMEM, NULL);
>
> while ((arg = strsep(&ap, " \t")) != NULL) {
> if (arg[0] == '\0') {
> @@ -127,8 +128,4 @@ fargs_cmdline(struct sess *sess, const s
> addargs(&args, "%s", f->sink);
>
> return args.list;
> -out:
> - freeargs(&args);
> - ERR("calloc");
> - return NULL;
> }
> Index: main.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/rsync/main.c,v
> retrieving revision 1.53
> diff -u -p -r1.53 main.c
> --- main.c 31 Mar 2021 19:45:16 -0000 1.53
> +++ main.c 7 May 2021 08:30:18 -0000
> @@ -83,18 +83,18 @@ fargs_parse(size_t argc, char *argv[], s
> /* Allocations. */
>
> if ((f = calloc(1, sizeof(struct fargs))) == NULL)
> - err(1, "calloc");
> + err(ERR_NOMEM, NULL);
>
> f->sourcesz = argc - 1;
> if ((f->sources = calloc(f->sourcesz, sizeof(char *))) == NULL)
> - err(1, "calloc");
> + err(ERR_NOMEM, NULL);
>
> for (i = 0; i < argc - 1; i++)
> if ((f->sources[i] = strdup(argv[i])) == NULL)
> - err(1, "strdup");
> + err(ERR_NOMEM, NULL);
>
> if ((f->sink = strdup(argv[i])) == NULL)
> - err(1, "strdup");
> + err(ERR_NOMEM, NULL);
>
> /*
> * Test files for its locality.
> @@ -109,15 +109,16 @@ fargs_parse(size_t argc, char *argv[], s
> if (fargs_is_remote(f->sink)) {
> f->mode = FARGS_SENDER;
> if ((f->host = strdup(f->sink)) == NULL)
> - err(1, "strdup");
> + err(ERR_NOMEM, NULL);
> }
>
> if (fargs_is_remote(f->sources[0])) {
> if (f->host != NULL)
> - errx(1, "both source and destination cannot be remote
> files");
> + errx(ERR_SYNTAX, "both source and destination "
> + "cannot be remote files");
> f->mode = FARGS_RECEIVER;
> if ((f->host = strdup(f->sources[0])) == NULL)
> - err(1, "strdup");
> + err(ERR_NOMEM, NULL);
> }
>
> if (f->host != NULL) {
> @@ -127,7 +128,8 @@ fargs_parse(size_t argc, char *argv[], s
> len = strlen(f->host) - 8 + 1;
> memmove(f->host, f->host + 8, len);
> if ((cp = strchr(f->host, '/')) == NULL)
> - errx(1, "rsync protocol requires a module
> name");
> + errx(ERR_SYNTAX,
> + "rsync protocol requires a module name");
> *cp++ = '\0';
> f->module = cp;
> if ((cp = strchr(f->module, '/')) != NULL)
> @@ -152,9 +154,9 @@ fargs_parse(size_t argc, char *argv[], s
> }
> }
> if ((len = strlen(f->host)) == 0)
> - errx(1, "empty remote host");
> + errx(ERR_SYNTAX, "empty remote host");
> if (f->remote && strlen(f->module) == 0)
> - errx(1, "empty remote module");
> + errx(ERR_SYNTAX, "empty remote module");
> }
>
> /* Make sure we have the same "hostspec" for all files. */
> @@ -164,7 +166,7 @@ fargs_parse(size_t argc, char *argv[], s
> for (i = 0; i < f->sourcesz; i++) {
> if (!fargs_is_remote(f->sources[i]))
> continue;
> - errx(1,
> + errx(ERR_SYNTAX,
> "remote file in list of local sources: %s",
> f->sources[i]);
> }
> @@ -174,20 +176,20 @@ fargs_parse(size_t argc, char *argv[], s
> !fargs_is_daemon(f->sources[i]))
> continue;
> if (fargs_is_daemon(f->sources[i]))
> - errx(1, "remote daemon in list of "
> - "remote sources: %s",
> - f->sources[i]);
> - errx(1, "local file in list of remote sources:
> %s",
> - f->sources[i]);
> + errx(ERR_SYNTAX,
> + "remote daemon in list of remote "
> + "sources: %s", f->sources[i]);
> + errx(ERR_SYNTAX, "local file in list of "
> + "remote sources: %s", f->sources[i]);
> }
> } else {
> if (f->mode != FARGS_RECEIVER)
> - errx(1, "sender mode for remote "
> + errx(ERR_SYNTAX, "sender mode for remote "
> "daemon receivers not yet supported");
> for (i = 0; i < f->sourcesz; i++) {
> if (fargs_is_daemon(f->sources[i]))
> continue;
> - errx(1, "non-remote daemon file "
> + errx(ERR_SYNTAX, "non-remote daemon file "
> "in list of remote daemon sources: "
> "%s", f->sources[i]);
> }
> @@ -233,7 +235,7 @@ fargs_parse(size_t argc, char *argv[], s
> *ccp = '\0';
> if (strncmp(cp, f->host, len) ||
> (cp[len] != '/' && cp[len] != '\0'))
> - errx(1, "different remote host: %s",
> + errx(ERR_SYNTAX, "different remote host: %s",
> f->sources[i]);
> memmove(f->sources[i],
> f->sources[i] + len + 8 + 1,
> @@ -246,7 +248,7 @@ fargs_parse(size_t argc, char *argv[], s
> /* host::path */
> if (strncmp(cp, f->host, len) ||
> (cp[len] != ':' && cp[len] != '\0'))
> - errx(1, "different remote host: %s",
> + errx(ERR_SYNTAX, "different remote host: %s",
> f->sources[i]);
> memmove(f->sources[i], f->sources[i] + len + 2,
> j - len - 1);
> @@ -257,7 +259,7 @@ fargs_parse(size_t argc, char *argv[], s
> /* host:path */
> if (strncmp(cp, f->host, len) ||
> (cp[len] != ':' && cp[len] != '\0'))
> - errx(1, "different remote host: %s",
> + errx(ERR_SYNTAX, "different remote host: %s",
> f->sources[i]);
> memmove(f->sources[i],
> f->sources[i] + len + 1, j - len);
> @@ -318,7 +320,7 @@ main(int argc, char *argv[])
>
> if (pledge("stdio unix rpath wpath cpath dpath inet fattr chown dns
> getpw proc exec unveil",
> NULL) == -1)
> - err(1, "pledge");
> + err(ERR_IPC, "pledge");
>
> memset(&opts, 0, sizeof(struct opts));
>
> @@ -391,7 +393,8 @@ main(int argc, char *argv[])
> case 5:
> poll_timeout = strtonum(optarg, 0, 60*60, &errstr);
> if (errstr != NULL)
> - errx(1, "timeout is %s: %s", errstr, optarg);
> + errx(ERR_SYNTAX, "timeout is %s: %s",
> + errstr, optarg);
> break;
> case 6:
> opts.no_motd = 1;
> @@ -461,43 +464,36 @@ main(int argc, char *argv[])
>
> if (pledge("stdio unix rpath wpath cpath dpath fattr chown getpw proc
> exec unveil",
> NULL) == -1)
> - err(1, "pledge");
> + err(ERR_IPC, "pledge");
>
> /* Create a bidirectional socket and start our child. */
>
> if (socketpair(AF_UNIX, SOCK_STREAM | SOCK_NONBLOCK, 0, fds) == -1)
> - err(1, "socketpair");
> + err(ERR_IPC, "socketpair");
>
> switch ((child = fork())) {
> case -1:
> - err(1, "fork");
> + err(ERR_IPC, "fork");
> case 0:
> close(fds[0]);
> if (pledge("stdio exec", NULL) == -1)
> - err(1, "pledge");
> + err(ERR_IPC, "pledge");
>
> memset(&sess, 0, sizeof(struct sess));
> sess.opts = &opts;
>
> - if ((args = fargs_cmdline(&sess, fargs, NULL)) == NULL) {
> - ERRX1("fargs_cmdline");
> - _exit(1);
> - }
> + args = fargs_cmdline(&sess, fargs, NULL);
>
> for (i = 0; args[i] != NULL; i++)
> LOG2("exec[%d] = %s", i, args[i]);
>
> /* Make sure the child's stdin is from the sender. */
> - if (dup2(fds[1], STDIN_FILENO) == -1) {
> - ERR("dup2");
> - _exit(1);
> - }
> - if (dup2(fds[1], STDOUT_FILENO) == -1) {
> - ERR("dup2");
> - _exit(1);
> - }
> + if (dup2(fds[1], STDIN_FILENO) == -1)
> + err(ERR_IPC, "dup2");
> + if (dup2(fds[1], STDOUT_FILENO) == -1)
> + err(ERR_IPC, "dup2");
> execvp(args[0], args);
> - _exit(1);
> + _exit(ERR_IPC);
> /* NOTREACHED */
> default:
> close(fds[1]);
> @@ -511,7 +507,7 @@ main(int argc, char *argv[])
> close(fds[0]);
>
> if (waitpid(child, &st, 0) == -1)
> - err(1, "waitpid");
> + err(ERR_WAITPID, "waitpid");
>
> /*
> * If we don't already have an error (rc == 0), then inherit the
> @@ -519,10 +515,14 @@ main(int argc, char *argv[])
> * If it hasn't exited, it overrides our return value.
> */
>
> - if (WIFEXITED(st) && rc == 0)
> - rc = WEXITSTATUS(st);
> - else if (!WIFEXITED(st))
> - rc = 1;
> + if (rc == 0) {
> + if (WIFEXITED(st))
> + rc = WEXITSTATUS(st);
> + else if (WIFSIGNALED(st))
> + rc = ERR_TERMIMATED;
> + else
> + rc = ERR_WAITPID;
> + }
>
> exit(rc);
> usage:
> @@ -532,5 +532,5 @@ usage:
> "[--rsync-path=program]\n\t[--timeout=seconds] [--version] "
> "source ... directory\n",
> getprogname());
> - exit(1);
> + exit(ERR_SYNTAX);
> }
> Index: misc.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/rsync/misc.c,v
> retrieving revision 1.2
> diff -u -p -r1.2 misc.c
> --- misc.c 22 Mar 2021 11:14:42 -0000 1.2
> +++ misc.c 7 May 2021 08:19:17 -0000
> @@ -45,7 +45,7 @@ addargs(arglist *args, const char *fmt,
> r = vasprintf(&cp, fmt, ap);
> va_end(ap);
> if (r == -1)
> - err(1, "addargs: argument too long");
> + err(ERR_NOMEM, "addargs: argument too long");
>
> nalloc = args->nalloc;
> if (args->list == NULL) {
> @@ -54,9 +54,10 @@ addargs(arglist *args, const char *fmt,
> } else if (args->num+2 >= nalloc)
> nalloc *= 2;
>
> - args->list = recallocarray(args->list, args->nalloc, nalloc,
> sizeof(char *));
> + args->list = recallocarray(args->list, args->nalloc, nalloc,
> + sizeof(char *));
> if (!args->list)
> - err(1, "malloc");
> + err(ERR_NOMEM, NULL);
> args->nalloc = nalloc;
> args->list[args->num++] = cp;
> args->list[args->num] = NULL;
> Index: mkpath.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/rsync/mkpath.c,v
> retrieving revision 1.4
> diff -u -p -r1.4 mkpath.c
> --- mkpath.c 8 May 2019 21:30:11 -0000 1.4
> +++ mkpath.c 7 May 2021 08:19:55 -0000
> @@ -46,28 +46,34 @@ mkpath(char *path)
> {
> struct stat sb;
> char *slash;
> - int done = 0;
> + int done;
>
> slash = path;
>
> - while (!done) {
> + for (;;) {
> slash += strspn(slash, "/");
> slash += strcspn(slash, "/");
>
> done = (*slash == '\0');
> *slash = '\0';
>
> - if (stat(path, &sb)) {
> - if (errno != ENOENT || (mkdir(path, 0777) &&
> - errno != EEXIST)) {
> - ERR("%s: stat", path);
> + if (mkdir(path, 0777) != 0) {
> + int mkdir_errno = errno;
> +
> + if (stat(path, &sb) == -1) {
> + /* Not there; use mkdir()s errno */
> + errno = mkdir_errno;
> + return (-1);
> + }
> + if (!S_ISDIR(sb.st_mode)) {
> + /* Is there, but isn't a directory */
> + errno = ENOTDIR;
> return (-1);
> }
> - } else if (!S_ISDIR(sb.st_mode)) {
> - errno = ENOTDIR;
> - ERR("%s: stat", path);
> - return (-1);
> }
> +
> + if (done)
> + break;
>
> *slash = '/';
> }
> Index: receiver.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/rsync/receiver.c,v
> retrieving revision 1.26
> diff -u -p -r1.26 receiver.c
> --- receiver.c 6 May 2021 07:29:59 -0000 1.26
> +++ receiver.c 7 May 2021 08:21:48 -0000
> @@ -20,6 +20,7 @@
> #include <sys/stat.h>
>
> #include <assert.h>
> +#include <err.h>
> #include <errno.h>
> #include <fcntl.h>
> #include <inttypes.h>
> @@ -180,15 +181,12 @@ rsync_receiver(struct sess *sess, int fd
> struct upload *ul = NULL;
> mode_t oumask;
>
> - if (pledge("stdio unix rpath wpath cpath dpath fattr chown getpw
> unveil", NULL) == -1) {
> - ERR("pledge");
> - goto out;
> - }
> + if (pledge("stdio unix rpath wpath cpath dpath fattr chown getpw
> unveil", NULL) == -1)
> + err(ERR_IPC, "pledge");
>
> /* Client sends zero-length exclusions. */
>
> - if (!sess->opts->server &&
> - !io_write_int(sess, fdout, 0)) {
> + if (!sess->opts->server && !io_write_int(sess, fdout, 0)) {
> ERRX1("io_write_int");
> goto out;
> }
> @@ -240,14 +238,10 @@ rsync_receiver(struct sess *sess, int fd
> */
>
> if (!sess->opts->dry_run) {
> - if ((tofree = strdup(root)) == NULL) {
> - ERR("strdup");
> - goto out;
> - } else if (mkpath(tofree) < 0) {
> - ERRX1("%s: mkpath", root);
> - free(tofree);
> - goto out;
> - }
> + if ((tofree = strdup(root)) == NULL)
> + err(ERR_NOMEM, NULL);
> + if (mkpath(tofree) < 0)
> + err(ERR_FILE_IO, "%s: mkpath", tofree);
> free(tofree);
> }
>
> @@ -260,10 +254,8 @@ rsync_receiver(struct sess *sess, int fd
>
> if (!sess->opts->dry_run) {
> dfd = open(root, O_RDONLY | O_DIRECTORY, 0);
> - if (dfd == -1) {
> - ERR("%s: open", root);
> - goto out;
> - }
> + if (dfd == -1)
> + err(ERR_FILE_IO, "%s: open", root);
> }
>
> /*
> @@ -285,13 +277,10 @@ rsync_receiver(struct sess *sess, int fd
> * writing into other parts of the file-system.
> */
>
> - if (unveil(root, "rwc") == -1) {
> - ERR("%s: unveil", root);
> - goto out;
> - } else if (unveil(NULL, NULL) == -1) {
> - ERR("%s: unveil", root);
> - goto out;
> - }
> + if (unveil(root, "rwc") == -1)
> + err(ERR_IPC, "%s: unveil", root);
> + if (unveil(NULL, NULL) == -1)
> + err(ERR_IPC, "unveil");
>
> /* If we have a local set, go for the deletion. */
>
> Index: server.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/rsync/server.c,v
> retrieving revision 1.12
> diff -u -p -r1.12 server.c
> --- server.c 8 May 2019 21:30:11 -0000 1.12
> +++ server.c 7 May 2021 08:29:14 -0000
> @@ -57,7 +57,7 @@ rsync_server(const struct opts *opts, si
>
> if (pledge("stdio unix rpath wpath cpath dpath fattr chown getpw
> unveil",
> NULL) == -1)
> - err(1, "pledge");
> + err(ERR_IPC, "pledge");
>
> memset(&sess, 0, sizeof(struct sess));
> sess.opts = opts;
> Index: socket.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/rsync/socket.c,v
> retrieving revision 1.29
> diff -u -p -r1.29 socket.c
> --- socket.c 31 Mar 2021 19:45:16 -0000 1.29
> +++ socket.c 7 May 2021 08:29:20 -0000
> @@ -281,7 +281,7 @@ rsync_connect(const struct opts *opts, i
>
> if (pledge("stdio unix rpath wpath cpath dpath inet fattr chown dns
> getpw unveil",
> NULL) == -1)
> - err(1, "pledge");
> + err(ERR_IPC, "pledge");
>
> memset(&sess, 0, sizeof(struct sess));
> sess.opts = opts;
> @@ -365,7 +365,7 @@ rsync_socket(const struct opts *opts, in
>
> if (pledge("stdio unix rpath wpath cpath dpath fattr chown getpw
> unveil",
> NULL) == -1)
> - err(1, "pledge");
> + err(ERR_IPC, "pledge");
>
> memset(&sess, 0, sizeof(struct sess));
> sess.lver = RSYNC_PROTOCOL;
> @@ -374,10 +374,7 @@ rsync_socket(const struct opts *opts, in
> assert(f->host != NULL);
> assert(f->module != NULL);
>
> - if ((args = fargs_cmdline(&sess, f, &skip)) == NULL) {
> - ERRX1("fargs_cmdline");
> - exit(1);
> - }
> + args = fargs_cmdline(&sess, f, &skip);
>
> /* Initiate with the rsyncd version and module request. */
>
>
