Hello,
Milos Nikic, le ven. 03 avril 2026 13:04:06 -0700, a ecrit:
> I capped this batch at 20 files to keep it easy to review.
The size is not a problem.
The mixture of different kinds of cleanups is, however, because that
makes the review way more difficult, having to keep in mind different
kinds of changes.
Please really focus each mail on one type of cleanup, so the review can
be done for only that type.
Also, some of the cleanups can be questionable, and mixing things then
blocks applying the other cleanups.
> +#include <mach.h>
> +#include <mach/mig_errors.h>
> +#include <mach_init.h>
> +#include <mach/std_types.h>
> +#include <mach/task_special_ports.h>
> +#include <mach/message.h>
> +#include <mach/mach_types.h>
> +#include <mach/kern_return.h>
> +#include <mach/mach_port.h>
> +#include <mach/port.h>
> +#include <mach/mach_traps.h>
> +#include <mach/mach_interface.h>
I didn't catch it in the previous patch, but I don't think we want this:
mach.h explicitly already includes a lot of these, it's part of the API.
We don't want to clutter C files with too much redundancy which is not
really required.
> @@ -150,7 +174,7 @@ diskfs_start_bootstrap (void)
> Hurd server bootstrap: bootfs[bootdev] exec ourfs
> */
> printf ("\nContinuing on new root filesystem %s:", diskfs_disk_name);
> - fflush (stdout);
> + (void) fflush (stdout);
Take care with these. Silencing a warning just for the sake of it is not
generally a good thing.
Here it's startup code so we can't do much about the error, but better
assert it so we get feedback if things go so wrong that a mere fflush
doesn't work.
In general otherwise, we'd rather stop and return the error.
And similarly for the signal() calls.
> @@ -288,7 +312,7 @@ diskfs_start_bootstrap (void)
> assert_backtrace (pathbuf[0] == '\0');
>
> err = ports_create_port (diskfs_control_class, diskfs_port_bucket,
> - sizeof (struct port_info), &bootinfo);
> + sizeof (struct port_info), (void*)&bootinfo);
?
In C that's completely useless.
> diff --git a/libdiskfs/dead-name.c b/libdiskfs/dead-name.c
> index 5e0cf787..2cb2624d 100644
> --- a/libdiskfs/dead-name.c
> +++ b/libdiskfs/dead-name.c
> @@ -18,34 +18,14 @@
> along with this program; if not, write to the Free Software
> Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111, USA. */
>
> -#include "priv.h"
> +#include <mach/port.h>
> +#include <mach/notify.h>
> +#include <libfshelp/fshelp.h>
> +#include <libports/ports.h>
>
> void
> ports_dead_name (void *notify, mach_port_t dead_name)
> {
> -#if 0
Being #if 0 doesn't mean that it should be removed. It still means that
somebody wrote it, so it did made sense to that author. Whether it
should actually be removed or not needs to be carefully checked.
> - /* FIXME: This makes no sense, the dead name could be np->sockaddr, but not
> - the protid; it's pointless to look it up this way. It also leaks a
> - reference. */
> - struct protid *pi = ports_lookup_port (diskfs_port_bucket, dead_name,
> - diskfs_protid_class);
> - struct node *np;
> -
> - if (pi)
> - {
> - np = pi->po->np;
> - pthread_mutex_lock (&np->lock);
> - if (dead_name == np->sockaddr)
> - {
> - mach_port_deallocate (mach_task_self (), np->sockaddr);
> - np->sockaddr = MACH_PORT_NULL;
> - diskfs_nput (np);
> - }
> - else
> - pthread_mutex_unlock (&np->lock);
> - }
> -#endif
> -
> fshelp_remove_active_translator (dead_name);
>
> ports_interrupt_notified_rpcs (notify, dead_name, MACH_NOTIFY_DEAD_NAME);
> diskfs_demuxer (mach_msg_header_t *inp,
> - mach_msg_header_t *outp)
> + mach_msg_header_t *outp)
> {
> - mig_routine_t routine;
> - if ((routine = diskfs_io_server_routine (inp)) ||
> - (routine = diskfs_fs_server_routine (inp)) ||
> - (routine = ports_notify_server_routine (inp)) ||
> - (routine = diskfs_fsys_server_routine (inp)) ||
> - (routine = ports_interrupt_server_routine (inp)) ||
> - (diskfs_shortcut_ifsock ?
> - (routine = diskfs_ifsock_server_routine (inp)) : 0) ||
> - (routine = diskfs_startup_notify_server_routine (inp)) ||
> - (routine = diskfs_exec_startup_server_routine (inp)))
> + mig_routine_t routine = diskfs_io_server_routine (inp);
> +
> + if (!routine)
> + routine = diskfs_fs_server_routine (inp);
> + if (!routine)
> + routine = ports_notify_server_routine (inp);
> + if (!routine)
> + routine = diskfs_fsys_server_routine (inp);
> + if (!routine)
> + routine = ports_interrupt_server_routine (inp);
> + if (!routine && diskfs_shortcut_ifsock)
> + routine = diskfs_ifsock_server_routine (inp);
> + if (!routine)
> + routine = diskfs_startup_notify_server_routine (inp);
> + if (!routine)
> + routine = diskfs_exec_startup_server_routine (inp);
Here we end up in a matter of taste. The existing code is really a
common idiomatic way to write it.
> diff --git a/libdiskfs/file-exec.c b/libdiskfs/file-exec.c
> index 254e52ad..f1decf60 100644
> --- a/libdiskfs/file-exec.c
> +++ b/libdiskfs/file-exec.c
> @@ -140,8 +158,8 @@ diskfs_S_file_exec_paths (struct protid *cred,
> if ((mode & S_IFMT) == S_IFDIR)
> RETURN (EACCES);
>
> - suid = mode & S_ISUID;
> - sgid = mode & S_ISGID;
> + suid = (int)(mode & S_ISUID);
> + sgid = (int)(mode & S_ISGID);
Again, this is just silencing a warning. It looks like it should
actually have been
suid = (mode & S_ISUID) != 0;
since it's used as a boolean later on. At least it makes much more sense
than just keeping the S_ISUID bit and casting to int.
> if (!_diskfs_nosuid && (suid || sgid))
> {
> int secure = 0;
Samuel
