[PATCH 1/3] testsuite: split out function to compare outputs exactly

[PATCH 1/3] testsuite: split out function to compare outputs exactly

[Lucas De Marchi]

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)
+{
+	int r, rmatch, done = 0;
+
+	r = read(fd, buf, sizeof(buf->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);
+		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

[PATCH 2/3] testsuite: add support for testing output against regex

[Lucas De Marchi]

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.
---
 testsuite/testsuite.c | 104 +++++++++++++++++++++++++++++++++++++++++-
 testsuite/testsuite.h |   6 +++
 2 files changed, 108 insertions(+), 2 deletions(-)

diff --git a/testsuite/testsuite.c b/testsuite/testsuite.c
index 550c711..db36324 100644
--- a/testsuite/testsuite.c
+++ b/testsuite/testsuite.c
@@ -20,6 +20,7 @@
 #include <fcntl.h>
 #include <getopt.h>
 #include <limits.h>
+#include <regex.h>
 #include <stdarg.h>
 #include <stdio.h>
 #include <stdlib.h>
@@ -293,8 +294,98 @@ static int check_activity(int fd, bool activity,  const char *path,
 #define BUFSZ 4096
 struct buffer {
 	char buf[BUFSZ];
+	unsigned int head;
 };
 
+
+static bool cmpbuf_regex_one(const char *pattern, const char *s)
+{
+	_cleanup_(regfree) regex_t re = { };
+
+	return !regcomp(&re, pattern, REG_EXTENDED|REG_NOSUB) &&
+	       !regexec(&re, s, 0, NULL, 0);
+}
+
+/*
+ * 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)
+{
+	char *p, *p_match;
+	int done = 0, done_match = 0, r;
+
+	r = read(fd, buf->buf + buf->head, sizeof(buf->buf) - buf->head - 1);
+	if (r <= 0)
+		return true;
+
+	buf->head += r;
+
+	/*
+	 * Process as many lines as read from fd and that fits in the buffer -
+	 * it's assumed that we we get N lines from fd, we should be able to
+	 * get the same amount from fd_match
+	 */
+	for (;;) {
+		p = memchr(buf->buf + done, '\n', buf->head - done);
+		if (!p)
+			break;
+		*p = 0;
+
+		p_match = memchr(buf_match->buf + done_match, '\n',
+				 buf_match->head - 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);
+			if (r <= 0) {
+				ERR("could not read match fd %d\n", fd_match);
+				return false;
+			}
+			buf_match->head += r;
+			p_match = memchr(buf_match->buf + done_match, '\n',
+					 buf_match->head - done_match);
+			if (!p_match) {
+				ERR("could not find match line from fd %d\n", 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);
+			return false;
+		}
+
+		done = p - buf->buf + 1;
+		done_match = p_match - buf_match->buf + 1;
+	}
+
+	/*
+	 * Prepare for the next call: anything we processed we remove from the
+	 * buffer by memmoving the remaining bytes up to the beginning
+	 */
+
+	if (done) {
+		if (buf->head - done)
+			memmove(buf->buf, buf->buf + done, buf->head - done);
+		buf->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;
+	}
+
+	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,
@@ -428,6 +519,7 @@ static bool test_run_parent_check_outputs(const struct test *t,
 
 		for (i = 0;  i < fdcount; i++) {
 			int fd = *(int *)ev[i].data.ptr;
+			bool ret;
 
 			if (ev[i].events & EPOLLIN) {
 				int fd_match;
@@ -452,9 +544,17 @@ static bool test_run_parent_check_outputs(const struct test *t,
 
 				buf_match = buf + 1;
 
-				if (!cmpbuf_exact(t, prefix,  fd, fd_match,
-						  buf, buf_match))
+				if (t->output.regex)
+					ret = cmpbuf_regex(t, prefix, fd, fd_match,
+							   buf, buf_match);
+				else
+					ret = cmpbuf_exact(t, prefix,  fd, fd_match,
+							   buf, buf_match);
+
+				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);
diff --git a/testsuite/testsuite.h b/testsuite/testsuite.h
index 2b31483..7ed96bf 100644
--- a/testsuite/testsuite.h
+++ b/testsuite/testsuite.h
@@ -88,6 +88,12 @@ struct test {
 		/* File with correct stderr */
 		const char *err;
 
+		/*
+		 * whether to treat the correct files as regex to the real
+		 * output
+		 */
+		bool regex;
+
 		/*
 		 * Vector with pair of files
 		 * key = correct file
-- 
2.20.0

Re: [PATCH 2/3] testsuite: add support for testing output against regex

[Yauheni Kaliuta]

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.
 > ---
 >  testsuite/testsuite.c | 104 +++++++++++++++++++++++++++++++++++++++++-
 >  testsuite/testsuite.h |   6 +++
 >  2 files changed, 108 insertions(+), 2 deletions(-)

 > diff --git a/testsuite/testsuite.c b/testsuite/testsuite.c
 > index 550c711..db36324 100644
 > --- a/testsuite/testsuite.c
 > +++ b/testsuite/testsuite.c
 > @@ -20,6 +20,7 @@
 >  #include <fcntl.h>
 >  #include <getopt.h>
 >  #include <limits.h>
 > +#include <regex.h>
 >  #include <stdarg.h>
 >  #include <stdio.h>
 >  #include <stdlib.h>
 > @@ -293,8 +294,98 @@ static int check_activity(int fd, bool activity,  const char *path,
 >  #define BUFSZ 4096
 >  struct buffer {
 >  	char buf[BUFSZ];
 > +	unsigned int head;
 >  };
 
 > +
 > +static bool cmpbuf_regex_one(const char *pattern, const char *s)
 > +{
 > +	_cleanup_(regfree) regex_t re = { };
 > +
 > +	return !regcomp(&re, pattern, REG_EXTENDED|REG_NOSUB) &&
 > +	       !regexec(&re, s, 0, NULL, 0);
 > +}
 > +
 > +/*
 > + * 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)
 > +{
 > +	char *p, *p_match;
 > +	int done = 0, done_match = 0, r;
 > +
 > +	r = read(fd, buf->buf + buf->head, sizeof(buf->buf) - buf->head - 1);

Why do you need -1 if you are replacing existing '\n' and using
mem* functions with explicit size?


 > +	if (r <= 0)
 > +		return true;
 > +
 > +	buf->head += r;
 > +
 > +	/*
 > +	 * Process as many lines as read from fd and that fits in the buffer -
 > +	 * it's assumed that we we get N lines from fd, we should be able to
                          ^^^^^^^^^ extra "we"?

 > +	 * get the same amount from fd_match
 > +	 */
 > +	for (;;) {
 > +		p = memchr(buf->buf + done, '\n', buf->head - done);
 > +		if (!p)
 > +			break;
 > +		*p = 0;

Would '\0' be better to emphasize the string end?

 > +
 > +		p_match = memchr(buf_match->buf + done_match, '\n',
 > +				 buf_match->head - 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);
 > +			if (r <= 0) {
 > +				ERR("could not read match fd %d\n", fd_match);
 > +				return false;
 > +			}
 > +			buf_match->head += r;
 > +			p_match = memchr(buf_match->buf + done_match, '\n',
 > +					 buf_match->head - done_match);
 > +			if (!p_match) {
 > +				ERR("could not find match line from fd %d\n", fd_match);
 > +				return false;
 > +			}
 > +		}
 > +		*p_match = 0;

ditto.

 > +
 > +		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);
 > +			return false;
 > +		}
 > +
 > +		done = p - buf->buf + 1;
 > +		done_match = p_match - buf_match->buf + 1;
 > +	}
 > +
 > +	/*
 > +	 * Prepare for the next call: anything we processed we remove from the
 > +	 * buffer by memmoving the remaining bytes up to the beginning
 > +	 */
 > +
 > +	if (done) {
 > +		if (buf->head - done)
 > +			memmove(buf->buf, buf->buf + done, buf->head - done);
 > +		buf->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;
 > +	}
 > +
 > +	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,
 > @@ -428,6 +519,7 @@ static bool test_run_parent_check_outputs(const struct test *t,
 
 >  		for (i = 0;  i < fdcount; i++) {
 >  			int fd = *(int *)ev[i].data.ptr;
 > +			bool ret;
 
 >  			if (ev[i].events & EPOLLIN) {
 >  				int fd_match;
 > @@ -452,9 +544,17 @@ static bool test_run_parent_check_outputs(const struct test *t,
 
 >  				buf_match = buf + 1;
 
 > -				if (!cmpbuf_exact(t, prefix,  fd, fd_match,
 > -						  buf, buf_match))
 > +				if (t->output.regex)
 > +					ret = cmpbuf_regex(t, prefix, fd, fd_match,
 > +							   buf, buf_match);
 > +				else
 > +					ret = cmpbuf_exact(t, prefix,  fd, fd_match,
 > +							   buf, buf_match);
 > +
 > +				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);
 > diff --git a/testsuite/testsuite.h b/testsuite/testsuite.h
 > index 2b31483..7ed96bf 100644
 > --- a/testsuite/testsuite.h
 > +++ b/testsuite/testsuite.h
 > @@ -88,6 +88,12 @@ struct test {
 >  		/* File with correct stderr */
 >  		const char *err;
 
 > +		/*
 > +		 * whether to treat the correct files as regex to the real
 > +		 * output
 > +		 */
 > +		bool regex;
 > +
 >  		/*
 >  		 * Vector with pair of files
 >  		 * key = correct file
 > -- 
 > 2.20.0


-- 
WBR,
Yauheni Kaliuta

Re: [PATCH 2/3] testsuite: add support for testing output against regex

[Lucas De Marchi]

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: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.
>  > ---
>  >  testsuite/testsuite.c | 104 +++++++++++++++++++++++++++++++++++++++++-
>  >  testsuite/testsuite.h |   6 +++
>  >  2 files changed, 108 insertions(+), 2 deletions(-)
>
>  > diff --git a/testsuite/testsuite.c b/testsuite/testsuite.c
>  > index 550c711..db36324 100644
>  > --- a/testsuite/testsuite.c
>  > +++ b/testsuite/testsuite.c
>  > @@ -20,6 +20,7 @@
>  >  #include <fcntl.h>
>  >  #include <getopt.h>
>  >  #include <limits.h>
>  > +#include <regex.h>
>  >  #include <stdarg.h>
>  >  #include <stdio.h>
>  >  #include <stdlib.h>
>  > @@ -293,8 +294,98 @@ static int check_activity(int fd, bool activity,  const char *path,
>  >  #define BUFSZ 4096
>  >  struct buffer {
>  >      char buf[BUFSZ];
>  > +    unsigned int head;
>  >  };
>
>  > +
>  > +static bool cmpbuf_regex_one(const char *pattern, const char *s)
>  > +{
>  > +    _cleanup_(regfree) regex_t re = { };
>  > +
>  > +    return !regcomp(&re, pattern, REG_EXTENDED|REG_NOSUB) &&
>  > +           !regexec(&re, s, 0, NULL, 0);
>  > +}
>  > +
>  > +/*
>  > + * 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)
>  > +{
>  > +    char *p, *p_match;
>  > +    int done = 0, done_match = 0, r;
>  > +
>  > +    r = read(fd, buf->buf + buf->head, sizeof(buf->buf) - buf->head - 1);
>
> Why do you need -1 if you are replacing existing '\n' and using
> mem* functions with explicit size?

so buf + head never points outside of the buffer and so avoid a
segfault. It could be rather replace by sanity checks on top of the
function though. I'm changing that.


>
>
>  > +    if (r <= 0)
>  > +            return true;
>  > +
>  > +    buf->head += r;
>  > +
>  > +    /*
>  > +     * Process as many lines as read from fd and that fits in the buffer -
>  > +     * it's assumed that we we get N lines from fd, we should be able to
>                           ^^^^^^^^^ extra "we"?

fixed
>
>  > +     * get the same amount from fd_match
>  > +     */
>  > +    for (;;) {
>  > +            p = memchr(buf->buf + done, '\n', buf->head - done);
>  > +            if (!p)
>  > +                    break;
>  > +            *p = 0;
>
> Would '\0' be better to emphasize the string end?

I use them interchangeably... no preference. I'll change it here though.

>
>  > +
>  > +            p_match = memchr(buf_match->buf + done_match, '\n',
>  > +                             buf_match->head - 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);
>  > +                    if (r <= 0) {
>  > +                            ERR("could not read match fd %d\n", fd_match);
>  > +                            return false;
>  > +                    }
>  > +                    buf_match->head += r;
>  > +                    p_match = memchr(buf_match->buf + done_match, '\n',
>  > +                                     buf_match->head - done_match);
>  > +                    if (!p_match) {
>  > +                            ERR("could not find match line from fd %d\n", fd_match);
>  > +                            return false;
>  > +                    }
>  > +            }
>  > +            *p_match = 0;
>
> ditto.

fixed

Thanks,
Lucas De Marchi

>
>  > +
>  > +            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);
>  > +                    return false;
>  > +            }
>  > +
>  > +            done = p - buf->buf + 1;
>  > +            done_match = p_match - buf_match->buf + 1;
>  > +    }
>  > +
>  > +    /*
>  > +     * Prepare for the next call: anything we processed we remove from the
>  > +     * buffer by memmoving the remaining bytes up to the beginning
>  > +     */
>  > +
>  > +    if (done) {
>  > +            if (buf->head - done)
>  > +                    memmove(buf->buf, buf->buf + done, buf->head - done);
>  > +            buf->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;
>  > +    }
>  > +
>  > +    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,
>  > @@ -428,6 +519,7 @@ static bool test_run_parent_check_outputs(const struct test *t,
>
>  >              for (i = 0;  i < fdcount; i++) {
>  >                      int fd = *(int *)ev[i].data.ptr;
>  > +                    bool ret;
>
>  >                      if (ev[i].events & EPOLLIN) {
>  >                              int fd_match;
>  > @@ -452,9 +544,17 @@ static bool test_run_parent_check_outputs(const struct test *t,
>
>  >                              buf_match = buf + 1;
>
>  > -                            if (!cmpbuf_exact(t, prefix,  fd, fd_match,
>  > -                                              buf, buf_match))
>  > +                            if (t->output.regex)
>  > +                                    ret = cmpbuf_regex(t, prefix, fd, fd_match,
>  > +                                                       buf, buf_match);
>  > +                            else
>  > +                                    ret = cmpbuf_exact(t, prefix,  fd, fd_match,
>  > +                                                       buf, buf_match);
>  > +
>  > +                            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);
>  > diff --git a/testsuite/testsuite.h b/testsuite/testsuite.h
>  > index 2b31483..7ed96bf 100644
>  > --- a/testsuite/testsuite.h
>  > +++ b/testsuite/testsuite.h
>  > @@ -88,6 +88,12 @@ struct test {
>  >              /* File with correct stderr */
>  >              const char *err;
>
>  > +            /*
>  > +             * whether to treat the correct files as regex to the real
>  > +             * output
>  > +             */
>  > +            bool regex;
>  > +
>  >              /*
>  >               * Vector with pair of files
>  >               * key = correct file
>  > --
>  > 2.20.0
>
>
> --
> WBR,
> Yauheni Kaliuta



-- 
Lucas De Marchi

Re: [PATCH 2/3] testsuite: add support for testing output against regex

[Yauheni Kaliuta]

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;
+	char buf[BUFSZ];
+	char buf_match[BUFSZ];
+	unsigned int head;
+	unsigned int head_match;
+};
+
+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

Re: [PATCH 2/3] testsuite: add support for testing output against regex

[Lucas De Marchi]

On Thu, Dec 20, 2018 at 7:01 AM Yauheni Kaliuta
<yauheni.kaliuta@redhat.com> 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

Re: [PATCH 2/3] testsuite: add support for testing output against regex

[Yauheni Kaliuta]

Hi, Lucas!

>>>>> On Thu, 3 Jan 2019 12:50:18 -0800, Lucas De Marchi  wrote:

 > On Thu, Dec 20, 2018 at 7:01 AM Yauheni Kaliuta
 > <yauheni.kaliuta@redhat.com> 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?

Still used for error reporting (in comparation functions and
check_activity()). Or what would be you proposal for that?


 >> +       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?

Sure, fine for me.


 > 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

-- 
WBR,
Yauheni Kaliuta

Re: [PATCH 2/3] testsuite: add support for testing output against regex

[Lucas De Marchi]

On Thu, Jan 3, 2019 at 1:06 PM Yauheni Kaliuta
<yauheni.kaliuta@redhat.com> wrote:
>
> Hi, Lucas!
>
> >>>>> On Thu, 3 Jan 2019 12:50:18 -0800, Lucas De Marchi  wrote:
>
>  > On Thu, Dec 20, 2018 at 7:01 AM Yauheni Kaliuta
>  > <yauheni.kaliuta@redhat.com> 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?
>
> Still used for error reporting (in comparation functions and
> check_activity()). Or what would be you proposal for that?

I'm fine with removing prefix and using stream_name everywhere.

Lucas De Marchi

>
>
>  >> +       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?
>
> Sure, fine for me.
>
>
>  > 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
>
> --
> WBR,
> Yauheni Kaliuta



-- 
Lucas De Marchi

[PATCH 3/3] testsuite: move --show-exports test to use regex

[Lucas De Marchi]

This allows it to pass if the kernel is configured with
CONFIG_MODVERSIONS.
---
 .../rootfs-pristine/test-modprobe/show-exports/correct.txt      | 2 +-
 testsuite/test-modprobe.c                                       | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/testsuite/rootfs-pristine/test-modprobe/show-exports/correct.txt b/testsuite/rootfs-pristine/test-modprobe/show-exports/correct.txt
index bc2d045..0d659ef 100644
--- a/testsuite/rootfs-pristine/test-modprobe/show-exports/correct.txt
+++ b/testsuite/rootfs-pristine/test-modprobe/show-exports/correct.txt
@@ -1 +1 @@
-0x00000000	printA
+0x[0-9a-fA-F]+	printA
diff --git a/testsuite/test-modprobe.c b/testsuite/test-modprobe.c
index 52a6621..1cace82 100644
--- a/testsuite/test-modprobe.c
+++ b/testsuite/test-modprobe.c
@@ -114,6 +114,7 @@ DEFINE_TEST(modprobe_show_exports,
 	},
 	.output = {
 		.out = TESTSUITE_ROOTFS "test-modprobe/show-exports/correct.txt",
+		.regex = true,
 	});
 
 
-- 
2.20.0

Re: [PATCH 1/3] testsuite: split out function to compare outputs exactly

[Yauheni Kaliuta]

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.

 > +	int r, rmatch, done = 0;
 > +
 > +	r = read(fd, buf, sizeof(buf->buf) - 1);

                     buf->buf?

 > +	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

Re: [PATCH 1/3] testsuite: split out function to compare outputs exactly

[Lucas De Marchi]

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