[lockdev-devel] patches?

[Roger Leigh]

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.

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.
  - 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.
  - sample.c could use argv[0] rather than hardcoding executable name.

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?

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 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?
  - 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.
  - Now the lock file names have changed, how do we manage migration
    of existing services holding locks?


Regards,
Roger

-- 
  .''`.  Roger Leigh
 : :' :  Debian GNU/Linux             http://people.debian.org/~rleigh/
 `. `'   Printing on GNU/Linux?       http://gutenprint.sourceforge.net/
   `-    GPG Public Key: 0x25BFB848   Please GPG sign your mail.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.alioth.debian.org/pipermail/lockdev-devel/attachments/20100227/515aa933/attachment.pgp>