Re: [PATCH hurd 0/6] Add file record locking support

[Samuel Thibault]

Svante Signell, le Thu 08 Jan 2015 16:46:55 +0100, a écrit :
> On Thu, 2015-01-08 at 13:38 +0100, Justus Winter wrote:
> > Hello Svante :)
> 
> > 
> > > 1/6: hurd_new_RPC.patch: add new RPC: file_record_lock
> > > 2/6: libdiskfs_file_record_lock.patch: implement
> > > diskfs_S_file_record_lock and modify diskfs_S_* accordingly, initialize
> > > and release lock_status.
> > > 3/6: libfshelp_rlock.patch: implement fshelp_rlock_* functions
> > > 4/6: libfshelp-tests_rlock.patch: implement file_record_lock tests
> > > 5/6: libnetfs_file_record_lock.patch: implement netfs_S_file_record_lock
> > > 6/6: libtrivfs_file_record_lock.patch: implement
> > > trivfs_S_file_record_lock
> > 
> > For my taste, the order of patches is wrong.  Please try to make sure,
> > that the source tree compiles at every point.  If you add an RPC with
> > the first patch, the build will break.  Instead, you have to add the
> > server implementation first, and the RPC definition later.
> 
> Well, nothing hinders the committer to apply them in any order.

Yes, but please help him by already providing the right order.

This is a sad thing, but submitters really need to make efforts to save
the commiter most of its time.

> Additionally, the original patches were one big blob, I can easily
> merge everything like that again.

Which wouldn't be a good thing, as I said it's better to split the
changes where it can be, to make bisecting easier.

> To be a little more constructive: Is this patch order correct?
> 1) libfshelp_rlock.patch: implement fshelp_rlock_* functions
> 2) libdiskfs_file_record_lock.patch: implement diskfs_S_file_record_lock
> and modify diskfs_S_* accordingly, initialize and release lock_status.
> 3) libnetfs_file_record_lock.patch: implement netfs_S_file_record_lock
> 4) libtrivfs_file_record_lock.patch: implement trivfs_S_file_record_lock
> 5) hurd_new_RPC.patch: add new RPC: file_record_lock
> 6) libfshelp-tests_rlock.patch: implement file_record_lock tests

Possibly. making sure by at least running make at each step would
confirm this.

Samuel