Patchwork [1,of,3,STABLE] util: add utility function to skip avoiding file stat ambiguity if EPERM

login
register
mail settings
Submitter Katsunori FUJIWARA
Date Nov. 12, 2016, 9:16 p.m.
Message ID <b496a464399cb68628b0.1478985368@feefifofum>
Download mbox | patch
Permalink /patch/17527/
State Accepted
Delegated to: Yuya Nishihara
Headers show

Comments

Katsunori FUJIWARA - Nov. 12, 2016, 9:16 p.m.
# HG changeset patch
# User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
# Date 1478984783 -32400
#      Sun Nov 13 06:06:23 2016 +0900
# Branch stable
# Node ID b496a464399cb68628b09e52aa8cf379c98428e6
# Parent  4ed8bb8a153f91420777d98dea10ebbcd403a375
util: add utility function to skip avoiding file stat ambiguity if EPERM

Now, advancing stat.st_mtime by os.utime() is used to avoid file stat
ambiguity. But according to POSIX specification, utime(2) with an
explicit time information is permitted only for a process with:

  - the effective user ID equal to the user ID of the file, or
  - appropriate privileges

  http://pubs.opengroup.org/onlinepubs/9699919799/functions/utime.html

Therefore, just having group write access to a file causes EPERM at
applying os.utime() on it (e.g. working on the repository shared by
group access permission).

This patch adds class filestat utility function avoidamgig() to avoid
file stat ambiguity but skip it if EPERM.

It is reasonable to always ignore EPERM, because utime(2) causes EPERM
only in the case described above (EACCES is used only for utime(2)
with NULL).
Yuya Nishihara - Nov. 14, 2016, 1:43 p.m.
On Sun, 13 Nov 2016 06:16:08 +0900, FUJIWARA Katsunori wrote:
> # HG changeset patch
> # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> # Date 1478984783 -32400
> #      Sun Nov 13 06:06:23 2016 +0900
> # Branch stable
> # Node ID b496a464399cb68628b09e52aa8cf379c98428e6
> # Parent  4ed8bb8a153f91420777d98dea10ebbcd403a375
> util: add utility function to skip avoiding file stat ambiguity if EPERM

The series looks good to me. I found a nit, but it isn't important. I'll
push the patches tomorrow if there are no other comments.

> --- a/mercurial/util.py
> +++ b/mercurial/util.py
> @@ -1497,6 +1497,24 @@ class filestat(object):
>          except AttributeError:
>              return False
>  
> +    def avoidambig(self, path, old):
> +        """Change file stat of specified path to avoid ambiguity
> +
> +        'old' should be previous filestat of 'path'.
> +
> +        This skips avoiding ambiguity, if a process doesn't have
> +        appropriate privileges for 'path'.
> +        """
> +        advanced = (old.stat.st_mtime + 1) & 0x7fffffff
> +        try:
> +            os.utime(path, (advanced, advanced))
> +        except OSError as inst:
> +            if inst.errno == errno.EPERM:
> +                # utime() on the file created by another user causes EPERM,
> +                # if a process doesn't have appropriate privileges
> +                return
> +            raise

avoidambig() has no relation to self, which requires path and old filestat as
arguments. So I don't think it should be a method of filestat class.
Katsunori FUJIWARA - Nov. 15, 2016, 1:33 p.m.
At Mon, 14 Nov 2016 22:43:04 +0900,
Yuya Nishihara wrote:
> 
> On Sun, 13 Nov 2016 06:16:08 +0900, FUJIWARA Katsunori wrote:
> > # HG changeset patch
> > # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> > # Date 1478984783 -32400
> > #      Sun Nov 13 06:06:23 2016 +0900
> > # Branch stable
> > # Node ID b496a464399cb68628b09e52aa8cf379c98428e6
> > # Parent  4ed8bb8a153f91420777d98dea10ebbcd403a375
> > util: add utility function to skip avoiding file stat ambiguity if EPERM
> 
> The series looks good to me. I found a nit, but it isn't important. I'll
> push the patches tomorrow if there are no other comments.
> 
> > --- a/mercurial/util.py
> > +++ b/mercurial/util.py
> > @@ -1497,6 +1497,24 @@ class filestat(object):
> >          except AttributeError:
> >              return False
> >  
> > +    def avoidambig(self, path, old):
> > +        """Change file stat of specified path to avoid ambiguity
> > +
> > +        'old' should be previous filestat of 'path'.
> > +
> > +        This skips avoiding ambiguity, if a process doesn't have
> > +        appropriate privileges for 'path'.
> > +        """
> > +        advanced = (old.stat.st_mtime + 1) & 0x7fffffff
> > +        try:
> > +            os.utime(path, (advanced, advanced))
> > +        except OSError as inst:
> > +            if inst.errno == errno.EPERM:
> > +                # utime() on the file created by another user causes EPERM,
> > +                # if a process doesn't have appropriate privileges
> > +                return
> > +            raise
> 
> avoidambig() has no relation to self, which requires path and old filestat as
> arguments. So I don't think it should be a method of filestat class.

Oh, this useless "self" is a vestige of my wavering, sorry.

In fact, avoidambig() was implemented as a combination of examining
ambiguity (= using self.stat) and advancing st_mtime for convenience,
at first.

  before:
    if newstat.isambig(oldstat):
        advanced = (oldstat.stat.st_mtime + 1) & 0x7fffffff
        os.utime(dstpath, (advanced, advanced))

  after:
    newstat.avoidambig(oldstat, dstpath)

But finally, this patch separates advancing st_mtime from examining
ambiguity for review-ability of fixing urgent bug on stable
branch. With this simplified changes, reviewer can focus only on
centralizing code path around os.utime() into avoidambig().

Then, which fixing below would you like on stable ?

  1. define avoidambig() as a regular function in util.py

  2. mark avoidambig() by @static annotation

     this can centralize logic related to ExactCacheValidationPlan
     into filestat class

  3. integrate advancing st_mtime and examining ambiguity into
     avoidambig() for convenience

     this requires "self" for avoidambig()

----------------------------------------------------------------------
[FUJIWARA Katsunori]                             foozy@lares.dti.ne.jp
Yuya Nishihara - Nov. 15, 2016, 2:08 p.m.
On Tue, 15 Nov 2016 22:33:31 +0900, FUJIWARA Katsunori wrote:
> At Mon, 14 Nov 2016 22:43:04 +0900,
> Yuya Nishihara wrote:
> > 
> > On Sun, 13 Nov 2016 06:16:08 +0900, FUJIWARA Katsunori wrote:
> > > # HG changeset patch
> > > # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> > > # Date 1478984783 -32400
> > > #      Sun Nov 13 06:06:23 2016 +0900
> > > # Branch stable
> > > # Node ID b496a464399cb68628b09e52aa8cf379c98428e6
> > > # Parent  4ed8bb8a153f91420777d98dea10ebbcd403a375
> > > util: add utility function to skip avoiding file stat ambiguity if EPERM
> > 
> > The series looks good to me. I found a nit, but it isn't important. I'll
> > push the patches tomorrow if there are no other comments.
> > 
> > > --- a/mercurial/util.py
> > > +++ b/mercurial/util.py
> > > @@ -1497,6 +1497,24 @@ class filestat(object):
> > >          except AttributeError:
> > >              return False
> > >  
> > > +    def avoidambig(self, path, old):
> > > +        """Change file stat of specified path to avoid ambiguity
> > > +
> > > +        'old' should be previous filestat of 'path'.
> > > +
> > > +        This skips avoiding ambiguity, if a process doesn't have
> > > +        appropriate privileges for 'path'.
> > > +        """
> > > +        advanced = (old.stat.st_mtime + 1) & 0x7fffffff
> > > +        try:
> > > +            os.utime(path, (advanced, advanced))
> > > +        except OSError as inst:
> > > +            if inst.errno == errno.EPERM:
> > > +                # utime() on the file created by another user causes EPERM,
> > > +                # if a process doesn't have appropriate privileges
> > > +                return
> > > +            raise
> > 
> > avoidambig() has no relation to self, which requires path and old filestat as
> > arguments. So I don't think it should be a method of filestat class.
> 
> Oh, this useless "self" is a vestige of my wavering, sorry.
> 
> In fact, avoidambig() was implemented as a combination of examining
> ambiguity (= using self.stat) and advancing st_mtime for convenience,
> at first.
> 
>   before:
>     if newstat.isambig(oldstat):
>         advanced = (oldstat.stat.st_mtime + 1) & 0x7fffffff
>         os.utime(dstpath, (advanced, advanced))
> 
>   after:
>     newstat.avoidambig(oldstat, dstpath)
> 
> But finally, this patch separates advancing st_mtime from examining
> ambiguity for review-ability of fixing urgent bug on stable
> branch. With this simplified changes, reviewer can focus only on
> centralizing code path around os.utime() into avoidambig().
> 
> Then, which fixing below would you like on stable ?
> 
>   1. define avoidambig() as a regular function in util.py
> 
>   2. mark avoidambig() by @static annotation
> 
>      this can centralize logic related to ExactCacheValidationPlan
>      into filestat class
> 
>   3. integrate advancing st_mtime and examining ambiguity into
>      avoidambig() for convenience
> 
>      this requires "self" for avoidambig()

I've just pushed these patches. If we have a plan to extend avoidambig()
in default branch, I think the current state is fine. Anyway, that's a
really minor nit.

Patch

diff --git a/mercurial/util.py b/mercurial/util.py
--- a/mercurial/util.py
+++ b/mercurial/util.py
@@ -1497,6 +1497,24 @@  class filestat(object):
         except AttributeError:
             return False
 
+    def avoidambig(self, path, old):
+        """Change file stat of specified path to avoid ambiguity
+
+        'old' should be previous filestat of 'path'.
+
+        This skips avoiding ambiguity, if a process doesn't have
+        appropriate privileges for 'path'.
+        """
+        advanced = (old.stat.st_mtime + 1) & 0x7fffffff
+        try:
+            os.utime(path, (advanced, advanced))
+        except OSError as inst:
+            if inst.errno == errno.EPERM:
+                # utime() on the file created by another user causes EPERM,
+                # if a process doesn't have appropriate privileges
+                return
+            raise
+
     def __ne__(self, other):
         return not self == other