FilePlugin argument checking - primitiveFailed vs PrimErrBadArgument

>
> 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.

> 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?

>  
> 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.

>  
> 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.

>  
> 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)?

> 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*

>
> > 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.
>

>
> 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.