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.
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.
...
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
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
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().
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
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.
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?
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:
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.
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.
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).
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.,
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.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
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]
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.
...
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...
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
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:
Last edit: Sergio Baldoví 2017-02-19
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)
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.
It was a quick and dirty fix. I've committed the patch [51aa4e] with a proper track-to-cylinder conversion. Thanks.
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]
Here is a "new" patch.
bitmap.h: .. there is no any doubt
Thanks. I've run some tests and got some segmentation faults, e.g., "F1 Tornado Tape Master Disk - Side A.dsk":
"Edd The Duck Master Disk - Side A.dsk" runs good if I don't merge side B, otherwise there is a segmentation fault.
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
Thanks, committed in [11efa0].
BTW, are LIBSPECTRUM_DISK_FLAG_OPEN_DS and LIBSPECTRUM_DISK_OFLAG_CPC_MASK unused now?
Related
Commit: [11efa0]
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]
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]
Thanks.
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:
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.
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.
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
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 (?)...
Hi Gergerly! I'm glad to see you here. I'll retake this patch as soon as possible.
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.,
There are also disks that fail to open on this branch but works on master, e.g.,
The console shows an "Unsupported file feature" message.
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...
Related
Bugs: #451
Bugs: #452
Bugs: #453
Bugs: #454
Bugs: #455