[lockdev-devel] patches?

[Ludwig Nussel]

Roger Leigh wrote:
> On Thu, Feb 25, 2010 at 01:05:09PM +0100, Ludwig Nussel wrote:
> > Roger Leigh wrote:
> > > On Tue, Feb 16, 2010 at 11:55:59AM +0100, Ludwig Nussel wrote:
> > > > Any objections to using automake to get rid of that custom Makefile?
> > > 
> > > Not at all.  That's one thing I was thinking of doing when I
> > > got a chance!
> > 
> > Done :-) I've also pushed my changes to
> > http://www.suse.de/~lnussel/lockdev.git . Pulling from there is
> > probably easier for you than applying lots of patches.
> 
> Thanks.  I've reviewed and merged all of the changes.  I have a few
> comments about a few of the patches, detailed below.

Note that I didn't write the RedHat patches, I've just forwarded them.

> RedHat patch 1:
>   - LOCKDEV_PATH hardcoded.  Should use configured $sbindir.
>   - int used where pid_t is most appropriate, removing need to cast
>     wait(2) return value.

yes.

>   - use of access(2) breaks setuid programs.  access(2) checks are
>     racy, so may be inappropriate.  But most importantly, the check
>     must use the effective UID rather than real UID.
>     See http://www.nikhef.nl/~dennisvd/schroot.html for one example
>     of the breakage this causes.

There's already a call to switch real and effective gid due to
https://bugzilla.redhat.com/show_bug.cgi?id=52029 . I guess uids
could be switched too. Alternatively we could use open(). access()
being racy doesn't really matter here AFAICT. We're not doing
anything with the actual device. Also, open() doesn't make it
better. lockdev is always racy anyways as one could ask it to lock a
device and then switch consoles or log out (ie udev removes acls).

>   - sample.c could use argv[0] rather than hardcoding executable name.

As long as it's just used for printing the help, yes.

> RedHat patch 4:
>   - Why are we stripping whitespace and using DEV_PATH when getting
>     the device name, rather than just stripping off the entire path
>     (which the code was doing previously)?  What situations warrant
>     doing this?  What if the path is not DEV_PATH?

The original code doesn't work for subdirectories in /dev which are
rather common nowadays. The bug at RedHat for that is
https://bugzilla.redhat.com/show_bug.cgi?id=74454 . Stripping
leading whitespace indeed looks useless. Not DEV_PATH is undefined I
guess. Current code fails with anything not in DEV_PATH.

> Lock format patch:
>   - Is using the SYSVr4 format strictly necessary?  Is the SVR4
>     convention documented anywhere, i.e. is it a standard or
>     merely convention?  Are there any other programs which strictly
>     depend on the naming convention, rather than using the lockdev
>     functions?

The convention is documented by uucp and kermit:
http://www.columbia.edu/kermit/ckuins.html#x10
http://www.airs.com/ian/uucp-doc/uucp_7.html#SEC86
Code for that method is also in minicom and pppd.

>   - The use of st_dev major and st_rdev major and minor (rather than
>     just st_rdev major and minor) appears suspect.  Why should it
>     matter what the major of the filesystem *containing* the device
>     node is?  There are situations where device nodes might exist on
>     multiple filesystems, and this would break locking by allowing
>     multiple locks on the same actual device node.  Surely st_rdev
>     major and minor are the only sane components to the lock filename?
>     i.e. what is the purpose of using st_dev in /any/ situation?

IMO it doesn't make any sense :-) It's just the way history defines
it. The only sane way for locking would be to have mandatory kernel
locks. That would also work for chroot'ed or otherwise confined
processes, different name spaces etc.
So I could imagine a Linux specific solution that implements the
ttylock interface via fcntl. Then a signal could be emitted that
causes udev to run a helper which in turn installs the compatibility
locks in /var/lock.

>   - We do need to cater for block device locking, which the new code
>     does do.  Could we use 'b' rather than 'x' for the prefix though,
>     and fall back on 'x' if it's neither block nor character as a
>     safety measure?  This just makes the naming more descriptive.

Sure.

>   - Now the lock file names have changed, how do we manage migration
>     of existing services holding locks?

IMHO we may just ignore that. The more common name based method
stays the same anyways.

cu
Ludwig

-- 
 (o_   Ludwig Nussel
 //\   
 V_/_  http://www.suse.de/
SUSE LINUX Products GmbH, GF: Markus Rex, HRB 16746 (AG Nuernberg)