FilePlugin argument checking - primitiveFailed vs PrimErrBadArgument

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

FilePlugin argument checking - primitiveFailed vs PrimErrBadArgument

Alistair Grant
 
Hi Everyone,

I'm currently extending FilePlugin to allow files to be opened using
either a file descriptor or FILE*.  If you're interested more details
at:

Original PR: https://github.com/pharo-project/pharo-vm/pull/108
Updated PR: https://github.com/pharo-project/pharo-vm/pull/142


While tidying up the argument checking I noticed that FilePlugin has as
part of #primitiveFileOpen:


    namePointer := interpreterProxy stackValue: 1.
    (interpreterProxy isBytes: namePointer)
       ifFalse: [^ interpreterProxy primitiveFail].


My understanding is that this is bad practice as #primitiveFail won't
correctly retry if the argument is a forwarder.  So it should be:


    namePointer := interpreterProxy stackValue: 1.
    (interpreterProxy isBytes: namePointer)
       ifFalse: [^ interpreterProxy primitiveFailFor: PrimErrBadArgument].


Can someone confirm that I'm not misunderstanding, and I'll go through
and check FilePlugin further.


Thanks,
Alistair
Reply | Threaded
Open this post in threaded view
|

Re: FilePlugin argument checking - primitiveFailed vs PrimErrBadArgument

Eliot Miranda-2
 
Hi Alistair,

On Thu, Mar 1, 2018 at 11:21 AM, Alistair Grant <[hidden email]> wrote:

Hi Everyone,

I'm currently extending FilePlugin to allow files to be opened using
either a file descriptor or FILE*.  If you're interested more details
at:

Original PR: https://github.com/pharo-project/pharo-vm/pull/108
Updated PR: https://github.com/pharo-project/pharo-vm/pull/142


While tidying up the argument checking I noticed that FilePlugin has as
part of #primitiveFileOpen:


    namePointer := interpreterProxy stackValue: 1.
    (interpreterProxy isBytes: namePointer)
       ifFalse: [^ interpreterProxy primitiveFail].


My understanding is that this is bad practice as #primitiveFail won't
correctly retry if the argument is a forwarder. 

No, that's not so.  Any primitive failure is fine.  Remember that when we discussed this the FileAttributesPlugin was answering error codes as the result of a primitive instead of failing.  This is the approach that won't work.  What happens is that when a primitive fails (with any code including nil, the non-code used when failing via primitiveFail) is that the VM scans the parameters to the primitive, following forwarders, and retries the primitive if it finds any.  So there's no need to change primitiveFail into primitiveFailFor: PrimErrXXX.

However, good error codes can allow much nicer primitive failure code.  Testing the error code can be faster and simpler than interrogating the arguments to the primitive.  For example, here are two versions of ByteString>>at:put: that deal with read-only objects.

at: index put: aCharacter
"Primitive. Store the Character in the field of the receiver indicated by
the index. Fail if the index is not an Integer or is out of bounds, or if
the argument is not a Character. Essential. See Object documentation
whatIsAPrimitive."

<primitive: 64 error: ec>
aCharacter isCharacter 
ifFalse:[^self errorImproperStore].
aCharacter isOctetCharacter ifFalse:[
"Convert to WideString"
self becomeForward: (WideString from: self).
^self at: index put: aCharacter.
].
index isInteger
ifTrue: [ (index between: 1 and: self size)
ifFalse: [ self errorSubscriptBounds: index ] ]
ifFalse: [self errorNonIntegerIndex].
self isReadOnlyObject 
ifTrue: [ ^ self modificationForbiddenFor: #at:put: index: index value: aCharacter ]

and now testing the error code directly:

at: index put: aCharacter
"Primitive. Store the Character in the field of the receiver indicated by the index.
Fail if the index is not an Integer or is out of bounds, or if the argument is not a
Character, or the Character's code is outside the 0-255 range, or if the receiver
is read-only. Essential. See Object documentation whatIsAPrimitive."

<primitive: 64 error: ec>
aCharacter isCharacter ifFalse:
[^self errorImproperStore].
index isInteger
ifTrue:
[ec == #'no modification' ifTrue:
[^self modificationForbiddenFor: #at:put: index: index value: aCharacter].
aCharacter isOctetCharacter ifFalse: "Convert to WideString"
[self becomeForward: (WideString from: self).
^self at: index put: aCharacter].
self errorSubscriptBounds: index]
ifFalse: [self errorNonIntegerIndex]

So it should be:


    namePointer := interpreterProxy stackValue: 1.
    (interpreterProxy isBytes: namePointer)
       ifFalse: [^ interpreterProxy primitiveFailFor: PrimErrBadArgument].


Can someone confirm that I'm not misunderstanding, and I'll go through
and check FilePlugin further.

Feel free to add better error codes, but do understand that it is orthogonal to the forwarder following issue.  The important thing is that primitives must fail, not answer error codes as results, i.e. that primitives be implemented as they always have been.

Cheers!

_,,,^..^,,,_
best, Eliot
Reply | Threaded
Open this post in threaded view
|

Re: FilePlugin argument checking - primitiveFailed vs PrimErrBadArgument

Alistair Grant
 
Hi Eliot,

On 1 March 2018 at 20:38, Eliot Miranda <[hidden email]> wrote:

>
> Hi Alistair,
>
> On Thu, Mar 1, 2018 at 11:21 AM, Alistair Grant <[hidden email]> wrote:
>>
>>
>> Hi Everyone,
>>
>> I'm currently extending FilePlugin to allow files to be opened using
>> either a file descriptor or FILE*.  If you're interested more details
>> at:
>>
>> Original PR: https://github.com/pharo-project/pharo-vm/pull/108
>> Updated PR: https://github.com/pharo-project/pharo-vm/pull/142
>>
>>
>> While tidying up the argument checking I noticed that FilePlugin has as
>> part of #primitiveFileOpen:
>>
>>
>>     namePointer := interpreterProxy stackValue: 1.
>>     (interpreterProxy isBytes: namePointer)
>>        ifFalse: [^ interpreterProxy primitiveFail].
>>
>>
>> My understanding is that this is bad practice as #primitiveFail won't
>> correctly retry if the argument is a forwarder.
>
>
> No, that's not so.  Any primitive failure is fine.  Remember that when we discussed this the FileAttributesPlugin was answering error codes as the result of a primitive instead of failing.  This is the approach that won't work.  What happens is that when a primitive fails (with any code including nil, the non-code used when failing via primitiveFail) is that the VM scans the parameters to the primitive, following forwarders, and retries the primitive if it finds any.  So there's no need to change primitiveFail into primitiveFailFor: PrimErrXXX.

Ah right, thanks for the clarification and reminder.


> However, good error codes can allow much nicer primitive failure code.  Testing the error code can be faster and simpler than interrogating the arguments to the primitive.  For example, here are two versions of ByteString>>at:put: that deal with read-only objects.
>
> at: index put: aCharacter
> "Primitive. Store the Character in the field of the receiver indicated by
> the index. Fail if the index is not an Integer or is out of bounds, or if
> the argument is not a Character. Essential. See Object documentation
> whatIsAPrimitive."
>
> <primitive: 64 error: ec>
> aCharacter isCharacter
> ifFalse:[^self errorImproperStore].
> aCharacter isOctetCharacter ifFalse:[
> "Convert to WideString"
> self becomeForward: (WideString from: self).
> ^self at: index put: aCharacter.
> ].
> index isInteger
> ifTrue: [ (index between: 1 and: self size)
> ifFalse: [ self errorSubscriptBounds: index ] ]
> ifFalse: [self errorNonIntegerIndex].
> self isReadOnlyObject
> ifTrue: [ ^ self modificationForbiddenFor: #at:put: index: index value: aCharacter ]
>
> and now testing the error code directly:
>
> at: index put: aCharacter
> "Primitive. Store the Character in the field of the receiver indicated by the index.
> Fail if the index is not an Integer or is out of bounds, or if the argument is not a
> Character, or the Character's code is outside the 0-255 range, or if the receiver
> is read-only. Essential. See Object documentation whatIsAPrimitive."
>
> <primitive: 64 error: ec>
> aCharacter isCharacter ifFalse:
> [^self errorImproperStore].
> index isInteger
> ifTrue:
> [ec == #'no modification' ifTrue:
> [^self modificationForbiddenFor: #at:put: index: index value: aCharacter].
> aCharacter isOctetCharacter ifFalse: "Convert to WideString"
> [self becomeForward: (WideString from: self).
> ^self at: index put: aCharacter].
> self errorSubscriptBounds: index]
> ifFalse: [self errorNonIntegerIndex]
>
>> So it should be:
>>
>>
>>     namePointer := interpreterProxy stackValue: 1.
>>     (interpreterProxy isBytes: namePointer)
>>        ifFalse: [^ interpreterProxy primitiveFailFor: PrimErrBadArgument].
>>
>>
>> Can someone confirm that I'm not misunderstanding, and I'll go through
>> and check FilePlugin further.
>
>
> Feel free to add better error codes, but do understand that it is orthogonal to the forwarder following issue.  The important thing is that primitives must fail, not answer error codes as results, i.e. that primitives be implemented as they always have been.

I'll add better error codes in the new primitives and leave existing
primitives unchanged.

Thanks again,
Alistair
Reply | Threaded
Open this post in threaded view
|

Re: FilePlugin argument checking - primitiveFailed vs PrimErrBadArgument

timrowledge
In reply to this post by Alistair Grant
 


> On 01-03-2018, at 11:21 AM, Alistair Grant <[hidden email]> wrote:
>
>
> Hi Everyone,
>
> I'm currently extending FilePlugin to allow files to be opened using
> either a file descriptor or FILE*.  

If you have some time, and the interest, to do stuff for FilePlugin then please, please, consider doing a real rewrite. For some time (as in "since 1997") we've needed better file connections. The original file primitives (way befor eplugins were even imagined) were written to make use of the most basic posix library. Thus we get the utter insanity of the 'set file pointer' and 'write to file pinter' separation.

It would be nice if the prims that need a file name would only accept instances of a class specifically for filenames, so we can separate out deriving the file path from plain String stuff. It would be nice to have a prim that asks the OS to canonicalise the name - ie take 'fred.jpg' and work out the full, exact, name for us. RISC OS has a nice call to do exactly this [1] and it's hard to imagine other OSs can't do the same; after all they have to do it at *some* point anyway. I guess the prim could fail if needed and defer to image code that tries to do The Right Thing.

Merging in your file attributes would be nice. Dave Lewis did some directory related plugin stuff a while back. Being able to read & set modes and permissions and metadata would help. That ancient and horrible FileCopyPlugin I wrote? Needed only because you couldn't read nor write such stuff and it was essential to copying vmmaker related scripts etc.

We ought to do so much better. There have been plenty of discussions over the years but no real change. Maybe you can effect some?


tim
[1] RISC OS has a lot of nice stuff that other OSs should damn well have copied decades ago. One nice example - back in the last millennium Eliot was able to use ffi calls to actually implement a way to read DOS formatted discs on an Acorn Archimedes, purely within image code.
--
tim Rowledge; [hidden email]; http://www.rowledge.org/tim
"Daddy, what does FORMATTING DRIVE C mean?"


Reply | Threaded
Open this post in threaded view
|

Re: FilePlugin argument checking - primitiveFailed vs PrimErrBadArgument

Alistair Grant
 
On Thu, Mar 01, 2018 at 02:20:23PM -0800, tim Rowledge wrote:
>
> > On 01-03-2018, at 11:21 AM, Alistair Grant <[hidden email]> wrote:
> >
> > Hi Everyone,
> >
> > I'm currently extending FilePlugin to allow files to be opened using
> > either a file descriptor or FILE*.  
>

> If you have some time, and the interest, to do stuff for FilePlugin
> then please, please, consider doing a real rewrite. For some time (as
> in "since 1997") we've needed better file connections. The original
> file primitives (way befor eplugins were even imagined) were written
> to make use of the most basic posix library. Thus we get the utter
> insanity of the 'set file pointer' and 'write to file pinter'
> separation.
>
> It would be nice if the prims that need a file name would only accept
> instances of a class specifically for filenames, so we can separate
> out deriving the file path from plain String stuff. It would be nice
> to have a prim that asks the OS to canonicalise the name - ie take
> 'fred.jpg' and work out the full, exact, name for us. RISC OS has a
> nice call to do exactly this [1] and it's hard to imagine other OSs
> can't do the same; after all they have to do it at *some* point
> anyway. I guess the prim could fail if needed and defer to image code
> that tries to do The Right Thing.
>
> Merging in your file attributes would be nice. Dave Lewis did some
> directory related plugin stuff a while back. Being able to read & set
> modes and permissions and metadata would help. That ancient and
> horrible FileCopyPlugin I wrote? Needed only because you couldn't read
> nor write such stuff and it was essential to copying vmmaker related
> scripts etc.
>
> We ought to do so much better. There have been plenty of discussions
> over the years but no real change. Maybe you can effect some?

I'm a little nervous about taking on a complete rewrite of FilePlugin at
the moment. :-)  I agree with your comments about being able to set file
attributes, and they are a practical small step that can be taken.  It
is also reasonable to add that functionality to FileAttributesPlugin.

Dave, I took a quick look at DirectoryPlugin and it looks like there
isn't any support for setting file attributes there.  But please correct
me if I'm wrong.

There's also ongoing discussion within Pharo about rewriting the file io.  
I haven't been part of it, so I don't know if it included the
primitives or just the class hierarchy.  But I'm happy to take a look
and see what can be done.

I want to get the FileAttributesPlugin smalltalk code integrated in to
Pharo first.  I'll look at being able to set file attributes after that.

Cheers,
Alistair


# vim: tw=72
Reply | Threaded
Open this post in threaded view
|

Re: FilePlugin argument checking - primitiveFailed vs PrimErrBadArgument

David T. Lewis
 
On Sun, Mar 04, 2018 at 11:24:41AM +0000, Alistair Grant wrote:

>  
> On Thu, Mar 01, 2018 at 02:20:23PM -0800, tim Rowledge wrote:
> >
> > > On 01-03-2018, at 11:21 AM, Alistair Grant <[hidden email]> wrote:
> > >
> > > Hi Everyone,
> > >
> > > I'm currently extending FilePlugin to allow files to be opened using
> > > either a file descriptor or FILE*.  
> >
>
> > If you have some time, and the interest, to do stuff for FilePlugin
> > then please, please, consider doing a real rewrite. For some time (as
> > in "since 1997") we've needed better file connections. The original
> > file primitives (way befor eplugins were even imagined) were written
> > to make use of the most basic posix library. Thus we get the utter
> > insanity of the 'set file pointer' and 'write to file pinter'
> > separation.
> >
> > It would be nice if the prims that need a file name would only accept
> > instances of a class specifically for filenames, so we can separate
> > out deriving the file path from plain String stuff. It would be nice
> > to have a prim that asks the OS to canonicalise the name - ie take
> > 'fred.jpg' and work out the full, exact, name for us. RISC OS has a
> > nice call to do exactly this [1] and it's hard to imagine other OSs
> > can't do the same; after all they have to do it at *some* point
> > anyway. I guess the prim could fail if needed and defer to image code
> > that tries to do The Right Thing.
> >
> > Merging in your file attributes would be nice. Dave Lewis did some
> > directory related plugin stuff a while back. Being able to read & set
> > modes and permissions and metadata would help. That ancient and
> > horrible FileCopyPlugin I wrote? Needed only because you couldn't read
> > nor write such stuff and it was essential to copying vmmaker related
> > scripts etc.
> >
> > We ought to do so much better. There have been plenty of discussions
> > over the years but no real change. Maybe you can effect some?
>
> I'm a little nervous about taking on a complete rewrite of FilePlugin at
> the moment. :-)  I agree with your comments about being able to set file
> attributes, and they are a practical small step that can be taken.  It
> is also reasonable to add that functionality to FileAttributesPlugin.
>
> Dave, I took a quick look at DirectoryPlugin and it looks like there
> isn't any support for setting file attributes there.  But please correct
> me if I'm wrong.

Correct, there is no support for setting file attributes in DirectoryPlugin.

I did make some change about a year ago that may be of interest. I decided
that DirectoryPlugin was too much of a mashup of unrelated concepts, so
I split it into two separate plugins, and added a better primitive for
reading file attributes. But there is nothing there for setting attributes.

Here are the changes that I did:

   Name: VMConstruction-Plugins-DirectoryPlugin-dtl.9
   Author: dtl
   Time: 27 April 2017, 6:53:25.9 pm
   UUID: 4e2e427e-be7a-4ed5-a7e7-626598b3ea9d
   Ancestors: VMConstruction-Plugins-DirectoryPlugin-dtl.8
   
   DirectoryPlugin was originally developed to improve directory access
   performance, and it implemented both directory stream and file stat primitives.
   Improvements elsewhere in the image or VM make the directory stream primitives
   less useful, but the file stat primitives still have value.
   
   Directory streams and file access checks are reasonably distinct concepts.
   Therefore split DirectoryPlugin in two, PosixDirectoryStreamPlugin and
   PosixFileStatPlugin. For now, retain the original DirectoryPlugin for
   reference, but it may be removed in the future.
   
   Name: VMConstruction-Plugins-DirectoryPlugin-dtl.10
   Author: dtl
   Time: 29 April 2017, 8:34:18.983 pm
   UUID: 9c1d225b-0f9e-4a29-a466-06cb88bb6dbb
   Ancestors: VMConstruction-Plugins-DirectoryPlugin-dtl.9
   
   Implement primitiveStatStruct to answer a complete array of stat structure
   values. This is different from primitiveStat, which answers an array with
   values suitable for use in creating directory entries in Smalltalk.


The new primitive is:

        <primitive: 'primitiveStatStruct' module: 'PosixFileStatPlugin'>

The primitive answers an array with:

        st_dev   /* ID of device containing file */
        st_ino   /* inode number */
        st_mode /* protection */
        st_nlink /* number of hard links */
        st_uid   /* user ID of owner */
        st_gid   /* group ID of owner */
        st_rdev /* device ID (if special file) */
        st_size /* total size, in bytes */
        st_blksize /* blocksize for filesystem I/O */
        st_blocks /* number of 512B blocks allocated */
        st_atime /* time of last access */
        st_mtime /* time of last modification */
        st_ctime /* time of last status change */ "

Please feel free to borrow or adapt anything that might be useful. The
repository is at http://www.squeaksource.com/DirectoryPlugin.

Dave

Reply | Threaded
Open this post in threaded view
|

Re: FilePlugin argument checking - primitiveFailed vs PrimErrBadArgument

Alistair Grant
 
Hi David,

On Sun, Mar 04, 2018 at 10:04:37AM -0500, David T. Lewis wrote:

>  
> On Sun, Mar 04, 2018 at 11:24:41AM +0000, Alistair Grant wrote:
> >  
> > On Thu, Mar 01, 2018 at 02:20:23PM -0800, tim Rowledge wrote:
> > >
> > > > On 01-03-2018, at 11:21 AM, Alistair Grant <[hidden email]> wrote:
> > > >
> > > > Hi Everyone,
> > > >
> > > > I'm currently extending FilePlugin to allow files to be opened using
> > > > either a file descriptor or FILE*.  
> > >
> >
> > > If you have some time, and the interest, to do stuff for FilePlugin
> > > then please, please, consider doing a real rewrite. For some time (as
> > > in "since 1997") we've needed better file connections. The original
> > > file primitives (way befor eplugins were even imagined) were written
> > > to make use of the most basic posix library. Thus we get the utter
> > > insanity of the 'set file pointer' and 'write to file pinter'
> > > separation.
> > >
> > > It would be nice if the prims that need a file name would only accept
> > > instances of a class specifically for filenames, so we can separate
> > > out deriving the file path from plain String stuff. It would be nice
> > > to have a prim that asks the OS to canonicalise the name - ie take
> > > 'fred.jpg' and work out the full, exact, name for us. RISC OS has a
> > > nice call to do exactly this [1] and it's hard to imagine other OSs
> > > can't do the same; after all they have to do it at *some* point
> > > anyway. I guess the prim could fail if needed and defer to image code
> > > that tries to do The Right Thing.
> > >
> > > Merging in your file attributes would be nice. Dave Lewis did some
> > > directory related plugin stuff a while back. Being able to read & set
> > > modes and permissions and metadata would help. That ancient and
> > > horrible FileCopyPlugin I wrote? Needed only because you couldn't read
> > > nor write such stuff and it was essential to copying vmmaker related
> > > scripts etc.
> > >
> > > We ought to do so much better. There have been plenty of discussions
> > > over the years but no real change. Maybe you can effect some?
> >
> > I'm a little nervous about taking on a complete rewrite of FilePlugin at
> > the moment. :-)  I agree with your comments about being able to set file
> > attributes, and they are a practical small step that can be taken.  It
> > is also reasonable to add that functionality to FileAttributesPlugin.
> >
> > Dave, I took a quick look at DirectoryPlugin and it looks like there
> > isn't any support for setting file attributes there.  But please correct
> > me if I'm wrong.
>
> Correct, there is no support for setting file attributes in DirectoryPlugin.
>
> I did make some change about a year ago that may be of interest. I decided
> that DirectoryPlugin was too much of a mashup of unrelated concepts, so
> I split it into two separate plugins, and added a better primitive for
> reading file attributes. But there is nothing there for setting attributes.
>
> Here are the changes that I did:
>
>    Name: VMConstruction-Plugins-DirectoryPlugin-dtl.9
>    Author: dtl
>    Time: 27 April 2017, 6:53:25.9 pm
>    UUID: 4e2e427e-be7a-4ed5-a7e7-626598b3ea9d
>    Ancestors: VMConstruction-Plugins-DirectoryPlugin-dtl.8
>    
>    DirectoryPlugin was originally developed to improve directory access
>    performance, and it implemented both directory stream and file stat primitives.
>    Improvements elsewhere in the image or VM make the directory stream primitives
>    less useful, but the file stat primitives still have value.
>    
>    Directory streams and file access checks are reasonably distinct concepts.
>    Therefore split DirectoryPlugin in two, PosixDirectoryStreamPlugin and
>    PosixFileStatPlugin. For now, retain the original DirectoryPlugin for
>    reference, but it may be removed in the future.
>    
>    Name: VMConstruction-Plugins-DirectoryPlugin-dtl.10
>    Author: dtl
>    Time: 29 April 2017, 8:34:18.983 pm
>    UUID: 9c1d225b-0f9e-4a29-a466-06cb88bb6dbb
>    Ancestors: VMConstruction-Plugins-DirectoryPlugin-dtl.9
>    
>    Implement primitiveStatStruct to answer a complete array of stat structure
>    values. This is different from primitiveStat, which answers an array with
>    values suitable for use in creating directory entries in Smalltalk.
>
>
> The new primitive is:
>
> <primitive: 'primitiveStatStruct' module: 'PosixFileStatPlugin'>
>
> The primitive answers an array with:
>
> st_dev   /* ID of device containing file */
> st_ino   /* inode number */
> st_mode /* protection */
> st_nlink /* number of hard links */
> st_uid   /* user ID of owner */
> st_gid   /* group ID of owner */
> st_rdev /* device ID (if special file) */
> st_size /* total size, in bytes */
> st_blksize /* blocksize for filesystem I/O */
> st_blocks /* number of 512B blocks allocated */
> st_atime /* time of last access */
> st_mtime /* time of last modification */
> st_ctime /* time of last status change */ "
>
> Please feel free to borrow or adapt anything that might be useful. The
> repository is at http://www.squeaksource.com/DirectoryPlugin.

Thanks.

I looked at DirectoryPlugin about a year ago, but it must have been just
before you did the refactoring.  FileAttributesPlugin provides basically
the same functionality, but also the creation (birth) time (on Windows),
efficient retrieval of a single attribute (which Pharo's FileReference
provides) and access to the access() function.

Following on from Tim's comments above, how much work do you think is
required to turn PosixDirectoryStreamPlugin and
PosixFileStatPlugin in to a FilePlugin replacement (I think merging
PosixFileStatPlugin and FileAttributesPlugin will be straightforward)?

Cheers,
Alistair

Reply | Threaded
Open this post in threaded view
|

Re: FilePlugin argument checking - primitiveFailed vs PrimErrBadArgument

David T. Lewis
 
On Sun, Mar 04, 2018 at 06:46:50PM +0000, Alistair Grant wrote:

>  
> Hi David,
>
> On Sun, Mar 04, 2018 at 10:04:37AM -0500, David T. Lewis wrote:
> >  
> > On Sun, Mar 04, 2018 at 11:24:41AM +0000, Alistair Grant wrote:
> > >  
> > > On Thu, Mar 01, 2018 at 02:20:23PM -0800, tim Rowledge wrote:
> > > >
> > > > > On 01-03-2018, at 11:21 AM, Alistair Grant <[hidden email]> wrote:
> > > > >
> > > > > Hi Everyone,
> > > > >
> > > > > I'm currently extending FilePlugin to allow files to be opened using
> > > > > either a file descriptor or FILE*.  
> > > >
> > >
> > > > If you have some time, and the interest, to do stuff for FilePlugin
> > > > then please, please, consider doing a real rewrite. For some time (as
> > > > in "since 1997") we've needed better file connections. The original
> > > > file primitives (way befor eplugins were even imagined) were written
> > > > to make use of the most basic posix library. Thus we get the utter
> > > > insanity of the 'set file pointer' and 'write to file pinter'
> > > > separation.
> > > >
> > > > It would be nice if the prims that need a file name would only accept
> > > > instances of a class specifically for filenames, so we can separate
> > > > out deriving the file path from plain String stuff. It would be nice
> > > > to have a prim that asks the OS to canonicalise the name - ie take
> > > > 'fred.jpg' and work out the full, exact, name for us. RISC OS has a
> > > > nice call to do exactly this [1] and it's hard to imagine other OSs
> > > > can't do the same; after all they have to do it at *some* point
> > > > anyway. I guess the prim could fail if needed and defer to image code
> > > > that tries to do The Right Thing.
> > > >
> > > > Merging in your file attributes would be nice. Dave Lewis did some
> > > > directory related plugin stuff a while back. Being able to read & set
> > > > modes and permissions and metadata would help. That ancient and
> > > > horrible FileCopyPlugin I wrote? Needed only because you couldn't read
> > > > nor write such stuff and it was essential to copying vmmaker related
> > > > scripts etc.
> > > >
> > > > We ought to do so much better. There have been plenty of discussions
> > > > over the years but no real change. Maybe you can effect some?
> > >
> > > I'm a little nervous about taking on a complete rewrite of FilePlugin at
> > > the moment. :-)  I agree with your comments about being able to set file
> > > attributes, and they are a practical small step that can be taken.  It
> > > is also reasonable to add that functionality to FileAttributesPlugin.
> > >
> > > Dave, I took a quick look at DirectoryPlugin and it looks like there
> > > isn't any support for setting file attributes there.  But please correct
> > > me if I'm wrong.
> >
> > Correct, there is no support for setting file attributes in DirectoryPlugin.
> >
> > I did make some change about a year ago that may be of interest. I decided
> > that DirectoryPlugin was too much of a mashup of unrelated concepts, so
> > I split it into two separate plugins, and added a better primitive for
> > reading file attributes. But there is nothing there for setting attributes.
> >
> > Here are the changes that I did:
> >
> >    Name: VMConstruction-Plugins-DirectoryPlugin-dtl.9
> >    Author: dtl
> >    Time: 27 April 2017, 6:53:25.9 pm
> >    UUID: 4e2e427e-be7a-4ed5-a7e7-626598b3ea9d
> >    Ancestors: VMConstruction-Plugins-DirectoryPlugin-dtl.8
> >    
> >    DirectoryPlugin was originally developed to improve directory access
> >    performance, and it implemented both directory stream and file stat primitives.
> >    Improvements elsewhere in the image or VM make the directory stream primitives
> >    less useful, but the file stat primitives still have value.
> >    
> >    Directory streams and file access checks are reasonably distinct concepts.
> >    Therefore split DirectoryPlugin in two, PosixDirectoryStreamPlugin and
> >    PosixFileStatPlugin. For now, retain the original DirectoryPlugin for
> >    reference, but it may be removed in the future.
> >    
> >    Name: VMConstruction-Plugins-DirectoryPlugin-dtl.10
> >    Author: dtl
> >    Time: 29 April 2017, 8:34:18.983 pm
> >    UUID: 9c1d225b-0f9e-4a29-a466-06cb88bb6dbb
> >    Ancestors: VMConstruction-Plugins-DirectoryPlugin-dtl.9
> >    
> >    Implement primitiveStatStruct to answer a complete array of stat structure
> >    values. This is different from primitiveStat, which answers an array with
> >    values suitable for use in creating directory entries in Smalltalk.
> >
> >
> > The new primitive is:
> >
> > <primitive: 'primitiveStatStruct' module: 'PosixFileStatPlugin'>
> >
> > The primitive answers an array with:
> >
> > st_dev   /* ID of device containing file */
> > st_ino   /* inode number */
> > st_mode /* protection */
> > st_nlink /* number of hard links */
> > st_uid   /* user ID of owner */
> > st_gid   /* group ID of owner */
> > st_rdev /* device ID (if special file) */
> > st_size /* total size, in bytes */
> > st_blksize /* blocksize for filesystem I/O */
> > st_blocks /* number of 512B blocks allocated */
> > st_atime /* time of last access */
> > st_mtime /* time of last modification */
> > st_ctime /* time of last status change */ "
> >
> > Please feel free to borrow or adapt anything that might be useful. The
> > repository is at http://www.squeaksource.com/DirectoryPlugin.
>
> Thanks.
>
> I looked at DirectoryPlugin about a year ago, but it must have been just
> before you did the refactoring.  FileAttributesPlugin provides basically
> the same functionality, but also the creation (birth) time (on Windows),
> efficient retrieval of a single attribute (which Pharo's FileReference
> provides) and access to the access() function.
>
> Following on from Tim's comments above, how much work do you think is
> required to turn PosixDirectoryStreamPlugin and
> PosixFileStatPlugin in to a FilePlugin replacement (I think merging
> PosixFileStatPlugin and FileAttributesPlugin will be straightforward)?


Hi Alistair,

I don't really think that these plugins would provide a suitable path toward
replacing FilePlugin. I see them more as small (and optional) add-on functions.
They are also quite Unix specific, so a general solution would require a bit
of thought regarding what abstractions to use.

Just as an example, file systems on Windows use a very different approach
for keeping track of file access times. On Windows, it is creation time,
last access time, and last write time, while on Unix (including OSX) it
is last access time, last modification time, and last status change time.
There are other fundamental differences too, so you can't really get away
with applying Posix rules to other kinds of systems.

Dave
Reply | Threaded
Open this post in threaded view
|

Re: FilePlugin argument checking - primitiveFailed vs PrimErrBadArgument

timrowledge
 


> On 04-03-2018, at 12:09 PM, David T. Lewis <[hidden email]> wrote:
>
>
> Just as an example, file systems on Windows use a very different approach
> for keeping track of file access times. On Windows, it is creation time,
> last access time, and last write time, while on Unix (including OSX) it
> is last access time, last modification time, and last status change time.

And on RISC OS it's only the last-modified time.

Platform spread is *hard*

tim
--
tim Rowledge; [hidden email]; http://www.rowledge.org/tim
Strange OpCodes: WAF: Warn After the Fact


Reply | Threaded
Open this post in threaded view
|

Re: FilePlugin argument checking - primitiveFailed vs PrimErrBadArgument

timrowledge
 


> On 04-03-2018, at 12:36 PM, tim Rowledge <[hidden email]> wrote:
>
>
>
>> On 04-03-2018, at 12:09 PM, David T. Lewis <[hidden email]> wrote:
>>
>>
>> Just as an example, file systems on Windows use a very different approach
>> for keeping track of file access times. On Windows, it is creation time,
>> last access time, and last write time, while on Unix (including OSX) it
>> is last access time, last modification time, and last status change time.
>
> And on RISC OS it's only the last-modified time.
>
> Platform spread is *hard*

Oh, and let's consider the problems that can be caused by invalid assumptions about how file systems etc work; for example the way that directory lookup for the root directory works.

For a start not all systems have a meaningful concept of 'a root'. Windows/DOS has all the separate disk letters, and something else for network connected storage. RISC OS can have many different filing systems connected each with differing rules! My 'favourite' annoyance was always that to fake similar behaviour to unix, RISC OS had to scan for all connected FS handlers (which couldn't be done very well programmatically) and finding out what directories might exist on a CD drive meant spinning up the blasted drive! Imagine the delay involved in that. Although one can complain a bit about the implementation decisions made by people, we are at fault for implementing a less than helpful file system widget that insists on querying far too many directories to no real purpose.

tim
--
tim Rowledge; [hidden email]; http://www.rowledge.org/tim
Strange OpCodes: BW: Branch on Whim


Reply | Threaded
Open this post in threaded view
|

File system interface (was: FilePlugin argument checking - primitiveFailed vs PrimErrBadArgument)

David T. Lewis
 
On Sun, Mar 04, 2018 at 12:57:39PM -0800, tim Rowledge wrote:

>
> > On 04-03-2018, at 12:36 PM, tim Rowledge <[hidden email]> wrote:
> >
> >> On 04-03-2018, at 12:09 PM, David T. Lewis <[hidden email]> wrote:
> >>
> >> Just as an example, file systems on Windows use a very different approach
> >> for keeping track of file access times. On Windows, it is creation time,
> >> last access time, and last write time, while on Unix (including OSX) it
> >> is last access time, last modification time, and last status change time.
> >
> > And on RISC OS it's only the last-modified time.
> >
> > Platform spread is *hard*
>
> Oh, and let's consider the problems that can be caused by invalid assumptions about how file systems etc work; for example the way that directory lookup for the root directory works.
>
> For a start not all systems have a meaningful concept of 'a root'. Windows/DOS has all the separate disk letters, and something else for network connected storage. RISC OS can have many different filing systems connected each with differing rules! My 'favourite' annoyance was always that to fake similar behaviour to unix, RISC OS had to scan for all connected FS handlers (which couldn't be done very well programmatically) and finding out what directories might exist on a CD drive meant spinning up the blasted drive! Imagine the delay involved in that. Although one can complain a bit about the implementation decisions made by people, we are at fault for implementing a less than helpful file system widget that insists on querying far too many directories to no real purpose.
>

I think we are drifting a bit off topic here, but this makes me wonder where
one might look to find an example of a good abstraction layer for file
systems. We cannot be the first ever to have encountered this issue, so
what might be an example of an existing system that interfaces with file
systems through a small, well-designed set of functions?

At a high level, the FileMan package (http://wiki.squeak.org/squeak/6333 and
currently used in Cuis) might provide some inspiration, although of course
under the covers it still using the existing FilePlugin. But maybe if you
think of it from the point of view of designing a plugin that would do the
best possible job of supporting FileMan (the FileIOAccessor actually) across
a range of systems, it might lead to a good result.

Dave
 
Reply | Threaded
Open this post in threaded view
|

Re: FilePlugin argument checking - primitiveFailed vs PrimErrBadArgument

Alistair Grant
In reply to this post by David T. Lewis
 
Hi David,

Apologies for my slow response.


On 4 March 2018 at 21:09, David T. Lewis <[hidden email]> wrote:

>
> On Sun, Mar 04, 2018 at 06:46:50PM +0000, Alistair Grant wrote:
>>
>> Hi David,
>>
>>
>> Following on from Tim's comments above, how much work do you think is
>> required to turn PosixDirectoryStreamPlugin and
>> PosixFileStatPlugin in to a FilePlugin replacement (I think merging
>> PosixFileStatPlugin and FileAttributesPlugin will be straightforward)?
>
>
> Hi Alistair,
>
> I don't really think that these plugins would provide a suitable path toward
> replacing FilePlugin. I see them more as small (and optional) add-on functions.
> They are also quite Unix specific, so a general solution would require a bit
> of thought regarding what abstractions to use.

OK.  I'll work at adding functionality to FileAttributesPlugin for now
and think about a FilePlugin re-write down the track after learning (a
lot) more about writing primitives.

Thanks!
Alistair




> Just as an example, file systems on Windows use a very different approach
> for keeping track of file access times. On Windows, it is creation time,
> last access time, and last write time, while on Unix (including OSX) it
> is last access time, last modification time, and last status change time.
> There are other fundamental differences too, so you can't really get away
> with applying Posix rules to other kinds of systems.
>
> Dave