On Thu, Jan 13, 2022 at 11:42:24AM +0100, Claudio Jeker wrote:
> This diff adds the code to pass --compare-dest to rsync. This will be
> used once there is a valid cache and then the rsync repo will just act as
> a delta on top.
>
> Now --compare-dest is a bit strange as in the directory passed is relative
> to the destination directory (last argument of rsync command). Because of
> this the right amount of ../../../ need to be added to the compare-dest
> path. rsync_fixup_dest() is taking care of that.
Wow
>
> While there upgrade an assert to a proper error condition since it would
> lead to an access beyond the ids array.
ok
one small comment inline
> --
> :wq Claudio
>
> Index: main.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/main.c,v
> retrieving revision 1.173
> diff -u -p -r1.173 main.c
> --- main.c 11 Jan 2022 13:06:07 -0000 1.173
> +++ main.c 13 Jan 2022 10:06:56 -0000
> @@ -261,6 +261,7 @@ rsync_fetch(unsigned int id, const char
> b = io_new_buffer();
> io_simple_buffer(b, &id, sizeof(id));
> io_str_buffer(b, local);
> + io_str_buffer(b, NULL);
> io_str_buffer(b, uri);
> io_close_buffer(&rsyncq, b);
> }
> Index: rsync.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/rsync.c,v
> retrieving revision 1.31
> diff -u -p -r1.31 rsync.c
> --- rsync.c 22 Dec 2021 09:35:14 -0000 1.31
> +++ rsync.c 13 Jan 2022 10:08:46 -0000
> @@ -99,6 +99,31 @@ rsync_base_uri(const char *uri)
> return base_uri;
> }
>
> +/*
> + * The directory passed as --compare-dest needs to be relative to
> + * the destination directory. This function takes care of that.
> + */
> +static char *
> +rsync_fixup_dest(char *destdir, char *compdir)
> +{
> + const char *dotdot = "../../../../../../"; /* should be enough */
> + int dirs = 1;
> + char *fn;
> + char c;
> +
> + while ((c = *destdir++) != '\0')
> + if (c == '/')
> + dirs++;
> +
> + if (dirs > 6)
> + /* too deep for us */
> + return NULL;
> +
> + if ((asprintf(&fn, "%.*s%s", dirs * 3, dotdot, compdir)) == -1)
> + err(1, NULL);
> + return fn;
> +}
> +
> static void
> proc_child(int signal)
> {
> @@ -181,7 +206,7 @@ proc_rsync(char *prog, char *bind_addr,
> err(1, NULL);
>
> for (;;) {
> - char *uri = NULL, *dst = NULL;
> + char *uri, *dst, *compdst;
> unsigned int id;
> pid_t pid;
> int st;
> @@ -209,7 +234,8 @@ proc_rsync(char *prog, char *bind_addr,
> for (i = 0; i < idsz; i++)
> if (ids[i].pid == pid)
> break;
> - assert(i < idsz);
> + if (!(i < idsz))
I would use
if (i >= idsz)
> + errx(1, "waitpid: %d unexpected", pid);
>
> if (!WIFEXITED(st)) {
> warnx("rsync %s terminated abnormally",
> @@ -261,6 +287,7 @@ proc_rsync(char *prog, char *bind_addr,
> /* Read host and module. */
> io_read_buf(b, &id, sizeof(id));
> io_read_str(b, &dst);
> + io_read_str(b, &compdst);
> io_read_str(b, &uri);
>
> ibuf_free(b);
> @@ -275,6 +302,7 @@ proc_rsync(char *prog, char *bind_addr,
>
> if (pid == 0) {
> char *args[32];
> + char *reldst;
>
> if (pledge("stdio exec", NULL) == -1)
> err(1, "pledge");
> @@ -295,6 +323,11 @@ proc_rsync(char *prog, char *bind_addr,
> args[i++] = "--address";
> args[i++] = (char *)bind_addr;
> }
> + if (compdst != NULL &&
> + (reldst = rsync_fixup_dest(dst, compdst)) != NULL) {
> + args[i++] = "--compare-dest";
> + args[i++] = reldst;
> + }
> args[i++] = uri;
> args[i++] = dst;
> args[i] = NULL;
> @@ -323,6 +356,7 @@ proc_rsync(char *prog, char *bind_addr,
> /* Clean up temporary values. */
>
> free(dst);
> + free(compdst);
> }
>
> /* No need for these to be hanging around. */
>
