lclevy / ADFlib

A free, portable and open implementation of the Amiga filesystem
GNU General Public License v2.0
84 stars 29 forks source link

API #42

Open t-w opened 1 year ago

t-w commented 1 year ago

There is a lot of discussions already about it, I'd expect more, so maybe we can make a place for it (to list, brainstorm and set directions).

Some ideas to (eventually) consider (please update as needed):

  1. file / directory APIs (done)
    • a standard style to implement?
    • changes vs. wrappers
    • issues: #38, #40
  2. consistent naming (ie. submodule prefixes) across the library (done)
  3. isolating client API from internal one (separated headers in include/?) (mostly done, but may be improved)
    • eg. for file API, expose only a wrapper over the low level functions(?)
  4. devices (done)
    • allow more types of devices (not only one native and "dump"), also custom, user-implemented
    • add a memory/ramdisk device - useful ie. for testing (see the comment below
    • unify the device interface for all types
      • the type of the device is currently is set in adfOpenDev() and depend on the name (ie filename) of the device
      • instead: differentiate opening/creating depending on type (need rather different functions as different arguments are needed, eg. filename for dump, size for ramdisk device)
      • make the same device structure with interface also for dump devices
  5. review error codes (see the comment below) and their use (partially done, can be improved)
    • reduce amount of (or remove completely) string output to stdout/err (or leave if a "logger" defined/enabled)
    • replace output warnings with proper return codes
      • bitfield vs enumeration
      • bitfield would allow to aggregate more than one in single return code
      • that was the orig. idea (there is even a test hasRC() - but it is not used anywhere...)
    • user readable (string) error info: logger vs errno/last error info
      • errno/last error info
        • only static error string or dynamic (that would include more info like filename, block number or other data relevant for the error)
        • drawbacks: single, global (what can be problematic, even though the lib is not supporting multi-threading)
      • use logger for more detailed error info (if logger enabled/defined)?

Each of these might be a separated issue (or even issues), that could be linked here (so that it is easier to navigate).

kyz commented 1 year ago

I have a few ideas around the "native" interface. I put a simple example in the ramdisk branch, where instead of a single native driver option, it is a linked list, and adfOpenDev() looks at each driver in turn, and there are methods to add/remove a "native driver" from the list. It is then used in tests with a "ramdisk" driver, which is just a block of ram, so no on-disk I/O is needed to check the correctness of FS manipulation, and therefore tests run a little faster.

It is possible to go further with this. There is no need for a native driver slot, and special-casing which one is being used. The adf_dump.c functions can simply be another driver, the default one.

There are then two ways to go. Personally I like the first option, but the second is more explicit.

  1. Maintain a chain of potential drivers, each one with a selection function (the "IsDevNative" function, I propose it be called "WillHandle" function, it answers whether the driver will handle the path being opened or not). adfOpenDev() goes through them in turn, calls their WillHandle function, and selects the first one that says yes. There is always one device on the chain, the dump file reading/writing device. Depending on how ADFlib is compiled, there might be two by default, the linux/windows native device reader, which has first priority, and then the dump file reader/writer, which handles everything else.
  2. adfOpenDev() requires you pass the device you want, possibly just via a convenience function, e.g. adfNativeDevice() is defined in linux/adf_nativ.c or win32/adf_nativ.c and returns a pointer to a static struct AdfNativeDevice, while adfDumpDevice() is defined in adf_dump.c and returns the same, but for reading from dump files
kyz commented 1 year ago

Should there be more error codes than RC_OK and RC_ERROR?

Well, there are, they're just rarely used. Almost all errors are RC_ERROR. A handful are RC_MALLOC. Two are RC_VOLFULL, and one commented-out line attempts to apply RC_BLOCKSUM

Should we review the errors, and start returning the most appropriate one? Do we want a system like most DOSes where the return code of the function is just yes/no, but the actual error that caused a "no" is available, e.g. dev->lastError, or adfGetLastError(dev), or similar?

Should errors be a bitfield? This is how they're defined, but never used in practise. Or should they just be an enumeration?