Fix dos33.c getopt for BSAVE arguments #22

jquast · 2023-07-24T03:12:15Z

Fix dos33.c getopt for BSAVE arguments on non-linux systems, #18

tested on OSX and Debian arm64
bugfix display of length when -d debug is used
update doc usage arguments, <> pairs should be []

jquast · 2023-07-24T05:43:07Z

Tested on Linux and MacOSX arm64, with and without optional BSAVE -l and -a, arguments, with and without local and apple filename and other arguments like CATALOG.

Also verifies this fixes Makefile targets that use dos33, for example project in demos/fire, on OSX currently fails:

$ make
ca65 -o fire.o fire.s -l fire.lst
ld65 -o FIRE fire.o -C ../../linker_scripts/apple2_1000.inc
ca65 -o fire_tiny.o fire_tiny.s -l fire_tiny.lst
ld65 -o FIRE_TINY fire_tiny.o -C ../../linker_scripts/apple2_70.inc
ca65 -o fire_firmware.o fire_firmware.s -l fire_firmware.lst
ld65 -o FIRE_FIRMWARE fire_firmware.o -C ../../linker_scripts/apple2_70.inc
ca65 -o fire_extreme.o fire_extreme.s -l fire_extreme.lst
ld65 -o FIRE_EXTREME fire_extreme.o -C ../../linker_scripts/apple2_70.inc
ca65 -o fire_qkumba.o fire_qkumba.s -l fire_qkumba.lst
ld65 -o FIRE_QKUMBA fire_qkumba.o -C ../../linker_scripts/apple2_70.inc
../../utils/asoft_basic-utils/tokenize_asoft < hello.bas > HELLO
ca65 -o cool_effect.o cool_effect.s -l cool_effect.lst
ld65 -o COOL_EFFECT cool_effect.o -C ../../linker_scripts/apple2_70.inc
ca65 -o raster.o raster.s -l raster.lst
ld65 -o RASTER raster.o -C ../../linker_scripts/apple2_70.inc
ld65 -o FIRE_ELSEWHERE fire_extreme.o -C ../../linker_scripts/apple2_c0.inc
../../utils/dos33fs-utils/dos33 -y fire.dsk SAVE A HELLO
Warning!  HELLO exists!
Deleting previous version...
../../utils/dos33fs-utils/dos33 -y fire.dsk BSAVE -a 0x1000 FIRE
Error!  First char of filename must be ASCII 64 or above!
make: *** [fire.dsk] Error 255

With this change, it now succeeds:

$ make
ca65 -o fire.o fire.s -l fire.lst
ld65 -o FIRE fire.o -C ../../linker_scripts/apple2_1000.inc
ca65 -o fire_tiny.o fire_tiny.s -l fire_tiny.lst
ld65 -o FIRE_TINY fire_tiny.o -C ../../linker_scripts/apple2_70.inc
ca65 -o fire_firmware.o fire_firmware.s -l fire_firmware.lst
ld65 -o FIRE_FIRMWARE fire_firmware.o -C ../../linker_scripts/apple2_70.inc
ca65 -o fire_extreme.o fire_extreme.s -l fire_extreme.lst
ld65 -o FIRE_EXTREME fire_extreme.o -C ../../linker_scripts/apple2_70.inc
ca65 -o fire_qkumba.o fire_qkumba.s -l fire_qkumba.lst
ld65 -o FIRE_QKUMBA fire_qkumba.o -C ../../linker_scripts/apple2_70.inc
../../utils/asoft_basic-utils/tokenize_asoft < hello.bas > HELLO
ca65 -o cool_effect.o cool_effect.s -l cool_effect.lst
ld65 -o COOL_EFFECT cool_effect.o -C ../../linker_scripts/apple2_70.inc
ca65 -o raster.o raster.s -l raster.lst
ld65 -o RASTER raster.o -C ../../linker_scripts/apple2_70.inc
ld65 -o FIRE_ELSEWHERE fire_extreme.o -C ../../linker_scripts/apple2_c0.inc
../../utils/dos33fs-utils/dos33 -y fire.dsk SAVE A HELLO
Warning!  HELLO exists!
Deleting previous version...
../../utils/dos33fs-utils/dos33 -y fire.dsk BSAVE -a 0x1000 FIRE
Warning!  FIRE exists!
Deleting previous version...
../../utils/dos33fs-utils/dos33 -y fire.dsk BSAVE -a 0x70 FIRE_TINY
Warning!  FIRE_TINY exists!
Deleting previous version...
../../utils/dos33fs-utils/dos33 -y fire.dsk BSAVE -a 0x70 FIRE_FIRMWARE
Warning!  FIRE_FIRMWARE exists!
Deleting previous version...
../../utils/dos33fs-utils/dos33 -y fire.dsk BSAVE -a 0x70 FIRE_EXTREME
Warning!  FIRE_EXTREME exists!
Deleting previous version...
../../utils/dos33fs-utils/dos33 -y fire.dsk BSAVE -a 0x70 FIRE_QKUMBA
Warning!  FIRE_QKUMBA exists!
Deleting previous version...
../../utils/dos33fs-utils/dos33 -y fire.dsk BSAVE -a 0x70 RASTER
Warning!  RASTER exists!
Deleting previous version...
../../utils/dos33fs-utils/dos33 -y fire.dsk BSAVE -a 0x70 COOL_EFFECT
Warning!  COOL_EFFECT exists!
Deleting previous version...
../../utils/dos33fs-utils/dos33 -y fire.dsk BSAVE -a 0xC0 FIRE_ELSEWHERE
Warning!  FIRE_ELSEWHERE exists!
Deleting previous version...

micahcowan · 2023-07-24T19:03:32Z

Hi @jquast, and thanks for giving attention to my issue!
However, I feel like your current solution is perhaps a bit fragile - it's going to stop working in the future if BSAVE ever gets a non-argument option, because then all of a sudden getopt's "option-cuddling" will not be supported.

As mentioned in #18, there's no reason to avoid using getopt to process those args, and continuing to use getopt would keep the code much simpler. You can simply use getopt to process the general args until it returns -1, then if !strcmp(*argv, "BSAVE"), you can ++argv and go right back to using getopt to process the options to BSAVE. That strikes me as a more robust (future-proof) solution, and should also involve a lot less logic duplication (eliminating the need for lines 1245-1284 in the current version of your branch).

Cheers!

micahcowan · 2023-07-24T19:06:55Z

utils/dos33fs-utils/dos33.c

+			retval = -ERROR_INVALID_PARAMATER;
+			goto exit_and_close;
+		}
+


See my comment on #22 for why this section is unnecessary.

deater#18 - tested on OSX and Debian arm64 - bugfix display of length when `-d` debug is used - small fixes to usage terms

jquast · 2023-08-16T20:53:40Z

@micahcowan Sorry that it took so long to reply, it is because I could not implement the code like you request.

BSD and Linux differ very much in regards to getopt(3), and it is suggested to always use getopt_long(3) where compatibility matters, but that would require changing the command line api of this program.

While I was able to use getopt a second time, please review, it does require re-scanning for position of BSAVE in argv after it is mutated and resetting optind, and, re-parsing for -a and -l arguments in the second pass where it is required on BSD but not with linux.

All of this is described with a very long comment inline, but I will re-describe the most important issue here.

Linux mutates argv after processing, while bsd does not, and this causes any second-pass processing of argv to be unreliable to perform on both systems.

In getopt call,

while ((c = getopt(argc, argv, "a:l:t:s:dhvxy")) != -1)

With example command, ../dos33 -d test.dsk BSAVE -a 0x100 -l 1 SINCOS abc
On linux, argv is mutated,

  argv[0]=../dos33
  argv[1]=-d
- argv[2]=test.dsk
- argv[3]=BSAVE
- argv[4]=-a
- argv[5]=0x100
- argv[6]=-l
- argv[7]=1
+ argv[2]=-a
+ argv[3]=0x100
+ argv[4]=-l
+ argv[5]=1
+ argv[6]=test.dsk
+ argv[7]=BSAVE
   argv[8]=SINCOS
   argv[9]=abc

optind becomes 6, pointing at test.dsk, then incremented to become command "BSAVE" and remaining arguments, local file "SINCOS" and apple file "abc".

Also exclusive to linux, when l:a: is removed from the getopt string, getopt will cause stderr message, ../dos33: invalid option -- 'a' to display, though it does continue parsing. While this message does not display on BSD, as it has stopped at BSAVE command before reaching them. Linux also mutates argv into something totally unusable in this case: ["../dos33", "-d", "-a", "-l", "test.dsk", "BSAVE", "0x100", "1", "SINCOS", "abc"]

While on BSD, things are much simpler, argv is not mutated, and l:a: can be included or excluded in first getopt call in the given code without error, and optind is 2, pointing at test.dsk with remaining arguments unmodified for re-processing by a second pass of getopt, "BSAVE", "-a", "-x100", "-l", "1", "SINCOS", and "abc".

micahcowan · 2023-08-26T07:57:24Z

@jquast Before we go any further, I should maybe point out that in my experience @deater hasn't had much of a tendency to respond to or incorporate pull requests, so probably won't see this anyway (I'm sorry I didn't point this out earlier, before you'd done the work).

I'm not really clear on why GNU getopt's permuation of argv should present a problem for my proposed solution that re-invokes getopt, but somehow not for yours which does the same... not least because no actual reinvocation is necessary on the platform that actually does this permutation.

But I guess it's time to stop explaining with words, and start showing code. #23 demonstrates the concept I meant. It's probably not perfect, but I believe it has the following advantages over this implementation (#22):

It avoids redundant/repeated logic for separate getopt-invocation spots (that can easily get “out of sync” if changes are made).
Scalable: avoids things getting rather out of hand if other commands besides BSAVE gain options in the future (say, a -v to CATALOG for extra info, or something).
No rescanning argv from the top to search for "BSAVE" or other commands (that wasn't actually necessary in yours, either: the command will always be at argv[optind+1] when getopt exits with -1, regardless of whether permutations occurred or not. Otherwise dos33 wouldn't have been able to find the command in the first place, to handle BSAVE).
Requires less added code overall.

If you see the merit in this approach, please feel free to take it over and adapt it in your (this) pull request. Mine lacks your regression test, and could (IMO) be improved by moving the whole getopt stuff out into a function that gets called twice, rather than using an extra loop.

jquast marked this pull request as ready for review July 24, 2023 03:14

jquast marked this pull request as draft July 24, 2023 04:30

This comment was marked as outdated.

Sign in to view

jquast force-pushed the jq/dos33-getopt-BSAVE-fixes branch from d03c721 to 05def98 Compare July 24, 2023 05:34

jquast marked this pull request as ready for review July 24, 2023 05:44

jquast mentioned this pull request Jul 24, 2023

Various utils (at least dos33fs-utils and prodos-utils): documentation of options placement is wrong on BSD-ish platforms. #18

Open

micahcowan reviewed Jul 24, 2023

View reviewed changes

jquast force-pushed the jq/dos33-getopt-BSAVE-fixes branch from aacd574 to b2c43ea Compare August 16, 2023 20:43

- Fix dos33.c getopt for BSAVE arguments

4fa77b5

deater#18 - tested on OSX and Debian arm64 - bugfix display of length when `-d` debug is used - small fixes to usage terms

jquast force-pushed the jq/dos33-getopt-BSAVE-fixes branch from b2c43ea to 4fa77b5 Compare August 16, 2023 20:45

spellfix

96f99f3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix dos33.c getopt for BSAVE arguments #22

Fix dos33.c getopt for BSAVE arguments #22

jquast commented Jul 24, 2023 •

edited

Loading

This comment was marked as outdated.

jquast commented Jul 24, 2023

micahcowan commented Jul 24, 2023

micahcowan Jul 24, 2023

jquast commented Aug 16, 2023 •

edited

Loading

micahcowan commented Aug 26, 2023

Fix dos33.c getopt for BSAVE arguments #22

Are you sure you want to change the base?

Fix dos33.c getopt for BSAVE arguments #22

Conversation

jquast commented Jul 24, 2023 • edited Loading

This comment was marked as outdated.

jquast commented Jul 24, 2023

micahcowan commented Jul 24, 2023

micahcowan Jul 24, 2023

Choose a reason for hiding this comment

jquast commented Aug 16, 2023 • edited Loading

micahcowan commented Aug 26, 2023

jquast commented Jul 24, 2023 •

edited

Loading

jquast commented Aug 16, 2023 •

edited

Loading