Re,
This is containing a lot of different changes...
Always remember about the power of git bisect. We do want to separate
unrelated changes, to easily determine the culprits of regressions.
Also, that'll allow to apply parts of your changes that don't pose
questions, while others are discussed.
Alperen Erkan, le dim. 20 sept. 2026 11:53:42 +0300, a ecrit:
> poll() and wake_pipe Architecture: The temporary select() loop in the
> main function has been replaced with a
> modern poll() mechanism.
Why doing this?
I never blindly buy the "modern" argument. We select on just one file
descriptor, so how can poll() be better in that case?
> Incoming read requests (queue_read) now wake the main loop via a
> wake_pipe to dynamically monitor stdin.
That looks unnecessarily complex. Why doing all that?
> Dedicated Select Thread: A dedicated select_thread and queue (selq),
> supported by CLOCK_MONOTONIC,
> have been added to safely process non-console select requests without
> clock drift side-effects.
Again, what for? Also, it seems like it belongs to the next patch which
actually makes use of it
> Buffer and Memory Safeguards: Console and in-band reads have been
> limited by `CONSOLE_READ_MAX`
> and `IO_INBAND_MAX`.
That's a completely separate matter, better have it separate.
> To prevent OOL queue page leaks, appropriate
> `memcpy` + `munmap` pruning operations
> have been implemented within `read_reply`,
There is no need to mmap/memcpy, you can just munmap the spurious part
of the initial mmap.
> and a feature for handling zero-length reads immediately has been
> added.
Ok.
> Lock Clean-up and EOF Handling: Lock boundaries (queuelock,
> selq_lock)
> have been simplified,
This really deservers a separate patch, so it can be properly reviewed
and easily bisected away if needed.
> the old spin lock within `unlock_readlock` has been
> removed,
Why removing it? Once we have unlocked, we do need to service the read.
> and atomic `stdin_eof` monitoring has been integrated to respond
> immediately to EOF conditions.
Atomic variables are very difficult to maintain. Better find a simpler
solution.
> diff --git a/hurd/boot/boot.c b/hurd/boot/boot.c
> --- a/hurd/boot/boot.c
> +++ b/hurd/boot/boot.c
> @@ -907,24 +946,63 @@ main (int argc, char **argv, char **envp)
> mach_port_deallocate (mach_task_self (), pseudo_master_device_port);
>
> err = pthread_create (&pthread_id, NULL, msg_thread, NULL);
> - if (!err)
> - pthread_detach (pthread_id);
> - else
> - {
> - errno = err;
> - perror ("pthread_create");
> - }
> + if (err)
> + error (1, err, "pthread_create");
> + pthread_detach (pthread_id);
> +
> + err = pthread_create (&pthread_id, NULL, select_thread, NULL);
> + if (err)
> + error (1, err, "pthread_create");
> + pthread_detach (pthread_id);
This is a cleanup that belongs to the second patch.
> @@ -987,136 +1218,367 @@ struct qr
> enum read_type type;
> mach_port_t reply_port;
> mach_msg_type_name_t reply_type;
> - int amount;
> + vm_size_t amount;
Such type cleanup change deserves its own patch.
> @@ -1372,11 +1367,6 @@ ds_device_open (mach_port_t master_port,
>
> if (!strcmp (name, "console"))
> {
> -#if 0
> - mach_port_insert_right (mach_task_self (), pseudo_console,
> - pseudo_console, MACH_MSG_TYPE_MAKE_SEND);
> - console_send_rights++;
> -#endif
Why is such code actually not needed?
(also, it's unrelated)
> console_mscount++;
> *device = pseudo_console;
> *devicetype = MACH_MSG_TYPE_MAKE_SEND;
> @@ -1430,7 +1430,7 @@ ds_device_open_new (mach_port_t master_port,
> kern_return_t
> ds_device_close (device_t device)
> {
> - if (device != pseudo_console && device != pseudo_root)
> + if (device != pseudo_console && device != pseudo_root && device !=
> pseudo_time)
It's unrelated, that should be a separate cleanup patch.
> return D_NO_SUCH_DEVICE;
> return 0;
> }
> @@ -1447,24 +1448,25 @@ ds_device_write (device_t device,
> {
> if (device == pseudo_console)
> {
> + *bytes_written = write (1, data, datalen);
> + if (*bytes_written == -1)
> {
> + if (verbose)
> + fprintf (stderr, "console write: %s\r\n", strerror (errno));
Such change belongs to another cleanup patch.
> + return D_IO_ERROR;
> }
> -#endif
>
> @@ -1522,42 +1532,52 @@ ds_device_read (device_t device,
> mach_msg_type_number_t *datalen)
> {
> error_t err;
> +
> + /* Zero-length requests get an immediate empty answer. */
> + if (bytes_wanted == 0)
> + {
> + *data = 0;
> + *datalen = 0;
> + return D_SUCCESS;
> + }
That can also be a separate cleanup patch.
> @@ -1721,6 +1724,9 @@ ds_device_get_status (device_t device,
> case DEV_GET_SIZE:
> if (*statuslen < DEV_GET_SIZE_COUNT)
> return D_INVALID_SIZE;
> + if (root_store->size > UINT32_MAX
> + || root_store->block_size > UINT32_MAX)
dev_status_t is rather an int, better using INT_MAX.
> + return D_INVALID_SIZE;
> status[DEV_GET_SIZE_DEVICE_SIZE] = root_store->size;
> status[DEV_GET_SIZE_RECORD_SIZE] = root_store->block_size;
> *statuslen = DEV_GET_SIZE_COUNT;
> @@ -1732,6 +1735,9 @@ ds_device_get_status (device_t device,
> case DEV_GET_RECORDS:
> if (*statuslen < DEV_GET_RECORDS_COUNT)
> return D_INVALID_SIZE;
> + if (root_store->blocks > UINT32_MAX
> + || root_store->block_size > UINT32_MAX)
> + return D_INVALID_SIZE;
ditto.
And also, it's again unrelated to this patch.
> @@ -1818,8 +1817,7 @@ do_mach_notify_no_senders (mach_port_t notify,
> {
> bye:
> restore_termstate ();
> - err = write (2, "bye\n", 4);
> - assert_backtrace (err == 4);
> + write_diag ("bye\n", 4);
That belongs to the previous patch.
> @@ -1835,6 +1836,7 @@ do_mach_notify_no_senders (mach_port_t notify,
> if (foo != MACH_PORT_NULL)
> mach_port_deallocate (mach_task_self (), foo);
> }
> + return 0;
Also deserves its own patch.
> @@ -1853,10 +1849,6 @@ kern_return_t
> do_mach_notify_dead_name (mach_port_t notify,
> mach_port_t name)
> {
> -#if 0
> - if (name == child_task && notify == bootport)
> - host_exit (0);
> -#endif
> if (notify != dead_task_notification_port)
> return EOPNOTSUPP;
> task_died (name);
Why is that not needed in the end?
> @@ -1894,17 +1897,20 @@ S_io_read (mach_port_t object,
> if (object != pseudo_console)
> return EOPNOTSUPP;
>
> -#if 0
> - if (console_send_rights)
> + if (amount > CONSOLE_READ_MAX)
> + amount = CONSOLE_READ_MAX;
As mentioned, deserves its own patch.
>
> pthread_spin_lock (&readlock);
> - ioctl (0, FIONREAD, &avail);
> + if (ioctl (0, FIONREAD, &avail) < 0)
> + {
> + pthread_spin_unlock (&readlock);
> + return errno;
> + }
Belongs to the second patch.
Samuel
