Seagate / openSeaChest

Cross platform utilities useful for performing various operations on SATA, SAS, NVMe, and USB storage devices.
Other
436 stars 60 forks source link

what if we put everything in one binary? #115

Closed Artoria2e5 closed 1 year ago

Artoria2e5 commented 1 year ago

OSC is currently spread across 16 programs with overlapping "common" functionality and specialized things. The result is a bunch of manual pages and help output with repeated content and on the compiled side, a bunch of wasted space as every binary statically links pull in the same functions.

Maybe it's time to make a new utils/C/openSeaChest.c that just does... everything. The new commands will be of the form

openSeaChest [common options]
openSeaChest [common options] subcommand [command-specific options]

In other words, openSeaChest_Basic --Scan becomes openSeaChest --Scan; openSeaChest_SMART -d /dev/sg1 --shortDST --poll becomes openSeaChest -d /dev/sg1 SMART --shortDST --poll. The subcommand arrangement should keep things reasonably scoped, while isolating common options should make documentation a little less hectic. In some unspecified future it may even make sense to reduce the amount of uppercase letters so I don't hold shift when typing so much.


Some fishy bits in arg parsing:

No optional args?

https://github.com/Seagate/openSeaChest/blob/3fc1fb6154f079e6db4cec35bb8971205a68fc10/utils/C/openSeaChest/openSeaChest_Firmware.c#L123

Er, optional arguments are not a GNU extension but part of the POSIX getopt behavior. Long opts are on the other hand a GNU extension. And none of this has anything to do with the OS or the compiler because you are already using your own wingetopt.

This seems to be from the initial commit, before wingetopt became the default 8 months ago. Might be a good idea to remove.

while(1)?

https://github.com/Seagate/openSeaChest/blob/3fc1fb6154f079e6db4cec35bb8971205a68fc10/utils/C/openSeaChest/openSeaChest_Firmware.c#LL195C1-L195C102

Could be bad if some option like that still exists. Still, new binary, can change a thing or two.

too much strcmp

https://github.com/Seagate/openSeaChest/blob/3fc1fb6154f079e6db4cec35bb8971205a68fc10/utils/C/openSeaChest/openSeaChest_Firmware.c#LL206C1-L207C87

The uh, preferred way to do this is to load a non-character value into option.val, so you can just case on it. The type to take is int, so you have a lot of numbers starting with 256 to play with.

xahmad commented 1 year ago

Hi @Artoria2e5

Thanks for submitting the issue. There are a few things involved in it.

But your overall premise of one tool vs multiple tools is covered in this on going poll and discussion here: https://github.com/Seagate/openSeaChest/discussions/106

If you agree that the discussion is a better place to carry forward the pros and cons of single vs multiple binaries, please close this issue & add your comments there.

The specific concerns you have with getopts & strcmp, should be separated out as issues that need code improvements.

Artoria2e5 commented 1 year ago

hah! Of course I shouldve checked the discussions.

vonericsen commented 12 months ago

I've cleaned up that extremely old and long irrelevant comment on optional arguments. I don't remember the details at this point, but as the code was being developed and ported to some platform the macro for optional arguments was not supported on some system we were trying to support at the time, which is when that comment was added. That was probably a comment from 10 years ago at this point.

I agree with @xahmad about creating issues for these other getopt parsing issues so we can look into how to work on them.