#378 libspectrum unit tests broken by fix for bug #372

[Philip Kendall]

The fix for [bugs:#372] has broken a couple of the libspectrum unit tests which used a TZX file with no data:

$ test/test
[...]
libspectrum error: tzx_read_turbo_block: corrupt file, bad value in used bits in last byte
Test 2: TZX turbo data with zero pilot pulses and zero data... NOT COMPLETE
[...]
/home/philip/spectrum/fuse/libspectrum/test/.libs/test: reading `./test/empty-drb.tzx' did not give expected result
Test 8: Empty TZX DRB... NOT COMPLETE

I couldn't instantly tell from the commit log exactly what behaviour we were trying to test for here.

[Sergio Baldoví]

Test 2: [bugs:#84] and [bugs:#85]
Test 8: [bugs:#106]

Was the former code trying to silently skip bytes with bits_in_last_byte=0?
[183190] takes the opposite way and rejects the tape. If we revert the commit we should also fix the pure_data endless loop from [bugs:#372].

[Fredrick Meunier]

I'm inclined to think that any block with a "used bits in last byte" field (where a zero value doesn't make sense) but no data content (which makes any value 1-8 not make sense) should be rejected. If there is a data portion but no requirement to have data implied by the other fields in the block maybe that should allow the data to be optional? I'd be happy for us to keep the new patch and update the unit tests.

Are there any known published TZXs with these problems?

[Sergio Baldoví]

Are there any known published TZXs with these problems?

A way to find out it is running a test on a large collection of TZXs and check for this specific error. It should be easy to modify test.c to play an external tape.

[Fredrick Meunier]

I checked 12555 TZX files from the TZX Vault, WoS etc and found 0 files encoded with a value of 0 in bits used in last byte for turbo or pure data blocks.

[Fredrick Meunier]

I started writing some code to double down on rejecting as corrupt these problem files and amending the tests, but on reflecting on the (sadly) slim chance of the OTLA tool being fixed and the number of TZXs being churned out with it as well as the interoperability principle of RFC1122 section 1.2.2, I think I was wrong in arguing that we should reject these files outright on the balance of things.

The attached patch tries to rewrite a bad bits in last byte value, truncating a block with a bits in last byte value of 0 by a byte and rewriting a bits in last byte value over 8 to 8. This gets the Vallation TZX to load and lets the error cases in the test suite pass.

Any comments or thoughts?

[Philip Kendall]

Sergio: never intended to imply that we should revert the change. The new way of the world is much better than the endless loop. Just that we should ensure our unit tests pass (by either modifying them or removing them as appropriate) when we make changes ;-)

Fred: I'm inclined to agree with your revised approach here. OTLA isn't going to be fixed, and it's more useful for our users to load the file than to throw them out.

[Sergio Baldoví]

Sergio: never intended to imply that we should revert the change. The new way of the world is much better than the endless loop. Just that we should ensure our unit tests pass (by either modifying them or removing them as appropriate) when we make changes ;-)

No problem. I'm happy changing the position if that's an improvement :-)

[Fredrick Meunier]

Thanks all, committed in [77718f].