Thanks for the review, Samuel. Understood on keeping things separate. I'll make sure future patches focus on only one type of cleanup at a time.
Regarding the headers: I come from a background where "Include What You Use" (IWYU) is strictly enforced for stability, so my instinct is to strip away "kitchen sink" headers and make every dependency explicit. However, it sounds like this codebase prefers using umbrella headers and relying on transitive resolution (e.g., relying on llibdiskfs/priv.h -> hurd/iohelp.h -> hurd/hurd_types.h -> mach/kern_return.h to provide kern_return_t). If that's the preferred style, is there actually any value in me doing header cleanups as long as the file compiles? I can focus on other things if this is how it should be. Let me know what makes the most sense. Thanks, Milos On Mon, Apr 6, 2026 at 9:45 AM Samuel Thibault <[email protected]> wrote: > 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 >
