Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

what if we put everything in one binary? #115

Closed
Artoria2e5 opened this issue Jun 9, 2023 · 3 comments
Closed

what if we put everything in one binary? #115

Artoria2e5 opened this issue Jun 9, 2023 · 3 comments

Comments

@Artoria2e5
Copy link

Artoria2e5 commented Jun 9, 2023

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?

//add -- options to this structure DO NOT ADD OPTIONAL ARGUMENTS! Optional arguments are a GNU extension and are not supported in Unix or some compilers- TJE

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
Copy link
Contributor

xahmad commented Jun 20, 2023

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:
#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
Copy link
Author

hah! Of course I shouldve checked the discussions.

vonericsen added a commit that referenced this issue Jul 20, 2023
…arguments

A very old comment about optional arguments from about 10 years ago was still saying not to use them.
This no longer is necessary and the comment is no longer needed.

[#115]

Signed-off-by: Tyler Erickson <tyler.erickson@seagate.com>
@vonericsen
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants