#385 Move disk image handling layer to libspectrum

[Gergely Szasz]

This patch move the disk code from fuse to libspectrum.

We need some testing around "trdos_insert_boot_loader()"

[Sergio Baldoví]

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.

[Gergely Szasz]

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]

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í]

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

[Sergio Baldoví]

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?

[Gergely Szasz]

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í]

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.

[Gergely Szasz]

Here is a "new" patch.

bitmap.h: .. there is no any doubt

[Sergio Baldoví]

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]

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"

[Sergio Baldoví]

Thanks, committed in [11efa0].

BTW, are LIBSPECTRUM_DISK_FLAG_OPEN_DS and LIBSPECTRUM_DISK_OFLAG_CPC_MASK unused now?

[Sergio Baldoví]

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.

[Sergio Baldoví]

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?

[Gergely Szasz]

Thanks.

[Sergio Baldoví]

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/

[Gergely Szasz]

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í]

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

[Sergio Baldoví]

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.

[Gergely Szasz]

I think, after you reviewed 'new' .dsk patches ([bugs:#451], [bugs:#452], [bugs:#453], [bugs:#454], [bugs:#455]), i merge the changes from master to this branch, and than all of above segfaults should disappear...