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

Remove userspace app command clutter and enable autocompletion #259

Closed
ydahhrk opened this Issue Jan 26, 2018 · 0 comments

Comments

Projects
None yet
1 participant
@ydahhrk
Copy link
Member

ydahhrk commented Jan 26, 2018

I know that there are more pressing matters to attend (MAP-T, dynamic pool4, per-customer granularity), but the upcoming major version release feels like the one and only right time to change this kind of thing.

I've grown to dislike this kind of syntax:

$ jool --pool4 --add 192.0.2.1 --icmp --tcp

--pool4 and --add do not feel like options (in that they are not all that optional), and it doesn't really make that much sense to allow a variant like this:

$ jool --tcp 192.0.2.1 --add --icmp --pool4

The ability to reorder seems like a feature whose only purpose is to obfuscate the meaning of the command. The whole thing is also pretty lame in that it doesn't support bash autocompletion.

For a while I've wanted to remove a few double slashes and enforce order like commands such as ip and git do:

$ jool pool4 add 192.0.2.1 --icmp --tcp

Where the positioning of pool4 and add is fixed, but the remaining arguments can be given in any order. It'd be less GNU but more down-to-earth, IMO.

Aside from a little eye-candy, this would allow some internal architectural refactors that are now difficult because of the way argp_parse returns arguments. The core source file of the userspace app is one of Jool's messiest right now. Not to the point where it doesn't scale okay still, but definitely without its annoyances.

For a while this has seemed like too much of a whim to be worth any effort, but now that autocompletion popped into my head (and it feeds the drive for those refactors) the whole thing feels more justified.

If you disagree or have more ideas, please post below.

@ydahhrk ydahhrk added the Enhancement label Jan 26, 2018

@ydahhrk ydahhrk added this to the 4.0.0 milestone Jan 26, 2018

ydahhrk added a commit that referenced this issue Feb 2, 2018

Refactor the living hell out of the userspace app
There was a lot of mismatch between the grammar expected from the
app arguments and the means through which argp expects to handle
them, which led to a lot of clumsy, patchy code.

Actually, there's a bit of a lot going on in this commit:

- There's the drive to build a jool lib so the translator can be
  easily ported to other frameworks. I separated the argp parsing
  from the netlink request building in preparation for this
  purpose.
- I'm building a module for centralized management of global config
  values. This is because the relevant code was very poorly
  designed, which led to too much paperwork needed whenever a new
  global field was needed.

The code is largely untested and very likely unstable.

--file and --joold have not been adapted:

- I left --file alone because getting the JSON parser to cooperate
  with the global central module is getting a little tricky. Will
  patch --file back in the near future.
- --joold is a royal pain in the ass an I'm thinking about
  releasing the first iteration of Jool 4 without it.

Largely done coding #259.

ydahhrk added a commit that referenced this issue Oct 17, 2018

Refactor the userspace clients
Instance naming and mandatory NAT64 pool6 made bare argp much too
convoluted for my patience. Jool 4 is likely not going to see the
light of day (because Device Driver Jool is just going to be
another iteration of Jool 3 now), and since we're changing minor
version number in this release, now is the time.

- Remove double dashes in many command line arguments; issue #259.
- Centralize global variable management. Removes a lot of paperwork
  every time we need to add/change/remove some global value.
- Separate the original massive argp parsing into one argp parsing
  per [mode, operation] tuple. Lasagna code FTW.

There are still bugs lurking around.

@ydahhrk ydahhrk modified the milestones: 4.0.0, 3.6.0 Nov 23, 2018

@ydahhrk ydahhrk modified the milestones: 3.6.0, 4.0.0 Jan 9, 2019

@ydahhrk ydahhrk closed this in 7bb6dea Jan 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment