Re: libpciaccess, rumpkernel, hurd: [PATCH] - working rumpdisk support for rootfs

[Samuel Thibault]

Hello,

Please attach patch as inline text, possibly in several mails, but at
the very least as inline text, to make it more efficient to review them.

Damien Zammit wrote:
> I have tried to keep the patches separate for now, but easily can be squashed.

Keeping them separate is really what we want. It's way better to have
separate commits, both for review and later digging. It also allows to
easily apply what can already be applied.

> The server using libpciaccess currently needs to be faked with 
> netfs_server_name "pci-arbiter".

That should be fine for now.

> There is no communication between startup/shutdown <-> rumpdisk yet, so hangs 
> when you try to reboot. (How do I fix this?)

What/how does it hang exactly?

To get a shutdown notification, you'll need to call
startup_request_notification like other translators. That can only be
done after the whole system is up and running, normally it's done in
fsys_init, which I guess ext2fs would have to forward to its bootstrap
parent.

Note: answers to my questions below should probably be either code
change, or a comment in the code, for the future reader :)

hurd/0001-libmachdev-Add-resume-for-bootstrap-server.patch
> @@ -94,12 +95,9 @@ ds_device_open (mach_port_t open_port, mach_port_t 
> reply_port,
>                  char *name, device_t *devp, mach_msg_type_name_t *devicePoly)
>  {
>    int i;
> -  io_return_t err;
> -
> -  /* Open must be called on the master device port.  */
> -  if (!machdev_is_master_device (open_port))
> -    return D_INVALID_OPERATION;

? Don't we want to keep the check? AIUI the caller should still be
calling the master port?

> +resume_bootstrap_server(mach_port_t server_task, const char *server_name)
> +{
[...]
> +  //XXX no console yet?
> +  //printf (" %s", server_name);
> +  //fflush (stdout);

Does it actually crash if you leave this? See diskfs_console_stdio()
that opens a proper stdout to be going through mach's console.

hurd/0003-libmachdev-Implement-S_i386_io_perm_create.patch
> +kern_return_t
> +S_i386_io_perm_create (mach_port_t master_port,
> +                              io_port_t from,
> +                              io_port_t to,
> +                              io_perm_t *io_perm)
> +{
> +  return i386_io_perm_create (_hurd_device_master, from, to, io_perm);
> +}

I'd say we want to check that master_port really is the device master
port with machdev_is_master_device()?

hurd/0004-libmachdev-Add-new-RPC-server-stubs-for-ds_device_in.patch
Applied, thanks!

hurd/0005-libmachdev-Remove-deviceUser-this-lib-is-a-deviceSer.patch
> Subject: [PATCH 5/6] libmachdev: Remove deviceUser, this lib is a deviceServer
> -SRCS = deviceUser.c machUser.c ds_routines.c trivfs_server.c \
> +SRCS = machUser.c ds_routines.c trivfs_server.c \
>         device_replyUser.c deviceServer.c notifyServer.c mach_i386Server.c

Indeed, but don't we want to remove device_replyUser.c machUser.c as
well? libmachuser.so should be providing us everything already.

Also, please make it earlier in the patch series so we can commit it
right away without having to wait for the rest of the series.

hurd/0006-rumpdisk-Use-bootstrap-resume-of-fs-task-in-machdev.patch
I applied the cleaning parts of it

> -  io_return_t err = D_SUCCESS;
> +  io_return_t err = D_NO_SUCH_DEVICE;

I'm surprised: you mean if one reopens the device, one would get
D_NO_SUCH_DEVICE?  Wouldn't D_ALREADY_OPEN be more appropriate?

>        err = rump_sys_open (dev_name, dev_mode_to_rump_mode (bd->mode));
>        if (err < 0)
>         {
> -         err = rump_errno2host (errno);
> +         err = D_NO_SUCH_DEVICE;

Mmm, why overriding the error?

libpciaccess/0001-hurd_pci-Use-__pci_conf_-variants-of-pci_conf_.patch
libpciaccess/0002-hurd_pci.c-Add-missing-pci-probe-for-regions.patch
libpciaccess/0003-x86-Use-gnumach-device-instead-of-dev-mem-on-GNU-sys.patch
libpciaccess/0004-x86-Remove-mapping-of-regions-during-probe-otherwise.patch
You can already submit these upstream.

rumpkernel/0001-WIP-Remove-hardfail-on-missing-dev-urandom-for-PRNG-.patch
rumpkernel/0002-buildrump-Add-Wno-sign-compare-for-now-until-device-.patch
I added them to the debian package

What signedness warning shows up exactly?

rumpkernel/0003-pci-userspace-Use-new-mach-IRQ-device-RPC-interfaces.patch
> +-RUMPCOMP_USER_SRCS=   pci_user-gnu.c experimentalUser.c mach_debugUser.c
> ++RUMPCOMP_USER_SRCS=   pci_user-gnu.c gnumachUser.c deviceUser.c 
> mach_debugUser.c

You shouldn't be needing gnumachUser.c deviceUser.c here either,
libmachuser should already be providing what you need, at worse by just
recompiling glibc.  I have now started such a rebuild.

Samuel