[PATCH 0/3] CI: Improvements to *-tools-test-* jobs

[PATCH 0/3] CI: Improvements to *-tools-test-* jobs

[Andrew Cooper]

Rearrange tools/tests to be more ameanable to running in CI, and drop the
special casing holding it together.



Andrew Cooper (3):
  tools/tests: Drop depriv-fd-checker
  tools/tests: Install tests into $(LIBEXEC)/tests
  CI: Drop custom handling of tools/tests

 .gitignore                             |   1 -
 automation/scripts/build               |   1 -
 automation/scripts/qubes-x86-64.sh     |   7 +-
 automation/scripts/run-tools-tests     |  43 ++-
 tools/tests/Makefile                   |   1 -
 tools/tests/cpu-policy/Makefile        |   6 +-
 tools/tests/depriv/Makefile            |  52 ---
 tools/tests/depriv/depriv-fd-checker.c | 436 -------------------------
 tools/tests/paging-mempool/Makefile    |   6 +-
 tools/tests/rangeset/Makefile          |   6 +-
 tools/tests/resource/Makefile          |   6 +-
 tools/tests/tsx/Makefile               |   6 +-
 tools/tests/xenstore/Makefile          |   6 +-
 13 files changed, 40 insertions(+), 537 deletions(-)
 delete mode 100644 tools/tests/depriv/Makefile
 delete mode 100644 tools/tests/depriv/depriv-fd-checker.c

-- 
2.39.5

[PATCH 1/3] tools/tests: Drop depriv-fd-checker

[Andrew Cooper]

Unlike the other tests, this is not standalone.  It requires poking at a live
system, making it unweildly to use.

It hasn't been touched in 7 years, despite changes in libraries and kernel
devices using the deprivilege infrastructure.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Anthony PERARD <anthony.perard@vates.tech>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
 .gitignore                             |   1 -
 tools/tests/Makefile                   |   1 -
 tools/tests/depriv/Makefile            |  52 ---
 tools/tests/depriv/depriv-fd-checker.c | 436 -------------------------
 4 files changed, 490 deletions(-)
 delete mode 100644 tools/tests/depriv/Makefile
 delete mode 100644 tools/tests/depriv/depriv-fd-checker.c

diff --git a/.gitignore b/.gitignore
index 53f5df000383..4a4e20680464 100644
--- a/.gitignore
+++ b/.gitignore
@@ -165,7 +165,6 @@ tools/misc/xencov
 tools/pkg-config/*
 tools/qemu-xen-build
 tools/xentrace/xenalyze
-tools/tests/depriv/depriv-fd-checker
 tools/tests/x86_emulator/*.bin
 tools/tests/x86_emulator/*.tmp
 tools/tests/x86_emulator/32/x86_emulate
diff --git a/tools/tests/Makefile b/tools/tests/Makefile
index 3e60ab6b268d..36928676a666 100644
--- a/tools/tests/Makefile
+++ b/tools/tests/Makefile
@@ -9,7 +9,6 @@ ifneq ($(clang),y)
 SUBDIRS-$(CONFIG_X86) += x86_emulator
 endif
 SUBDIRS-y += xenstore
-SUBDIRS-y += depriv
 SUBDIRS-y += rangeset
 SUBDIRS-y += vpci
 SUBDIRS-y += paging-mempool
diff --git a/tools/tests/depriv/Makefile b/tools/tests/depriv/Makefile
deleted file mode 100644
index 5404a12f4780..000000000000
--- a/tools/tests/depriv/Makefile
+++ /dev/null
@@ -1,52 +0,0 @@
-XEN_ROOT=$(CURDIR)/../../..
-include $(XEN_ROOT)/tools/Rules.mk
-
-CFLAGS += $(CFLAGS_xeninclude)
-CFLAGS += $(CFLAGS_libxenctrl)
-CFLAGS += $(CFLAGS_libxencall)
-CFLAGS += $(CFLAGS_libxenevtchn)
-CFLAGS += $(CFLAGS_libxengnttab)
-CFLAGS += $(CFLAGS_libxenforeignmemory)
-CFLAGS += $(CFLAGS_libxendevicemodel)
-CFLAGS += $(CFLAGS_libxentoolcore)
-CFLAGS += $(CFLAGS_libxentoollog)
-
-LDLIBS += $(LDLIBS_xeninclude)
-LDLIBS += $(LDLIBS_libxenctrl)
-LDLIBS += $(LDLIBS_libxencall)
-LDLIBS += $(LDLIBS_libxenevtchn)
-LDLIBS += $(LDLIBS_libxengnttab)
-LDLIBS += $(LDLIBS_libxenforeignmemory)
-LDLIBS += $(LDLIBS_libxendevicemodel)
-LDLIBS += $(LDLIBS_libxentoolcore)
-LDLIBS += $(LDLIBS_libxentoollog)
-
-INSTALL_PRIVBIN-y += depriv-fd-checker
-INSTALL_PRIVBIN := $(INSTALL_PRIVBIN-y)
-TARGETS += $(INSTALL_PRIVBIN)
-
-.PHONY: all
-all: build
-
-.PHONY: build
-build: $(TARGETS)
-
-.PHONY: clean
-clean:
-	$(RM) *.o $(TARGETS) *~ $(DEPS_RM)
-
-.PHONY: distclean
-distclean: clean
-
-depriv-fd-checker: depriv-fd-checker.o
-	$(CC) $(LDFLAGS) -o $@ $< $(LDLIBS) $(APPEND_LDFLAGS)
-
-install: all
-	$(INSTALL_DIR) $(DESTDIR)$(LIBEXEC_BIN)
-	$(INSTALL_PROG) $(INSTALL_PRIVBIN) $(DESTDIR)$(LIBEXEC_BIN)
-
-.PHONY: uninstall
-uninstall:
-	rm -f $(addprefix $(DESTDIR)$(LIBEXEC_BIN)/, $(INSTALL_PRIVBIN))
-
--include $(DEPS_INCLUDE)
diff --git a/tools/tests/depriv/depriv-fd-checker.c b/tools/tests/depriv/depriv-fd-checker.c
deleted file mode 100644
index 98a27a03d543..000000000000
--- a/tools/tests/depriv/depriv-fd-checker.c
+++ /dev/null
@@ -1,436 +0,0 @@
-/*
- * depriv-fd-checker
- *
- * utility to check whether file descriptor(s) are deprivileged
- *
- * usage:
- *  .../depriv-fd-checker CLASS FD X-INFO [CLASS FD X-INFO...]
- *
- * CLASS is one of:
- *    privcmd gntdev evtchn     FD should be appropriate Xen control fd
- *    readonly                  FD is expected to be readonly
- *    appendonly                FD is expected to be append write only
- #    tun                       FD is expected to be an open tun device
- *
- * In each case FD is probably a reference to an open-file stolen
- * from another process, eg by the use of fishdescriptor.
- *
- * X-INFO is simply appended to the discursive reportage.
- *
- * It is an error if depriv-fd-checker cannot open the control
- * facilities itself, or something goes wrong with checking, or an FD
- * is entirely the wrong kind for the specified CLASS.  Otherwise:
- *
- * depriv-fd-checker will perhaps print, for each triplet:
- *   CLASS checking FD INFORMATION... X-INFO
- * and in any case print, for each triplet, exactly one of:
- *   CLASS pass|fail FD INFORMATION... X-INFO
- *   tun maybe FD IFNAME X-INFO
- *
- * "pass" means that the descriptor was restricted as expected.
- * "fail" means that the descriptor was unrestricted.
- * "maybe" means that further information is printed, as detailed above,
- *         and the caller should check that it is as expected
- */
-/*
- * Copyright (C)2018 Citrix Systems R&D
- *
- * This program is free software; you can redistribute it and/or
- * modify it under the terms of the GNU Lesser General Public License
- * as published by the Free Software Foundation; version 2.1 of the
- * License.
- *
- * This library is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
- * Lesser General Public License for more details.
- *
- * You should have received a copy of the GNU Lesser General Public
- * License along with this library; If not, see
- * <http://www.gnu.org/licenses/>.
- */
-
-#include <stdlib.h>
-#include <errno.h>
-#include <string.h>
-#include <stdio.h>
-#include <assert.h>
-#include <string.h>
-#include <unistd.h>
-#include <fcntl.h>
-#include <poll.h>
-
-#include <err.h>
-
-#include <xenctrl.h>
-#include <xencall.h>
-#include <xengnttab.h>
-#include <xenevtchn.h>
-
-/*
- * Every class needs setup.  setup is called once per class at program
- * startup.
- *
- * Then it can have
- *     open test getfd close
- * In which case the core code will for every fd
- *     open test getfd dup2 test close
- * And test should call blocked or succeeded and then immediately
- * return, or error out
- *
- * Or it can have
- *     check
- * which should call report, or error out
- *
- * Errors: use trouble for simple syscall errors.  Or use err or errx
- * and maybe print fd_desc and test_which, according to the comments
- * in struct classinfo.
- */
-
-static xentoollog_logger *logger;
-
-static int object_fd;
-static const char *classname;
-static const char *fd_desc;
-static const char *test_which;
-
-static const char *test_wh_unrest = "test (unrestricted)";
-static const char *test_wh_rest   = "test (restricted)";
-
-
-static void trouble(const char *what) __attribute__((noreturn));
-static void trouble(const char *what) {
-    fprintf(stderr,
-	    "trouble: %s %s %d (%s) %s: %s\n",
-	    classname, test_which, object_fd, fd_desc, what, strerror(errno));
-    exit(-1);
-}
-
-static void report(const char *pass_or_fail, const char *what,
-		   const char *notes) {
-    printf("%s %s %d %s (%s) %s\n",
-	   classname, pass_or_fail,
-	   object_fd, what, notes, fd_desc);
-    if (ferror(stdout) || fflush(stdout)) err(16,"stdout");
-}
-
-static void succeeded(const char *what) {
-    if (test_which == test_wh_unrest) {
-	/* ok */
-	test_which = 0;
-    } else if (test_which == test_wh_rest) {
-	report("fail",what,"unexpectedly succeeded");
-	test_which = 0;
-    } else {
-	abort();
-    }
-}
-
-static void blocked(const char *what) {
-    if (test_which == test_wh_rest) {
-	/* yay */
-	report("pass", what,"blocked");
-	test_which = 0;
-    } else if (test_which == test_wh_unrest) {
-	err(4,"test blocked on unrestricted fd: %s {%s}",what,test_which);
-    } else {
-	abort();
-    }
-}
-
-/* privcmd */
-
-static xc_interface *xch;
-static void setup_privcmd(void) { }
-static void open_privcmd(void) {
-    xch = xc_interface_open(logger,0,0);
-    if (!xch) trouble("xc_interface_open");
-}
-static void test_privcmd(void) {
-    int r = xc_get_online_cpus(xch);
-    if (r>0)
-	succeeded("xc_get_online_cpus");
-    else if (r==0)
-	errx(-1,"xc_get_online_cpus{%s, %s}=0", test_which, fd_desc);
-    else if (errno==EPERM || errno==EACCES)
-	blocked("xc_get_online_cpus");
-    else
-	trouble("xc_get_online_cpus");
-}
-static int getfd_privcmd(void) {
-    return xencall_fd(xc_interface_xcall_handle(xch));
-}
-static void close_privcmd(void) {
-    xc_interface_close(xch);
-}
-
-/* gntdev */
-
-static xengntshr_handle *xgs;
-static uint32_t gntshr_gref;
-static xengnttab_handle *xgt;
-static void setup_gntdev(void) {
-    void *r;
-    xgs = xengntshr_open(logger,0);
-    if (!xgs) trouble("xengntshr_open");
-    r = xengntshr_share_pages(xgs, 0, 1, &gntshr_gref, 1);
-    if (!r || r==(void*)-1) trouble("xengntshr_share_pages");
-    memset(r, 0x55, XC_PAGE_SIZE);
-}
-static void open_gntdev(void) {
-    xgt = xengnttab_open(logger,0);
-    if (!xgt) trouble("xengnttab_open");
-}
-static void test_gntdev(void) {
-    char mybuf[XC_PAGE_SIZE];
-    memset(mybuf, 0xaa, XC_PAGE_SIZE);
-    xengnttab_grant_copy_segment_t seg;
-    seg.source.foreign.ref = gntshr_gref;
-    seg.source.foreign.offset = 0;
-    seg.source.foreign.domid = 0;
-    seg.dest.virt = mybuf;
-    seg.len = 1;
-    seg.flags = GNTCOPY_source_gref;
-    for (;;) {
-	seg.status = 0;
-	int r = xengnttab_grant_copy(xgt,1,&seg);
-	if (r<0) {
-	    if (errno==EPERM || errno==EACCES || errno==ENOTTY)
-		blocked("xengnttab_grant_copy");
-	    else
-		trouble("xengnttab_grant_copy");
-	} else if (r==0) {
-	    if (seg.status==GNTST_okay)
-		succeeded("xengnttab_grant_copy okay");
-	    else if (seg.status==GNTST_eagain)
-		continue;
-	    else errx(-1,"xengnttab_grant_copy=%d {%s, %s} but .status=%d",
-		      r, test_which, fd_desc,(int)seg.status);
-	} else {
-	    errx(-1,"xengnttab_grant_copy=%d {%s, %s}",
-		 r, test_which, fd_desc);
-	}
-	break;
-    }
-}
-static int getfd_gntdev(void) {
-    return xengnttab_fd(xgt);
-}
-static void close_gntdev(void) {
-    xengnttab_close(xgt);
-}
-
-/* evtchn */
-
-static xenevtchn_handle *xce_recip, *xce;
-static void setup_evtchn(void) {
-    xce_recip = xenevtchn_open(logger, 0);
-    if (!xce_recip) err(-1,"xenevtchn_open (donor)");
-}
-static void open_evtchn(void) {
-    xce = xenevtchn_open(logger, 0);
-    if (!xce) err(-1,"xenevtchn_open");
-}
-static void test_evtchn(void) {
-    xenevtchn_port_or_error_t
-        recip_port=-1, test_unbound_port=-1, test_send_port=-1;
-
-    recip_port = xenevtchn_bind_unbound_port(xce_recip, 0);
-    if (recip_port < 0) trouble("xenevtchn_bind_unbound_port");
-
-    test_unbound_port = xenevtchn_bind_unbound_port(xce, 0);
-    if (test_unbound_port >= 0) {
-        succeeded("xenevtchn_bind_unbound_port");
-        goto out;
-    }
-
-    test_send_port = xenevtchn_bind_interdomain(xce, 0, recip_port);
-    /* bind_interdomain marks the channel pending */
-    struct pollfd pfd;
-    for (;;) {
-        pfd.fd = xenevtchn_fd(xce_recip);
-        pfd.events = POLLIN;
-        pfd.revents = 0;
-        int r = poll(&pfd,1,0);
-        if (r>=0) break;
-        if (errno!=EINTR) err(-1,"poll(xce_recip)");
-    }
-    if (pfd.revents & POLLIN) {
-        xenevtchn_port_or_error_t p3 = xenevtchn_pending(xce_recip);
-        if (p3 < 0) err(-1,"xenevtchn_pending(check)");
-        if (p3 != recip_port)
-            errx(-1,"xenevtchn_pending=%d expected %d",p3,recip_port);
-        xenevtchn_unmask(xce_recip, recip_port);
-    }
-
-    if (test_send_port>=0 && (pfd.revents & POLLIN)) {
-        succeeded("xenevtchn_bind_interdomain/poll");
-        /* we make no attempt to undo what we did to this stolen fd;
-         * the rightful owner will see a spurious event on test_send_port */
-    } else if (test_send_port==-1 && !(pfd.revents & POLLIN) &&
-               (errno==EPERM || errno==EACCES || errno==ENOTTY)) {
-	blocked("xenevtchn_notify");
-    } else {
-        err(-1,"%s %s xenevtchn_bind_interdomain=%d .revents=0x%x",
-             test_which, fd_desc, test_send_port, pfd.revents);
-    }
-
- out:
-    if (recip_port        > 0) xenevtchn_unbind(xce, recip_port);
-    if (test_unbound_port > 0) xenevtchn_unbind(xce, test_unbound_port);
-    if (test_send_port    > 0) xenevtchn_unbind(xce, test_send_port);
-}
-static int getfd_evtchn(void) {
-    return xenevtchn_fd(xce);
-}
-static void close_evtchn(void) {
-    xenevtchn_close(xce);
-}
-
-/* fcntl */
-
-#define CHECK_FCNTL(openmode)				\
-    int r = fcntl(object_fd, F_GETFL);			\
-    if (r < 0) trouble("fcntl F_GETFL");		\
-    int m = r & (O_RDONLY | O_WRONLY | O_RDWR);		\
-							\
-    char mbuf[100 + 30*3];				\
-    snprintf(mbuf,sizeof(mbuf),				\
-	     "F_GETFL=%#o m=%#o " #openmode "=%#o",	\
-	     r,m,(int)openmode);			\
-							\
-    if (m != openmode) {				\
-	report("fail", #openmode, mbuf);		\
-	return;						\
-    }
-
-/* readonly */
-
-static void setup_readonly(void) { }
-static void check_readonly(void) {
-    CHECK_FCNTL(O_RDONLY);
-    report("pass", "fcntl", mbuf);
-}
-
-/* appendonly */
-
-static void setup_appendonly(void) { }
-static void check_appendonly(void) {
-    CHECK_FCNTL(O_WRONLY);
-    if (!(r & O_APPEND)) {
-	report("fail", "O_APPEND", mbuf);
-	return;
-    }
-    report("pass", "fcntl", mbuf);
-}
-
-#if defined(__linux__)
-#include <sys/ioctl.h>
-#include <sys/types.h>
-#include <sys/socket.h>
-#include <linux/if.h>
-#include <linux/if_tun.h>
-#ifndef TUNGETIFF
-#define TUNGETIFF _IOR('T', 210, unsigned int)
-#endif
-
-/* linux tun */
-
-static void setup_tun(void) { }
-static void check_tun(void) {
-    struct ifreq ifr;
-    int r;
-
-    memset(&ifr,0,sizeof(ifr));
-    r = ioctl(object_fd, TUNGETIFF, (void*)&ifr);
-    if (r<0) trouble("TUNGETIFF");
-    printf("tun maybe %d %.*s %s\n", object_fd,
-           (int)IFNAMSIZ, ifr.ifr_ifrn.ifrn_name,
-           fd_desc);
-}
-
-#define PLATFORM_CLASSES \
-    DEFCHECK(tun),
-
-#else /* !defined(__linux__) */
-#define PLATFORM_CLASSES /* empty */
-#endif
-
-/* class table and main program */
-
-#define DEFCLASS(cl) \
-    { #cl, setup_##cl, 0, open_##cl, test_##cl, getfd_##cl, close_##cl }
-#define DEFCHECK(meth) \
-    { #meth, setup_##meth, check_##meth }
-
-static const struct classinfo {
-    const char *name;     /* errors: print fd_desc   test_which */
-    void (*setup)(void);  /*               best not   best not  */
-    void (*check)(void);  /*               must       may       */
-    void (*open)(void);   /*               must       may       */
-    void (*test)(void);   /*               must       must      */
-    int (*getfd)(void);   /*               must       may       */
-    void (*close)(void);  /*               must       may       */
-} classinfos[] = {
-    DEFCLASS(privcmd),
-    DEFCLASS(gntdev),
-    DEFCLASS(evtchn),
-    DEFCHECK(readonly),
-    DEFCHECK(appendonly),
-    PLATFORM_CLASSES
-    { 0 }
-};
-
-int main(int argc, char **argv) {
-    const struct classinfo *cli;
-    int r;
-
-    argv++;
-
-    logger = (xentoollog_logger*)xtl_createlogger_stdiostream
-	(stderr, XTL_NOTICE, XTL_STDIOSTREAM_HIDE_PROGRESS);
-
-    fd_desc = "setup";
-    test_which = "setup";
-    for (cli = classinfos; cli->name; cli++)
-	cli->setup();
-
-    while ((classname = *argv++)) {
-	if (!*argv) errx(8,"need fd after class");
-	object_fd = atoi(*argv++);
-
-	fd_desc = *argv++;
-	if (!fd_desc) errx(8,"need info after fd");
-
-	for (cli = classinfos; cli->name; cli++)
-	    if (!strcmp(cli->name, classname))
-		goto found;
-	report("fail","unknown class","");
-	continue;
-
-    found:
-	if (cli->check) {
-	    report("checking","check","in progress");
-	    test_which = "check";
-	    cli->check();
-	} else {
-	    test_which = "open";
-	    report("checking","dup-hack","in progress");
-                                                  cli->open();
-
-	    test_which = test_wh_unrest;          cli->test();
-	    assert(!test_which);
-
-	    test_which = "getfd"; int intern_fd = cli->getfd();
-	    r = dup2(object_fd, intern_fd);
-	    if (r != intern_fd) err(-1, "dup2");
-
-	    test_which = test_wh_rest;             cli->test();
-	    assert(!test_which);
-
-	    test_which = "close";                  cli->close();
-	}
-    }
-
-    return 0;
-}
-- 
2.39.5

[PATCH 2/3] tools/tests: Install tests into $(LIBEXEC)/tests

[Andrew Cooper]

$(LIBEXEC_BIN) is a dumping ground of many things.  Separate the "clearly
tests" from everything else so we can clean up how they're run in CI.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Anthony PERARD <anthony.perard@vates.tech>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
 tools/tests/cpu-policy/Makefile     | 6 +++---
 tools/tests/paging-mempool/Makefile | 6 +++---
 tools/tests/rangeset/Makefile       | 6 +++---
 tools/tests/resource/Makefile       | 6 +++---
 tools/tests/tsx/Makefile            | 6 +++---
 tools/tests/xenstore/Makefile       | 6 +++---
 6 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/tools/tests/cpu-policy/Makefile b/tools/tests/cpu-policy/Makefile
index 5df9b1ebbd7e..24f87e2eca2a 100644
--- a/tools/tests/cpu-policy/Makefile
+++ b/tools/tests/cpu-policy/Makefile
@@ -29,12 +29,12 @@ distclean: clean
 
 .PHONY: install
 install: all
-	$(INSTALL_DIR) $(DESTDIR)$(LIBEXEC_BIN)
-	$(if $(TARGETS),$(INSTALL_PROG) $(TARGETS) $(DESTDIR)$(LIBEXEC_BIN))
+	$(INSTALL_DIR) $(DESTDIR)$(LIBEXEC)/tests
+	$(if $(TARGETS),$(INSTALL_PROG) $(TARGETS) $(DESTDIR)$(LIBEXEC)/tests)
 
 .PHONY: uninstall
 uninstall:
-	$(RM) -- $(addprefix $(DESTDIR)$(LIBEXEC_BIN)/,$(TARGETS))
+	$(RM) -- $(addprefix $(DESTDIR)$(LIBEXEC)/tests/,$(TARGETS))
 
 CFLAGS += -D__XEN_TOOLS__
 CFLAGS += $(CFLAGS_xeninclude)
diff --git a/tools/tests/paging-mempool/Makefile b/tools/tests/paging-mempool/Makefile
index a081b3baa514..a1e12584ce80 100644
--- a/tools/tests/paging-mempool/Makefile
+++ b/tools/tests/paging-mempool/Makefile
@@ -16,12 +16,12 @@ distclean: clean
 
 .PHONY: install
 install: all
-	$(INSTALL_DIR) $(DESTDIR)$(LIBEXEC_BIN)
-	$(INSTALL_PROG) $(TARGET) $(DESTDIR)$(LIBEXEC_BIN)
+	$(INSTALL_DIR) $(DESTDIR)$(LIBEXEC)/tests
+	$(INSTALL_PROG) $(TARGET) $(DESTDIR)$(LIBEXEC)/tests
 
 .PHONY: uninstall
 uninstall:
-	$(RM) -- $(DESTDIR)$(LIBEXEC_BIN)/$(TARGET)
+	$(RM) -- $(DESTDIR)$(LIBEXEC)/tests/$(TARGET)
 
 CFLAGS += $(CFLAGS_xeninclude)
 CFLAGS += $(CFLAGS_libxenctrl)
diff --git a/tools/tests/rangeset/Makefile b/tools/tests/rangeset/Makefile
index 3dafcbd0546c..e3bfce471cd3 100644
--- a/tools/tests/rangeset/Makefile
+++ b/tools/tests/rangeset/Makefile
@@ -20,12 +20,12 @@ distclean: clean
 
 .PHONY: install
 install: all
-	$(INSTALL_DIR) $(DESTDIR)$(LIBEXEC_BIN)
-	$(INSTALL_PROG) $(TARGET) $(DESTDIR)$(LIBEXEC_BIN)
+	$(INSTALL_DIR) $(DESTDIR)$(LIBEXEC)/tests
+	$(INSTALL_PROG) $(TARGET) $(DESTDIR)$(LIBEXEC)/tests
 
 .PHONY: uninstall
 uninstall:
-	$(RM) -- $(addprefix $(DESTDIR)$(LIBEXEC_BIN)/,$(TARGET))
+	$(RM) -- $(addprefix $(DESTDIR)$(LIBEXEC)/tests/,$(TARGET))
 
 list.h: $(XEN_ROOT)/xen/include/xen/list.h
 rangeset.h: $(XEN_ROOT)/xen/include/xen/rangeset.h
diff --git a/tools/tests/resource/Makefile b/tools/tests/resource/Makefile
index a5856bf09590..09d678fffe3e 100644
--- a/tools/tests/resource/Makefile
+++ b/tools/tests/resource/Makefile
@@ -20,12 +20,12 @@ distclean: clean
 
 .PHONY: install
 install: all
-	$(INSTALL_DIR) $(DESTDIR)$(LIBEXEC_BIN)
-	$(INSTALL_PROG) $(TARGET) $(DESTDIR)$(LIBEXEC_BIN)
+	$(INSTALL_DIR) $(DESTDIR)$(LIBEXEC)/tests
+	$(INSTALL_PROG) $(TARGET) $(DESTDIR)$(LIBEXEC)/tests
 
 .PHONY: uninstall
 uninstall:
-	$(RM) -- $(DESTDIR)$(LIBEXEC_BIN)/$(TARGET)
+	$(RM) -- $(DESTDIR)$(LIBEXEC)/tests/$(TARGET)
 
 CFLAGS += $(CFLAGS_xeninclude)
 CFLAGS += $(CFLAGS_libxenctrl)
diff --git a/tools/tests/tsx/Makefile b/tools/tests/tsx/Makefile
index a4f516b72597..0bb7e7010347 100644
--- a/tools/tests/tsx/Makefile
+++ b/tools/tests/tsx/Makefile
@@ -16,12 +16,12 @@ distclean: clean
 
 .PHONY: install
 install: all
-	$(INSTALL_DIR) $(DESTDIR)$(LIBEXEC_BIN)
-	$(INSTALL_PROG) $(TARGET) $(DESTDIR)$(LIBEXEC_BIN)
+	$(INSTALL_DIR) $(DESTDIR)$(LIBEXEC)/tests
+	$(INSTALL_PROG) $(TARGET) $(DESTDIR)$(LIBEXEC)/tests
 
 .PHONY: uninstall
 uninstall:
-	$(RM) -- $(DESTDIR)$(LIBEXEC_BIN)/$(TARGET)
+	$(RM) -- $(DESTDIR)$(LIBEXEC)/tests/$(TARGET)
 
 .PHONY: uninstall
 uninstall:
diff --git a/tools/tests/xenstore/Makefile b/tools/tests/xenstore/Makefile
index 202dda0d3c23..2ee4a1327e75 100644
--- a/tools/tests/xenstore/Makefile
+++ b/tools/tests/xenstore/Makefile
@@ -20,12 +20,12 @@ distclean: clean
 
 .PHONY: install
 install: all
-	$(INSTALL_DIR) $(DESTDIR)$(LIBEXEC_BIN)
-	$(if $(TARGETS),$(INSTALL_PROG) $(TARGETS) $(DESTDIR)$(LIBEXEC_BIN))
+	$(INSTALL_DIR) $(DESTDIR)$(LIBEXEC)/tests
+	$(if $(TARGETS),$(INSTALL_PROG) $(TARGETS) $(DESTDIR)$(LIBEXEC)/tests)
 
 .PHONY: uninstall
 uninstall:
-	$(RM) -- $(addprefix $(DESTDIR)$(LIBEXEC_BIN)/,$(TARGETS))
+	$(RM) -- $(addprefix $(DESTDIR)$(LIBEXEC)/tests/,$(TARGETS))
 
 CFLAGS += $(CFLAGS_libxenstore)
 CFLAGS += $(APPEND_CFLAGS)
-- 
2.39.5

[PATCH 3/3] CI: Drop custom handling of tools/tests

[Andrew Cooper]

... and use them from their installed location.

The full recusive copy of tools/tests brings in all build and intermediate
artefacts.  e.g. for test-tsx alone:

  ./tests/tsx
  ./tests/tsx/.test-tsx.o.d
  ./tests/tsx/test-tsx.o
  ./tests/tsx/.gitignore
  ./tests/tsx/test-tsx
  ./tests/tsx/Makefile
  ./tests/tsx/test-tsx.c

duplicating the test binary which is also in ./usr/lib/xen/tests

Rewrite run-tools-tests to run tests from their installed
location (/usr/lib/xen/tests in alpine), which effectively removes the outer
loop over $dir.

No practical change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Anthony PERARD <anthony.perard@vates.tech>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

This doesn't change any tests that run, although in the XML we get two fewer skips.

Both skips can be fixed by giving vpci and x86_emulator some install targets
---
 automation/scripts/build           |  1 -
 automation/scripts/qubes-x86-64.sh |  7 +++--
 automation/scripts/run-tools-tests | 43 +++++++++++++-----------------
 3 files changed, 22 insertions(+), 29 deletions(-)

diff --git a/automation/scripts/build b/automation/scripts/build
index cdb8cd7c722b..0e7494ff6d87 100755
--- a/automation/scripts/build
+++ b/automation/scripts/build
@@ -109,6 +109,5 @@ else
     # even though dist/ contains everything, while some containers don't even
     # build Xen
     (cd dist/install; find | cpio -o -H newc | gzip) > binaries/xen-tools.cpio.gz
-    cp -r tools/tests binaries/
     collect_xen_artefacts
 fi
diff --git a/automation/scripts/qubes-x86-64.sh b/automation/scripts/qubes-x86-64.sh
index aa47ba6bf5c0..577a00238a75 100755
--- a/automation/scripts/qubes-x86-64.sh
+++ b/automation/scripts/qubes-x86-64.sh
@@ -136,7 +136,7 @@ done
         passed="test passed"
         domU_check=""
         dom0_check="
-/tests/run-tools-tests /tests /tmp/tests-junit.xml && echo \"${passed}\"
+/root/run-tools-tests /usr/lib/xen/tests /tmp/tests-junit.xml && echo \"${passed}\"
 nc -l -p 8080 < /tmp/tests-junit.xml >/dev/null &
 "
         if [ "${test_variant}" = "tools-tests-pvh" ]; then
@@ -195,9 +195,8 @@ cat binaries/xen-tools.cpio.gz >> binaries/dom0-rootfs.cpio.gz
 # test-local configuration
 mkdir -p rootfs
 cd rootfs
-mkdir -p boot etc/local.d
-cp -ar ../binaries/tests .
-cp -a ../automation/scripts/run-tools-tests tests/
+mkdir -p boot etc/local.d root
+cp -a ../automation/scripts/run-tools-tests root/
 
 echo "#!/bin/bash
 
diff --git a/automation/scripts/run-tools-tests b/automation/scripts/run-tools-tests
index 770e97c3e943..8d7aa8fa5140 100755
--- a/automation/scripts/run-tools-tests
+++ b/automation/scripts/run-tools-tests
@@ -12,30 +12,25 @@ printf '<?xml version="1.0" encoding="UTF-8"?>\n' > "$xml_out"
 printf '<testsuites name="tools.tests">\n' >> "$xml_out"
 printf ' <testsuite name="tools.tests">\n' >> "$xml_out"
 failed=
-for dir in "$1"/*; do
-    [ -d "$dir" ] || continue
-    echo "Running test in $dir"
-    printf '  <testcase name="%s">\n' "$dir" >> "$xml_out"
-    ret=
-    for f in "$dir"/*; do
-        [ -f "$f" ] || continue
-        [ -x "$f" ] || continue
-        "$f" 2>&1 | tee /tmp/out
-        ret=$?
-        if [ "$ret" -ne 0 ]; then
-            echo "FAILED: $ret"
-            failed+=" $dir"
-            printf '   <failure type="failure" message="binary %s exited with code %d">\n' "$f" "$ret" >> "$xml_out"
-            # TODO: could use xml escaping... but current tests seems to
-            # produce sane output
-            cat /tmp/out >> "$xml_out"
-            printf '   </failure>\n' >> "$xml_out"
-        else
-            echo "PASSED"
-        fi
-    done
-    if [ -z "$ret" ]; then
-        printf '   <skipped type="skipped" message="no executable test found in %s"/>\n' "$dir" >> "$xml_out"
+for f in "$1"/*; do
+    if [ -x "$f" ]; then
+        echo "SKIP: $f not executable"
+        continue
+    fi
+    echo "Running $f"
+    printf '  <testcase name="%s">\n' "$f" >> "$xml_out"
+    "$f" 2>&1 | tee /tmp/out
+    ret=$?
+    if [ "$ret" -ne 0 ]; then
+        echo "FAILED: $f"
+        failed+=" $f"
+        printf '   <failure type="failure" message="binary %s exited with code %d">\n' "$f" "$ret" >> "$xml_out"
+        # TODO: could use xml escaping... but current tests seems to
+        # produce sane output
+        cat /tmp/out >> "$xml_out"
+        printf '   </failure>\n' >> "$xml_out"
+    else
+        echo "PASSED"
     fi
     printf '  </testcase>\n' >> "$xml_out"
 done
-- 
2.39.5

Re: [PATCH 0/3] CI: Improvements to *-tools-test-* jobs

[dmkhn]

On Tue, May 20, 2025 at 09:52:36PM +0100, Andrew Cooper wrote:
> Rearrange tools/tests to be more ameanable to running in CI, and drop the
> special casing holding it together.
> 
> 
> 
> Andrew Cooper (3):
>   tools/tests: Drop depriv-fd-checker
>   tools/tests: Install tests into $(LIBEXEC)/tests
>   CI: Drop custom handling of tools/tests

The code changes look good to me

Reviewed-by: Denis Mukhin <dmukhin@ford.com>

for the series.

> 
>  .gitignore                             |   1 -
>  automation/scripts/build               |   1 -
>  automation/scripts/qubes-x86-64.sh     |   7 +-
>  automation/scripts/run-tools-tests     |  43 ++-
>  tools/tests/Makefile                   |   1 -
>  tools/tests/cpu-policy/Makefile        |   6 +-
>  tools/tests/depriv/Makefile            |  52 ---
>  tools/tests/depriv/depriv-fd-checker.c | 436 -------------------------
>  tools/tests/paging-mempool/Makefile    |   6 +-
>  tools/tests/rangeset/Makefile          |   6 +-
>  tools/tests/resource/Makefile          |   6 +-
>  tools/tests/tsx/Makefile               |   6 +-
>  tools/tests/xenstore/Makefile          |   6 +-
>  13 files changed, 40 insertions(+), 537 deletions(-)
>  delete mode 100644 tools/tests/depriv/Makefile
>  delete mode 100644 tools/tests/depriv/depriv-fd-checker.c
> 
> --
> 2.39.5
> 
> 

Re: [PATCH 1/3] tools/tests: Drop depriv-fd-checker

[Anthony PERARD]

On Tue, May 20, 2025 at 09:52:37PM +0100, Andrew Cooper wrote:
> Unlike the other tests, this is not standalone.  It requires poking at a live
> system, making it unweildly to use.
> 
> It hasn't been touched in 7 years, despite changes in libraries and kernel
> devices using the deprivilege infrastructure.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Anthony PERARD <anthony.perard@vates.tech>

Thanks,

-- 
Anthony PERARD

Re: [PATCH 3/3] CI: Drop custom handling of tools/tests

[Andrew Cooper]

On 20/05/2025 9:52 pm, Andrew Cooper wrote:
> diff --git a/automation/scripts/run-tools-tests b/automation/scripts/run-tools-tests
> index 770e97c3e943..8d7aa8fa5140 100755
> --- a/automation/scripts/run-tools-tests
> +++ b/automation/scripts/run-tools-tests
> @@ -12,30 +12,25 @@ printf '<?xml version="1.0" encoding="UTF-8"?>\n' > "$xml_out"
>  printf '<testsuites name="tools.tests">\n' >> "$xml_out"
>  printf ' <testsuite name="tools.tests">\n' >> "$xml_out"
>  failed=
> -for dir in "$1"/*; do
> -    [ -d "$dir" ] || continue
> -    echo "Running test in $dir"
> -    printf '  <testcase name="%s">\n' "$dir" >> "$xml_out"
> -    ret=
> -    for f in "$dir"/*; do
> -        [ -f "$f" ] || continue
> -        [ -x "$f" ] || continue
> -        "$f" 2>&1 | tee /tmp/out
> -        ret=$?
> -        if [ "$ret" -ne 0 ]; then
> -            echo "FAILED: $ret"
> -            failed+=" $dir"
> -            printf '   <failure type="failure" message="binary %s exited with code %d">\n' "$f" "$ret" >> "$xml_out"
> -            # TODO: could use xml escaping... but current tests seems to
> -            # produce sane output
> -            cat /tmp/out >> "$xml_out"
> -            printf '   </failure>\n' >> "$xml_out"
> -        else
> -            echo "PASSED"
> -        fi
> -    done
> -    if [ -z "$ret" ]; then
> -        printf '   <skipped type="skipped" message="no executable test found in %s"/>\n' "$dir" >> "$xml_out"
> +for f in "$1"/*; do
> +    if [ -x "$f" ]; then
> +        echo "SKIP: $f not executable"
> +        continue

This should be ! -x

I had that hunk in the wrong patch when posting this series.

~Andrew

> +    fi
> +    echo "Running $f"
> +    printf '  <testcase name="%s">\n' "$f" >> "$xml_out"
> +    "$f" 2>&1 | tee /tmp/out
> +    ret=$?
> +    if [ "$ret" -ne 0 ]; then
> +        echo "FAILED: $f"
> +        failed+=" $f"
> +        printf '   <failure type="failure" message="binary %s exited with code %d">\n' "$f" "$ret" >> "$xml_out"
> +        # TODO: could use xml escaping... but current tests seems to
> +        # produce sane output
> +        cat /tmp/out >> "$xml_out"
> +        printf '   </failure>\n' >> "$xml_out"
> +    else
> +        echo "PASSED"
>      fi
>      printf '  </testcase>\n' >> "$xml_out"
>  done

Re: [PATCH 2/3] tools/tests: Install tests into $(LIBEXEC)/tests

[Anthony PERARD]

On Tue, May 20, 2025 at 09:52:38PM +0100, Andrew Cooper wrote:
> $(LIBEXEC_BIN) is a dumping ground of many things.  Separate the "clearly
> tests" from everything else so we can clean up how they're run in CI.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Anthony PERARD <anthony.perard@vates.tech>

Thanks,

-- 
Anthony PERARD

Re: [PATCH 3/3] CI: Drop custom handling of tools/tests

[Anthony PERARD]

On Mon, May 26, 2025 at 05:45:29PM +0100, Andrew Cooper wrote:
> On 20/05/2025 9:52 pm, Andrew Cooper wrote:
> > diff --git a/automation/scripts/run-tools-tests b/automation/scripts/run-tools-tests
> > index 770e97c3e943..8d7aa8fa5140 100755
> > --- a/automation/scripts/run-tools-tests
> > +++ b/automation/scripts/run-tools-tests
> > @@ -12,30 +12,25 @@ printf '<?xml version="1.0" encoding="UTF-8"?>\n' > "$xml_out"
> >  printf '<testsuites name="tools.tests">\n' >> "$xml_out"
> >  printf ' <testsuite name="tools.tests">\n' >> "$xml_out"
> >  failed=
> > -for dir in "$1"/*; do
> > -    [ -d "$dir" ] || continue
> > -    echo "Running test in $dir"
> > -    printf '  <testcase name="%s">\n' "$dir" >> "$xml_out"
> > -    ret=
> > -    for f in "$dir"/*; do
> > -        [ -f "$f" ] || continue
> > -        [ -x "$f" ] || continue
> > -        "$f" 2>&1 | tee /tmp/out
> > -        ret=$?
> > -        if [ "$ret" -ne 0 ]; then
> > -            echo "FAILED: $ret"
> > -            failed+=" $dir"
> > -            printf '   <failure type="failure" message="binary %s exited with code %d">\n' "$f" "$ret" >> "$xml_out"
> > -            # TODO: could use xml escaping... but current tests seems to
> > -            # produce sane output
> > -            cat /tmp/out >> "$xml_out"
> > -            printf '   </failure>\n' >> "$xml_out"
> > -        else
> > -            echo "PASSED"
> > -        fi
> > -    done
> > -    if [ -z "$ret" ]; then
> > -        printf '   <skipped type="skipped" message="no executable test found in %s"/>\n' "$dir" >> "$xml_out"
> > +for f in "$1"/*; do
> > +    if [ -x "$f" ]; then
> > +        echo "SKIP: $f not executable"
> > +        continue
> 
> This should be ! -x
> 
> I had that hunk in the wrong patch when posting this series.

With that fixed:
Reviewed-by: Anthony PERARD <anthony.perard@vates.tech>

But I think there's an issue with the script...

> > +    "$f" 2>&1 | tee /tmp/out
> > +    ret=$?
> > +    if [ "$ret" -ne 0 ]; then

Is this checking the correct exit value? It seems that without `set -o
pipefail`, we only have the exit value of `tee` which should never fail.
But I think we should grab the value of ${PIPESTATUS[0]} to actually
read the exit value of $f.

Thanks,

-- 
Anthony PERARD

Re: [PATCH 3/3] CI: Drop custom handling of tools/tests

[Andrew Cooper]

On 26/05/2025 6:22 pm, Anthony PERARD wrote:
> On Mon, May 26, 2025 at 05:45:29PM +0100, Andrew Cooper wrote:
>> On 20/05/2025 9:52 pm, Andrew Cooper wrote:
>>> diff --git a/automation/scripts/run-tools-tests b/automation/scripts/run-tools-tests
>>> index 770e97c3e943..8d7aa8fa5140 100755
>>> --- a/automation/scripts/run-tools-tests
>>> +++ b/automation/scripts/run-tools-tests
>>> @@ -12,30 +12,25 @@ printf '<?xml version="1.0" encoding="UTF-8"?>\n' > "$xml_out"
>>>  printf '<testsuites name="tools.tests">\n' >> "$xml_out"
>>>  printf ' <testsuite name="tools.tests">\n' >> "$xml_out"
>>>  failed=
>>> -for dir in "$1"/*; do
>>> -    [ -d "$dir" ] || continue
>>> -    echo "Running test in $dir"
>>> -    printf '  <testcase name="%s">\n' "$dir" >> "$xml_out"
>>> -    ret=
>>> -    for f in "$dir"/*; do
>>> -        [ -f "$f" ] || continue
>>> -        [ -x "$f" ] || continue
>>> -        "$f" 2>&1 | tee /tmp/out
>>> -        ret=$?
>>> -        if [ "$ret" -ne 0 ]; then
>>> -            echo "FAILED: $ret"
>>> -            failed+=" $dir"
>>> -            printf '   <failure type="failure" message="binary %s exited with code %d">\n' "$f" "$ret" >> "$xml_out"
>>> -            # TODO: could use xml escaping... but current tests seems to
>>> -            # produce sane output
>>> -            cat /tmp/out >> "$xml_out"
>>> -            printf '   </failure>\n' >> "$xml_out"
>>> -        else
>>> -            echo "PASSED"
>>> -        fi
>>> -    done
>>> -    if [ -z "$ret" ]; then
>>> -        printf '   <skipped type="skipped" message="no executable test found in %s"/>\n' "$dir" >> "$xml_out"
>>> +for f in "$1"/*; do
>>> +    if [ -x "$f" ]; then
>>> +        echo "SKIP: $f not executable"
>>> +        continue
>> This should be ! -x
>>
>> I had that hunk in the wrong patch when posting this series.
> With that fixed:
> Reviewed-by: Anthony PERARD <anthony.perard@vates.tech>

Thanks.

>
> But I think there's an issue with the script...
>
>>> +    "$f" 2>&1 | tee /tmp/out
>>> +    ret=$?
>>> +    if [ "$ret" -ne 0 ]; then
> Is this checking the correct exit value? It seems that without `set -o
> pipefail`, we only have the exit value of `tee` which should never fail.
> But I think we should grab the value of ${PIPESTATUS[0]} to actually
> read the exit value of $f.

Hmm yes, I think this needs adjusting.

It turns out there are multiple problems with junit, including the fact
that putting failures in here doesn't cause the outer job to fail. 

The internet suggest having a 'script: grep "<failure" junit.xml' step
to work around this.

I think that wants to be a separate series.  The question is whether to
do this series first or second.  I expect I'm going to need to backport
all of this work to eventually get XTF back onto the older trees.

~Andrew

Re: [PATCH 1/3] tools/tests: Drop depriv-fd-checker

[Stefano Stabellini]

On Mon, 26 May 2025, Anthony PERARD wrote:
> On Tue, May 20, 2025 at 09:52:37PM +0100, Andrew Cooper wrote:
> > Unlike the other tests, this is not standalone.  It requires poking at a live
> > system, making it unweildly to use.
> > 
> > It hasn't been touched in 7 years, despite changes in libraries and kernel
> > devices using the deprivilege infrastructure.
> > 
> > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> Acked-by: Anthony PERARD <anthony.perard@vates.tech>

Acked-by: Stefano Stabellini <sstabellini@kernel.org>

Re: [PATCH 2/3] tools/tests: Install tests into $(LIBEXEC)/tests

[Stefano Stabellini]

On Mon, 26 May 2025, Anthony PERARD wrote:
> On Tue, May 20, 2025 at 09:52:38PM +0100, Andrew Cooper wrote:
> > $(LIBEXEC_BIN) is a dumping ground of many things.  Separate the "clearly
> > tests" from everything else so we can clean up how they're run in CI.
> > 
> > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> Acked-by: Anthony PERARD <anthony.perard@vates.tech>

Acked-by: Stefano Stabellini <sstabellini@kernel.org>

Re: [PATCH 3/3] CI: Drop custom handling of tools/tests

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 3058 bytes --]

On Mon, 26 May 2025, Andrew Cooper wrote:
> On 26/05/2025 6:22 pm, Anthony PERARD wrote:
> > On Mon, May 26, 2025 at 05:45:29PM +0100, Andrew Cooper wrote:
> >> On 20/05/2025 9:52 pm, Andrew Cooper wrote:
> >>> diff --git a/automation/scripts/run-tools-tests b/automation/scripts/run-tools-tests
> >>> index 770e97c3e943..8d7aa8fa5140 100755
> >>> --- a/automation/scripts/run-tools-tests
> >>> +++ b/automation/scripts/run-tools-tests
> >>> @@ -12,30 +12,25 @@ printf '<?xml version="1.0" encoding="UTF-8"?>\n' > "$xml_out"
> >>>  printf '<testsuites name="tools.tests">\n' >> "$xml_out"
> >>>  printf ' <testsuite name="tools.tests">\n' >> "$xml_out"
> >>>  failed=
> >>> -for dir in "$1"/*; do
> >>> -    [ -d "$dir" ] || continue
> >>> -    echo "Running test in $dir"
> >>> -    printf '  <testcase name="%s">\n' "$dir" >> "$xml_out"
> >>> -    ret=
> >>> -    for f in "$dir"/*; do
> >>> -        [ -f "$f" ] || continue
> >>> -        [ -x "$f" ] || continue
> >>> -        "$f" 2>&1 | tee /tmp/out
> >>> -        ret=$?
> >>> -        if [ "$ret" -ne 0 ]; then
> >>> -            echo "FAILED: $ret"
> >>> -            failed+=" $dir"
> >>> -            printf '   <failure type="failure" message="binary %s exited with code %d">\n' "$f" "$ret" >> "$xml_out"
> >>> -            # TODO: could use xml escaping... but current tests seems to
> >>> -            # produce sane output
> >>> -            cat /tmp/out >> "$xml_out"
> >>> -            printf '   </failure>\n' >> "$xml_out"
> >>> -        else
> >>> -            echo "PASSED"
> >>> -        fi
> >>> -    done
> >>> -    if [ -z "$ret" ]; then
> >>> -        printf '   <skipped type="skipped" message="no executable test found in %s"/>\n' "$dir" >> "$xml_out"
> >>> +for f in "$1"/*; do
> >>> +    if [ -x "$f" ]; then
> >>> +        echo "SKIP: $f not executable"
> >>> +        continue
> >> This should be ! -x
> >>
> >> I had that hunk in the wrong patch when posting this series.
> > With that fixed:
> > Reviewed-by: Anthony PERARD <anthony.perard@vates.tech>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> Thanks.
> 
> >
> > But I think there's an issue with the script...
> >
> >>> +    "$f" 2>&1 | tee /tmp/out
> >>> +    ret=$?
> >>> +    if [ "$ret" -ne 0 ]; then
> > Is this checking the correct exit value? It seems that without `set -o
> > pipefail`, we only have the exit value of `tee` which should never fail.
> > But I think we should grab the value of ${PIPESTATUS[0]} to actually
> > read the exit value of $f.
> 
> Hmm yes, I think this needs adjusting.
> 
> It turns out there are multiple problems with junit, including the fact
> that putting failures in here doesn't cause the outer job to fail. 
> 
> The internet suggest having a 'script: grep "<failure" junit.xml' step
> to work around this.
> 
> I think that wants to be a separate series.  The question is whether to
> do this series first or second.  I expect I'm going to need to backport
> all of this work to eventually get XTF back onto the older trees.
 
I'll leave the choice to you

Re: [PATCH 3/3] CI: Drop custom handling of tools/tests

[Andrew Cooper]

On 28/05/2025 1:16 am, Stefano Stabellini wrote:
> On Mon, 26 May 2025, Andrew Cooper wrote:
>> On 26/05/2025 6:22 pm, Anthony PERARD wrote:
>>> On Mon, May 26, 2025 at 05:45:29PM +0100, Andrew Cooper wrote:
>>>> On 20/05/2025 9:52 pm, Andrew Cooper wrote:
>>>>> diff --git a/automation/scripts/run-tools-tests b/automation/scripts/run-tools-tests
>>>>> index 770e97c3e943..8d7aa8fa5140 100755
>>>>> --- a/automation/scripts/run-tools-tests
>>>>> +++ b/automation/scripts/run-tools-tests
>>>>> @@ -12,30 +12,25 @@ printf '<?xml version="1.0" encoding="UTF-8"?>\n' > "$xml_out"
>>>>>  printf '<testsuites name="tools.tests">\n' >> "$xml_out"
>>>>>  printf ' <testsuite name="tools.tests">\n' >> "$xml_out"
>>>>>  failed=
>>>>> -for dir in "$1"/*; do
>>>>> -    [ -d "$dir" ] || continue
>>>>> -    echo "Running test in $dir"
>>>>> -    printf '  <testcase name="%s">\n' "$dir" >> "$xml_out"
>>>>> -    ret=
>>>>> -    for f in "$dir"/*; do
>>>>> -        [ -f "$f" ] || continue
>>>>> -        [ -x "$f" ] || continue
>>>>> -        "$f" 2>&1 | tee /tmp/out
>>>>> -        ret=$?
>>>>> -        if [ "$ret" -ne 0 ]; then
>>>>> -            echo "FAILED: $ret"
>>>>> -            failed+=" $dir"
>>>>> -            printf '   <failure type="failure" message="binary %s exited with code %d">\n' "$f" "$ret" >> "$xml_out"
>>>>> -            # TODO: could use xml escaping... but current tests seems to
>>>>> -            # produce sane output
>>>>> -            cat /tmp/out >> "$xml_out"
>>>>> -            printf '   </failure>\n' >> "$xml_out"
>>>>> -        else
>>>>> -            echo "PASSED"
>>>>> -        fi
>>>>> -    done
>>>>> -    if [ -z "$ret" ]; then
>>>>> -        printf '   <skipped type="skipped" message="no executable test found in %s"/>\n' "$dir" >> "$xml_out"
>>>>> +for f in "$1"/*; do
>>>>> +    if [ -x "$f" ]; then
>>>>> +        echo "SKIP: $f not executable"
>>>>> +        continue
>>>> This should be ! -x
>>>>
>>>> I had that hunk in the wrong patch when posting this series.
>>> With that fixed:
>>> Reviewed-by: Anthony PERARD <anthony.perard@vates.tech>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

Thanks.

>
>
>> Thanks.
>>
>>> But I think there's an issue with the script...
>>>
>>>>> +    "$f" 2>&1 | tee /tmp/out
>>>>> +    ret=$?
>>>>> +    if [ "$ret" -ne 0 ]; then
>>> Is this checking the correct exit value? It seems that without `set -o
>>> pipefail`, we only have the exit value of `tee` which should never fail.
>>> But I think we should grab the value of ${PIPESTATUS[0]} to actually
>>> read the exit value of $f.
>> Hmm yes, I think this needs adjusting.
>>
>> It turns out there are multiple problems with junit, including the fact
>> that putting failures in here doesn't cause the outer job to fail. 
>>
>> The internet suggest having a 'script: grep "<failure" junit.xml' step
>> to work around this.
>>
>> I think that wants to be a separate series.  The question is whether to
>> do this series first or second.  I expect I'm going to need to backport
>> all of this work to eventually get XTF back onto the older trees.
>  
> I'll leave the choice to you

In which case I'll get this committed now, because it's one useful step
forward and reduces my queue a bit.

~Andrew