Hi Alistair,

On Mon, Jan 1, 2018 at 12:40 AM, Alistair Grant <akgrant0710@gmail.com> wrote:

Hi Eliot,

I've posted a new version of the FileAttributesPlugin
(FileAttributesPlugin.oscog-AlistairGrant.19):

http://smalltalkhub.com/#!/~Alistair/FileAttributesPlugin/versions/FileAttributesPlugin.oscog-AlistairGrant.19

Looking good!  I have some minor quibbles, but so far see nothing major wrong.

So...

- the most important is not to query the SecurityPlugin more than once.  So follow the pattern in FilePlugin where the inst vars such as sCOFfn are set up in initialiseModule.  You're querying the SecurityPlugin on every access as in

canStatFilePath: aPathCString length: length
...
(hasSecurityPlugin = 0) ifTrue: [^ true].
sCOFfn := interpreterProxy ioLoadFunction: 'secCanOpenFileOfSizeWritable' From: 'SecurityPlugin'.
sCOFfn ~= 0
...

These are minor:

- accessAttributesForFilename:into:startingAt: doesn't need the remapOop:in:.  No instantiations occur so the GC will never kick in.
- primitives such as primitiveClosedir that use pointerFor: don't need to check the argument via is: dirPointerOop KindOf: 'ByteArray'; pointerFor: does all the checking one needs.
- in primitives such as primitiveFileExists I would rewrite

accessFlag = 0
ifTrue: [interpreterProxy pop: 2 thenPush: interpreterProxy trueObject]
ifFalse: [interpreterProxy pop: 2 thenPush: interpreterProxy falseObject]

  as

interpreterProxy pop: 2 thenPush: (accessFlag = 0
ifTrue: [iinterpreterProxy trueObject]
ifFalse: [interpreterProxy falseObject])

- given that you're accessing W_OK et al via Symbols (see #W_OK et al in accessAttributesForFilename:into:startingAt:) do you need fileWriteableFlag et al?  Or should you use them instead of the Symbols?  The latter is easier when we have to simulate the plugin when debugging the VM at some future date.

- there are still some users of cPreprocessorDirective:.  It is much more preferable to use cppIf:ifTrue:[ifFalse:]


Changes:

- Convert error values from coded values to #primitiveFailForOSError:
- #primitiveClosedir & #primitiveRewinddir return PrimErrBadArgument
  instead of a coded error for a bad argument


The updated Pharo code to use the modified plugin is in github:

https://github.com/akgrant43/FileAttributes

Would you please review the plugin code again (taking in to account the
feedback I sent in "Re: 20294 Add FileAttributesPlugin to the linux 32
bit VM", sent 6 Sep 2017).


Thanks!
Alistair



On 23 December 2017 at 19:00, Eliot Miranda <eliot.miranda@gmail.com> wrote:
>
> Hi Alistair,
>
> On Sat, Dec 23, 2017 at 9:35 AM, Alistair Grant <akgrant0710@gmail.com> wrote:
>>
>>
>> Hi Eliot,
>>
>> I've made progress:
>>
>> - Updated FileAttributesPlugin to use #primitiveFailForOSError:.
>> - Loaded PrimitiveOSError
>> - Updated SmalltalkImage>>newSpecialObjectsArray
>> - Run SmalltalkImage current recreateSpecialObjectsArray.
>> - And triggered an error which generated the appropriate
>> PrimitiveOSError object.
>>
>> Given I've already made changes, and have a bit more tidying up to do,
>> it is probably isn't worthwhile you reviewing the code again until I
>> post an updated version.  I'll let you know when it is ready.
>
>
> OK, that's great.  Thanks and have a great holiday!
>
>>
>>
>> Merry Christmas!
>> Alistair



--
_,,,^..^,,,_
best, Eliot