Menu

#385 Move disk image handling layer to libspectrum

future
open
nobody
5
2019-05-09
2017-02-03
No

This patch move the disk code from fuse to libspectrum.

We need some testing around "trdos_insert_boot_loader()"

2 Attachments

Discussion

  • Sergio Baldoví

    Sergio Baldoví - 2017-02-05

    Thanks, Gergely. Much appreciated.

    Patches added to branches/patches-385-libspectrum-disk-handling in [9e0eb7] and [61b729] for further work.

    I've cleaned the mixed spaces and tabs used for identation at the begining of lines in disk.c, as there is no previous history in libspectrum repository and improves readability. I think this is the proper time to do it.

    The TR-DOS boot loader is failing. I'll try to debug it next week.

     

    Related

    Commit: [61b729]
    Commit: [9e0eb7]

    • Gergely Szasz

      Gergely Szasz - 2017-02-06

      Thanks!

      I busy to polish the (e)cpc/(e)dsk loading code in order to Fuse can load all WOS dsk image. There are some trouble around empty disks (e.g.: championship_run_master_disk_backup_-side_b.dsk), and some other 'copy protected' ones.
      The problematic images are:
      championship_run_master_disk_backup
      -side_b.dsk
      championship_run_tape_master_disk
      -side_b.dsk
      discology
      +3.dsk
      edd_the_duck_master_disk_-side_b.dsk
      edd_the_duck_tape_master_disk
      -side_b.dsk
      f1_tornado_tape_master_disk
      -side_b.dsk
      full_throttle_2_tape_master_disk
      -side_b.dsk
      locoscript_start_of_day_25-10-88
      -side_b.dsk
      neighbours_master_disk
      -side_b.dsk
      round_the_bend_master_disk
      -side_b.dsk
      santa_s_christmas_caper_tape_master_disk
      -side_b.dsk
      sharkey_s_moll_tape_master_disk
      -side_b.dsk
      sp5.dsk
      stack_up_tape_master_disk
      -side_b.dsk
      tai-chi_tortoise_tape_master_disk
      -side_b.dsk
      toobin.dsk
      trigger_lightgun
      .dsk
      world_cricket_tape_master_disk_-_side_b.dsk
      zx_cpm_v2.2.dsk

      All ...side_b.dsks are empty disks, so only 5 have other problem.
      ...

       
  • Gergely Szasz

    Gergely Szasz - 2017-02-12

    Here is two patch for patches-385-libspectrum-disk-handling branch (fuse and libspectrum)
    1. disk_close() -> libspectrum_disk_close() and disk_preformat() -> libspectrum_dosk_preformat()
    2. open_cpc() changes:
    - now tracks len calculated from "file header - tlen" in case of "standard" .dsk and from track len table in case of "extended" .dsk
    - new flag (LIBSPECTRUM_DISK_FLAG_NOADD_MISSING) control the "unrecorded" empty tracks (default: to add empty tracks)
    - less strict behaviour on checking track numbers - only a warning issued if expected != recorded

    So, only one dsk left: toobin.dsk...

     
    • Sergio Baldoví

      Sergio Baldoví - 2017-02-13

      Thanks. Renamed functions committed in [47fedf] and [087578]. I'll give a look at the .dsk patch soon.

       

      Related

      Commit: [087578]
      Commit: [47fedf]


      Last edit: Sergio Baldoví 2017-02-19
  • Sergio Baldoví

    Sergio Baldoví - 2017-02-19

    The attached patch fixes TRDOS autoboot. It is abusing of current libspectrum_disk_set_track() implementation, though. I should find a proper way to set a track index.

    Any opinions about the proposed public API? Future modifications and deletions will have a big impact.

    Some thoughts that come to my mind:

    • Verify whether all applicable functions use "libspectrum_disk_error_t" type instead of generic "int" , e.g., libspectrum_disk_strerror() or libspectrum_disk_seek().
    • Harden input validation, e.g., libspectrum_disk_set_track()
    • Maybe we should use alloc() / free() naming like tapes, snapshots, microdrives, rzx, etc.
    • Are static inline functions (bitmap) in public headers a good practice?
     

    Last edit: Sergio Baldoví 2017-02-19
  • Gergely Szasz

    Gergely Szasz - 2017-02-21

    Here is an updated open_cpc/()dsk patch. Now we can open all dsk from WOS (I hope..)

    Hmm.. i can't see the point: libspectrum_disk_set_track( d, t, 0 ); / workaround for "alt" schema /
    ??? internal disk format uses 'ALT' schema...always. We calculate track idx with cyl * sides + head .. if we swap cyl and head in the formula... :-?

    public API:
    - it would be nice if we can build a disk util (in fuse-utils) around the API, wich can be create any type of disk image from scratch.
    - can analyze any disk image (tracks, sectors, size, etc)
    - conversion between formats (this is o.k. now ;-)
    - add/extract sectors, sector data, tracks, track data
    - detect and works with file systems (format, del, add files, defragment etc)

    • "Verify whether ..." - agree...
    • "Harden input validation..." - uhum
    • "Maybe we should use alloc() / free() naming..." - uhum
    • "Are static inline functions (bitmap)..." - hmm.. it was just a copy-and-paste from bitmap.h
     
    • Sergio Baldoví

      Sergio Baldoví - 2017-02-27

      Here is an updated open_cpc/()dsk patch. Now we can open all dsk from WOS (I hope..)

      The patch doesn't apply cleanly, e.g., it is trying to modify "CPC_ISSUE_3a" that hasn't been previously submitted. Could you please send a patch to the current HEAD? I'm not sure about the final state.

      Hmm.. i can't see the point: libspectrum_disk_set_track( d, t, 0 ); / workaround for "alt" schema /
      ??? internal disk format uses 'ALT' schema...always. We calculate track idx with cyl * sides + head .. if we swap cyl and head in the formula... :-?

      It was a quick and dirty fix. I've committed the patch [51aa4e] with a proper track-to-cylinder conversion. Thanks.

      "Are static inline functions (bitmap)..." - hmm.. it was just a copy-and-paste from bitmap.h

      My concern is that bitmap.h is an internal header and libspectrum.h is a public header that could be used in projects with different languages. Instead of defining a black-box implementation accesible via entry points in a shared library, a static inline funcion should be compiled in a external program/compiler, with potential problems like inline keywords in C90.

       

      Related

      Commit: [51aa4e]

      • Gergely Szasz

        Gergely Szasz - 2017-03-01

        Here is a "new" patch.

        bitmap.h: .. there is no any doubt

         
        • Sergio Baldoví

          Sergio Baldoví - 2017-03-01

          Thanks. I've run some tests and got some segmentation faults, e.g., "F1 Tornado Tape Master Disk - Side A.dsk":

          #0  __memcpy_avx_unaligned () at ../sysdeps/x86_64/multiarch/memcpy-avx-unaligned.S:271
          #1  0x00007ffff7ba0bb2 in memcpy (len=18446744073709551360, src=<optimised out>, __dest=<optimised out>) at /usr/include/x86_64-linux-gnu/bits/string3.h:53
          #2  buffread (data=<optimised out>, len=len@entry=18446744073709551360, buffer=buffer@entry=0x7fffffffd6f0) at disk.c:218
          #3  0x00007ffff7ba23d5 in data_add (d=d@entry=0xed2148 <specplus3_drives+40>, buffer=buffer@entry=0x7fffffffd6f0, data=data@entry=0x0, len=len@entry=0, ddam=<optimised out>, 
              gaptype=gaptype@entry=5, crc_error=0, autofill=0, start_data=0x7fffffffd4ec) at disk.c:907
          #4  0x00007ffff7ba2cfe in open_cpc (buffer=buffer@entry=0x7fffffffd6f0, d=d@entry=0xed2148 <specplus3_drives+40>) at disk.c:1971
          #5  0x00007ffff7ba6cad in libspectrum_disk_open (d=0xed2148 <specplus3_drives+40>, buffer=<optimised out>, length=<optimised out>) at disk.c:2393
          #6  0x00000000004541e2 in disk_open2 ()
          #7  0x000000000045464f in disk_open ()
          #8  0x000000000043b9c6 in ui_media_drive_insert ()
          #9  0x000000000044933f in specplus3_disk_insert ()
          #10 0x000000000043bcb4 in utils_open_file ()
          #11 0x0000000000417f4a in do_start_files ()
          #12 0x000000000041735d in fuse_init ()
          #13 0x0000000000416e0f in main ()
          

          "Edd The Duck Master Disk - Side A.dsk" runs good if I don't merge side B, otherwise there is a segmentation fault.

           
          • Gergely Szasz

            Gergely Szasz - 2017-03-04

            Here is a fix:
            we have to check b->idx > b->len, because idx and len are unsigned... and have to check "end-of-image"

             

            Last edit: Gergely Szasz 2017-03-04
            • Sergio Baldoví

              Sergio Baldoví - 2017-03-05

              Thanks, committed in [11efa0].

              BTW, are LIBSPECTRUM_DISK_FLAG_OPEN_DS and LIBSPECTRUM_DISK_OFLAG_CPC_MASK unused now?

               

              Related

              Commit: [11efa0]

  • Sergio Baldoví

    Sergio Baldoví - 2017-03-19

    Changed some integers to libspectrum_disk_error_t in [76e373] and [babc79].

    Attached proposal for hardened input validation. Public API should validate function parameters to avoid crashes.

     

    Related

    Commit: [76e373]
    Commit: [babc79]

  • Sergio Baldoví

    Sergio Baldoví - 2017-04-17

    Input validation committed in [d3e0fe] and a firt attempt to document the API in libspectrum.txt in [cdc6c1]. Any modifications are welcome.

    Next, I plan to rename libspectrum_disk_t to libspectrum_disk, as libspectrum naming convention (libspectrum_tape, libspectrum_snap, libspectrum_rzx, etc). And rename 'fmf' variables to 'mfm' as it seems like a typo. Any objections?

     

    Related

    Commit: [cdc6c1]
    Commit: [d3e0fe]

    • Gergely Szasz

      Gergely Szasz - 2017-04-18

      Thanks.

       
  • Sergio Baldoví

    Sergio Baldoví - 2017-05-14

    Renamed libspectrum_disk_t in [935cff] and fmf variables [3b89db], plus a few other changes.

    There are 3 points I would like to discuss/amend before merging:

    1. It's almost mandatory to set the disk type and filename before calling libspectrum_disk_open(), so I think these values should be set as function parameters. Patch attached.

    2. I wonder if "_t" suffix for type definitions should be avoided. Stuart pointed that this namespace is reserved for POSIX ([1]). Probabably I should rename these types before going public: libspectrum_disk_flag_t, libspectrum_disk_error_t, libspectrum_disk_type_t, libspectrum_disk_desc_t, libspectrum_disk_dens_t, libspectrum_disk_prop_t and libspectrum_disk_open_flag_t.

    3. I'm still not convinced about defining static inline functions in libspectrum.h (i.e. libspectrum_{bitmapset,reset,test}). They are not C89 and not distributed like a library entry point. I'm thinking on turning them private for libspectrum (disk) and fuse (fdd).

    [1] https://sourceforge.net/p/fuse-emulator/mailman/message/34099145/

     

    Related

    Commit: [3b89db]
    Commit: [935cff]


    Last edit: Sergio Baldoví 2017-05-14
    • Gergely Szasz

      Gergely Szasz - 2019-04-06

      Hi Sergio,

      Back after a "big sleep". What about this branch... is anything missing yet?

      IMHO all your point is o.k. for me (of course :) for 3. ... or just a macro (?)...

       
      • Sergio Baldoví

        Sergio Baldoví - 2019-04-07

        Hi Gergerly! I'm glad to see you here. I'll retake this patch as soon as possible.

         
  • Sergio Baldoví

    Sergio Baldoví - 2019-04-21

    The branch has been synched with master and libspectrum_disk_open() parameters have been refactored. I'll give a whirl to the type definition names latter. Using macro definitions to manipulate bits looks good to me.

    There are disks that produce a segmentation fault, e.g.,

    Avon (1989)(Topologika)[+3 disk].dsk
    Judge Dredd (1990)(Virgin Games)[samdisk].dsk
    Skate Crazy (1988)(Gremlin Graphics Software)[samdisk].dsk
    Switchblade (1991)(Gremlin Graphics Software)[samdisk].dsk
    

    There are also disks that fail to open on this branch but works on master, e.g.,

    Into the Eagle's Nest (1987)(Pandora).dsk
    Star Raiders II (1987)(Electric Dreams Software).dsk
    Tetris (1988)(Mirrorsoft).dsk
    Tintin on the Moon (1989)(Infogrames).dsk
    

    The console shows an "Unsupported file feature" message.

     

Log in to post a comment.

Want the latest updates on software, tech news, and AI?
Get latest updates about software, tech news, and AI from SourceForge directly in your inbox once a month.