From: Lucas De Marchi <lucas.de.marchi@gmail.com>
To: Yauheni Kaliuta <yauheni.kaliuta@redhat.com>
Cc: Lucas De Marchi <lucas.demarchi@intel.com>,
linux-modules <linux-modules@vger.kernel.org>
Subject: Re: [PATCH 1/3] testsuite: split out function to compare outputs exactly
Date: Thu, 3 Jan 2019 12:09:57 -0800 [thread overview]
Message-ID: <CAKi4VALMbE563DVSfwpsrBiBuAMessk3SWgOCKYvQmARKXc=mg@mail.gmail.com> (raw)
In-Reply-To: <xuny36qslbys.fsf@redhat.com>
On Thu, Dec 20, 2018 at 7:00 AM Yauheni Kaliuta
<yauheni.kaliuta@redhat.com> wrote:
>
> Hi, Lucas!
>
> >>>>> On Tue, 18 Dec 2018 16:02:27 -0800, Lucas De Marchi wrote:
>
> > Move functionality to compare the exact output to a separate function
> > and allocate one buffer per output/match pair. This will allow us to
> > extend this to allow other types of comparisons.
> > ---
> > testsuite/testsuite.c | 124 ++++++++++++++++++++++++------------------
> > 1 file changed, 70 insertions(+), 54 deletions(-)
>
> > diff --git a/testsuite/testsuite.c b/testsuite/testsuite.c
> > index 8512b56..550c711 100644
> > --- a/testsuite/testsuite.c
> > +++ b/testsuite/testsuite.c
> > @@ -290,13 +290,64 @@ static int check_activity(int fd, bool activity, const char *path,
> > return -1;
> > }
>
> > -static inline bool test_run_parent_check_outputs(const struct test *t,
> > - int fdout, int fderr, int fdmonitor, pid_t child)
> > +#define BUFSZ 4096
> > +struct buffer {
> > + char buf[BUFSZ];
> > +};
> > +
> > +/* 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)
> > +{
>
> Not so clear (either from description) without the next patch,
> why the buffers are not local for the function.
I will amend the commit message. Besides needing it for the regex,
which is in the next patches, it's also much more efficient to
allocate the buffer once and reuse it for multiple calls to the
comparison function than allocate and free on every invocation.
>
> > + int r, rmatch, done = 0;
> > +
> > + r = read(fd, buf, sizeof(buf->buf) - 1);
>
> buf->buf?
thanks, fixed.
Lucas De Marchi
>
> > + 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);
> > + if (rmatch == 0)
> > + break;
> > +
> > + if (rmatch < 0) {
> > + if (errno == EINTR)
> > + continue;
> > + ERR("could not read match fd %d\n", fd_match);
> > + return false;
> > + }
> > +
> > + done += rmatch;
> > + }
> > +
> > + buf->buf[r] = '\0';
> > + buf_match->buf[r] = '\0';
> > +
> > + if (t->print_outputs)
> > + printf("%s: %s\n", prefix, buf->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);
> > + return false;
> > + }
> > +
> > + return true;
> > +}
> > +
> > +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;
> > unsigned long long end_usec, start_usec;
> > + _cleanup_free_ struct buffer *buf_out = NULL, *buf_err = NULL;
>
> > fd_ep = epoll_create1(EPOLL_CLOEXEC);
> > if (fd_ep < 0) {
> > @@ -320,6 +371,7 @@ static inline bool test_run_parent_check_outputs(const struct test *t,
> > ERR("could not add fd to epoll: %m\n");
> > goto out;
> > }
> > + buf_out = calloc(2, sizeof(*buf_out));
> > } else
> > fdout = -1;
>
> > @@ -340,6 +392,7 @@ static inline bool test_run_parent_check_outputs(const struct test *t,
> > ERR("could not add fd to epoll: %m\n");
> > goto out;
> > }
> > + buf_err = calloc(2, sizeof(*buf_err));
> > } else
> > fderr = -1;
>
> > @@ -374,76 +427,39 @@ static inline bool test_run_parent_check_outputs(const struct test *t,
> > }
>
> > for (i = 0; i < fdcount; i++) {
> > - int *fd = ev[i].data.ptr;
> > + int fd = *(int *)ev[i].data.ptr;
>
> > if (ev[i].events & EPOLLIN) {
> > - ssize_t r, done = 0;
> > - char buf[4096];
> > - char bufmatch[4096];
> > int fd_match;
> > + struct buffer *buf, *buf_match;
> > + const char *prefix;
>
> > - /*
> > - * compare the output from child with the one
> > - * saved as correct
> > - */
> > -
> > - r = read(*fd, buf, sizeof(buf) - 1);
> > - if (r <= 0)
> > - continue;
> > -
> > - if (*fd == fdout) {
> > + if (fd == fdout) {
> > fd_match = fd_matchout;
> > + buf = buf_out;
> > fd_activityout = true;
> > - } else if (*fd == fderr) {
> > + 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;
> > goto out;
> > }
>
> > - for (;;) {
> > - int rmatch = read(fd_match,
> > - bufmatch + done, r - done);
> > - if (rmatch == 0)
> > - break;
> > -
> > - if (rmatch < 0) {
> > - if (errno == EINTR)
> > - continue;
> > - err = -errno;
> > - ERR("could not read match fd %d\n",
> > - fd_match);
> > - goto out;
> > - }
> > -
> > - done += rmatch;
> > - }
> > + buf_match = buf + 1;
>
> > - buf[r] = '\0';
> > - bufmatch[r] = '\0';
> > -
> > - if (t->print_outputs)
> > - printf("%s: %s\n",
> > - fd_match == fd_matchout ? "STDOUT:" : "STDERR:",
> > - buf);
> > -
> > - if (!streq(buf, bufmatch)) {
> > - ERR("Outputs do not match on %s:\n",
> > - fd_match == fd_matchout ? "STDOUT" : "STDERR");
> > - ERR("correct:\n%s\n", bufmatch);
> > - ERR("wrong:\n%s\n", buf);
> > - err = -1;
> > + if (!cmpbuf_exact(t, prefix, fd, fd_match,
> > + buf, buf_match))
> > 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);
> > + if (epoll_ctl(fd_ep, EPOLL_CTL_DEL, fd, NULL) < 0) {
> > + ERR("could not remove fd %d from epoll: %m\n", fd);
> > }
> > - *fd = -1;
> > + *(int *)ev[i].data.ptr = -1;
> > }
> > }
> > }
> > --
> > 2.20.0
>
>
> --
> WBR,
> Yauheni Kaliuta
--
Lucas De Marchi
prev parent reply other threads:[~2019-01-03 20:10 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-12-19 0:02 [PATCH 1/3] testsuite: split out function to compare outputs exactly Lucas De Marchi
2018-12-19 0:02 ` [PATCH 2/3] testsuite: add support for testing output against regex Lucas De Marchi
2018-12-20 10:41 ` Yauheni Kaliuta
2019-01-03 20:41 ` Lucas De Marchi
2018-12-20 10:44 ` Yauheni Kaliuta
2019-01-03 20:50 ` Lucas De Marchi
2019-01-03 21:06 ` Yauheni Kaliuta
2019-01-03 21:14 ` Lucas De Marchi
2018-12-19 0:02 ` [PATCH 3/3] testsuite: move --show-exports test to use regex Lucas De Marchi
2018-12-20 10:38 ` [PATCH 1/3] testsuite: split out function to compare outputs exactly Yauheni Kaliuta
2019-01-03 20:09 ` Lucas De Marchi [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAKi4VALMbE563DVSfwpsrBiBuAMessk3SWgOCKYvQmARKXc=mg@mail.gmail.com' \
--to=lucas.de.marchi@gmail.com \
--cc=linux-modules@vger.kernel.org \
--cc=lucas.demarchi@intel.com \
--cc=yauheni.kaliuta@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).