Re: Comment on glibc2.28: Re: [PATCH glibc] Add file record locking support

[Svante Signell]

On Mon, 2018-12-03 at 16:25 +0100, Samuel Thibault wrote:
> Svante Signell, le lun. 03 déc. 2018 16:01:37 +0100, a ecrit:
> > On Sat, 2018-12-01 at 19:30 +0100, Samuel Thibault wrote:
> > > I forgot here:
> > 
> > Is this really needed?
> 
> Please quote a bit more than just that, otherwise we don't have any
> actual context.
> 
> But actually, are you really talking about the mail you are quoting?
> (which is about the GETLK part only, not 64bit vs 32bit questions.)
> 
> What is "this" in your sentence, exactly? The lines below? Why quoting
> an unrelated sentence then?

I commented out the following three debian patches and updated my patch to
fcntl.c.
#hurd-i386/git-fcntl64.diff
#hurd-i386/git-lockf-0.diff
#hurd-i386/tg-WRLCK-upgrade.diff
hurd-i386/fcntl.diff

And in the first patch we have the stuff below (that I did not have in the patch
for fcntl.c). Added now.

> > From git-fcntl64.diff:
> > Index: glibc-2.28/sysdeps/mach/hurd/fcntl.c
> > ===================================================================
> > --- glibc-2.28.orig/sysdeps/mach/hurd/fcntl.c
> > +++ glibc-2.28/sysdeps/mach/hurd/fcntl.c
> > ...
> > @@ -215,3 +210,4 @@ strong_alias (__libc_fcntl, __libc_fcntl
> >  libc_hidden_def (__libc_fcntl64)
> >  weak_alias (__libc_fcntl64, __fcntl64)
> >  libc_hidden_weak (__fcntl64)
> > +weak_alias (__fcntl64, fcntl64)
> > 
> > Index: glibc-2.28/sysdeps/mach/hurd/fcntl64.c
> > ===================================================================
> > --- /dev/null
> > +++ glibc-2.28/sysdeps/mach/hurd/fcntl64.c
> > @@ -0,0 +1 @@
> > +/* fcntl64 is defined in fcntl.c as an alias.  */
> 
> Well, yes? Like on Linux, fcntl and fcntl64 are just made the same, and
> it's the cmd parameter which decides between 32bit vs 64bit.
> 
> Or are you saying that we do not need the fcntl64 alias?  Well, we do,
> since it's declared and used by upstream commit
> 06ab719d30b01da401150068054d3b8ea93dd12f
> 
> > Additionally, sysdeps/mach/hurd/flock.c calls __file_lock() and the file-
> > record-locking patches need to patch lib{diskfs,netfs,trivfs} file-lock.c
> > files too.
...
> It's not so simple: see my mail from 5th march 2015
> 
> http://lists.gnu.org/archive/html/bug-hurd/2015-03/msg00032.html
> 
> flock is supposed to be per opened file, thus inherited through forks,
> while fcntl/lockf to be per process, thus not inherited through forks.
> 
> In my mail I mentioned that we'd use the rendezvous port to distinguish
> between both, but for now we'll defer that part and just pass NULL.
> 
> In other words, the steps would be:
> 
> - just keep flock as it is for now, make use fcntl use the RPC with
>   rendezvous==NULL, and still make it per-process. tdb will be happy.

Unfortunately tdb will not be happy, it does use fork a lot and fork should not
inherit locks, see
http://lists.gnu.org/archive/html/bug-hurd/2015-01/msg00092.html
http://lists.gnu.org/archive/html/bug-hurd/2015-01/msg00095.html

Never mind for now.

> - add rendezvous support in the RPC. Then we can make the
>   rendezvous!=NULL mean per-process, and make the rendezvous==NULL mean
>   per-open.
> - Then we can make flock.c use it instead of file_lock. Notice however
>   that we can't just call fcntl like sysdeps/posix/flock.c does, since
>   flock wants per-open lock, not per-process.
> - then we can scratch file-lock.c (after we are fairly convinced that
>   people have migrated to a libc that supports using the new locking
>   RPC).
>
> > What about libtreefs/s-file.c:treefs_S_file_lock() and it's hooks?
> 
> I don't see any users of this hook, and google doesn't either, so I'd
> say we will be fine with scratching it at the same time as file-lock.C
> 
> > What it the main usage application of libtreefs?
> 
> I don't know.

OK, good roadmap idea!

Thanks!