Re: [PATCH 1/6] libfshelp_rlock

[Samuel Thibault]

Hello,

Svante Signell, on Thu 15 Dec 2016 12:31:55 +0100, wrote:
> On Thu, 2015-03-05 at 02:34 +0100, Samuel Thibault wrote:
> > Olaf was wondering about flock being reimplemented over this new
> > implementation.  It's actually a tricky question.  I believe there are
> > two issues:
> > 
> > 
> > All in all, I'd suggest the following:
> ...
> > - let's make GETLK however return ENOSYS: we can't fill the l_pid, and
> >   this is not worse than what we have ATM.
> 
> Is it OK to add to fcntl.diff?
> -       return __flock (fd, cmd);
> +       err = HURD_FD_PORT_USE (d,
> __file_record_lock (port, cmd, fl));
> +       if (fl->l_pid == -1)
> +       {
> +      
>   errno = ENOSYS;
> +         err = -1;
> +       } 
> +       result = err ?
> __hurd_dfail (fd, err) : 0;
> (and in one other place)

No, I'm unsure what we want: do we trust the FS server as to provide
the proper l_pid, just like we trust it to return proper file owners?
I guess we might not have the choice actually, but I'm perhaps wrong.
People, any thoughts on this?  Since the locker process is not involved
at all when another process calls GETLK, I don't think there is a way
to make the FS server have something to *prove* to the process calling
GETLK that a given pid is the locker of a given range?  A way I could
see would be to implement a trusted lock-auth server that FS servers
talk with, the FS server would get a record-lock-identity token from it,
and use an rendez-vous handshake to prove to the process calling GETLK
that it has that token.

Anyway, really that's for later, the implementation for now won't allow
the server to return anything meaningful in l_pid, so this is code
that will actually never get run for now, so rather than try to write
something which might actually reveal to be wrong, let's for now just
return a plain ENOSYS for GETLK.

> > - let's already put the process token in the new RPC API.  Setting it to
> >   non-NULL from the client will mean "I want a per-process lock", and
> >   for now let's return ENOSYS in that case.
> 
> Which RPC and which token name are you thinking of here?

I mean the __file_record_lock RPC of course: "I want a per-process
lock", what else could it be?  For now in your patch it only provides
the section of the file to be locked.  When we'll implement a
per-process lock, we'll need to say something more than just the
section, so rather than then defining yet another RPC, let's just pave
the way already by adding a for-now-unused port argument which will
later be used as a token.

> > - later, we'll implement proper per-process locks, by using the token as an
> >   identifier of the process.
> 
> Which token?

The token passed by the locker process when calling __file_record_lock
for per-process lock.  That's really like the ref port of
io_reauthenticate: when the locker process calls __file_record_lock
for a per-process lock, it also provides to the RPC a send right
for a new port.  It send passes a send right for the same port to
proc_user_identify.  The FS server calls proc_server_identify with the
received send right, and the proc server returns to the FS server the
pid of the process.  The FS server can now trust that pid information
and store it internally.

> > - we'll then be able to implement GETLK, and in case a per-open lock was
> >   taken, we'll put -1 in l_pid, like BSD does.
> 
> Please explain!

Please be polite.  Asking it this way just makes me want to stop my mail
here...

Once __file_record_lock has the token parameter (which for now will
just be NUL), the FS server can know whether the lock was requested
per-process, or per-open, and when available record the pid along the
lock. When GETLK is called, it becomes trivial for the FS server to
either return l_pid = -1 for the per-open case, or return the l_pid for
the per-process case.

But anyway, as I said that's for later. Just make it a mach_port_send_t
for now, just like in io_reauthenticate, and we'll use it later.

> > - when per-opened file record locks get standardized, we can trivially
> >   implement it.
> 
> See above.

See above: it'll just be a matter of passing a NUL token.

Svante Signell, on Mon 19 Dec 2016 18:25:58 +0100, wrote:
> It seems like the open mode for /dev/null does not contain O_READ/O_WRITE 
> flags

? It does, see cred->po->openmodes.

> making flock tests as well as fcntl tests fail, when proxied through
> libtrivfs/file-lock.c (trivfs_S_file_lock).

Well, do we really want to support locks on /dev/null?  Neal's
implementation proxies the support to the realnode when there is one
(null doesn't have one), I don't think we want to achieve more than
this.

> Any advice given here is appreciated. Maybe the special devices, should be
> opened with modes similar to regular files?

We don't care about null.  We may want to care about e.g. locking
/dev/com0, but I don't think applications actually hope that this is
portable at all, so I don't think we want to care, and just get the
implementation in for regular files at last.

Samuel