Re: [RFC PATCH 1/2] add basic user-space tests with qemu

[Luca Dariz]

Hi,

Il 22/10/23 17:19, Samuel Thibault ha scritto:
> Luca Dariz, le jeu. 19 oct. 2023 20:57:46 +0200, a ecrit:
> > * tests/configfrag.ac: add MIGUSER, required for user-space tests as
> >    it might be different from the one used to ubild the kernel.
>
> Can't we just automatically select the proper mig? depending on
> user-land bitness we can use i686-gnu-mig or x86_64-gnu-mig.

yes that should be the default indeed, but I would keep the possibility 
to override it, to test a new mig if needed.

> > * tests/include/kern/printf.h: reuse kern/printf.h
> > * tests/include/util/atoi.h: reuse util/atoi.h
>
> Rather use a symlink for such files?

they are already a symlink, maybe I'll update the commit description to 
be clearer

> > diff --git a/tests/Makefrag.am b/tests/Makefrag.am
> > index 2723f64a..c16326e8 100644
> > --- a/tests/Makefrag.am
> > +++ b/tests/Makefrag.am
> > @@ -23,4 +23,129 @@
>
> > +if HOST_ix86
> > +QEMU=qemu-system-i386
>
> Using qemu-system-i386 fails on my system: grub starts, loads the
> kernel, but booting it fails. It doesn't seem related to this patch
> series, as qemu-system-i386 -kernel gnumach seems to fail the same way.
> But we'd rather fix it before adding the test support :)

I've tried changing some options and I see the kernel rebooting only 
with -cpu pentium (pentium2 works). I'm on debian 12 with qemu 
7.2+dfsg-7+deb12u2, I tried with both debian's current gnumach and 
compiling from master.

Does it work for you if using qemu-system-x86_64? I would avoid using it 
for the 32-bit gnumach tests as it confuses gdb.

> > +QEMU_GDB_PORT ?= 11234
>
> Typo?

Actually this is somehow related to my .gdbinit, as sometimes I was 
running two debug session in parallel, to compare the 32 and 64 bit 
behavior. Having it customizable is probably enough, with one single 
default.

> > +       cat $(srcdir)/tests/grub.cfg.single.template | \
> > +               sed -e s/BOOTMODULE/$</g | \
> > +               sed -e "s/GNUMACHARGS/$(GNUMACH_ARGS)/g" | \
> > +               cat >$(builddir)/tests/isofiles/boot/grub/grub.cfg
>
> rather
>         < $(srcdir)/tests/grub.cfg.single.template \
>                 sed -e s/BOOTMODULE/$</g \
>                     -e "s/GNUMACHARGS/$(GNUMACH_ARGS)/g" \
>                 >$(builddir)/tests/isofiles/boot/grub/grub.cfg

will do

> > diff --git a/tests/run-qemu.sh.template b/tests/run-qemu.sh.template
> > new file mode 100644
> > index 00000000..1f116354
> > --- /dev/null
> > +++ b/tests/run-qemu.sh.template
> > @@ -0,0 +1,22 @@
> > +#!/bin/sh
> > +
> > +set -e
> > +
> > +cmd="QEMU QEMU_OPTS -cdrom test-TESTNAME.iso"
> > +out=out-TESTNAME
> > +if which QEMU >/dev/null ; then
> > +    if ! timeout -v --foreground --kill-after=3 15s $cmd > $out; then
> > +        tail -n +18 $out  # skip terminal reconfiguration
>
> Better use sed -n '/start module-/,$p' or such to properly skip all
> kernel startup prints. You could also make the grub script print
> something, that you can catch in the sed script. And you can put the sed
> script instead of "> $out", so the output file is already clean and you
> don't have to duplicate the one-liner to print it.

Good idea, no need to hardcode the number of lines then.

> Also, please think about cleaning files :)

do you mean make clean or some other style cleanup in the code?

> > diff --git a/tests/testlib.c b/tests/testlib.c
> > new file mode 100644
> > index 00000000..13aa2614
> > --- /dev/null
> > +++ b/tests/testlib.c
> > @@ -0,0 +1,125 @@
> > +// from glibc's sysdeps/mach/usleep.c
>
> No need to keep the reference, it's small and trivial enough that it's
> not copyrightable :)

ok

> > +int msleep(uint32_t timeout)
> > +{
> > +  mach_port_t recv = mach_reply_port();
> > +  return mach_msg(NULL, MACH_RCV_MSG|MACH_RCV_TIMEOUT|MACH_RCV_INTERRUPT,
> > +                  0, 0, recv, timeout, MACH_PORT_NULL);
> > +}
> > +
> > +const char* e2s(int err)
> > +{
> > +    switch (err)
> > +    {
> > +    case MACH_SEND_INVALID_DATA: return "MACH_SEND_INVALID_DATA";
>
> You could use a macro
>
> #define E2S(s) \
>      case s: return #s;
>
>         E2S(MACH_SEND_INVALID_DATA)

In addition to this I was also thinking about auto-generating the 
switch/case from message.h kern_return.h and mig_errors.h, with the 
possibility to extend it.


Luca