On Tue, May 30, 2023 at 02:38:23PM +0200, Claudio Jeker wrote:
> On Wed, May 24, 2023 at 04:18:30PM +0000, Job Snijders wrote:
> > Dear all,
> >
> > Claudio made some suggestions to pass the desired modification times
> > around in a different way, below is an updated patch proposal.
> > I also added some instrumentation to also adjust GBRs and TAKs.
> >
> > RIPE & APNIC informally indicated some interest in this hack.
> >
>
> This looks good. Some feedback below.
>
> > Index: parser.c
> > ===================================================================
> > RCS file: /cvs/src/usr.sbin/rpki-client/parser.c,v
> > retrieving revision 1.94
> > diff -u -p -r1.94 parser.c
> > --- parser.c 11 May 2023 20:13:30 -0000 1.94
> > +++ parser.c 24 May 2023 16:11:15 -0000
> > @@ -615,8 +621,11 @@ parse_entity(struct entityq *q, struct m
> > struct mft *mft;
> > struct roa *roa;
> > struct aspa *aspa;
> > + struct gbr *gbr;
> > + struct tak *tak;
> > struct ibuf *b;
> > unsigned char *f;
> > + time_t mtime = 0;
>
> Please initialize mtime to 0 inside of the
> while ((entp = TAILQ_FIRST(q)) != NULL) {
> loop. e.g right were f and file are set to 0.
>
> > size_t flen;
> > char *file, *crlfile;
> > int c;
>
> ...
>
> > @@ -693,6 +709,11 @@ parse_entity(struct entityq *q, struct m
> > io_simple_buffer(b2, &entp->talid,
> > sizeof(entp->talid));
> > io_str_buffer(b2, crlfile);
> > + /*
> > + * FIXME: MFT CMS signing-time is used as mtime
> > + * instead of CRL lastUpdate.
> > + */
Forgot to mention, for this here I would suggest we pass in a time_t
crlmtime to proc_parser_mft() and set the crlmtime there right before we
crl_insert() the crl. I find that a bit better than returning a pointer to
struct crl (since we're not supposed to crl_free() that one).
> > + io_simple_buffer(b2, &mtime, sizeof(mtime));
> > free(crlfile);
> >
> > io_close_buffer(msgq, b2);
>
> > @@ -727,13 +758,18 @@ parse_entity(struct entityq *q, struct m
> > case RTYPE_TAK:
> > file = parse_load_file(entp, &f, &flen);
> > io_str_buffer(b, file);
> > - proc_parser_tak(file, f, flen, entp->mftaki);
> > + tak = proc_parser_tak(file, f, flen, entp->mftaki);
> > + if (tak != NULL)
> > + mtime = tak->signtime;
> > + io_simple_buffer(b, &mtime, sizeof(mtime));
> > + tak_free(tak);
> > break;
>
> Thanks for plugging this memory leak here.
>
> > @@ -1540,6 +1542,26 @@ repo_move_valid(struct filepath_tree *tr
> >
> > if (repo_mkpath(AT_FDCWD, fn) == -1)
> > continue;
> > +
> > + /*
> > + * Adjust file last modification time in order to minimize
> > + * RSYNC synchronization load after transport failover.
> > + * While serializing RRDP datastructures to disk, set the last
> > + * modified timestamp to the CMS signing-time or the X.509
> > + * notBefore timestamp.
> > + */
> > + if (fp->mtime != 0 &&
> > + strncmp(fp->file, ".rrdp/", rrdpsz) == 0) {
> > + struct timespec ts[2];
> > +
> > + ts[0].tv_nsec = UTIME_OMIT;
> > + ts[1].tv_sec = fp->mtime;
> > + ts[1].tv_nsec = 0;
> > + if (utimensat(AT_FDCWD, fp->file, ts, 0) == -1) {
> > + warn("utimensat %s", fp->file);
> > + continue;
> > + }
> > + }
>
> I would move this code up into the else branch of
> if (strncmp(fp->file, ".rsync/", rsyncsz) == 0) {
> } else {
> base = strchr(fp->file + rrdpsz, '/');
> assert(base != NULL);
> fn = base + 1;
>
> /* HERE */
> }
>
> Then it is enough to just check fp->mtime != 0.
>
> >
> > if (rename(fp->file, fn) == -1) {
> > warn("rename %s", fp->file);
> >
>
> --
> :wq Claudio
>
--
:wq Claudio
