Re: [PATCH 01/14] USER32: change default to disabled and make it a general option

2023-12-29 Thread Samuel Thibault
Applied, thanks!

Luca Dariz, le jeu. 28 déc. 2023 20:42:48 +0100, a ecrit:
> * configfrag.ac: add the global option USER32; although it makes sense
>   for 64-bit versions only, it can be used by future 64-bit
>   architectiures and not only x86_64.
>   Also, change the default setting to be disabled; now that we have a
>   working full 64-bit system, it makes sense to consider it the common
>   choice.
> * x86_64/configfrag.ac: define the correct 32-bit cpu if USER32 is
>   enabled.
> ---
>  configure.ac | 10 ++
>  x86_64/configfrag.ac | 14 --
>  2 files changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/configure.ac b/configure.ac
> index 3665d47a..52205eee 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -147,6 +147,16 @@ AC_ARG_ENABLE([device-drivers],
>  [;;
>  esac]
>  
> +AC_ARG_ENABLE([user32],
> +AS_HELP_STRING([--enable-user32], [enable 32-bit user space on a 64-bit 
> kernel]))
> +[if [ x"$enable_user32" = xyes ]; then]
> +AC_DEFINE([USER32], [], [enable 32-bit user on 64-bit kernel])
> +AM_CONDITIONAL([enable_user32], [true])
> +[else]
> +AM_CONDITIONAL([enable_user32], [false])
> +[fi]
> +
> +
>  # Platform-specific configuration.
>  
>  # PC AT.
> diff --git a/x86_64/configfrag.ac b/x86_64/configfrag.ac
> index 71f8d8c1..f119a9a3 100644
> --- a/x86_64/configfrag.ac
> +++ b/x86_64/configfrag.ac
> @@ -27,22 +27,16 @@ dnl USE OF THIS SOFTWARE.
>  # Determines the size of the CPU cache line.
>  AC_DEFINE([CPU_L1_SHIFT], [6], [CPU_L1_SHIFT])
>  
> -AC_ARG_ENABLE([user32],
> -  AS_HELP_STRING([--disable-user32], [disable 32-bit user on 64-bit 
> kernel]))
> -[if [ x"$enable_user32" != xno ]; then]
> -  AC_DEFINE([USER32], [], [enable 32-bit user on 64-bit kernel])
> -  AM_CONDITIONAL([enable_user32], [true])
> -[else]
> -  AM_CONDITIONAL([enable_user32], [false])
> -[fi]
> +[if test x"$enable_user32" = xyes ; then
> +  user32_cpu=i686
> +fi]
>  
>  [# Does the architecture provide machine-specific interfaces?
>  mach_machine_routines=1
>  
>  enable_pae=yes;;
>*)]
> -AM_CONDITIONAL([HOST_x86_64], [false])
> -AM_CONDITIONAL([enable_user32], [true])[;;
> +AM_CONDITIONAL([HOST_x86_64], [false])[;;
>  esac
>  
>  case $host_platform in
> -- 
> 2.39.2
> 
> 

-- 
Samuel
---
Pour une évaluation indépendante, transparente et rigoureuse !
Je soutiens la Commission d'Évaluation de l'Inria.



Re: [PATCH 02/14] add basic user-space tests with qemu

2023-12-29 Thread Samuel Thibault
Hello,

Thanks, this looks good!

Luca Dariz, le jeu. 28 déc. 2023 20:42:49 +0100, a ecrit:
> new file mode 100644
> index ..4cf25891
> --- /dev/null
> +++ b/tests/README
> @@ -0,0 +1,37 @@
> +
> +There are some basic tests that can be run qith qemu. You can run all the 
> tests with
> +
> +$ make check
> +
> +or selectively with:
> +
> +$ make run-hello
> +
> +Also, you can debug the existing tests, or a new one, by starting on one 
> shell
> +
> +$ make debug-hello
> +
> +and on another shell you can attach with gdb, load the symbols of the
> +bootstrap module and break on its _start():
> +
> +$ gdb gnuamch

Typo ;)

> +++ b/tests/run-qemu.sh.template
> +++ b/tests/test-hello.c
> +++ b/tests/user-qemu.mk

These are non-trivial, and so definitely need a copyright header.

> diff --git a/tests/syscalls.S b/tests/syscalls.S
> new file mode 100644
> index ..df9c9bc0
> --- /dev/null
> +++ b/tests/syscalls.S
> @@ -0,0 +1,4 @@
> +

Spurious line?

> +#include 
> +
> +kernel_trap(invalid_syscall,-31,0)



Re: [PATCH 03/14] add mach_host tests

2023-12-29 Thread Samuel Thibault
Luca Dariz, le jeu. 28 déc. 2023 20:42:50 +0100, a ecrit:
> ---
>  tests/test-mach_host.c | 54 ++
>  1 file changed, 54 insertions(+)
>  create mode 100644 tests/test-mach_host.c
> 
> diff --git a/tests/test-mach_host.c b/tests/test-mach_host.c
> new file mode 100644
> index ..99fc4aca
> --- /dev/null
> +++ b/tests/test-mach_host.c

Such files are also non-trivial, and thus deserve a copyright header too.

> @@ -0,0 +1,54 @@
> +
> +#include 
> +
> +#include 
> +
> +void test_kernel_version()
> +{
> +  int err;
> +  kernel_version_t kver;
> +  err = host_get_kernel_version(mach_host_self(), kver);
> +  ASSERT_RET(err, "host_kernel_info");
> +  printf("kernel version: %s\n", kver);
> +}
> +
> +void test_host_info()
> +{
> +  int err;
> +  mach_msg_type_number_t count;
> +  mach_port_t thishost = mach_host_self();
> +
> +  host_basic_info_data_t binfo;
> +  count = HOST_BASIC_INFO_COUNT;
> +  err = host_info(thishost, HOST_BASIC_INFO, (host_info_t)&binfo, &count);
> +  ASSERT_RET(err, "host_basic_info");
> +  ASSERT(count == HOST_BASIC_INFO_COUNT, "");

Perhaps also make some basic checks, such as binfo.avail_cpus <=
binfo.max_cpus.

> +  const int maxcpus = 255;
> +  int proc_slots[maxcpus];
> +  count = maxcpus;
> +  err = host_info(thishost, HOST_PROCESSOR_SLOTS, (host_info_t)&proc_slots, 
> &count);
> +  ASSERT_RET(err, "host_processor_slots");
> +  ASSERT((1 <= count) && (count <= maxcpus), "");
> +
> +  host_sched_info_data_t sinfo;
> +  count = HOST_SCHED_INFO_COUNT;
> +  err = host_info(thishost, HOST_SCHED_INFO, (host_info_t)&sinfo, &count);
> +  ASSERT_RET(err, "host_sched_info");
> +  ASSERT(count == HOST_SCHED_INFO_COUNT, "");

Here you can probably check that the min_* values is smaller than e.g.
1000, to catch randomly-filled values.

Also, it would be useful to compile the tests with
-ftrivial-auto-var-init=pattern so as to fill the structures with random
values before making the gnumach calls.

Samuel



Re: [PATCH 02/14] add basic user-space tests with qemu

2023-12-29 Thread Luca Dariz

Hi,

Il 29/12/23 14:37, Samuel Thibault ha scritto:

Hello,

Thanks, this looks good!

Luca Dariz, le jeu. 28 déc. 2023 20:42:49 +0100, a ecrit:

new file mode 100644
index ..4cf25891
--- /dev/null
+++ b/tests/README
@@ -0,0 +1,37 @@
+
+There are some basic tests that can be run qith qemu. You can run all the 
tests with
+
+$ make check
+
+or selectively with:
+
+$ make run-hello
+
+Also, you can debug the existing tests, or a new one, by starting on one shell
+
+$ make debug-hello
+
+and on another shell you can attach with gdb, load the symbols of the
+bootstrap module and break on its _start():
+
+$ gdb gnuamch


Typo ;)


What would be a better command? This is actually how I start gdb from 
the build directory, then I have a .gdbinit that connects to qemu and 
adds the userspace symbols if needed, sets breakpoints etc... Maybe I 
can add a minimal .gdbinit and copy it in the build directory?



+++ b/tests/run-qemu.sh.template
+++ b/tests/test-hello.c
+++ b/tests/user-qemu.mk


These are non-trivial, and so definitely need a copyright header.


Right, I forgot to add it to all files... I'll resend the patch set




diff --git a/tests/syscalls.S b/tests/syscalls.S
new file mode 100644
index ..df9c9bc0
--- /dev/null
+++ b/tests/syscalls.S
@@ -0,0 +1,4 @@
+


Spurious line?


I'll add the copyright header here also




+#include 
+
+kernel_trap(invalid_syscall,-31,0)





Re: [PATCH 04/14] add gsync tests

2023-12-29 Thread Samuel Thibault
Luca Dariz, le jeu. 28 déc. 2023 20:42:51 +0100, a ecrit:
> +static void single_t2(void *arg)
> +{
> +  int err;
> +  msleep(100);
> +  err = gsync_wake(mach_task_self(), (vm_offset_t)&single_shared, 0, 0);
> +  ASSERT_RET(err, "gsync_wake t2");
> +
> +  err = thread_terminate(mach_thread_self());
> +  ASSERT_RET(err, "thread_terminate");

thread_terminate is not actually supposed to return, so that'd rather be
an assert(0)

> +}
> +
> +static void test_single()
> +{
> +  int err;
> +  single_shared = 0;
> +  err = gsync_wait(mach_task_self(), (vm_offset_t)&single_shared, 0, 0, 100, 
> GSYNC_TIMED);
> +  ASSERT(err == KERN_TIMEDOUT, "gsync_wait 1");
> +
> +  single_shared = 1;
> +  err = gsync_wait(mach_task_self(), (vm_offset_t)&single_shared, 0, 0, 100, 
> GSYNC_TIMED);
> +  ASSERT(err == KERN_INVALID_ARGUMENT, "gsync_wait 2");
> +  err = gsync_wait(mach_task_self(), (vm_offset_t)&single_shared, 1, 0, 100, 
> GSYNC_TIMED);
> +  ASSERT(err == KERN_TIMEDOUT, "gsync_wait 2b");
> +
> +  err = gsync_wake(mach_task_self(), (vm_offset_t)&single_shared, 0, 0);
> +  ASSERT_RET(err, "gsync_wake 1");
> +
> +  err = gsync_wake(mach_task_self(), (vm_offset_t)&single_shared, 0, 
> GSYNC_BROADCAST);
> +  ASSERT_RET(err, "gsync_wake 2");

Here, you want to try gsync_wait again, and check that they time out
again (i.e. the kernel did forget about the gsync_wake, as expected).

> +  err = gsync_wake(mach_task_self(), (vm_offset_t)&single_shared, 2, 
> GSYNC_MUTATE);
> +  ASSERT_RET(err, "gsync_wake 2a");
> +  ASSERT(single_shared == 2, "gsync_wake single_shared");
> +
> +  err = gsync_wake(mach_task_self(), (vm_offset_t)&single_shared, 0, 0);
> +  ASSERT_RET(err, "gsync_wake 3");
> +  err = gsync_wake(mach_task_self(), (vm_offset_t)&single_shared, 0, 0);
> +  ASSERT_RET(err, "gsync_wake 3a");

Why two such calls?

> +  single_shared = 1;
> +  test_thread_start(mach_task_self(), single_t2, 0);
> +  err = gsync_wait(mach_task_self(), (vm_offset_t)&single_shared, 1, 0, 0, 
> 0);
> +  ASSERT_RET(err, "gsync_wait 4");

This is racy since the thread may start and call gsync_wake before
gsync_wait gets to block, and thus gsync_wait would stay stuck. Of
course in practice this doesn't happen because of the msleep(100) in the
thread, but that still leaves it racy and prone to fail if the system is
e.g. very busy. You want to make single_t2 modify single_shared, so that
if test_single2 happens to call gsync_wait after that, it will just not
block and return KERN_INVALID_ARGUMENT instead, as expected.

> +static void test_single2()
> +{
> +  int err;
> +  test_thread_start(mach_task_self(), single_t2, 0);
> +  single_shared = 2;
> +  err = gsync_wait(mach_task_self(), (vm_offset_t)&single_shared, 2, 0, 0, 
> 0);
> +  ASSERT_RET(err, "gsync_wait 1");
> +  // should this fail?
> +  err = gsync_wake(mach_task_self(), (vm_offset_t)&single_shared, 0, 0);
> +  ASSERT_RET(err, "gsync_wake 2");
> +}

I don't understand the test. Why waking after waiting? There is nobody
to wake. You also want to set single_shared before starting the thread.

Samuel



Re: [PATCH 02/14] add basic user-space tests with qemu

2023-12-29 Thread Samuel Thibault
Luca Dariz, le ven. 29 déc. 2023 14:51:31 +0100, a ecrit:
> Il 29/12/23 14:37, Samuel Thibault ha scritto:
> > Luca Dariz, le jeu. 28 déc. 2023 20:42:49 +0100, a ecrit:
> > > new file mode 100644
> > > index ..4cf25891
> > > --- /dev/null
> > > +++ b/tests/README
> > > @@ -0,0 +1,37 @@
> > > +
> > > +There are some basic tests that can be run qith qemu. You can run all 
> > > the tests with
> > > +
> > > +$ make check
> > > +
> > > +or selectively with:
> > > +
> > > +$ make run-hello
> > > +
> > > +Also, you can debug the existing tests, or a new one, by starting on one 
> > > shell
> > > +
> > > +$ make debug-hello
> > > +
> > > +and on another shell you can attach with gdb, load the symbols of the
> > > +bootstrap module and break on its _start():
> > > +
> > > +$ gdb gnuamch
> > 
> > Typo ;)
> 
> What would be a better command?

The command is fine, I just mean the typo gnuamch -> gnumach :)

> > > +++ b/tests/run-qemu.sh.template
> > > +++ b/tests/test-hello.c
> > > +++ b/tests/user-qemu.mk
> > 
> > These are non-trivial, and so definitely need a copyright header.
> 
> Right, I forgot to add it to all files... I'll resend the patch set

Perhaps wait for my reviews on the other patchs.

> > > diff --git a/tests/syscalls.S b/tests/syscalls.S
> > > new file mode 100644
> > > index ..df9c9bc0
> > > --- /dev/null
> > > +++ b/tests/syscalls.S
> > > @@ -0,0 +1,4 @@
> > > +
> > 
> > Spurious line?
> 
> I'll add the copyright header here also
> 
> > > +#include 
> > > +
> > > +kernel_trap(invalid_syscall,-31,0)

For such a trivial content it's not really useful.

Samuel



Re: [PATCH 02/14] add basic user-space tests with qemu

2023-12-29 Thread Luca Dariz

Il 29/12/23 14:57, Samuel Thibault ha scritto:

Luca Dariz, le ven. 29 déc. 2023 14:51:31 +0100, a ecrit:

Il 29/12/23 14:37, Samuel Thibault ha scritto:

Luca Dariz, le jeu. 28 déc. 2023 20:42:49 +0100, a ecrit:

new file mode 100644
index ..4cf25891
--- /dev/null
+++ b/tests/README
@@ -0,0 +1,37 @@
+
+There are some basic tests that can be run qith qemu. You can run all the 
tests with
+
+$ make check
+
+or selectively with:
+
+$ make run-hello
+
+Also, you can debug the existing tests, or a new one, by starting on one shell
+
+$ make debug-hello
+
+and on another shell you can attach with gdb, load the symbols of the
+bootstrap module and break on its _start():
+
+$ gdb gnuamch


Typo ;)


What would be a better command?


The command is fine, I just mean the typo gnuamch -> gnumach :)


aah ok :)




+++ b/tests/run-qemu.sh.template
+++ b/tests/test-hello.c
+++ b/tests/user-qemu.mk


These are non-trivial, and so definitely need a copyright header.


Right, I forgot to add it to all files... I'll resend the patch set


Perhaps wait for my reviews on the other patchs.


diff --git a/tests/syscalls.S b/tests/syscalls.S
new file mode 100644
index ..df9c9bc0
--- /dev/null
+++ b/tests/syscalls.S
@@ -0,0 +1,4 @@
+


Spurious line?


I'll add the copyright header here also


+#include 
+
+kernel_trap(invalid_syscall,-31,0)


For such a trivial content it's not really useful.

Samuel





Re: [PATCH 02/14] add basic user-space tests with qemu

2023-12-29 Thread Michael Banck via Bug reports for the GNU Hurd
Hi,

On Fri, Dec 29, 2023 at 02:51:31PM +0100, Luca Dariz wrote:
> > > +$ gdb gnuamch
> > 
> > Typo ;)
> 
> What would be a better command? This is actually how I start gdb from the
> build directory, 

You probably start gnumach though, and not gnuamch?


Michael



Re: [PATCH 05/14] add mach_port tests

2023-12-29 Thread Samuel Thibault
Luca Dariz, le jeu. 28 déc. 2023 20:42:52 +0100, a ecrit:
> +  mach_port_t newname = 123;

Why initializing it?

> +  err = mach_port_allocate(mach_task_self(), MACH_PORT_RIGHT_RECEIVE, 
> &newname);
> +  ASSERT_RET(err, "mach_port_allocate");
> +  err = mach_port_allocate(mach_task_self(), MACH_PORT_RIGHT_PORT_SET, 
> &newname);
> +  ASSERT_RET(err, "mach_port_allocate");
> +  err = mach_port_allocate(mach_task_self(), MACH_PORT_RIGHT_DEAD_NAME, 
> &newname);
> +  ASSERT_RET(err, "mach_port_allocate");

You could check that you are getting different names.

> +  err = mach_port_destroy(mach_task_self(), newname);
> +  ASSERT_RET(err, "mach_port_destroy");
> +
> +  err = mach_port_deallocate(mach_task_self(), 1002);
> +  ASSERT_RET(err, "mach_port_deallocate");
> +
> +  mach_port_urefs_t urefs;
> +  err = mach_port_get_refs(mach_task_self(), 1002, 
> MACH_PORT_RIGHT_DEAD_NAME, &urefs);
> +  ASSERT_RET(err, "mach_port_get_refs");

You could also check the references of the ports created above.

Samuel



Re: [PATCH 06/14] adjust range when changing memory pageabilityg

2023-12-29 Thread Samuel Thibault
Luca Dariz, le jeu. 28 déc. 2023 20:42:53 +0100, a ecrit:
> * vm/vm_map.c: if the start address is not in the map, try to find the
>   nearest entry instead of failing.
> 
> This caused the initial vm_wire_all(host, task VM_WIRE_ALL) in glibc
> startup to fail with KERN_NO_SPACE.
> ---
>  vm/vm_map.c | 21 -
>  1 file changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/vm/vm_map.c b/vm/vm_map.c
> index 26e18676..4a5861e3 100644
> --- a/vm/vm_map.c
> +++ b/vm/vm_map.c
> @@ -1774,13 +1774,24 @@ kern_return_t vm_map_pageable(
>  
>   if (!vm_map_lookup_entry(map, start, &start_entry)) {
>   /*
> -  *  Start address is not in map; this is fatal.
> +  *  Start address is not in map; try to find the nearest 
> entry

It is wrong to drop the error like this. The caller did ask for these
addresses. If they are wrong, the caller should be told so, and not
papered over. Possibly it's vm_map_pageable_all or such that actually
needs to be fixed.

Samuel



Re: [PATCH 07/14] add basic vm tests

2023-12-29 Thread Samuel Thibault
Luca Dariz, le jeu. 28 déc. 2023 20:42:54 +0100, a ecrit:
> +  // this emulates maptime()
> +  struct mapped_time_value *mtime;
> +  mach_port_t device, memobj;
> +  int err = device_open (device_priv(), 0, "time", &device);
> +  ASSERT_RET(err, "device_open");
> +  err = device_map (device, VM_PROT_READ, 0, sizeof(*mtime), &memobj, 0);
> +  ASSERT_RET(err, "device_map");
> +  err = mach_port_deallocate (mach_task_self (), device);
> +  ASSERT_RET(err, "mach_port_deallocate");
> +  mtime = 0;
> +  err =
> +vm_map (mach_task_self (), (vm_address_t *)&mtime, sizeof *mtime, 0, 1,
> +memobj, 0, 0, VM_PROT_READ, VM_PROT_READ, VM_INHERIT_NONE);
> +  ASSERT_RET(err, "vm_map");
> +  err = mach_port_deallocate (mach_task_self (), memobj);
> +  ASSERT_RET(err, "mach_port_deallocate");

I'd say try to print the content, so as to check that this did return a
pointer that is readable?

> +  err = vm_deallocate(mach_task_self(), (vm_address_t)mtime, sizeof(*mtime));
> +  ASSERT_RET(err, "vm_deallocate");
> +}



Re: [PATCH 08/14] add thread creation helper to tests

2023-12-29 Thread Samuel Thibault
Luca Dariz, le jeu. 28 déc. 2023 20:42:55 +0100, a ecrit:
> diff --git a/tests/testlib.c b/tests/testlib.c
> index 6abe8c4d..e6be46ee 100644
> --- a/tests/testlib.c
> +++ b/tests/testlib.c
> @@ -95,3 +95,56 @@ void _start()
>printf("%s: test %s exit code %x\n", TEST_SUCCESS_MARKER, argv[0], ret);
>halt();
>  }
> +
> +// started from 
> https://github.com/dwarfmaster/mach-ipc/blob/master/minimal_threads/main.c

So this *has* to reproduce the MIT license header of that repository.
Better put it in a separate file.

> +static uint32_t stack_top[PAGE_SIZE]  __attribute__ ((aligned (PAGE_SIZE)));
> +thread_t test_thread_start(task_t task, void(*routine)(void*), void* arg) {
> +  const vm_size_t stack_size = PAGE_SIZE * 16;
> +  kern_return_t ret;
> +  vm_address_t stack;
> +
> +  ret = vm_allocate(task, &stack, stack_size, TRUE);
> +  ASSERT_RET(ret, "can't allocate the stack for a new thread");
> +
> +  ret = vm_protect(task, stack, PAGE_SIZE, FALSE, VM_PROT_NONE);
> +  ASSERT_RET(ret, "can't protect the stack from overflows");
> +
> +  long *top = (long*)((vm_offset_t)stack_top + PAGE_SIZE) - 2;

> +  *top = 0;/* The return address */
> +#ifndef __x86_64__
> +  *(top + 1) = (long)arg; /* The argument is passed on the stack on x86_32 */
> +#endif
> +  ret = vm_write(task, stack + stack_size - PAGE_SIZE, 
> (vm_offset_t)stack_top, PAGE_SIZE);
> +  ASSERT_RET(ret, "can't initialize the stack for the new thread");
> +
> +  thread_t thread;
> +  ret = thread_create(task, &thread);
> +  ASSERT_RET(ret, "thread_create()");
> +
> +  struct i386_thread_state state;
> +  unsigned int count;
> +  count = i386_THREAD_STATE_COUNT;
> +  ret = thread_get_state(thread, i386_REGS_SEGS_STATE,
> + (thread_state_t) &state, &count);
> +  ASSERT_RET(ret, "thread_get_state()");
> +
> +#ifdef __x86_64__
> +  state.rip = (long) routine;
> +  state.ursp = (long) (stack + stack_size - 16);

Rather use sizeof(long)*2.

> +  state.rbp = 0;
> +  state.rdi = (long)arg;
> +#else
> +  state.eip = (long) routine;
> +  state.uesp = (long) (stack + stack_size - 8);

And here as well.

> +  state.ebp = 0;
> +#endif
> +  ret = thread_set_state(thread, i386_REGS_SEGS_STATE,
> + (thread_state_t) &state, i386_THREAD_STATE_COUNT);
> +  ASSERT_RET(ret, "thread_set_state");
> +
> +  ret = thread_resume(thread);
> +  ASSERT_RET(ret, "thread_resume");
> +
> +  return thread;
> +}
> -- 
> 2.39.2



Re: [PATCH 03/14] add mach_host tests

2023-12-29 Thread Luca Dariz

Il 29/12/23 14:44, Samuel Thibault ha scritto:

Luca Dariz, le jeu. 28 déc. 2023 20:42:50 +0100, a ecrit:

---
  tests/test-mach_host.c | 54 ++
  1 file changed, 54 insertions(+)
  create mode 100644 tests/test-mach_host.c

diff --git a/tests/test-mach_host.c b/tests/test-mach_host.c
new file mode 100644
index ..99fc4aca
--- /dev/null
+++ b/tests/test-mach_host.c


Such files are also non-trivial, and thus deserve a copyright header too.


@@ -0,0 +1,54 @@
+
+#include 
+
+#include 
+
+void test_kernel_version()
+{
+  int err;
+  kernel_version_t kver;
+  err = host_get_kernel_version(mach_host_self(), kver);
+  ASSERT_RET(err, "host_kernel_info");
+  printf("kernel version: %s\n", kver);
+}
+
+void test_host_info()
+{
+  int err;
+  mach_msg_type_number_t count;
+  mach_port_t thishost = mach_host_self();
+
+  host_basic_info_data_t binfo;
+  count = HOST_BASIC_INFO_COUNT;
+  err = host_info(thishost, HOST_BASIC_INFO, (host_info_t)&binfo, &count);
+  ASSERT_RET(err, "host_basic_info");
+  ASSERT(count == HOST_BASIC_INFO_COUNT, "");


Perhaps also make some basic checks, such as binfo.avail_cpus <=
binfo.max_cpus.


ok




+  const int maxcpus = 255;
+  int proc_slots[maxcpus];
+  count = maxcpus;
+  err = host_info(thishost, HOST_PROCESSOR_SLOTS, (host_info_t)&proc_slots, 
&count);
+  ASSERT_RET(err, "host_processor_slots");
+  ASSERT((1 <= count) && (count <= maxcpus), "");
+
+  host_sched_info_data_t sinfo;
+  count = HOST_SCHED_INFO_COUNT;
+  err = host_info(thishost, HOST_SCHED_INFO, (host_info_t)&sinfo, &count);
+  ASSERT_RET(err, "host_sched_info");
+  ASSERT(count == HOST_SCHED_INFO_COUNT, "");


Here you can probably check that the min_* values is smaller than e.g.
1000, to catch randomly-filled values.


ok



Also, it would be useful to compile the tests with
-ftrivial-auto-var-init=pattern so as to fill the structures with random
values before making the gnumach calls.


good idea, I didn't know this feature.


Luca




Re: [PATCH 09/14] add syscall tests

2023-12-29 Thread Samuel Thibault
Luca Dariz, le jeu. 28 déc. 2023 20:42:56 +0100, a ecrit:
> ---
>  tests/test-syscalls.c | 149 ++
>  1 file changed, 149 insertions(+)
>  create mode 100644 tests/test-syscalls.c
> 
> diff --git a/tests/test-syscalls.c b/tests/test-syscalls.c
> new file mode 100644
> index ..7e4234ac
> --- /dev/null
> +++ b/tests/test-syscalls.c
> @@ -0,0 +1,149 @@
> +
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +
> +
> +static struct {
> +  mach_port_t exception_port;
> +  mach_port_t thread;
> +  mach_port_t task;
> +  integer_t exception;
> +  integer_t code;
> +  integer_t subcode;
> +} last_exc;
> +kern_return_t catch_exception_raise(mach_port_t exception_port,
> +mach_port_t thread, mach_port_t task,
> +integer_t exception, integer_t code,
> +long_integer_t subcode)
> +{
> +  printf("received catch_exception_raise(%u %u %u %d %d %d)\n",
> + exception_port, thread, task, exception, code, subcode);
> +  last_exc.exception_port = exception_port;
> +  last_exc.thread = thread;
> +  last_exc.task = task;
> +  last_exc.exception = exception;
> +  last_exc.code = code;
> +  last_exc.subcode = subcode;
> +  return KERN_SUCCESS;
> +}
> +
> +static char simple_request_data[PAGE_SIZE];
> +static char simple_reply_data[PAGE_SIZE];
> +int simple_msg_server(boolean_t (*demuxer) (mach_msg_header_t *request,
> + mach_msg_header_t *reply),
> +  mach_port_t rcv_port_name,
> +  int num_msgs)
> +{
> +  int midx = 0, mok = 0;
> +  int ret;
> +  mig_reply_header_t *request = (mig_reply_header_t*)simple_request_data;
> +  mig_reply_header_t *reply = (mig_reply_header_t*)simple_reply_data;
> +  while ((midx - num_msgs) < 0)
> +{
> +  ret = mach_msg(&request->Head, MACH_RCV_MSG, 0, PAGE_SIZE,
> + rcv_port_name, 0, MACH_PORT_NULL);
> +  switch (ret)
> +{
> +case MACH_MSG_SUCCESS:
> +  if ((*demuxer)(&request->Head, &reply->Head))
> +mok++;  // TODO send reply
> +  else
> +FAILURE("demuxer didn't handle the message");
> +  break;
> +default:
> +  ASSERT_RET(ret, "receiving in msg_server");
> +  break;
> +}
> +  midx++;
> +}
> +  if (mok != midx)
> +FAILURE("wrong number of message received");
> +  return mok != midx;
> +}
> +
> +
> +void test_syscall_bad_arg_on_stack(void *arg)
> +{
> +  /* mach_msg() has 7 arguments, so the last one should be passed on the 
> stack
> + make RSP points the the wrong place to check the access check

"the the" typo

> +  */
> +#ifdef __x86_64__
> +  asm volatile("movq $0x123,%rsp;"   \
> +   "movq $-25,%rax;" \
> +   "syscall;"   \
> +   );
> +#else
> +  asm volatile("mov  $0x123,%esp;"   \
> +   "mov  $-25,%eax;" \
> +   "lcall$0x7,$0x0;" \
> +   );
> +#endif
> +  FAILURE("we shouldn't be here!");
> +}
> +
> +void test_bad_syscall_num(void *arg)
> +{
> +#ifdef __x86_64__
> +  asm volatile("movq $0x123456,%rax;"\
> +   "syscall;"   \
> +   );
> +#else
> +  asm volatile("mov  $0x123456,%eax;"\
> +   "lcall$0x7,$0x0;" \
> +   );
> +#endif
> +  FAILURE("we shouldn't be here!");
> +}
> +
> +
> +int main(int argc, char *argv[], int envc, char *envp[])
> +{
> +  int err;
> +  mach_port_t excp;
> +
> +  err = mach_port_allocate(mach_task_self (), MACH_PORT_RIGHT_RECEIVE, 
> &excp);
> +  ASSERT_RET(err, "creating exception port");
> +
> +  err = mach_port_insert_right(mach_task_self(), excp, excp,
> +   MACH_MSG_TYPE_MAKE_SEND);
> +  ASSERT_RET(err, "inserting sr into exception port");
> +
> +  err = task_set_special_port(mach_task_self(), TASK_EXCEPTION_PORT, excp);
> +  ASSERT_RET(err, "setting task exception port");
> +
> +  /* FIXME: receiving an exception with small size causes GP on 64 bit 
> userspace */
> +  /* mig_reply_header_t msg; */
> +  /* err = mach_msg(&msg.Head,   /\* The header *\/ */
> +  /*MACH_RCV_MSG, */
> +  /*0, */
> +  /*sizeof (msg),/\* Max receive Size *\/ */
> +  /*excp, */
> +  /*1000, */
> +  /*MACH_PORT_NULL); */
> +
> +  // FIXME: maybe MIG should provide this prototype?
> +  boolean_t exc_server
> +(mach_msg_header_t *InHeadP, mach_msg_header_t *OutHeadP);
> +
> +  memset(&last_exc, 0, sizeof(last_exc));
> +  test_thread_start(mach_task_self(), test_bad_syscall_num, NULL)

Re: [PATCH 10/14] expose MACH_MSG_USER_ALIGNMENT for manually-built messages

2023-12-29 Thread Samuel Thibault
Applied, thanks!

Luca Dariz, le jeu. 28 déc. 2023 20:42:57 +0100, a ecrit:
> ---
>  include/mach/message.h | 19 ++-
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/include/mach/message.h b/include/mach/message.h
> index 0b8b34d4..9790ef98 100644
> --- a/include/mach/message.h
> +++ b/include/mach/message.h
> @@ -401,6 +401,16 @@ typedef integer_t mach_msg_option_t;
>  
>  #define MACH_SEND_ALWAYS 0x0001  /* internal use only */
>  
> +#ifdef __x86_64__
> +#if defined(KERNEL) && defined(USER32)
> +#define MACH_MSG_USER_ALIGNMENT 4
> +#else
> +#define MACH_MSG_USER_ALIGNMENT 8
> +#endif
> +#else
> +#define MACH_MSG_USER_ALIGNMENT 4
> +#endif
> +
>  #ifdef KERNEL
>  /* This is the alignment of msg descriptors and the actual data
>   * for both in kernel messages and user land messages.
> @@ -411,15 +421,6 @@ typedef integer_t mach_msg_option_t;
>   * so that kernel messages are correctly aligned.
>   */
>  #define MACH_MSG_KERNEL_ALIGNMENT sizeof(uintptr_t)
> -#ifdef __x86_64__
> -#ifdef USER32
> -#define MACH_MSG_USER_ALIGNMENT 4
> -#else
> -#define MACH_MSG_USER_ALIGNMENT 8
> -#endif
> -#else
> -#define MACH_MSG_USER_ALIGNMENT 4
> -#endif
>  
>  #define mach_msg_align(x, alignment) \
>   ( ( ((vm_offset_t)(x)) + ((alignment)-1) ) & ~((alignment)-1) )
> -- 
> 2.39.2
> 
> 

-- 
Samuel
---
Pour une évaluation indépendante, transparente et rigoureuse !
Je soutiens la Commission d'Évaluation de l'Inria.



Re: [PATCH 11/14] add raw mach_msg tests

2023-12-29 Thread Samuel Thibault
Luca Dariz, le jeu. 28 déc. 2023 20:42:58 +0100, a ecrit:
> ---
>  tests/test-machmsg.c | 390 +++
>  1 file changed, 390 insertions(+)
>  create mode 100644 tests/test-machmsg.c
> 
> diff --git a/tests/test-machmsg.c b/tests/test-machmsg.c
> new file mode 100644
> index ..c262f5f4
> --- /dev/null
> +++ b/tests/test-machmsg.c
> @@ -0,0 +1,390 @@
> +
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +
> +#define ECHO_MAX_BODY_LEN 256
> +
> +static uint32_t align(uint32_t val, size_t aln)
> +{
> +  // we should check aln is a power of 2
> +  aln--;
> +  return (val + aln) & (~aln);
> +}
> +
> +#define align_inline(val, n) { val = align(val, n); }

Rather name it ALIGN_INLINE, so people don't mistake this for a
function (that doesn't have side-effects on its parameters).

> +void
> +run_test_simple(void *msg, mach_msg_size_t msglen, mach_msg_id_t msgid)
> +{
> +  mach_msg_header_t *head = msg;
> +  mach_port_t port, receive;
> +  int err;
> +
> +  err = syscall_mach_port_allocate (mach_task_self (),
> +MACH_PORT_RIGHT_RECEIVE, &port);
> +  ASSERT_RET(err, "syscall_mach_port_allocate");
> +
> +  err = syscall_mach_port_allocate (mach_task_self (),
> +MACH_PORT_RIGHT_RECEIVE, &receive);
> +  ASSERT_RET(err, "syscall_mach_port_allocate 2");
> +
> +  struct echo_params params;
> +  params.rx_port = port;
> +  params.rx_size = msglen;
> +  params.rx_number = 1;
> +  test_thread_start (mach_task_self (), echo_thread, ¶ms);
> +  msleep(100);

Is this msleep really needed? If yes, this is racy. If it's just to make
the test a bit more stressful, then ok.

Samuel



Re: [PATCH 12/14] add basic task tests

2023-12-29 Thread Samuel Thibault
Luca Dariz, le jeu. 28 déc. 2023 20:42:59 +0100, a ecrit:
> +void test_task()
> +{
> +  mach_port_t ourtask = mach_task_self();
> +  mach_msg_type_number_t count;
> +  int err;
> +
> +  struct task_basic_info binfo;
> +  count = TASK_BASIC_INFO_COUNT;
> +  err = task_info(ourtask, TASK_BASIC_INFO, (task_info_t)&binfo, &count);
> +  ASSERT_RET(err, "TASK_BASIC_INFO");

You can check that virtual_size >= resident_size.

Samuel



Re: [PATCH 13/14] add basic thread tests

2023-12-29 Thread Samuel Thibault
Luca Dariz, le jeu. 28 déc. 2023 20:43:00 +0100, a ecrit:
> ---
>  tests/test-threads.c | 88 
>  1 file changed, 88 insertions(+)
>  create mode 100644 tests/test-threads.c
> 
> diff --git a/tests/test-threads.c b/tests/test-threads.c
> new file mode 100644
> index ..659aaf3b
> --- /dev/null
> +++ b/tests/test-threads.c
> @@ -0,0 +1,88 @@
> +
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +
> +#include 
> +
> +void sleeping_thread(void* arg)
> +{
> +  printf("starting thread %d\n", arg);
> +  for (int i=0; i<100; i++)
> +  msleep(50);  // maybe we could just yield

Yielding wouldn't give you control on how much time is spent.

> +  printf("stopping thread %d\n", arg);
> +  int err = thread_terminate(mach_thread_self());
> +  ASSERT_RET(err, "thread_terminate");
> +}
> +
> +void test_many(void)
> +{
> +  int err;
> +  for (long tid=0; tid<10; tid++)
> +{
> +  test_thread_start(mach_task_self(), sleeping_thread, (void*)tid);
> +}
> +  // TODO: wait for thread end notifications
> +  msleep(6000);
> +}
> +
> +#ifdef __x86_64__
> +void test_fsgs_base_thread(void* tid)
> +{
> +  int err;
> +#if defined(__SEG_FS) && defined(__SEG_GS)
> +  long __seg_fs *fs_ptr;
> +  long __seg_gs *gs_ptr;
> +  long fs_value;
> +  long gs_value;
> +
> +  struct i386_fsgs_base_state state;
> +  state.fs_base = (unsigned long)&fs_value;
> +  state.gs_base = (unsigned long)&gs_value;
> +  err = thread_set_state(mach_thread_self(), i386_FSGS_BASE_STATE,
> + (thread_state_t) &state, 
> i386_FSGS_BASE_STATE_COUNT);
> +  ASSERT_RET(err, "thread_set_state");
> +
> +  fs_value = 0x100 + (long)tid;
> +  gs_value = 0x200 + (long)tid;
> +
> +  msleep(50);  // allow the others to set their segment base
> +
> +  fs_ptr = 0;
> +  gs_ptr = 0;
> +  long rdvalue = *fs_ptr;
> +  printf("FS expected %lx read %lx\n", fs_value, rdvalue);
> +  ASSERT(fs_value == rdvalue, "FS base error\n");
> +  rdvalue = *gs_ptr;
> +  printf("GS expected %lx read %lx\n", gs_value, rdvalue);
> +  ASSERT(gs_value == rdvalue, "GS base error\n");
> +#else
> +#error " missing __SEG_FS and __SEG_GS"
> +#endif
> +
> +  err = thread_terminate(mach_thread_self());
> +  ASSERT_RET(err, "thread_terminate");
> +}
> +#endif
> +
> +void test_fsgs_base(void)
> +{
> +#ifdef __x86_64__
> +  int err;
> +  for (long tid=0; tid<10; tid++)
> +{
> +  test_thread_start(mach_task_self(), test_fsgs_base_thread, (void*)tid);
> +}
> +  msleep(1000);  // TODO: wait for threads
> +#endif
> +}
> +
> +
> +int main(int argc, char *argv[], int envc, char *envp[])
> +{
> +  test_fsgs_base();
> +  test_many();
> +  return 0;
> +}
> -- 
> 2.39.2



Re: [PATCH 14/14] add tests to make check

2023-12-29 Thread Samuel Thibault
I'd say split this into the respective commits, so they are
self-contained.

Thanks for all that!

Samuel

Luca Dariz, le jeu. 28 déc. 2023 20:43:01 +0100, a ecrit:
> ---
>  tests/user-qemu.mk | 10 +-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/user-qemu.mk b/tests/user-qemu.mk
> index 50b04736..669eb77a 100644
> --- a/tests/user-qemu.mk
> +++ b/tests/user-qemu.mk
> @@ -178,7 +178,15 @@ clean-test-%:
>  
>  
>  USER_TESTS := \
> - tests/test-hello
> + tests/test-hello \
> + tests/test-mach_host \
> + tests/test-gsync \
> + tests/test-machmsg \
> + tests/test-mach_port \
> + tests/test-syscalls \
> + tests/test-task \
> + tests/test-threads \
> + tests/test-vm
>  
>  USER_TESTS_CLEAN = $(subst tests/,clean-,$(USER_TESTS))
>  
> -- 
> 2.39.2
> 
> 



[PATCH mig] Use char* for inlined arrays of char in user headers

2023-12-29 Thread Flavio Cruz
This changes how we declare RPC user prototypes for device_read_inband
to use "char *data" rather than "io_buf_ptr_inband_t data". It is more
standard to pass a pointer to represent arrays compared to "char [128]". This
fixes a warning in console-client since GCC won't complain we are not
passing an exact char [128].

Also updated code to use const_io_buf_ptr_inband_t for
device_write_inband. This is a pointer to const data rather than a const
pointer.
---
 utils.c | 30 --
 1 file changed, 24 insertions(+), 6 deletions(-)

diff --git a/utils.c b/utils.c
index 0d69cb2..a6c895b 100644
--- a/utils.c
+++ b/utils.c
@@ -160,12 +160,19 @@ UserVarQualifier(const argument_t *arg)
 if (!UserVarConst(arg))
return "";
 
-if (arg->argType->itIndefinite ||
-   arg->argType->itInName == MACH_MSG_TYPE_STRING_C ||
-   !strcmp(arg->argType->itUserType, "string_t"))
+const ipc_type_t *it = arg->argType;
+
+if (it->itIndefinite ||
+   it->itInName == MACH_MSG_TYPE_STRING_C ||
+   (it->itVarArray && !strcmp(it->itElement->itUserType, "char")) ||
+   !strcmp(it->itUserType, "string_t"))
 /* This is a pointer, so we have to use the const_foo type to
   make const qualify the data, not the pointer.
 
+  Or this is a pointer to a variable array. For now we only support 
arrays of char
+  but we can remove that condition if we define const typedefs for all 
types that
+  require it.
+
   Or this is a string_t, which should use const_string_t to avoid
   forcing the caller to respect the definite string size */
return "const_";
@@ -176,10 +183,21 @@ UserVarQualifier(const argument_t *arg)
 void
 WriteUserVarDecl(FILE *file, const argument_t *arg)
 {
-const char *qualif = UserVarQualifier(arg);
-const char *ref = arg->argByReferenceUser ? "*" : "";
+const ipc_type_t *it = arg->argType;
 
-fprintf(file, "\t%s%s %s%s", qualif, arg->argType->itUserType, ref, 
arg->argVarName);
+if (it->itInLine && it->itVarArray && !it->itIndefinite &&
+   !UserVarConst(arg) &&
+   !strcmp(it->itElement->itUserType, "char"))
+{
+   /* For variable arrays like "array[*:128] of char" we prefer to use 
"char *param"
+* as the argument since it is more standard than using "char 
param[128]".
+*/
+   fprintf(file, "\tchar *%s /* max of %d elements */", arg->argVarName, 
it->itNumber);
+} else {
+   const char *qualif = UserVarQualifier(arg);
+   const char *ref = arg->argByReferenceUser ? "*" : "";
+   fprintf(file, "\t%s%s %s%s", qualif, it->itUserType, ref, 
arg->argVarName);
+}
 }
 
 /* Returns whether parameter should be qualified with const because we will 
only
-- 
2.39.2




[PATCH hurd 1/3] pfinet and pci-arbiter: update server handlers to return kern_return_t to fix -Werror=enum-int-mismatch warnings

2023-12-29 Thread Flavio Cruz
---
 pci-arbiter/pci-ops.c | 10 +++
 pfinet/iioctl-ops.c   | 10 +++
 pfinet/io-ops.c   | 64 +--
 pfinet/main.c |  2 +-
 pfinet/pfinet-ops.c   |  4 +--
 pfinet/socket-ops.c   | 32 +++---
 pfinet/tunnel.c   | 24 
 7 files changed, 73 insertions(+), 73 deletions(-)

diff --git a/pci-arbiter/pci-ops.c b/pci-arbiter/pci-ops.c
index 2f9f529..bbe43f3 100644
--- a/pci-arbiter/pci-ops.c
+++ b/pci-arbiter/pci-ops.c
@@ -78,7 +78,7 @@ calculate_ndevs (struct iouser *user)
  *
  * `*datalen' is updated.
  */
-error_t
+kern_return_t
 S_pci_conf_read (struct protid * master, int reg, char **data,
 mach_msg_type_number_t * datalen, vm_size_t amount)
 {
@@ -127,7 +127,7 @@ S_pci_conf_read (struct protid * master, int reg, char 
**data,
 }
 
 /* Write `datalen' bytes from `data'. `amount' is updated. */
-error_t
+kern_return_t
 S_pci_conf_write (struct protid * master, int reg, const char *data, 
mach_msg_type_number_t datalen,
  vm_size_t * amount)
 {
@@ -165,7 +165,7 @@ S_pci_conf_write (struct protid * master, int reg, const 
char *data, mach_msg_ty
 }
 
 /* Write in `amount' the number of devices allowed for the user. */
-error_t
+kern_return_t
 S_pci_get_ndevs (struct protid * master, mach_msg_type_number_t * amount)
 {
   /* This RPC may only be addressed to the root node */
@@ -181,7 +181,7 @@ S_pci_get_ndevs (struct protid * master, 
mach_msg_type_number_t * amount)
  * Return in `data' the information about the available memory
  * regions in the given device.
  */
-error_t
+kern_return_t
 S_pci_get_dev_regions (struct protid * master, char **data, 
mach_msg_type_number_t * datalen)
 {
   error_t err;
@@ -232,7 +232,7 @@ S_pci_get_dev_regions (struct protid * master, char **data, 
mach_msg_type_number
 /*
  * Return in `data' the information about the expansion rom of the given device
  */
-error_t
+kern_return_t
 S_pci_get_dev_rom (struct protid * master, char **data, mach_msg_type_number_t 
* datalen)
 {
   error_t err;
diff --git a/pfinet/iioctl-ops.c b/pfinet/iioctl-ops.c
index 7673f3a..dc1abea 100644
--- a/pfinet/iioctl-ops.c
+++ b/pfinet/iioctl-ops.c
@@ -519,7 +519,7 @@ SIOCGIF (brdaddr, BRDADDR);
 SIOCGIF (netmask, NETMASK);
 
 /* 39 SIOCGIFHWADDR -- Get the hardware address of a network interface.  */
-error_t
+kern_return_t
 S_iioctl_siocgifhwaddr (struct sock_user *user,
ifname_t ifname,
sockaddr_t *addr)
@@ -544,7 +544,7 @@ S_iioctl_siocgifhwaddr (struct sock_user *user,
 }
 
 /* 51 SIOCGIFMTU -- Get mtu of a network interface.  */
-error_t
+kern_return_t
 S_iioctl_siocgifmtu (struct sock_user *user,
 ifname_t ifnam,
 int *mtu)
@@ -564,7 +564,7 @@ S_iioctl_siocgifmtu (struct sock_user *user,
 }
 
 /* 51 SIOCSIFMTU -- Set mtu of a network interface.  */
-error_t
+kern_return_t
 S_iioctl_siocsifmtu (struct sock_user *user,
 const ifname_t ifnam,
 int mtu)
@@ -598,7 +598,7 @@ S_iioctl_siocsifmtu (struct sock_user *user,
 }
 
 /* 100 SIOCGIFINDEX -- Get index number of a network interface.  */
-error_t
+kern_return_t
 S_iioctl_siocgifindex (struct sock_user *user,
   ifname_t ifnam,
   int *index)
@@ -618,7 +618,7 @@ S_iioctl_siocgifindex (struct sock_user *user,
 }
 
 /* 101 SIOCGIFNAME -- Get name of a network interface from index number.  */
-error_t
+kern_return_t
 S_iioctl_siocgifname (struct sock_user *user,
  ifname_t ifnam,
  int *index)
diff --git a/pfinet/io-ops.c b/pfinet/io-ops.c
index e1a6608..818f113 100644
--- a/pfinet/io-ops.c
+++ b/pfinet/io-ops.c
@@ -34,7 +34,7 @@
 #include 
 #include 
 
-error_t
+kern_return_t
 S_io_write (struct sock_user *user,
const_data_t data,
mach_msg_type_number_t datalen,
@@ -67,7 +67,7 @@ S_io_write (struct sock_user *user,
   return err;
 }
 
-error_t
+kern_return_t
 S_io_read (struct sock_user *user,
   data_t *data,
   mach_msg_type_number_t *datalen,
@@ -124,7 +124,7 @@ S_io_read (struct sock_user *user,
   return err;
 }
 
-error_t
+kern_return_t
 S_io_seek (struct sock_user *user,
   off_t offset,
   int whence,
@@ -133,7 +133,7 @@ S_io_seek (struct sock_user *user,
   return user ? ESPIPE : EOPNOTSUPP;
 }
 
-error_t
+kern_return_t
 S_io_readable (struct sock_user *user,
   vm_size_t *amount)
 {
@@ -183,7 +183,7 @@ S_io_readable (struct sock_user *user,
   return err;
 }
 
-error_t
+kern_return_t
 S_io_set_all_openmodes (struct sock_user *user,
int bits)
 {
@@ -199,7 +199,7 @@ S_io_set_all_openmodes (struct sock_user *user,
   return 0;
 }
 
-error_t
+kern_return_t
 S_io_get_openmodes (struct sock_user *user,
int *bits)
 {
@@ -223,7 +223,7 @@ S_io_get_openmodes (struct sock_user *user,
   return 0;
 }
 
-error_t
+kern_

[PATCH hurd 2/3] Replace deprecated sigmask with sigset_t calls

2023-12-29 Thread Flavio Cruz
---
 exec/hashexec.c |  9 +++--
 libdiskfs/disk-pager.c  |  4 +++-
 libpager/pager-memcpy.c | 11 ---
 libstore/memobj.c   | 11 ---
 startup/startup.c   |  9 ++---
 5 files changed, 32 insertions(+), 12 deletions(-)

diff --git a/exec/hashexec.c b/exec/hashexec.c
index 9e00704..a107291 100644
--- a/exec/hashexec.c
+++ b/exec/hashexec.c
@@ -53,6 +53,11 @@ check_hashbang (struct execdata *e,
   size_t new_argvlen;
   mach_port_t *new_dtable = NULL;
   mach_msg_type_number_t new_dtablesize;
+  sigset_t arg_env_sigset;
+
+  sigemptyset (&arg_env_sigset);
+  sigaddset (&arg_env_sigset, SIGSEGV);
+  sigaddset (&arg_env_sigset, SIGBUS);
 
   file_t user_fd (int fd)
 {
@@ -293,7 +298,7 @@ check_hashbang (struct execdata *e,
  if (strchr (name, '/') != NULL)
error = lookup (name, 0, &name_file);
  else if ((error = hurd_catch_signal
-   (sigmask (SIGBUS) | sigmask (SIGSEGV),
+   (arg_env_sigset,
 (vm_address_t) envp, (vm_address_t) envp + envplen,
 &search_path, SIG_ERR)))
name_file = MACH_PORT_NULL;
@@ -416,7 +421,7 @@ check_hashbang (struct execdata *e,
}
 
   /* Set up the arguments.  */
-  hurd_catch_signal (sigmask (SIGSEGV) | sigmask (SIGBUS),
+  hurd_catch_signal (arg_env_sigset,
 (vm_address_t) argv, (vm_address_t) argv + argvlen,
 &setup_args, &fault_handler);
 }
diff --git a/libdiskfs/disk-pager.c b/libdiskfs/disk-pager.c
index 1a5d8bf..7af0e3b 100644
--- a/libdiskfs/disk-pager.c
+++ b/libdiskfs/disk-pager.c
@@ -29,7 +29,6 @@ struct pager_requests *diskfs_disk_pager_requests;
 static void fault_handler (int sig, long int sigcode, struct sigcontext *scp);
 static struct hurd_signal_preemptor preemptor =
   {
-  signals: sigmask (SIGSEGV) | sigmask (SIGBUS),
   preemptor: NULL,
   handler: (sighandler_t) &fault_handler,
   };
@@ -73,6 +72,9 @@ diskfs_start_disk_pager (struct user_pager_info *upi,
   /* Set up the signal preemptor to catch faults on the disk image.  */
   preemptor.first = (vm_address_t) *image;
   preemptor.last = ((vm_address_t) *image + size);
+  sigemptyset (&preemptor.signals);
+  sigaddset (&preemptor.signals, SIGSEGV);
+  sigaddset (&preemptor.signals, SIGBUS);
   hurd_preempt_signals (&preemptor);
 
   /* We have the mapping; we no longer need the send right.  */
diff --git a/libpager/pager-memcpy.c b/libpager/pager-memcpy.c
index caaf6f2..2ef42a7 100644
--- a/libpager/pager-memcpy.c
+++ b/libpager/pager-memcpy.c
@@ -208,9 +208,14 @@ pager_memcpy (struct pager *pager, memory_object_t memobj,
   window_size = 0;
 
   if (sigsetjmp (buf, 1) == 0)
-hurd_catch_signal (sigmask (SIGSEGV) | sigmask (SIGBUS),
-  window, window + window_size,
-  &do_copy, (sighandler_t) &fault);
+{
+  sigset_t mask;
+  sigemptyset (&mask);
+  sigaddset (&mask, SIGSEGV);
+  sigaddset (&mask, SIGBUS);
+  hurd_catch_signal (mask, window, window + window_size,
+&do_copy, (sighandler_t) &fault);
+}
 
   if (! err)
 assert_backtrace (n == 0);
diff --git a/libstore/memobj.c b/libstore/memobj.c
index ea2d7b9..e98e1b2 100644
--- a/libstore/memobj.c
+++ b/libstore/memobj.c
@@ -113,9 +113,14 @@ memobj_memcpy (memory_object_t memobj,
 return 0;
 
   if (sigsetjmp (buf, 1) == 0)
-hurd_catch_signal (sigmask (SIGSEGV) | sigmask (SIGBUS),
-  window, window + windowsize,
-  ©, (sighandler_t) &fault);
+{
+  sigset_t mask;
+  sigemptyset (&mask);
+  sigaddset (&mask, SIGSEGV);
+  sigaddset (&mask, SIGBUS);
+  hurd_catch_signal (mask, window, window + windowsize,
+©, (sighandler_t) &fault);
+}
 
   if (window)
 munmap ((caddr_t) window, windowsize);
diff --git a/startup/startup.c b/startup/startup.c
index 862572c..27af818 100644
--- a/startup/startup.c
+++ b/startup/startup.c
@@ -826,9 +826,12 @@ main (int argc, char **argv, char **envp)
   /* All programs we start should ignore job control stop signals.
  That way Posix.1 B.2.2.2 is satisfied where it says that programs
  not run under job control shells are protected.  */
-  default_ints[INIT_SIGIGN] = (sigmask (SIGTSTP)
-  | sigmask (SIGTTIN)
-  | sigmask (SIGTTOU));
+  sigset_t sigmask;
+  sigemptyset (&sigmask);
+  sigaddset (&sigmask, SIGTSTP);
+  sigaddset (&sigmask, SIGTTIN);
+  sigaddset (&sigmask, SIGTTOU);
+  default_ints[INIT_SIGIGN] = sigmask;
 
   default_ports[INIT_PORT_BOOTSTRAP] = startup;
   run ("/hurd/proc", default_ports, &proctask, proc_insert_ports);
-- 
2.39.2




[PATCH hurd 3/3] Mark msg_thread as noreturn

2023-12-29 Thread Flavio Cruz
---
 boot/boot.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/boot/boot.c b/boot/boot.c
index 0d7ae74..d773bd1 100644
--- a/boot/boot.c
+++ b/boot/boot.c
@@ -775,7 +775,7 @@ main (int argc, char **argv, char **envp)
 }
 }
 
-void *
+void * __attribute__ ((noreturn))
 msg_thread (void *arg)
 {
   while (1)
-- 
2.39.2




Re: [PATCH mig] Use char* for inlined arrays of char in user headers

2023-12-29 Thread Samuel Thibault
Applied, thanks!

Flavio Cruz, le ven. 29 déc. 2023 11:08:14 -0500, a ecrit:
> This changes how we declare RPC user prototypes for device_read_inband
> to use "char *data" rather than "io_buf_ptr_inband_t data". It is more
> standard to pass a pointer to represent arrays compared to "char [128]". This
> fixes a warning in console-client since GCC won't complain we are not
> passing an exact char [128].
> 
> Also updated code to use const_io_buf_ptr_inband_t for
> device_write_inband. This is a pointer to const data rather than a const
> pointer.
> ---
>  utils.c | 30 --
>  1 file changed, 24 insertions(+), 6 deletions(-)
> 
> diff --git a/utils.c b/utils.c
> index 0d69cb2..a6c895b 100644
> --- a/utils.c
> +++ b/utils.c
> @@ -160,12 +160,19 @@ UserVarQualifier(const argument_t *arg)
>  if (!UserVarConst(arg))
>   return "";
>  
> -if (arg->argType->itIndefinite ||
> - arg->argType->itInName == MACH_MSG_TYPE_STRING_C ||
> - !strcmp(arg->argType->itUserType, "string_t"))
> +const ipc_type_t *it = arg->argType;
> +
> +if (it->itIndefinite ||
> + it->itInName == MACH_MSG_TYPE_STRING_C ||
> + (it->itVarArray && !strcmp(it->itElement->itUserType, "char")) ||
> + !strcmp(it->itUserType, "string_t"))
>  /* This is a pointer, so we have to use the const_foo type to
>  make const qualify the data, not the pointer.
>  
> +Or this is a pointer to a variable array. For now we only support 
> arrays of char
> +but we can remove that condition if we define const typedefs for all 
> types that
> +require it.
> +
>  Or this is a string_t, which should use const_string_t to avoid
>  forcing the caller to respect the definite string size */
>   return "const_";
> @@ -176,10 +183,21 @@ UserVarQualifier(const argument_t *arg)
>  void
>  WriteUserVarDecl(FILE *file, const argument_t *arg)
>  {
> -const char *qualif = UserVarQualifier(arg);
> -const char *ref = arg->argByReferenceUser ? "*" : "";
> +const ipc_type_t *it = arg->argType;
>  
> -fprintf(file, "\t%s%s %s%s", qualif, arg->argType->itUserType, ref, 
> arg->argVarName);
> +if (it->itInLine && it->itVarArray && !it->itIndefinite &&
> + !UserVarConst(arg) &&
> + !strcmp(it->itElement->itUserType, "char"))
> +{
> + /* For variable arrays like "array[*:128] of char" we prefer to use 
> "char *param"
> +  * as the argument since it is more standard than using "char 
> param[128]".
> +  */
> + fprintf(file, "\tchar *%s /* max of %d elements */", arg->argVarName, 
> it->itNumber);
> +} else {
> + const char *qualif = UserVarQualifier(arg);
> + const char *ref = arg->argByReferenceUser ? "*" : "";
> + fprintf(file, "\t%s%s %s%s", qualif, it->itUserType, ref, 
> arg->argVarName);
> +}
>  }
>  
>  /* Returns whether parameter should be qualified with const because we will 
> only
> -- 
> 2.39.2
> 
> 

-- 
Samuel
---
Pour une évaluation indépendante, transparente et rigoureuse !
Je soutiens la Commission d'Évaluation de l'Inria.



Re: [PATCH hurd 1/3] pfinet and pci-arbiter: update server handlers to return kern_return_t to fix -Werror=enum-int-mismatch warnings

2023-12-29 Thread Samuel Thibault
Applied, thanks!

Flavio Cruz, le ven. 29 déc. 2023 11:12:09 -0500, a ecrit:
> ---
>  pci-arbiter/pci-ops.c | 10 +++
>  pfinet/iioctl-ops.c   | 10 +++
>  pfinet/io-ops.c   | 64 +--
>  pfinet/main.c |  2 +-
>  pfinet/pfinet-ops.c   |  4 +--
>  pfinet/socket-ops.c   | 32 +++---
>  pfinet/tunnel.c   | 24 
>  7 files changed, 73 insertions(+), 73 deletions(-)
> 
> diff --git a/pci-arbiter/pci-ops.c b/pci-arbiter/pci-ops.c
> index 2f9f529..bbe43f3 100644
> --- a/pci-arbiter/pci-ops.c
> +++ b/pci-arbiter/pci-ops.c
> @@ -78,7 +78,7 @@ calculate_ndevs (struct iouser *user)
>   *
>   * `*datalen' is updated.
>   */
> -error_t
> +kern_return_t
>  S_pci_conf_read (struct protid * master, int reg, char **data,
>mach_msg_type_number_t * datalen, vm_size_t amount)
>  {
> @@ -127,7 +127,7 @@ S_pci_conf_read (struct protid * master, int reg, char 
> **data,
>  }
>  
>  /* Write `datalen' bytes from `data'. `amount' is updated. */
> -error_t
> +kern_return_t
>  S_pci_conf_write (struct protid * master, int reg, const char *data, 
> mach_msg_type_number_t datalen,
> vm_size_t * amount)
>  {
> @@ -165,7 +165,7 @@ S_pci_conf_write (struct protid * master, int reg, const 
> char *data, mach_msg_ty
>  }
>  
>  /* Write in `amount' the number of devices allowed for the user. */
> -error_t
> +kern_return_t
>  S_pci_get_ndevs (struct protid * master, mach_msg_type_number_t * amount)
>  {
>/* This RPC may only be addressed to the root node */
> @@ -181,7 +181,7 @@ S_pci_get_ndevs (struct protid * master, 
> mach_msg_type_number_t * amount)
>   * Return in `data' the information about the available memory
>   * regions in the given device.
>   */
> -error_t
> +kern_return_t
>  S_pci_get_dev_regions (struct protid * master, char **data, 
> mach_msg_type_number_t * datalen)
>  {
>error_t err;
> @@ -232,7 +232,7 @@ S_pci_get_dev_regions (struct protid * master, char 
> **data, mach_msg_type_number
>  /*
>   * Return in `data' the information about the expansion rom of the given 
> device
>   */
> -error_t
> +kern_return_t
>  S_pci_get_dev_rom (struct protid * master, char **data, 
> mach_msg_type_number_t * datalen)
>  {
>error_t err;
> diff --git a/pfinet/iioctl-ops.c b/pfinet/iioctl-ops.c
> index 7673f3a..dc1abea 100644
> --- a/pfinet/iioctl-ops.c
> +++ b/pfinet/iioctl-ops.c
> @@ -519,7 +519,7 @@ SIOCGIF (brdaddr, BRDADDR);
>  SIOCGIF (netmask, NETMASK);
>  
>  /* 39 SIOCGIFHWADDR -- Get the hardware address of a network interface.  */
> -error_t
> +kern_return_t
>  S_iioctl_siocgifhwaddr (struct sock_user *user,
>   ifname_t ifname,
>   sockaddr_t *addr)
> @@ -544,7 +544,7 @@ S_iioctl_siocgifhwaddr (struct sock_user *user,
>  }
>  
>  /* 51 SIOCGIFMTU -- Get mtu of a network interface.  */
> -error_t
> +kern_return_t
>  S_iioctl_siocgifmtu (struct sock_user *user,
>ifname_t ifnam,
>int *mtu)
> @@ -564,7 +564,7 @@ S_iioctl_siocgifmtu (struct sock_user *user,
>  }
>  
>  /* 51 SIOCSIFMTU -- Set mtu of a network interface.  */
> -error_t
> +kern_return_t
>  S_iioctl_siocsifmtu (struct sock_user *user,
>const ifname_t ifnam,
>int mtu)
> @@ -598,7 +598,7 @@ S_iioctl_siocsifmtu (struct sock_user *user,
>  }
>  
>  /* 100 SIOCGIFINDEX -- Get index number of a network interface.  */
> -error_t
> +kern_return_t
>  S_iioctl_siocgifindex (struct sock_user *user,
>  ifname_t ifnam,
>  int *index)
> @@ -618,7 +618,7 @@ S_iioctl_siocgifindex (struct sock_user *user,
>  }
>  
>  /* 101 SIOCGIFNAME -- Get name of a network interface from index number.  */
> -error_t
> +kern_return_t
>  S_iioctl_siocgifname (struct sock_user *user,
> ifname_t ifnam,
> int *index)
> diff --git a/pfinet/io-ops.c b/pfinet/io-ops.c
> index e1a6608..818f113 100644
> --- a/pfinet/io-ops.c
> +++ b/pfinet/io-ops.c
> @@ -34,7 +34,7 @@
>  #include 
>  #include 
>  
> -error_t
> +kern_return_t
>  S_io_write (struct sock_user *user,
>   const_data_t data,
>   mach_msg_type_number_t datalen,
> @@ -67,7 +67,7 @@ S_io_write (struct sock_user *user,
>return err;
>  }
>  
> -error_t
> +kern_return_t
>  S_io_read (struct sock_user *user,
>  data_t *data,
>  mach_msg_type_number_t *datalen,
> @@ -124,7 +124,7 @@ S_io_read (struct sock_user *user,
>return err;
>  }
>  
> -error_t
> +kern_return_t
>  S_io_seek (struct sock_user *user,
>  off_t offset,
>  int whence,
> @@ -133,7 +133,7 @@ S_io_seek (struct sock_user *user,
>return user ? ESPIPE : EOPNOTSUPP;
>  }
>  
> -error_t
> +kern_return_t
>  S_io_readable (struct sock_user *user,
>  vm_size_t *amount)
>  {
> @@ -183,7 +183,7 @@ S_io_readable (struct sock_user *user,
>return err;
>  }
>  
> -error_t
> +kern_return_t
>  S_io_

Re: [PATCH hurd 2/3] Replace deprecated sigmask with sigset_t calls

2023-12-29 Thread Samuel Thibault
Applied, thanks!

Flavio Cruz, le ven. 29 déc. 2023 11:12:10 -0500, a ecrit:
> ---
>  exec/hashexec.c |  9 +++--
>  libdiskfs/disk-pager.c  |  4 +++-
>  libpager/pager-memcpy.c | 11 ---
>  libstore/memobj.c   | 11 ---
>  startup/startup.c   |  9 ++---
>  5 files changed, 32 insertions(+), 12 deletions(-)
> 
> diff --git a/exec/hashexec.c b/exec/hashexec.c
> index 9e00704..a107291 100644
> --- a/exec/hashexec.c
> +++ b/exec/hashexec.c
> @@ -53,6 +53,11 @@ check_hashbang (struct execdata *e,
>size_t new_argvlen;
>mach_port_t *new_dtable = NULL;
>mach_msg_type_number_t new_dtablesize;
> +  sigset_t arg_env_sigset;
> +
> +  sigemptyset (&arg_env_sigset);
> +  sigaddset (&arg_env_sigset, SIGSEGV);
> +  sigaddset (&arg_env_sigset, SIGBUS);
>  
>file_t user_fd (int fd)
>  {
> @@ -293,7 +298,7 @@ check_hashbang (struct execdata *e,
> if (strchr (name, '/') != NULL)
>   error = lookup (name, 0, &name_file);
> else if ((error = hurd_catch_signal
> - (sigmask (SIGBUS) | sigmask (SIGSEGV),
> + (arg_env_sigset,
>(vm_address_t) envp, (vm_address_t) envp + envplen,
>&search_path, SIG_ERR)))
>   name_file = MACH_PORT_NULL;
> @@ -416,7 +421,7 @@ check_hashbang (struct execdata *e,
>   }
>  
>/* Set up the arguments.  */
> -  hurd_catch_signal (sigmask (SIGSEGV) | sigmask (SIGBUS),
> +  hurd_catch_signal (arg_env_sigset,
>(vm_address_t) argv, (vm_address_t) argv + argvlen,
>&setup_args, &fault_handler);
>  }
> diff --git a/libdiskfs/disk-pager.c b/libdiskfs/disk-pager.c
> index 1a5d8bf..7af0e3b 100644
> --- a/libdiskfs/disk-pager.c
> +++ b/libdiskfs/disk-pager.c
> @@ -29,7 +29,6 @@ struct pager_requests *diskfs_disk_pager_requests;
>  static void fault_handler (int sig, long int sigcode, struct sigcontext 
> *scp);
>  static struct hurd_signal_preemptor preemptor =
>{
> -  signals: sigmask (SIGSEGV) | sigmask (SIGBUS),
>preemptor: NULL,
>handler: (sighandler_t) &fault_handler,
>};
> @@ -73,6 +72,9 @@ diskfs_start_disk_pager (struct user_pager_info *upi,
>/* Set up the signal preemptor to catch faults on the disk image.  */
>preemptor.first = (vm_address_t) *image;
>preemptor.last = ((vm_address_t) *image + size);
> +  sigemptyset (&preemptor.signals);
> +  sigaddset (&preemptor.signals, SIGSEGV);
> +  sigaddset (&preemptor.signals, SIGBUS);
>hurd_preempt_signals (&preemptor);
>  
>/* We have the mapping; we no longer need the send right.  */
> diff --git a/libpager/pager-memcpy.c b/libpager/pager-memcpy.c
> index caaf6f2..2ef42a7 100644
> --- a/libpager/pager-memcpy.c
> +++ b/libpager/pager-memcpy.c
> @@ -208,9 +208,14 @@ pager_memcpy (struct pager *pager, memory_object_t 
> memobj,
>window_size = 0;
>  
>if (sigsetjmp (buf, 1) == 0)
> -hurd_catch_signal (sigmask (SIGSEGV) | sigmask (SIGBUS),
> -window, window + window_size,
> -&do_copy, (sighandler_t) &fault);
> +{
> +  sigset_t mask;
> +  sigemptyset (&mask);
> +  sigaddset (&mask, SIGSEGV);
> +  sigaddset (&mask, SIGBUS);
> +  hurd_catch_signal (mask, window, window + window_size,
> +  &do_copy, (sighandler_t) &fault);
> +}
>  
>if (! err)
>  assert_backtrace (n == 0);
> diff --git a/libstore/memobj.c b/libstore/memobj.c
> index ea2d7b9..e98e1b2 100644
> --- a/libstore/memobj.c
> +++ b/libstore/memobj.c
> @@ -113,9 +113,14 @@ memobj_memcpy (memory_object_t memobj,
>  return 0;
>  
>if (sigsetjmp (buf, 1) == 0)
> -hurd_catch_signal (sigmask (SIGSEGV) | sigmask (SIGBUS),
> -window, window + windowsize,
> -©, (sighandler_t) &fault);
> +{
> +  sigset_t mask;
> +  sigemptyset (&mask);
> +  sigaddset (&mask, SIGSEGV);
> +  sigaddset (&mask, SIGBUS);
> +  hurd_catch_signal (mask, window, window + windowsize,
> +  ©, (sighandler_t) &fault);
> +}
>  
>if (window)
>  munmap ((caddr_t) window, windowsize);
> diff --git a/startup/startup.c b/startup/startup.c
> index 862572c..27af818 100644
> --- a/startup/startup.c
> +++ b/startup/startup.c
> @@ -826,9 +826,12 @@ main (int argc, char **argv, char **envp)
>/* All programs we start should ignore job control stop signals.
>   That way Posix.1 B.2.2.2 is satisfied where it says that programs
>   not run under job control shells are protected.  */
> -  default_ints[INIT_SIGIGN] = (sigmask (SIGTSTP)
> -| sigmask (SIGTTIN)
> -| sigmask (SIGTTOU));
> +  sigset_t sigmask;
> +  sigemptyset (&sigmask);
> +  sigaddset (&sigmask, SIGTSTP);
> +  sigaddset (&sigmask, SIGTTIN);
> +  sigaddset (&sigmask, SIGTTOU);
> +  default_ints[INIT_SIG

Re: [PATCH hurd 3/3] Mark msg_thread as noreturn

2023-12-29 Thread Samuel Thibault
Applied, thanks!

Flavio Cruz, le ven. 29 déc. 2023 11:12:11 -0500, a ecrit:
> ---
>  boot/boot.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/boot/boot.c b/boot/boot.c
> index 0d7ae74..d773bd1 100644
> --- a/boot/boot.c
> +++ b/boot/boot.c
> @@ -775,7 +775,7 @@ main (int argc, char **argv, char **envp)
>  }
>  }
>  
> -void *
> +void * __attribute__ ((noreturn))
>  msg_thread (void *arg)
>  {
>while (1)
> -- 
> 2.39.2
> 
> 

-- 
Samuel
---
Pour une évaluation indépendante, transparente et rigoureuse !
Je soutiens la Commission d'Évaluation de l'Inria.



Re: [PATCH 04/14] add gsync tests

2023-12-29 Thread Luca Dariz

Il 29/12/23 14:56, Samuel Thibault ha scritto:

Luca Dariz, le jeu. 28 déc. 2023 20:42:51 +0100, a ecrit:

+static void single_t2(void *arg)
+{
+  int err;
+  msleep(100);
+  err = gsync_wake(mach_task_self(), (vm_offset_t)&single_shared, 0, 0);
+  ASSERT_RET(err, "gsync_wake t2");
+
+  err = thread_terminate(mach_thread_self());
+  ASSERT_RET(err, "thread_terminate");


thread_terminate is not actually supposed to return, so that'd rather be
an assert(0)


right, I think I added the FAILURE() macro later than this and I forgot 
to update



+}
+
+static void test_single()
+{
+  int err;
+  single_shared = 0;
+  err = gsync_wait(mach_task_self(), (vm_offset_t)&single_shared, 0, 0, 100, 
GSYNC_TIMED);
+  ASSERT(err == KERN_TIMEDOUT, "gsync_wait 1");
+
+  single_shared = 1;
+  err = gsync_wait(mach_task_self(), (vm_offset_t)&single_shared, 0, 0, 100, 
GSYNC_TIMED);
+  ASSERT(err == KERN_INVALID_ARGUMENT, "gsync_wait 2");
+  err = gsync_wait(mach_task_self(), (vm_offset_t)&single_shared, 1, 0, 100, 
GSYNC_TIMED);
+  ASSERT(err == KERN_TIMEDOUT, "gsync_wait 2b");
+
+  err = gsync_wake(mach_task_self(), (vm_offset_t)&single_shared, 0, 0);
+  ASSERT_RET(err, "gsync_wake 1");
+
+  err = gsync_wake(mach_task_self(), (vm_offset_t)&single_shared, 0, 
GSYNC_BROADCAST);
+  ASSERT_RET(err, "gsync_wake 2");


Here, you want to try gsync_wait again, and check that they time out
again (i.e. the kernel did forget about the gsync_wake, as expected).


ok added


+  err = gsync_wake(mach_task_self(), (vm_offset_t)&single_shared, 2, 
GSYNC_MUTATE);
+  ASSERT_RET(err, "gsync_wake 2a");
+  ASSERT(single_shared == 2, "gsync_wake single_shared");
+
+  err = gsync_wake(mach_task_self(), (vm_offset_t)&single_shared, 0, 0);
+  ASSERT_RET(err, "gsync_wake 3");
+  err = gsync_wake(mach_task_self(), (vm_offset_t)&single_shared, 0, 0);
+  ASSERT_RET(err, "gsync_wake 3a");


Why two such calls?


To be honest I don't remember, maybe a mistake. I'll try to add more 
descriptive messages on assertions.



+  single_shared = 1;
+  test_thread_start(mach_task_self(), single_t2, 0);
+  err = gsync_wait(mach_task_self(), (vm_offset_t)&single_shared, 1, 0, 0, 0);
+  ASSERT_RET(err, "gsync_wait 4");


This is racy since the thread may start and call gsync_wake before
gsync_wait gets to block, and thus gsync_wait would stay stuck. Of
course in practice this doesn't happen because of the msleep(100) in the
thread, but that still leaves it racy and prone to fail if the system is
e.g. very busy. You want to make single_t2 modify single_shared, so that
if test_single2 happens to call gsync_wait after that, it will just not
block and return KERN_INVALID_ARGUMENT instead, as expected.


ok




+static void test_single2()
+{
+  int err;
+  test_thread_start(mach_task_self(), single_t2, 0);
+  single_shared = 2;
+  err = gsync_wait(mach_task_self(), (vm_offset_t)&single_shared, 2, 0, 0, 0);
+  ASSERT_RET(err, "gsync_wait 1");
+  // should this fail?
+  err = gsync_wake(mach_task_self(), (vm_offset_t)&single_shared, 0, 0);
+  ASSERT_RET(err, "gsync_wake 2");
+}


I don't understand the test. Why waking after waiting? There is nobody
to wake. You also want to set single_shared before starting the thread.


Looking at it again, it really seems the same case as above, I don't 
remember why I added the additional wake. I'll make this a 
test_single_from_thread() or similar, merge your suggestion above and 
remove the duplicate test from test_single(), I guess the exact value 
used in the synchronization is not relevant, as long as it's the 
expected one.



Luca



Re: [PATCH 04/14] add gsync tests

2023-12-29 Thread Samuel Thibault
Luca Dariz, le ven. 29 déc. 2023 18:21:56 +0100, a ecrit:
> I guess the exact value used in the synchronization is not relevant,
> as long as it's the expected one.

Yes.

Samuel



[PATCH hurd 02/11] Cast bootinfo to struct diskfs_control * to silence warning

2023-12-29 Thread Flavio Cruz
---
 libdiskfs/boot-start.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libdiskfs/boot-start.c b/libdiskfs/boot-start.c
index e8c09bc..8c159f3 100644
--- a/libdiskfs/boot-start.c
+++ b/libdiskfs/boot-start.c
@@ -464,7 +464,7 @@ diskfs_S_fsys_getpriv (struct diskfs_control 
*init_bootstrap_port,
   error_t err;
 
   if (!init_bootstrap_port
-  || init_bootstrap_port != bootinfo)
+  || init_bootstrap_port != (struct diskfs_control *) bootinfo)
 return EOPNOTSUPP;
 
   err = get_privileged_ports (host_priv, dev_master);
-- 
2.39.2




[PATCH hurd 08/11] proxy-defpager: add missing return statement

2023-12-29 Thread Flavio Cruz
---
 trans/proxy-defpager.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/trans/proxy-defpager.c b/trans/proxy-defpager.c
index 386f1b6..314ce9f 100644
--- a/trans/proxy-defpager.c
+++ b/trans/proxy-defpager.c
@@ -124,6 +124,8 @@ S_default_pager_paging_storage_new (mach_port_t 
default_pager,
 return err;
 
   mach_port_deallocate (mach_task_self (), device);
+
+  return 0;
 }
 
 #ifndef __x86_64__
-- 
2.39.2




[PATCH hurd 10/11] libftpconn: add out >= 2 condition to make GCC happy

2023-12-29 Thread Flavio Cruz
---
 libftpconn/cmd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libftpconn/cmd.c b/libftpconn/cmd.c
index 9916d03..4b4c6fa 100644
--- a/libftpconn/cmd.c
+++ b/libftpconn/cmd.c
@@ -99,7 +99,7 @@ ftp_conn_cmd (struct ftp_conn *conn, const char *cmd, const 
char *arg,
snprintf (buf, sizeof buf, arg ? "%s %s\r\n" : "%s\r\n", cmd, arg);
   err = _write (conn->control, buf, out);
 
-  if (!err && conn->hooks && conn->hooks->cntl_debug)
+  if (!err && conn->hooks && conn->hooks->cntl_debug && out >= 2)
{
  buf[out - 2] = '\0';  /* Stomp the CR & NL.  */
  (* conn->hooks->cntl_debug) (conn, FTP_CONN_CNTL_DEBUG_CMD, buf);
-- 
2.39.2




[PATCH hurd 07/11] Fix a few pointer related warnings.

2023-12-29 Thread Flavio Cruz
---
 utils/rpctrace.c   | 4 ++--
 utils/vmallocate.c | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/utils/rpctrace.c b/utils/rpctrace.c
index 589ce8f..f7046a0 100644
--- a/utils/rpctrace.c
+++ b/utils/rpctrace.c
@@ -810,10 +810,10 @@ print_contents (mach_msg_header_t *inp,
   int first = 1;
 
   /* Process the message data, wrapping ports and printing data.  */
-  while (msg_buf_ptr < (char *) inp + inp->msgh_size)
+  while ((char *) msg_buf_ptr < (char *) inp + inp->msgh_size)
 {
   mach_msg_type_t *const type = msg_buf_ptr;
-  mach_msg_type_long_t *const lt = (void *) type;
+  mach_msg_type_long_t *const lt = (mach_msg_type_long_t *) type;
   void *data;
   mach_msg_type_number_t nelt; /* Number of data items.  */
   mach_msg_type_size_t eltsize; /* Bytes per item.  */
diff --git a/utils/vmallocate.c b/utils/vmallocate.c
index 039b309..b7eafed 100644
--- a/utils/vmallocate.c
+++ b/utils/vmallocate.c
@@ -207,7 +207,7 @@ main (int argc, char **argv)
 *p = 1;
 
   if (verbose > 1
-  && ((unsigned int) (p - address) & ((1U<<20) - 1)) == 0)
+  && ((uintptr_t) (p - address) & ((1UL<<20) - 1)) == 0)
 fprintf (stderr, "\r%"PRIu64,
  allocated
  + ((uint64_t) (uintptr_t) p - (uint64_t) address));
-- 
2.39.2




[PATCH hurd 09/11] Fix overflow issues in tmpfs and vmallocate

2023-12-29 Thread Flavio Cruz
---
 tmpfs/tmpfs.c  | 2 +-
 utils/vmallocate.c | 7 +--
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/tmpfs/tmpfs.c b/tmpfs/tmpfs.c
index 02d4bd8..d28806a 100644
--- a/tmpfs/tmpfs.c
+++ b/tmpfs/tmpfs.c
@@ -39,7 +39,7 @@ char *diskfs_disk_name = "none";
 int diskfs_default_sync_interval = 0;
 
 /* We must supply some claimed limits, though we don't impose any new ones.  */
-int diskfs_link_max = (1ULL << (sizeof (nlink_t) * CHAR_BIT)) - 1;
+int diskfs_link_max = INT_MAX;
 int diskfs_name_max = 255; /* dirent d_namlen limit */
 int diskfs_maxsymlinks = 8;
 
diff --git a/utils/vmallocate.c b/utils/vmallocate.c
index b7eafed..fde8e76 100644
--- a/utils/vmallocate.c
+++ b/utils/vmallocate.c
@@ -160,8 +160,11 @@ main (int argc, char **argv)
   struct child *c, *children = NULL;
   process_t proc = getproc ();
 
-  /* We must make sure that chunk_size fits into vm_size_t.  */
-  assert_backtrace (chunk_size <= 1U << (sizeof (vm_size_t) * 8 - 1));
+  /* We must make sure that chunk_size fits into vm_size_t.
+   * We assume sizeof (vm_size_t) = sizeof (uintptr_t). */
+  _Static_assert (sizeof (vm_size_t) == sizeof (uintptr_t),
+  "expected sizeof (vm_size_t) == sizeof (uintptr_t).");
+  assert_backtrace (chunk_size <= UINTPTR_MAX);
 
   /* Parse our arguments.  */
   argp_parse (&argp, argc, argv, 0, 0, 0);
-- 
2.39.2




[PATCH hurd 05/11] x86_64: utmp uses int32_t to store time so use a temporary variable

2023-12-29 Thread Flavio Cruz
---
 utils/login.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/utils/login.c b/utils/login.c
index 3134c4a..3ed5121 100644
--- a/utils/login.c
+++ b/utils/login.c
@@ -157,12 +157,17 @@ static void
 add_utmp_entry (char *args, unsigned args_len, int inherit_host)
 {
   struct utmp utmp;
+  struct timeval current_time;
   char const *host = 0;
   long addr = 0;
 
   memset (&utmp, 0, sizeof(utmp));
 
-  gettimeofday (&utmp.ut_tv, 0);
+  gettimeofday (¤t_time, 0);
+  /* For x86_64, sizeof(utmp.ut_tv) != sizeof(struct timeval) */
+  utmp.ut_tv.tv_sec = (int32_t) current_time.tv_sec;
+  utmp.ut_tv.tv_usec = (int32_t) current_time.tv_usec;
+
   strncpy (utmp.ut_name, envz_get (args, args_len, "USER") ?: "",
   sizeof (utmp.ut_name));
 
-- 
2.39.2




[PATCH hurd 04/11] Fix printf format specifiers

2023-12-29 Thread Flavio Cruz
---
 pci-arbiter/options.c | 2 +-
 utils/ftpdir.c| 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/pci-arbiter/options.c b/pci-arbiter/options.c
index e0455f4..e578810 100644
--- a/pci-arbiter/options.c
+++ b/pci-arbiter/options.c
@@ -379,7 +379,7 @@ netfs_append_args (char **argz, size_t * argz_len)
 }
 
   if (fs->params.node_cache_max != NODE_CACHE_MAX)
-ADD_OPT ("--ncache=%u", fs->params.node_cache_max);
+ADD_OPT ("--ncache=%zu", fs->params.node_cache_max);
 
   if (fs->params.next_task != MACH_PORT_NULL)
 ADD_OPT ("--next-task=%u", fs->params.next_task);
diff --git a/utils/ftpdir.c b/utils/ftpdir.c
index 86ef58f..dd4d65d 100644
--- a/utils/ftpdir.c
+++ b/utils/ftpdir.c
@@ -203,7 +203,7 @@ pdirent (const char *name, const struct stat *st, const 
char *symlink_target,
 {
   char timebuf[20];
   strftime (timebuf, sizeof timebuf, "%Y-%m-%d %H:%M", localtime 
(&st->st_mtime));
-  printf ("%6o %2ld %5d %5d %6" PRIi64 "  %s  %s\n",
+  printf ("%6o %2zu %5d %5d %6" PRIi64 "  %s  %s\n",
  st->st_mode, st->st_nlink, st->st_uid, st->st_gid, st->st_size,
  timebuf, name);
   if (symlink_target)
-- 
2.39.2




[PATCH hurd 01/11] Initialize a few error variables to avoid GCC warnings

2023-12-29 Thread Flavio Cruz
---
 ext2fs/pager.c | 2 +-
 fatfs/pager.c  | 2 +-
 trans/random.c | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/ext2fs/pager.c b/ext2fs/pager.c
index 2869f4d..61db0df 100644
--- a/ext2fs/pager.c
+++ b/ext2fs/pager.c
@@ -170,7 +170,7 @@ static error_t
 file_pager_read_page (struct node *node, vm_offset_t page,
  void **buf, int *writelock)
 {
-  error_t err;
+  error_t err = 0;
   int offs = 0;
   int partial = 0; /* A page truncated by the EOF.  */
   pthread_rwlock_t *lock = NULL;
diff --git a/fatfs/pager.c b/fatfs/pager.c
index 36e7012..efe0203 100644
--- a/fatfs/pager.c
+++ b/fatfs/pager.c
@@ -210,7 +210,7 @@ static error_t
 file_pager_read_huge_page (struct node *node, vm_offset_t page,
   void **buf, int *writelock)
 {
-  error_t err;
+  error_t err = 0;
   int offs = 0;
   pthread_rwlock_t *lock = NULL;
   int left = vm_page_size;
diff --git a/trans/random.c b/trans/random.c
index b68354e..7e3d3eb 100644
--- a/trans/random.c
+++ b/trans/random.c
@@ -158,7 +158,7 @@ update_random_seed_file (void)
 static error_t
 read_random_seed_file (void)
 {
-  error_t err;
+  error_t err = 0;
   int fd;
   struct stat s;
   void *map;
-- 
2.39.2




[PATCH hurd 03/11] Use mach_msg_type_number_t whenever required to avoid warnings

2023-12-29 Thread Flavio Cruz
---
 libfshelp-tests/race.c |  9 +
 libnetfs/file-get-translator.c |  2 +-
 pfinet/ethernet.c  |  2 +-
 pfinet/io-ops.c| 10 ++
 utils/mount.c  |  2 +-
 utils/msgport.c|  2 +-
 6 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/libfshelp-tests/race.c b/libfshelp-tests/race.c
index 376ada2..e1df296 100644
--- a/libfshelp-tests/race.c
+++ b/libfshelp-tests/race.c
@@ -33,7 +33,8 @@ int main (int argc, char **argv)
   mach_port_t rendezvous = MACH_PORT_NULL;
   int fd;
   int i;
-  mach_msg_type_number_t v;
+  mach_msg_type_number_t n_read;
+  vm_size_t v;
   int blocked = 0;
   char buf[10] = "";
   char *bufp;
@@ -61,12 +62,12 @@ int main (int argc, char **argv)
   if (err)
 error (1, err, "file_record_lock");
 
-  v = sizeof (buf);
+  v = n_read = sizeof (buf);
   bufp = buf;
-  io_read (fd, &bufp, &v, 0, v);
+  io_read (fd, &bufp, &n_read, 0, v);
 
   v = atoi (bufp);
-  sprintf (buf, "%d\n", v + 1);
+  sprintf (buf, "%d\n", (int) (v + 1));
 
   v = 10;
   io_write (fd, buf, sizeof (buf), 0, &v);
diff --git a/libnetfs/file-get-translator.c b/libnetfs/file-get-translator.c
index b3998b0..f402250 100644
--- a/libnetfs/file-get-translator.c
+++ b/libnetfs/file-get-translator.c
@@ -116,7 +116,7 @@ netfs_S_file_get_translator (struct protid *user,
   else if (np->nn_translated & S_IPTRANS)
 {
   char *string = NULL;
-  size_t len = 0;
+  mach_msg_type_number_t len = 0;
   err = netfs_get_translator (np, &string, &len);
   if (!err)
{
diff --git a/pfinet/ethernet.c b/pfinet/ethernet.c
index ad21917..65ec1e2 100644
--- a/pfinet/ethernet.c
+++ b/pfinet/ethernet.c
@@ -324,7 +324,7 @@ void
 setup_ethernet_device (char *name, struct device **device)
 {
   struct net_status netstat;
-  size_t count;
+  mach_msg_type_number_t count;
   int net_address[2];
   error_t err;
   struct ether_device *edev;
diff --git a/pfinet/io-ops.c b/pfinet/io-ops.c
index 818f113..5f83a02 100644
--- a/pfinet/io-ops.c
+++ b/pfinet/io-ops.c
@@ -135,10 +135,11 @@ S_io_seek (struct sock_user *user,
 
 kern_return_t
 S_io_readable (struct sock_user *user,
-  vm_size_t *amount)
+  vm_size_t *out_amount)
 {
   struct sock *sk;
   error_t err;
+  mach_msg_type_number_t amount = 0;
 
   if (!user)
 return EOPNOTSUPP;
@@ -160,7 +161,8 @@ S_io_readable (struct sock_user *user,
 {
 case SOCK_STREAM:
 case SOCK_SEQPACKET:
-  err = tcp_tiocinq (sk, amount);
+  err = tcp_tiocinq (sk, &amount);
+  *out_amount = amount;
   break;
 
 case SOCK_DGRAM:
@@ -169,7 +171,7 @@ S_io_readable (struct sock_user *user,
err = EINVAL;
   else
/* Boy, I really love the C language. */
-   *amount = (skb_peek (&sk->receive_queue)
+   *out_amount = (skb_peek (&sk->receive_queue)
   ? : &((struct sk_buff){}))->len;
   break;
 
@@ -358,7 +360,7 @@ S_io_reauthenticate (struct sock_user *user,
   struct sock_user *newuser;
   uid_t gubuf[20], ggbuf[20], aubuf[20], agbuf[20];
   uid_t *gen_uids, *gen_gids, *aux_uids, *aux_gids;
-  size_t genuidlen, gengidlen, auxuidlen, auxgidlen;
+  mach_msg_type_number_t genuidlen, gengidlen, auxuidlen, auxgidlen;
   error_t err;
   size_t i, j;
   auth_t auth;
diff --git a/utils/mount.c b/utils/mount.c
index 75211d7..898e056 100644
--- a/utils/mount.c
+++ b/utils/mount.c
@@ -493,7 +493,7 @@ do_query (struct fs *fs)
   error_t err;
   fsys_t fsys;
   char _opts[200], *opts = _opts;
-  size_t opts_len = sizeof opts;
+  mach_msg_type_number_t opts_len = sizeof opts;
   size_t nopts;
 
   err = fs_fsys (fs, &fsys);
diff --git a/utils/msgport.c b/utils/msgport.c
index a07cc0e..e3ea430 100644
--- a/utils/msgport.c
+++ b/utils/msgport.c
@@ -676,7 +676,7 @@ main(int argc, char *argv[])
   size_t num_cmds = 0;
   struct cmds_argp_params cmds_argp_params = { &cmds, &num_cmds };
   pid_t *pids = 0;  /* User-specified pids.  */
-  size_t num_pids = 0;
+  mach_msg_type_number_t num_pids = 0;
   struct pids_argp_params pids_argp_params = { &pids, &num_pids, 0 };
 
   error_t parse_opt (int key, char *arg, struct argp_state *state)
-- 
2.39.2




[PATCH hurd 11/11] pfinet: fix type alias

2023-12-29 Thread Flavio Cruz
---
 pfinet/stubs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/pfinet/stubs.c b/pfinet/stubs.c
index 9affcff..01ba2fa 100644
--- a/pfinet/stubs.c
+++ b/pfinet/stubs.c
@@ -50,7 +50,7 @@ void dev_activate (struct device *)
  __attribute__ ((alias ("dev_init_scheduler")));
 void dev_deactivate (struct device *)
  __attribute__ ((alias ("dev_init_scheduler")));
-void tcp_ioctl (void) __attribute__ ((alias ("dev_init_scheduler")));
+void tcp_ioctl (struct device *) __attribute__ ((alias 
("dev_init_scheduler")));
 
 /* This isn't quite a stub, but it's not quite right either.  */
 __u32 secure_tcp_sequence_number(__u32 saddr, __u32 daddr,
-- 
2.39.2




[PATCH hurd 06/11] x86_64: use 21 bytes in libps since %z might require more characters.

2023-12-29 Thread Flavio Cruz
This makes GCC happy.
---
 libps/spec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libps/spec.c b/libps/spec.c
index dec5704..9f64703 100644
--- a/libps/spec.c
+++ b/libps/spec.c
@@ -492,7 +492,7 @@ error_t
 ps_emit_nice_size_t (struct proc_stat *ps, struct ps_fmt_field *field,
 struct ps_stream *stream)
 {
-  char buf[20];
+  char buf[21];
   size_t value = FG_PROC_STAT (field, size_t)(ps);
   char *sfx = " KMG";
   size_t frac = 0;
-- 
2.39.2




Re: [PATCH hurd 01/11] Initialize a few error variables to avoid GCC warnings

2023-12-29 Thread Samuel Thibault
Applied, thanks!

Flavio Cruz, le ven. 29 déc. 2023 16:20:55 -0500, a ecrit:
> ---
>  ext2fs/pager.c | 2 +-
>  fatfs/pager.c  | 2 +-
>  trans/random.c | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/ext2fs/pager.c b/ext2fs/pager.c
> index 2869f4d..61db0df 100644
> --- a/ext2fs/pager.c
> +++ b/ext2fs/pager.c
> @@ -170,7 +170,7 @@ static error_t
>  file_pager_read_page (struct node *node, vm_offset_t page,
> void **buf, int *writelock)
>  {
> -  error_t err;
> +  error_t err = 0;
>int offs = 0;
>int partial = 0;   /* A page truncated by the EOF.  */
>pthread_rwlock_t *lock = NULL;
> diff --git a/fatfs/pager.c b/fatfs/pager.c
> index 36e7012..efe0203 100644
> --- a/fatfs/pager.c
> +++ b/fatfs/pager.c
> @@ -210,7 +210,7 @@ static error_t
>  file_pager_read_huge_page (struct node *node, vm_offset_t page,
>  void **buf, int *writelock)
>  {
> -  error_t err;
> +  error_t err = 0;
>int offs = 0;
>pthread_rwlock_t *lock = NULL;
>int left = vm_page_size;
> diff --git a/trans/random.c b/trans/random.c
> index b68354e..7e3d3eb 100644
> --- a/trans/random.c
> +++ b/trans/random.c
> @@ -158,7 +158,7 @@ update_random_seed_file (void)
>  static error_t
>  read_random_seed_file (void)
>  {
> -  error_t err;
> +  error_t err = 0;
>int fd;
>struct stat s;
>void *map;
> -- 
> 2.39.2
> 
> 

-- 
Samuel
---
Pour une évaluation indépendante, transparente et rigoureuse !
Je soutiens la Commission d'Évaluation de l'Inria.



Re: [PATCH hurd 02/11] Cast bootinfo to struct diskfs_control * to silence warning

2023-12-29 Thread Samuel Thibault
Applied, thanks!

Flavio Cruz, le ven. 29 déc. 2023 16:20:56 -0500, a ecrit:
> ---
>  libdiskfs/boot-start.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libdiskfs/boot-start.c b/libdiskfs/boot-start.c
> index e8c09bc..8c159f3 100644
> --- a/libdiskfs/boot-start.c
> +++ b/libdiskfs/boot-start.c
> @@ -464,7 +464,7 @@ diskfs_S_fsys_getpriv (struct diskfs_control 
> *init_bootstrap_port,
>error_t err;
>  
>if (!init_bootstrap_port
> -  || init_bootstrap_port != bootinfo)
> +  || init_bootstrap_port != (struct diskfs_control *) bootinfo)
>  return EOPNOTSUPP;
>  
>err = get_privileged_ports (host_priv, dev_master);
> -- 
> 2.39.2
> 
> 

-- 
Samuel
---
Pour une évaluation indépendante, transparente et rigoureuse !
Je soutiens la Commission d'Évaluation de l'Inria.



Re: [PATCH hurd 03/11] Use mach_msg_type_number_t whenever required to avoid warnings

2023-12-29 Thread Samuel Thibault
Applied, thanks!

Flavio Cruz, le ven. 29 déc. 2023 16:20:57 -0500, a ecrit:
> ---
>  libfshelp-tests/race.c |  9 +
>  libnetfs/file-get-translator.c |  2 +-
>  pfinet/ethernet.c  |  2 +-
>  pfinet/io-ops.c| 10 ++
>  utils/mount.c  |  2 +-
>  utils/msgport.c|  2 +-
>  6 files changed, 15 insertions(+), 12 deletions(-)
> 
> diff --git a/libfshelp-tests/race.c b/libfshelp-tests/race.c
> index 376ada2..e1df296 100644
> --- a/libfshelp-tests/race.c
> +++ b/libfshelp-tests/race.c
> @@ -33,7 +33,8 @@ int main (int argc, char **argv)
>mach_port_t rendezvous = MACH_PORT_NULL;
>int fd;
>int i;
> -  mach_msg_type_number_t v;
> +  mach_msg_type_number_t n_read;
> +  vm_size_t v;
>int blocked = 0;
>char buf[10] = "";
>char *bufp;
> @@ -61,12 +62,12 @@ int main (int argc, char **argv)
>if (err)
>  error (1, err, "file_record_lock");
>  
> -  v = sizeof (buf);
> +  v = n_read = sizeof (buf);
>bufp = buf;
> -  io_read (fd, &bufp, &v, 0, v);
> +  io_read (fd, &bufp, &n_read, 0, v);
>  
>v = atoi (bufp);
> -  sprintf (buf, "%d\n", v + 1);
> +  sprintf (buf, "%d\n", (int) (v + 1));
>  
>v = 10;
>io_write (fd, buf, sizeof (buf), 0, &v);
> diff --git a/libnetfs/file-get-translator.c b/libnetfs/file-get-translator.c
> index b3998b0..f402250 100644
> --- a/libnetfs/file-get-translator.c
> +++ b/libnetfs/file-get-translator.c
> @@ -116,7 +116,7 @@ netfs_S_file_get_translator (struct protid *user,
>else if (np->nn_translated & S_IPTRANS)
>  {
>char *string = NULL;
> -  size_t len = 0;
> +  mach_msg_type_number_t len = 0;
>err = netfs_get_translator (np, &string, &len);
>if (!err)
>   {
> diff --git a/pfinet/ethernet.c b/pfinet/ethernet.c
> index ad21917..65ec1e2 100644
> --- a/pfinet/ethernet.c
> +++ b/pfinet/ethernet.c
> @@ -324,7 +324,7 @@ void
>  setup_ethernet_device (char *name, struct device **device)
>  {
>struct net_status netstat;
> -  size_t count;
> +  mach_msg_type_number_t count;
>int net_address[2];
>error_t err;
>struct ether_device *edev;
> diff --git a/pfinet/io-ops.c b/pfinet/io-ops.c
> index 818f113..5f83a02 100644
> --- a/pfinet/io-ops.c
> +++ b/pfinet/io-ops.c
> @@ -135,10 +135,11 @@ S_io_seek (struct sock_user *user,
>  
>  kern_return_t
>  S_io_readable (struct sock_user *user,
> -vm_size_t *amount)
> +vm_size_t *out_amount)
>  {
>struct sock *sk;
>error_t err;
> +  mach_msg_type_number_t amount = 0;
>  
>if (!user)
>  return EOPNOTSUPP;
> @@ -160,7 +161,8 @@ S_io_readable (struct sock_user *user,
>  {
>  case SOCK_STREAM:
>  case SOCK_SEQPACKET:
> -  err = tcp_tiocinq (sk, amount);
> +  err = tcp_tiocinq (sk, &amount);
> +  *out_amount = amount;
>break;
>  
>  case SOCK_DGRAM:
> @@ -169,7 +171,7 @@ S_io_readable (struct sock_user *user,
>   err = EINVAL;
>else
>   /* Boy, I really love the C language. */
> - *amount = (skb_peek (&sk->receive_queue)
> + *out_amount = (skb_peek (&sk->receive_queue)
>  ? : &((struct sk_buff){}))->len;
>break;
>  
> @@ -358,7 +360,7 @@ S_io_reauthenticate (struct sock_user *user,
>struct sock_user *newuser;
>uid_t gubuf[20], ggbuf[20], aubuf[20], agbuf[20];
>uid_t *gen_uids, *gen_gids, *aux_uids, *aux_gids;
> -  size_t genuidlen, gengidlen, auxuidlen, auxgidlen;
> +  mach_msg_type_number_t genuidlen, gengidlen, auxuidlen, auxgidlen;
>error_t err;
>size_t i, j;
>auth_t auth;
> diff --git a/utils/mount.c b/utils/mount.c
> index 75211d7..898e056 100644
> --- a/utils/mount.c
> +++ b/utils/mount.c
> @@ -493,7 +493,7 @@ do_query (struct fs *fs)
>error_t err;
>fsys_t fsys;
>char _opts[200], *opts = _opts;
> -  size_t opts_len = sizeof opts;
> +  mach_msg_type_number_t opts_len = sizeof opts;
>size_t nopts;
>  
>err = fs_fsys (fs, &fsys);
> diff --git a/utils/msgport.c b/utils/msgport.c
> index a07cc0e..e3ea430 100644
> --- a/utils/msgport.c
> +++ b/utils/msgport.c
> @@ -676,7 +676,7 @@ main(int argc, char *argv[])
>size_t num_cmds = 0;
>struct cmds_argp_params cmds_argp_params = { &cmds, &num_cmds };
>pid_t *pids = 0;  /* User-specified pids.  */
> -  size_t num_pids = 0;
> +  mach_msg_type_number_t num_pids = 0;
>struct pids_argp_params pids_argp_params = { &pids, &num_pids, 0 };
>  
>error_t parse_opt (int key, char *arg, struct argp_state *state)
> -- 
> 2.39.2
> 
> 

-- 
Samuel
---
Pour une évaluation indépendante, transparente et rigoureuse !
Je soutiens la Commission d'Évaluation de l'Inria.



Re: [PATCH hurd 04/11] Fix printf format specifiers

2023-12-29 Thread Samuel Thibault
Applied, thanks!

Flavio Cruz, le ven. 29 déc. 2023 16:20:58 -0500, a ecrit:
> ---
>  pci-arbiter/options.c | 2 +-
>  utils/ftpdir.c| 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/pci-arbiter/options.c b/pci-arbiter/options.c
> index e0455f4..e578810 100644
> --- a/pci-arbiter/options.c
> +++ b/pci-arbiter/options.c
> @@ -379,7 +379,7 @@ netfs_append_args (char **argz, size_t * argz_len)
>  }
>  
>if (fs->params.node_cache_max != NODE_CACHE_MAX)
> -ADD_OPT ("--ncache=%u", fs->params.node_cache_max);
> +ADD_OPT ("--ncache=%zu", fs->params.node_cache_max);
>  
>if (fs->params.next_task != MACH_PORT_NULL)
>  ADD_OPT ("--next-task=%u", fs->params.next_task);
> diff --git a/utils/ftpdir.c b/utils/ftpdir.c
> index 86ef58f..dd4d65d 100644
> --- a/utils/ftpdir.c
> +++ b/utils/ftpdir.c
> @@ -203,7 +203,7 @@ pdirent (const char *name, const struct stat *st, const 
> char *symlink_target,
>  {
>char timebuf[20];
>strftime (timebuf, sizeof timebuf, "%Y-%m-%d %H:%M", localtime 
> (&st->st_mtime));
> -  printf ("%6o %2ld %5d %5d %6" PRIi64 "  %s  %s\n",
> +  printf ("%6o %2zu %5d %5d %6" PRIi64 "  %s  %s\n",
> st->st_mode, st->st_nlink, st->st_uid, st->st_gid, st->st_size,
> timebuf, name);
>if (symlink_target)
> -- 
> 2.39.2
> 
> 

-- 
Samuel
---
Pour une évaluation indépendante, transparente et rigoureuse !
Je soutiens la Commission d'Évaluation de l'Inria.



Re: [PATCH hurd 05/11] x86_64: utmp uses int32_t to store time so use a temporary variable

2023-12-29 Thread Samuel Thibault
Flavio Cruz, le ven. 29 déc. 2023 16:20:59 -0500, a ecrit:
> ---
>  utils/login.c | 7 ++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/utils/login.c b/utils/login.c
> index 3134c4a..3ed5121 100644
> --- a/utils/login.c
> +++ b/utils/login.c
> @@ -157,12 +157,17 @@ static void
>  add_utmp_entry (char *args, unsigned args_len, int inherit_host)
>  {
>struct utmp utmp;
> +  struct timeval current_time;
>char const *host = 0;
>long addr = 0;
>  
>memset (&utmp, 0, sizeof(utmp));
>  
> -  gettimeofday (&utmp.ut_tv, 0);
> +  gettimeofday (¤t_time, 0);
> +  /* For x86_64, sizeof(utmp.ut_tv) != sizeof(struct timeval) */
> +  utmp.ut_tv.tv_sec = (int32_t) current_time.tv_sec;
> +  utmp.ut_tv.tv_usec = (int32_t) current_time.tv_usec;

I don't think we want to explicitly cast? Glibc's utmp has a #if/#else
that possibly makes it use timeval.

> +
>strncpy (utmp.ut_name, envz_get (args, args_len, "USER") ?: "",
>  sizeof (utmp.ut_name));




Re: [PATCH hurd 06/11] x86_64: use 21 bytes in libps since %z might require more characters.

2023-12-29 Thread Samuel Thibault
Flavio Cruz, le ven. 29 déc. 2023 16:21:00 -0500, a ecrit:
> This makes GCC happy.
> ---
>  libps/spec.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libps/spec.c b/libps/spec.c
> index dec5704..9f64703 100644
> --- a/libps/spec.c
> +++ b/libps/spec.c
> @@ -492,7 +492,7 @@ error_t
>  ps_emit_nice_size_t (struct proc_stat *ps, struct ps_fmt_field *field,
>struct ps_stream *stream)
>  {
> -  char buf[20];
> +  char buf[21];

Mmm, the whole code seems not 64bit-clean. I'm fine with adding this
21th byte to shut the warning, although I don't think it can be useful
(the code makes sure value is < 1024, and frac_len is 3).

>size_t value = FG_PROC_STAT (field, size_t)(ps);
>char *sfx = " KMG";

This is lacking "TPE" to be able to support SIZE_MAX. And then we probably
want to add a compile-check that sizeof(size_t) <= 8 :)

Samuel



Re: [PATCH hurd 07/11] Fix a few pointer related warnings.

2023-12-29 Thread Samuel Thibault
Applied, thanks!

Flavio Cruz, le ven. 29 déc. 2023 16:21:01 -0500, a ecrit:
> ---
>  utils/rpctrace.c   | 4 ++--
>  utils/vmallocate.c | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/utils/rpctrace.c b/utils/rpctrace.c
> index 589ce8f..f7046a0 100644
> --- a/utils/rpctrace.c
> +++ b/utils/rpctrace.c
> @@ -810,10 +810,10 @@ print_contents (mach_msg_header_t *inp,
>int first = 1;
>  
>/* Process the message data, wrapping ports and printing data.  */
> -  while (msg_buf_ptr < (char *) inp + inp->msgh_size)
> +  while ((char *) msg_buf_ptr < (char *) inp + inp->msgh_size)
>  {
>mach_msg_type_t *const type = msg_buf_ptr;
> -  mach_msg_type_long_t *const lt = (void *) type;
> +  mach_msg_type_long_t *const lt = (mach_msg_type_long_t *) type;
>void *data;
>mach_msg_type_number_t nelt; /* Number of data items.  */
>mach_msg_type_size_t eltsize; /* Bytes per item.  */
> diff --git a/utils/vmallocate.c b/utils/vmallocate.c
> index 039b309..b7eafed 100644
> --- a/utils/vmallocate.c
> +++ b/utils/vmallocate.c
> @@ -207,7 +207,7 @@ main (int argc, char **argv)
>  *p = 1;
>  
>if (verbose > 1
> -  && ((unsigned int) (p - address) & ((1U<<20) - 1)) == 0)
> +  && ((uintptr_t) (p - address) & ((1UL<<20) - 1)) == 0)
>  fprintf (stderr, "\r%"PRIu64,
>   allocated
>   + ((uint64_t) (uintptr_t) p - (uint64_t) address));
> -- 
> 2.39.2
> 
> 

-- 
Samuel
---
Pour une évaluation indépendante, transparente et rigoureuse !
Je soutiens la Commission d'Évaluation de l'Inria.



Re: [PATCH hurd 08/11] proxy-defpager: add missing return statement

2023-12-29 Thread Samuel Thibault
Applied, thanks!
Flavio Cruz, le ven. 29 déc. 2023 16:21:02 -0500, a ecrit:
> ---
>  trans/proxy-defpager.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/trans/proxy-defpager.c b/trans/proxy-defpager.c
> index 386f1b6..314ce9f 100644
> --- a/trans/proxy-defpager.c
> +++ b/trans/proxy-defpager.c
> @@ -124,6 +124,8 @@ S_default_pager_paging_storage_new (mach_port_t 
> default_pager,
>  return err;
>  
>mach_port_deallocate (mach_task_self (), device);
> +
> +  return 0;
>  }
>  
>  #ifndef __x86_64__
> -- 
> 2.39.2
> 
> 

-- 
Samuel
---
Pour une évaluation indépendante, transparente et rigoureuse !
Je soutiens la Commission d'Évaluation de l'Inria.



Re: [PATCH hurd 09/11] Fix overflow issues in tmpfs and vmallocate

2023-12-29 Thread Samuel Thibault
Applied, thanks!

Flavio Cruz, le ven. 29 déc. 2023 16:21:03 -0500, a ecrit:
> ---
>  tmpfs/tmpfs.c  | 2 +-
>  utils/vmallocate.c | 7 +--
>  2 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/tmpfs/tmpfs.c b/tmpfs/tmpfs.c
> index 02d4bd8..d28806a 100644
> --- a/tmpfs/tmpfs.c
> +++ b/tmpfs/tmpfs.c
> @@ -39,7 +39,7 @@ char *diskfs_disk_name = "none";
>  int diskfs_default_sync_interval = 0;
>  
>  /* We must supply some claimed limits, though we don't impose any new ones.  
> */
> -int diskfs_link_max = (1ULL << (sizeof (nlink_t) * CHAR_BIT)) - 1;
> +int diskfs_link_max = INT_MAX;
>  int diskfs_name_max = 255;   /* dirent d_namlen limit */
>  int diskfs_maxsymlinks = 8;
>  
> diff --git a/utils/vmallocate.c b/utils/vmallocate.c
> index b7eafed..fde8e76 100644
> --- a/utils/vmallocate.c
> +++ b/utils/vmallocate.c
> @@ -160,8 +160,11 @@ main (int argc, char **argv)
>struct child *c, *children = NULL;
>process_t proc = getproc ();
>  
> -  /* We must make sure that chunk_size fits into vm_size_t.  */
> -  assert_backtrace (chunk_size <= 1U << (sizeof (vm_size_t) * 8 - 1));
> +  /* We must make sure that chunk_size fits into vm_size_t.
> +   * We assume sizeof (vm_size_t) = sizeof (uintptr_t). */
> +  _Static_assert (sizeof (vm_size_t) == sizeof (uintptr_t),
> +  "expected sizeof (vm_size_t) == sizeof (uintptr_t).");
> +  assert_backtrace (chunk_size <= UINTPTR_MAX);
>  
>/* Parse our arguments.  */
>argp_parse (&argp, argc, argv, 0, 0, 0);
> -- 
> 2.39.2
> 
> 

-- 
Samuel
---
Pour une évaluation indépendante, transparente et rigoureuse !
Je soutiens la Commission d'Évaluation de l'Inria.



Re: [PATCH hurd 10/11] libftpconn: add out >= 2 condition to make GCC happy

2023-12-29 Thread Samuel Thibault
Applied, thanks!

Flavio Cruz, le ven. 29 déc. 2023 16:21:04 -0500, a ecrit:
> ---
>  libftpconn/cmd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libftpconn/cmd.c b/libftpconn/cmd.c
> index 9916d03..4b4c6fa 100644
> --- a/libftpconn/cmd.c
> +++ b/libftpconn/cmd.c
> @@ -99,7 +99,7 @@ ftp_conn_cmd (struct ftp_conn *conn, const char *cmd, const 
> char *arg,
>   snprintf (buf, sizeof buf, arg ? "%s %s\r\n" : "%s\r\n", cmd, arg);
>err = _write (conn->control, buf, out);
>  
> -  if (!err && conn->hooks && conn->hooks->cntl_debug)
> +  if (!err && conn->hooks && conn->hooks->cntl_debug && out >= 2)
>   {
> buf[out - 2] = '\0';  /* Stomp the CR & NL.  */
> (* conn->hooks->cntl_debug) (conn, FTP_CONN_CNTL_DEBUG_CMD, buf);
> -- 
> 2.39.2
> 
> 

-- 
Samuel
---
Pour une évaluation indépendante, transparente et rigoureuse !
Je soutiens la Commission d'Évaluation de l'Inria.



Re: [PATCH hurd 11/11] pfinet: fix type alias

2023-12-29 Thread Samuel Thibault
Applied, thanks!

Flavio Cruz, le ven. 29 déc. 2023 16:21:05 -0500, a ecrit:
> ---
>  pfinet/stubs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/pfinet/stubs.c b/pfinet/stubs.c
> index 9affcff..01ba2fa 100644
> --- a/pfinet/stubs.c
> +++ b/pfinet/stubs.c
> @@ -50,7 +50,7 @@ void dev_activate (struct device *)
>   __attribute__ ((alias ("dev_init_scheduler")));
>  void dev_deactivate (struct device *)
>   __attribute__ ((alias ("dev_init_scheduler")));
> -void tcp_ioctl (void) __attribute__ ((alias ("dev_init_scheduler")));
> +void tcp_ioctl (struct device *) __attribute__ ((alias 
> ("dev_init_scheduler")));
>  
>  /* This isn't quite a stub, but it's not quite right either.  */
>  __u32 secure_tcp_sequence_number(__u32 saddr, __u32 daddr,
> -- 
> 2.39.2
> 
> 

-- 
Samuel
---
Pour une évaluation indépendante, transparente et rigoureuse !
Je soutiens la Commission d'Évaluation de l'Inria.