Patchwork [1,of,2] vfs: add the possibility to have a "ward" to check vfs usage

login
register
mail settings
Submitter Pierre-Yves David
Date Aug. 11, 2016, 10:52 a.m.
Message ID <b84b218971cfb131023b.1470912744@nodosa.octopoid.net>
Download mbox | patch
Permalink /patch/16248/
State Changes Requested
Delegated to: Yuya Nishihara
Headers show

Comments

Pierre-Yves David - Aug. 11, 2016, 10:52 a.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
# Date 1470323266 -7200
#      Thu Aug 04 17:07:46 2016 +0200
# Node ID b84b218971cfb131023b7e9550cb112e6195841b
# Parent  46d9a5bb2fb0a3629328995948f9ddb0ef0ca4a5
# EXP-Topic vfs.ward
vfs: add the possibility to have a "ward" to check vfs usage

The function will be called anytime we open a file. The first usage of this
'ward' will be to check that lock are properly taken before accessing file.
Later we might use it to ensure we use the right vfs to access files, allowing
more vfs to be introduced.

We currently only apply the ward on 'open' operation. We will extend this to
other operations like copy, creation and removal later. The current readonlyvfs
seems to have the same shortcoming.
timeless - Aug. 12, 2016, 12:56 a.m.
Pierre-Yves David wrote:
> +        # XXX should be call for other things than 'open'

called
Yuya Nishihara - Aug. 14, 2016, 3:03 a.m.
On Thu, 11 Aug 2016 12:52:24 +0200, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
> # Date 1470323266 -7200
> #      Thu Aug 04 17:07:46 2016 +0200
> # Node ID b84b218971cfb131023b7e9550cb112e6195841b
> # Parent  46d9a5bb2fb0a3629328995948f9ddb0ef0ca4a5
> # EXP-Topic vfs.ward
> vfs: add the possibility to have a "ward" to check vfs usage
> 
> The function will be called anytime we open a file. The first usage of this
> 'ward' will be to check that lock are properly taken before accessing file.
> Later we might use it to ensure we use the right vfs to access files, allowing
> more vfs to be introduced.
> 
> We currently only apply the ward on 'open' operation. We will extend this to
> other operations like copy, creation and removal later. The current readonlyvfs
> seems to have the same shortcoming.
> 
> diff -r 46d9a5bb2fb0 -r b84b218971cf mercurial/scmutil.py
> --- a/mercurial/scmutil.py	Mon Aug 08 18:05:10 2016 +0200
> +++ b/mercurial/scmutil.py	Thu Aug 04 17:07:46 2016 +0200
> @@ -482,7 +482,8 @@ class vfs(abstractvfs):
>      This class is used to hide the details of COW semantics and
>      remote file access from higher level code.
>      '''
> -    def __init__(self, base, audit=True, expandpath=False, realpath=False):
> +    def __init__(self, base, audit=True, expandpath=False, realpath=False,
> +                 ward=None):
>          if expandpath:
>              base = util.expandpath(base)
>          if realpath:
> @@ -491,6 +492,11 @@ class vfs(abstractvfs):
>          self.mustaudit = audit
>          self.createmode = None
>          self._trustnlink = None
> +        # optional function to validate operation on file
> +        # intended to be user for developer checks.
> +        #
> +        # XXX should be call for other things than 'open'
> +        self._ward = ward
>  
>      @property
>      def mustaudit(self):
> @@ -583,6 +589,8 @@ class vfs(abstractvfs):
>                          self._trustnlink = nlink > 1 or util.checknlink(f)
>                      if nlink > 1 or not self._trustnlink:
>                          util.rename(util.mktempcopy(f), f)
> +        if self._ward is not None:
> +            self._ward(f, mode)
>          fp = util.posixfile(f, mode)

Do you have an idea to extend this to the other operations and open of
atomictemp files? I doubt it wouldn't be as simple as a callback function.
Pierre-Yves David - Aug. 14, 2016, 11:47 a.m.
On 08/14/2016 05:03 AM, Yuya Nishihara wrote:
> On Thu, 11 Aug 2016 12:52:24 +0200, Pierre-Yves David wrote:
>> # HG changeset patch
>> # User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
>> # Date 1470323266 -7200
>> #      Thu Aug 04 17:07:46 2016 +0200
>> # Node ID b84b218971cfb131023b7e9550cb112e6195841b
>> # Parent  46d9a5bb2fb0a3629328995948f9ddb0ef0ca4a5
>> # EXP-Topic vfs.ward
>> vfs: add the possibility to have a "ward" to check vfs usage
>>
>> The function will be called anytime we open a file. The first usage of this
>> 'ward' will be to check that lock are properly taken before accessing file.
>> Later we might use it to ensure we use the right vfs to access files, allowing
>> more vfs to be introduced.
>>
>> We currently only apply the ward on 'open' operation. We will extend this to
>> other operations like copy, creation and removal later. The current readonlyvfs
>> seems to have the same shortcoming.
>>
>> diff -r 46d9a5bb2fb0 -r b84b218971cf mercurial/scmutil.py
>> --- a/mercurial/scmutil.py	Mon Aug 08 18:05:10 2016 +0200
>> +++ b/mercurial/scmutil.py	Thu Aug 04 17:07:46 2016 +0200
>> @@ -482,7 +482,8 @@ class vfs(abstractvfs):
>>      This class is used to hide the details of COW semantics and
>>      remote file access from higher level code.
>>      '''
>> -    def __init__(self, base, audit=True, expandpath=False, realpath=False):
>> +    def __init__(self, base, audit=True, expandpath=False, realpath=False,
>> +                 ward=None):
>>          if expandpath:
>>              base = util.expandpath(base)
>>          if realpath:
>> @@ -491,6 +492,11 @@ class vfs(abstractvfs):
>>          self.mustaudit = audit
>>          self.createmode = None
>>          self._trustnlink = None
>> +        # optional function to validate operation on file
>> +        # intended to be user for developer checks.
>> +        #
>> +        # XXX should be call for other things than 'open'
>> +        self._ward = ward
>>
>>      @property
>>      def mustaudit(self):
>> @@ -583,6 +589,8 @@ class vfs(abstractvfs):
>>                          self._trustnlink = nlink > 1 or util.checknlink(f)
>>                      if nlink > 1 or not self._trustnlink:
>>                          util.rename(util.mktempcopy(f), f)
>> +        if self._ward is not None:
>> +            self._ward(f, mode)
>>          fp = util.posixfile(f, mode)
>
> Do you have an idea to extend this to the other operations and open of
> atomictemp files? I doubt it wouldn't be as simple as a callback function.

Simple callback should do for the other operation, as they are also 
performed through vfs.

For atomictmp, we probably want to channel their usage through as vfs to 
be able to cover them.

Cheers,
Yuya Nishihara - Aug. 14, 2016, 12:30 p.m.
On Sun, 14 Aug 2016 13:47:38 +0200, Pierre-Yves David wrote:
> On 08/14/2016 05:03 AM, Yuya Nishihara wrote:
> > On Thu, 11 Aug 2016 12:52:24 +0200, Pierre-Yves David wrote:
> >> @@ -583,6 +589,8 @@ class vfs(abstractvfs):
> >>                          self._trustnlink = nlink > 1 or util.checknlink(f)
> >>                      if nlink > 1 or not self._trustnlink:
> >>                          util.rename(util.mktempcopy(f), f)
> >> +        if self._ward is not None:
> >> +            self._ward(f, mode)
> >>          fp = util.posixfile(f, mode)
> >
> > Do you have an idea to extend this to the other operations and open of
> > atomictemp files? I doubt it wouldn't be as simple as a callback function.
> 
> Simple callback should do for the other operation, as they are also 
> performed through vfs.

Something like ward('open', f, mode), ward('unlink', f), etc., or wardopen(),
wardunlink(), etc. ?

> For atomictmp, we probably want to channel their usage through as vfs to
> be able to cover them.

I mean atomictemp=True is handled before calling ward(). And I think atomic
operation should be allowed without taking a lock, but ward() should be called
no matter if operation is atomic or not.
Pierre-Yves David - Aug. 14, 2016, 2:48 p.m.
On 08/14/2016 02:30 PM, Yuya Nishihara wrote:
> On Sun, 14 Aug 2016 13:47:38 +0200, Pierre-Yves David wrote:
>> On 08/14/2016 05:03 AM, Yuya Nishihara wrote:
>>> On Thu, 11 Aug 2016 12:52:24 +0200, Pierre-Yves David wrote:
>>>> @@ -583,6 +589,8 @@ class vfs(abstractvfs):
>>>>                          self._trustnlink = nlink > 1 or util.checknlink(f)
>>>>                      if nlink > 1 or not self._trustnlink:
>>>>                          util.rename(util.mktempcopy(f), f)
>>>> +        if self._ward is not None:
>>>> +            self._ward(f, mode)
>>>>          fp = util.posixfile(f, mode)
>>>
>>> Do you have an idea to extend this to the other operations and open of
>>> atomictemp files? I doubt it wouldn't be as simple as a callback function.
>>
>> Simple callback should do for the other operation, as they are also
>> performed through vfs.
>
> Something like ward('open', f, mode), ward('unlink', f), etc., or wardopen(),
> wardunlink(), etc. ?

yep

>
>> For atomictmp, we probably want to channel their usage through as vfs to
>> be able to cover them.
>
> I mean atomictemp=True is handled before calling ward(). And I think atomic
> operation should be allowed without taking a lock, but ward() should be called
> no matter if operation is atomic or not.

Ha, good catch. yeah we should convey the value of atomictemp to the ward.
Pierre-Yves David - Aug. 14, 2016, 5:03 p.m.
On 08/14/2016 04:48 PM, Pierre-Yves David wrote:
> […]
>>> For atomictmp, we probably want to channel their usage through as vfs to
>>> be able to cover them.
>>
>> I mean atomictemp=True is handled before calling ward(). And I think
>> atomic
>> operation should be allowed without taking a lock, but ward() should
>> be called
>> no matter if operation is atomic or not.
>
> Ha, good catch. yeah we should convey the value of atomictemp to the ward.

So fixing the atomictemp case raise a whole new herd of warning. I'll 
have to have a closer look at what they are but we could come with many 
exceptions or rethink the approach a bit.

Cheers.

Patch

diff -r 46d9a5bb2fb0 -r b84b218971cf mercurial/scmutil.py
--- a/mercurial/scmutil.py	Mon Aug 08 18:05:10 2016 +0200
+++ b/mercurial/scmutil.py	Thu Aug 04 17:07:46 2016 +0200
@@ -482,7 +482,8 @@  class vfs(abstractvfs):
     This class is used to hide the details of COW semantics and
     remote file access from higher level code.
     '''
-    def __init__(self, base, audit=True, expandpath=False, realpath=False):
+    def __init__(self, base, audit=True, expandpath=False, realpath=False,
+                 ward=None):
         if expandpath:
             base = util.expandpath(base)
         if realpath:
@@ -491,6 +492,11 @@  class vfs(abstractvfs):
         self.mustaudit = audit
         self.createmode = None
         self._trustnlink = None
+        # optional function to validate operation on file
+        # intended to be user for developer checks.
+        #
+        # XXX should be call for other things than 'open'
+        self._ward = ward
 
     @property
     def mustaudit(self):
@@ -583,6 +589,8 @@  class vfs(abstractvfs):
                         self._trustnlink = nlink > 1 or util.checknlink(f)
                     if nlink > 1 or not self._trustnlink:
                         util.rename(util.mktempcopy(f), f)
+        if self._ward is not None:
+            self._ward(f, mode)
         fp = util.posixfile(f, mode)
         if nlink == 0:
             self._fixfilemode(f)