On Thu, Dec 20, 2018 at 7:01 AM Yauheni Kaliuta <yauheni.kaliuta@xxxxxxxxxx> wrote: > > Hi, Lucas! > > >>>>> On Tue, 18 Dec 2018 16:02:28 -0800, Lucas De Marchi wrote: > > > Allow to test outputs when they don't match exactly, but should follow > > some regex patterns. This can be used when the info we are printing is > > randomized or depends on kernel configuration. > > --- > > [...] > > > struct buffer { > > char buf[BUFSZ]; > > + unsigned int head; > > }; > > [...] > > I'm wondering, since you are now keeping some context, would it > make sence to group all of it in one place? > > Something like: > > diff --git a/testsuite/testsuite.c b/testsuite/testsuite.c > index db36324cfda3..e8b26d6bf60f 100644 > --- a/testsuite/testsuite.c > +++ b/testsuite/testsuite.c > @@ -273,32 +273,165 @@ static inline int test_run_child(const struct test *t, int fdout[2], > return test_run_spawned(t); > } > > -static int check_activity(int fd, bool activity, const char *path, > - const char *stream) > +#define BUFSZ 4096 > + > +enum fd_cmp_type { > + FD_CMP_MONITOR, > + FD_CMP_OUT, > + FD_CMP_ERR, > + FD_CMP_MAX = FD_CMP_ERR, > +}; > + > +struct fd_cmp { > + enum fd_cmp_type type; > + int fd; > + int fd_match; > + bool activity; > + const char *path; > + const char *prefix; > + const char *stream_name; maybe prefix and stream_name are redundant? > + char buf[BUFSZ]; > + char buf_match[BUFSZ]; > + unsigned int head; > + unsigned int head_match; > +}; It looks like a nice abstraction to be done on top. Would you mind sending it as a patch on top of this series? thanks, Lucas De Marchi > + > +static int fd_cmp_check_activity(struct fd_cmp *fd_cmp) > { > struct stat st; > > /* not monitoring or monitoring and it has activity */ > - if (fd < 0 || activity) > + if (fd_cmp == NULL || fd_cmp->fd < 0 || fd_cmp->activity) > return 0; > > /* monitoring, there was no activity and size matches */ > - if (stat(path, &st) == 0 && st.st_size == 0) > + if (stat(fd_cmp->path, &st) == 0 && st.st_size == 0) > return 0; > > - ERR("Expecting output on %s, but test didn't produce any\n", stream); > + ERR("Expecting output on %s, but test didn't produce any\n", > + fd_cmp->stream_name); > > return -1; > } > > -#define BUFSZ 4096 > -struct buffer { > - char buf[BUFSZ]; > - unsigned int head; > -}; > +static bool fd_cmp_is_active(struct fd_cmp *fd_cmp) > +{ > + return fd_cmp->fd != -1; > +} > + > +static int fd_cmp_open_monitor(struct fd_cmp *fd_cmp, int fd, int fd_ep) > +{ > + struct epoll_event ep = {}; > + > + ep.events = EPOLLHUP; > + ep.data.ptr = fd_cmp; > + if (epoll_ctl(fd_ep, EPOLL_CTL_ADD, fd, &ep) < 0) { > + ERR("could not add monitor fd to epoll: %m\n"); > + return -errno; > + } > + > + return 0; > +} > + > +static int fd_cmp_open_std(struct fd_cmp *fd_cmp, > + const char *fn, int fd, int fd_ep) > +{ > + struct epoll_event ep = {}; > + int fd_match; > + > + fd_match = open(fn, O_RDONLY); > + if (fd_match < 0) { > + ERR("could not open %s for read: %m\n", fn); > + return -errno; > + } > + ep.events = EPOLLIN; > + ep.data.ptr = fd_cmp; > + if (epoll_ctl(fd_ep, EPOLL_CTL_ADD, fd, &ep) < 0) { > + ERR("could not add fd to epoll: %m\n"); > + close(fd_match); > + return -errno; > + } > + > + return fd_match; > +} > + > +/* opens output file AND adds descriptor to epoll */ > +static int fd_cmp_open(struct fd_cmp **fd_cmp_out, > + enum fd_cmp_type type, const char *fn, int fd, > + int fd_ep) > +{ > + int err = 0; > + struct fd_cmp *fd_cmp; > + > + fd_cmp = calloc(1, sizeof(*fd_cmp)); > + if (fd_cmp == NULL) { > + ERR("could not allocate fd_cmp\n"); > + return -ENOMEM; > + } > + > + switch (type) { > + case FD_CMP_MONITOR: > + err = fd_cmp_open_monitor(fd_cmp, fd, fd_ep); > + break; > + case FD_CMP_OUT: > + fd_cmp->prefix = "STDOUT"; > + fd_cmp->stream_name = "stdout"; > + err = fd_cmp_open_std(fd_cmp, fn, fd, fd_ep); > + break; > + case FD_CMP_ERR: > + fd_cmp->prefix = "STDERR"; > + fd_cmp->stream_name = "stderr"; > + err = fd_cmp_open_std(fd_cmp, fn, fd, fd_ep); > + break; > + default: > + ERR("unknown fd type %d\n", type); > + err = -1; > + } > > + if (err < 0) { > + free(fd_cmp); > + return err; > + } > + > + fd_cmp->fd_match = err; > + fd_cmp->fd = fd; > + fd_cmp->type = type; > + fd_cmp->path = fn; > + > + *fd_cmp_out = fd_cmp; > + return 0; > +} > + > +static int fd_cmp_check_ev_in(struct fd_cmp *fd_cmp) > +{ > + if (fd_cmp->type == FD_CMP_MONITOR) { > + ERR("Unexpected activity on monitor pipe\n"); > + return -EINVAL; > + } > + fd_cmp->activity = true; > + > + return 0; > +} > + > +static void fd_cmp_delete_ep(struct fd_cmp *fd_cmp, int fd_ep) > +{ > + if (epoll_ctl(fd_ep, EPOLL_CTL_DEL, fd_cmp->fd, NULL) < 0) { > + ERR("could not remove fd %d from epoll: %m\n", fd_cmp->fd); > + } > + fd_cmp->fd = -1; > +} > + > +static void fd_cmp_close(struct fd_cmp *fd_cmp) > +{ > + if (fd_cmp == NULL) > + return; > + > + if (fd_cmp->fd >= 0) > + close(fd_cmp->fd); > + free(fd_cmp); > +} > > -static bool cmpbuf_regex_one(const char *pattern, const char *s) > +static bool fd_cmp_regex_one(const char *pattern, const char *s) > { > _cleanup_(regfree) regex_t re = { }; > > @@ -310,18 +443,17 @@ static bool cmpbuf_regex_one(const char *pattern, const char *s) > * read fd and fd_match, checking the first matches the regex of the second, > * line by line > */ > -static bool cmpbuf_regex(const struct test *t, const char *prefix, > - int fd, int fd_match, struct buffer *buf, > - struct buffer *buf_match) > +static bool fd_cmp_regex(struct fd_cmp *fd_cmp, const struct test *t) > { > char *p, *p_match; > int done = 0, done_match = 0, r; > > - r = read(fd, buf->buf + buf->head, sizeof(buf->buf) - buf->head - 1); > + r = read(fd_cmp->fd, fd_cmp->buf + fd_cmp->head, > + sizeof(fd_cmp->buf) - fd_cmp->head - 1); > if (r <= 0) > return true; > > - buf->head += r; > + fd_cmp->head += r; > > /* > * Process as many lines as read from fd and that fits in the buffer - > @@ -329,40 +461,40 @@ static bool cmpbuf_regex(const struct test *t, const char *prefix, > * get the same amount from fd_match > */ > for (;;) { > - p = memchr(buf->buf + done, '\n', buf->head - done); > + p = memchr(fd_cmp->buf + done, '\n', fd_cmp->head - done); > if (!p) > break; > *p = 0; > > - p_match = memchr(buf_match->buf + done_match, '\n', > - buf_match->head - done_match); > + p_match = memchr(fd_cmp->buf_match + done_match, '\n', > + fd_cmp->head_match - done_match); > if (!p_match) { > /* pump more data from file */ > - r = read(fd_match, buf_match->buf + buf_match->head, > - sizeof(buf_match->buf) - buf_match->head - 1); > + r = read(fd_cmp->fd_match, fd_cmp->buf_match + fd_cmp->head_match, > + sizeof(fd_cmp->buf_match) - fd_cmp->head_match - 1); > if (r <= 0) { > - ERR("could not read match fd %d\n", fd_match); > + ERR("could not read match fd %d\n", fd_cmp->fd_match); > return false; > } > - buf_match->head += r; > - p_match = memchr(buf_match->buf + done_match, '\n', > - buf_match->head - done_match); > + fd_cmp->head_match += r; > + p_match = memchr(fd_cmp->buf_match + done_match, '\n', > + fd_cmp->head_match - done_match); > if (!p_match) { > - ERR("could not find match line from fd %d\n", fd_match); > + ERR("could not find match line from fd %d\n", fd_cmp->fd_match); > return false; > } > } > *p_match = 0; > > - if (!cmpbuf_regex_one(buf_match->buf + done_match, buf->buf + done)) { > - ERR("Output does not match pattern on %s:\n", prefix); > - ERR("pattern: %s\n", buf_match->buf + done_match); > - ERR("output : %s\n", buf->buf + done); > + if (!fd_cmp_regex_one(fd_cmp->buf_match + done_match, fd_cmp->buf + done)) { > + ERR("Output does not match pattern on %s:\n", fd_cmp->prefix); > + ERR("pattern: %s\n", fd_cmp->buf_match + done_match); > + ERR("output : %s\n", fd_cmp->buf + done); > return false; > } > > - done = p - buf->buf + 1; > - done_match = p_match - buf_match->buf + 1; > + done = p - fd_cmp->buf + 1; > + done_match = p_match - fd_cmp->buf_match + 1; > } > > /* > @@ -371,59 +503,57 @@ static bool cmpbuf_regex(const struct test *t, const char *prefix, > */ > > if (done) { > - if (buf->head - done) > - memmove(buf->buf, buf->buf + done, buf->head - done); > - buf->head -= done; > + if (fd_cmp->head - done) > + memmove(fd_cmp->buf, fd_cmp->buf + done, fd_cmp->head - done); > + fd_cmp->head -= done; > } > > if (done_match) { > - if (buf_match->head - done_match) > - memmove(buf_match->buf, buf_match->buf + done_match, > - buf_match->head - done_match); > - buf_match->head -= done_match; > + if (fd_cmp->head_match - done_match) > + memmove(fd_cmp->buf_match, fd_cmp->buf_match + done_match, > + fd_cmp->head_match - done_match); > + fd_cmp->head_match -= done_match; > } > > return true; > } > > /* read fd and fd_match, checking they match exactly */ > -static bool cmpbuf_exact(const struct test *t, const char *prefix, > - int fd, int fd_match, struct buffer *buf, > - struct buffer *buf_match) > +static bool fd_cmp_exact(struct fd_cmp *fd_cmp, const struct test *t) > { > int r, rmatch, done = 0; > > - r = read(fd, buf, sizeof(buf->buf) - 1); > + r = read(fd_cmp->fd, fd_cmp->buf, sizeof(fd_cmp->buf) - 1); > if (r <= 0) > /* try again later */ > return true; > > /* read as much data from fd_match as we read from fd */ > for (;;) { > - rmatch = read(fd_match, buf_match->buf + done, r - done); > + rmatch = read(fd_cmp->fd_match, fd_cmp->buf_match + done, r - done); > if (rmatch == 0) > break; > > if (rmatch < 0) { > if (errno == EINTR) > continue; > - ERR("could not read match fd %d\n", fd_match); > + ERR("could not read match fd %d\n", fd_cmp->fd_match); > return false; > } > > done += rmatch; > } > > - buf->buf[r] = '\0'; > - buf_match->buf[r] = '\0'; > + fd_cmp->buf[r] = '\0'; > + fd_cmp->buf_match[r] = '\0'; > > if (t->print_outputs) > - printf("%s: %s\n", prefix, buf->buf); > + printf("%s: %s\n", fd_cmp->prefix, fd_cmp->buf); > > - if (!streq(buf->buf, buf_match->buf)) { > - ERR("Outputs do not match on %s:\n", prefix); > - ERR("correct:\n%s\n", buf_match->buf); > - ERR("wrong:\n%s\n", buf->buf); > + if (!streq(fd_cmp->buf, fd_cmp->buf_match)) { > + ERR("Outputs do not match on %s:\n", fd_cmp->prefix); > + ERR("correct:\n%s\n", fd_cmp->buf_match); > + ERR("wrong:\n%s\n", fd_cmp->buf); > return false; > } > > @@ -434,11 +564,12 @@ static bool test_run_parent_check_outputs(const struct test *t, > int fdout, int fderr, int fdmonitor, > pid_t child) > { > - struct epoll_event ep_outpipe, ep_errpipe, ep_monitor; > - int err, fd_ep, fd_matchout = -1, fd_matcherr = -1; > - bool fd_activityout = false, fd_activityerr = false; > + int err, fd_ep; > unsigned long long end_usec, start_usec; > - _cleanup_free_ struct buffer *buf_out = NULL, *buf_err = NULL; > + struct fd_cmp *fd_cmp_out = NULL; > + struct fd_cmp *fd_cmp_err = NULL; > + struct fd_cmp *fd_cmp_monitor = NULL; > + int n_fd = 0; > > fd_ep = epoll_create1(EPOLL_CLOEXEC); > if (fd_ep < 0) { > @@ -447,59 +578,30 @@ static bool test_run_parent_check_outputs(const struct test *t, > } > > if (t->output.out != NULL) { > - fd_matchout = open(t->output.out, O_RDONLY); > - if (fd_matchout < 0) { > - err = -errno; > - ERR("could not open %s for read: %m\n", > - t->output.out); > - goto out; > - } > - memset(&ep_outpipe, 0, sizeof(struct epoll_event)); > - ep_outpipe.events = EPOLLIN; > - ep_outpipe.data.ptr = &fdout; > - if (epoll_ctl(fd_ep, EPOLL_CTL_ADD, fdout, &ep_outpipe) < 0) { > - err = -errno; > - ERR("could not add fd to epoll: %m\n"); > + err = fd_cmp_open(&fd_cmp_out, > + FD_CMP_OUT, t->output.out, fdout, fd_ep); > + if (err < 0) > goto out; > - } > - buf_out = calloc(2, sizeof(*buf_out)); > - } else > - fdout = -1; > + n_fd++; > + } > > if (t->output.err != NULL) { > - fd_matcherr = open(t->output.err, O_RDONLY); > - if (fd_matcherr < 0) { > - err = -errno; > - ERR("could not open %s for read: %m\n", > - t->output.err); > + err = fd_cmp_open(&fd_cmp_err, > + FD_CMP_ERR, t->output.err, fderr, fd_ep); > + if (err < 0) > goto out; > + n_fd++; > + } > > - } > - memset(&ep_errpipe, 0, sizeof(struct epoll_event)); > - ep_errpipe.events = EPOLLIN; > - ep_errpipe.data.ptr = &fderr; > - if (epoll_ctl(fd_ep, EPOLL_CTL_ADD, fderr, &ep_errpipe) < 0) { > - err = -errno; > - ERR("could not add fd to epoll: %m\n"); > - goto out; > - } > - buf_err = calloc(2, sizeof(*buf_err)); > - } else > - fderr = -1; > - > - memset(&ep_monitor, 0, sizeof(struct epoll_event)); > - ep_monitor.events = EPOLLHUP; > - ep_monitor.data.ptr = &fdmonitor; > - if (epoll_ctl(fd_ep, EPOLL_CTL_ADD, fdmonitor, &ep_monitor) < 0) { > - err = -errno; > - ERR("could not add monitor fd to epoll: %m\n"); > + err = fd_cmp_open(&fd_cmp_monitor, FD_CMP_MONITOR, NULL, fdmonitor, fd_ep); > + if (err < 0) > goto out; > - } > + n_fd++; > > start_usec = now_usec(); > end_usec = start_usec + TEST_TIMEOUT_USEC; > > - for (err = 0; fdmonitor >= 0 || fdout >= 0 || fderr >= 0;) { > + for (err = 0; n_fd > 0;) { > int fdcount, i, timeout; > struct epoll_event ev[4]; > unsigned long long curr_usec = now_usec(); > @@ -518,68 +620,45 @@ static bool test_run_parent_check_outputs(const struct test *t, > } > > for (i = 0; i < fdcount; i++) { > - int fd = *(int *)ev[i].data.ptr; > + struct fd_cmp *fd_cmp = ev[i].data.ptr; > bool ret; > > if (ev[i].events & EPOLLIN) { > - int fd_match; > - struct buffer *buf, *buf_match; > - const char *prefix; > - > - if (fd == fdout) { > - fd_match = fd_matchout; > - buf = buf_out; > - fd_activityout = true; > - prefix = "STDOUT"; > - } else if (fd == fderr) { > - fd_match = fd_matcherr; > - buf = buf_err; > - fd_activityerr = true; > - prefix = "STDERR"; > - } else { > - ERR("Unexpected activity on monitor pipe\n"); > - err = -EINVAL; > + err = fd_cmp_check_ev_in(fd_cmp); > + if (err < 0) > goto out; > - } > - > - buf_match = buf + 1; > > if (t->output.regex) > - ret = cmpbuf_regex(t, prefix, fd, fd_match, > - buf, buf_match); > + ret = fd_cmp_regex(fd_cmp, t); > else > - ret = cmpbuf_exact(t, prefix, fd, fd_match, > - buf, buf_match); > + ret = fd_cmp_exact(fd_cmp, t); > > if (!ret) { > err = -1; > goto out; > } > } else if (ev[i].events & EPOLLHUP) { > - if (epoll_ctl(fd_ep, EPOLL_CTL_DEL, fd, NULL) < 0) { > - ERR("could not remove fd %d from epoll: %m\n", fd); > - } > - *(int *)ev[i].data.ptr = -1; > + fd_cmp_delete_ep(fd_cmp, fd_ep); > + n_fd--; > } > } > } > > - err = check_activity(fd_matchout, fd_activityout, t->output.out, "stdout"); > - err |= check_activity(fd_matcherr, fd_activityerr, t->output.err, "stderr"); > + err = fd_cmp_check_activity(fd_cmp_out); > + err |= fd_cmp_check_activity(fd_cmp_err); > > - if (err == 0 && fdmonitor >= 0) { > + if (err == 0 && fd_cmp_is_active(fd_cmp_monitor)) { > err = -EINVAL; > ERR("Test '%s' timed out, killing %d\n", t->name, child); > kill(child, SIGKILL); > } > > out: > - if (fd_matchout >= 0) > - close(fd_matchout); > - if (fd_matcherr >= 0) > - close(fd_matcherr); > - if (fd_ep >= 0) > - close(fd_ep); > + fd_cmp_close(fd_cmp_out); > + fd_cmp_close(fd_cmp_err); > + fd_cmp_close(fd_cmp_monitor); > + close(fd_ep); > + > return err == 0; > } > > > > > -- > WBR, > Yauheni Kaliuta -- Lucas De Marchi