#492 libmingwex.a: dirent.[ch] implementation issues

closed-accepted
2012-03-10
2011-08-21
Keith Marshall
No

To facilitate implementation of a globbing capability, within an embedded script interpreter for mingw-get, I initially set out to add support for the BSD d_type field in our existing dirent implementation.

The infrastructure needed to support this additional field is already present within the implementation -- the _findfirst() and _findnext() calls used to read directory content also return the required attribute data -- making addition of the feature fairly trivial. However, on inspection of the actual code, I observe the following deficiencies:--

1) The DIR and _WDIR data types, declared in dirent.h *should* be opaque data types, yet this implementation exposes all of the gory internal detail of the implementation, (which is also rather ugly, due to grossly inefficient use of memory).

2) Although ostensibly a POSIX compliant API, this implementation fails POSIX compliance in various aspects, most notably in incorrect use of errno.

3) The efficiency of the implementation is significantly degraded, as a consequence of performing certain of the underlying system calls within the wrong dirent API functions.

To remedy this, I've prepared the attached four stage cumulative patch set, delivering the following features:--

Stage 1: makes DIR and _WDIR data types opaque, as they should be.

Stage 2: rationalises memory usage within DIR and _WDIR, adding the d_type field to their associated struct dirent and struct _wdirent aggregates, with identification for BSD's DT_REG, DT_DIR and DT_UNKNOWN attributes.

Stage 3: corrects the misuse of errno.

Stage 4: refactors the code, to perform underlying system calls within more appropriate API function scope.

I propose to apply these patches in sequence, at intervals over the coming weeks. Okay to commit stage 1 after (say) seven days, to allow for preliminary review?

Discussion

  • Keith Marshall
    Keith Marshall
    2011-08-21

    Stage 1 Patch

     
  • Keith Marshall
    Keith Marshall
    2011-08-21

    Stage 2 Patch

     
  • Keith Marshall
    Keith Marshall
    2011-08-21

    Stage 3 Patch

     
  • Keith Marshall
    Keith Marshall
    2011-08-21

    Stage 4 Patch

     
  • Sounds like a good approach. I've reviewed patch 1 and I am fine with it.

     
  • Keith Marshall
    Keith Marshall
    2011-08-27

    Okay. On the basis of Chris' explicit approval, and accepting silence as tacit approval from others, I've committed the stage 1 patch. I'll leave it a while, before moving on to stage 2.

     
  • Keith Marshall
    Keith Marshall
    2011-10-01

    Stage 1 has now been in play for more than a month, with no negative reports. Therefore, I've gone ahead with stage 2, which I'll leave for another two or three weeks before proceeding to stage 3.

     
  • Keith Marshall
    Keith Marshall
    2012-03-10

    • milestone: 684956 --> IINR_-_Include_In_Next_Release
    • status: open --> closed-accepted
     
  • Keith Marshall
    Keith Marshall
    2012-03-10

    I've now committed all four patch sets -- the last one five weeks ago -- with no adverse (or indeed any) feedback. This completes this task, hence I'm closing this ticket.