Skip to content

Refactor command line parsing#183

Open
gtsiam wants to merge 13 commits intoapalrd:mainfrom
gtsiam:refactor-cmdline
Open

Refactor command line parsing#183
gtsiam wants to merge 13 commits intoapalrd:mainfrom
gtsiam:refactor-cmdline

Conversation

@gtsiam
Copy link
Contributor

@gtsiam gtsiam commented Mar 7, 2026

Part of ongoing janitorial work. I plan to do more, but in the interest of not making these impossible to review... I try to keep my commits sensible, so do look at them individually if you prefer (squashing is terrible etc. etc.).

The only user-visible change is a warning when eg. --syslog is combined with --mktun/--rmtun. Erroring out has the potential to break old setups unexpectedly, so I didn't.

EDIT: Also when positional arguments are included. Silently ignoring them was the previous behavior, now we're just loudly ignoring them.

Note: Before doing a 1.0, it may be a good idea to change the command line syntax to something like tayga mktun ..., tayga rmtun ..., tayga start .... Not doing so could make argument parsing really annoying down the line.

@gtsiam gtsiam force-pushed the refactor-cmdline branch 4 times, most recently from 395713d to 20f872f Compare March 7, 2026 21:06
gtsiam added 13 commits March 10, 2026 19:27
Previous behaviour was to silently ignore them. The correct approach
would be to error out, but in the interest of backwards compatibility,
a warning will have to do.
Admittedly my main motivation for this is to avoid calling malloc() in
config_init(), since that is potentially fallible and logging will not
have been set up at that point.
Useful for when you're at a loss.
@gtsiam gtsiam force-pushed the refactor-cmdline branch from 20f872f to 04db7c9 Compare March 10, 2026 18:48
@gtsiam
Copy link
Contributor Author

gtsiam commented Mar 10, 2026

@apalrd Okay, I dealt with the conflicts, but... Do you care to merge outside contributions or do you just want to be left alone? I just picked this up for fun, so I don't really mind either way.

If you only take this and want no more, I still have a dev branch you might want to pick from.

@apalrd
Copy link
Owner

apalrd commented Mar 10, 2026

I don't mind contributions at all! I just prefer them small enough to review, or not overlapping many issues at once.

This seems like an overall good PR. We could probably move some of these vars to gcfg (and make gfcg static instead of in heap, which I saw is another change). We could also move the cmdline parsing to config_init, or to conffile.c in general, and then have everything configuration-related in one file. All of these are entirely optional though.

As to mktun/rmtun, I agree it would be good as a command instead of an argument. I think the default should be to run (not mktun/rmtun), so no need for 'start'. mktun/rmtun are less useful now that tayga can configure its own tun adapter, since before the access pattern was to mktun, then call ip a bunch of times to set stuff up, then call tayga to actually translate. Now most of the common ip calls can be done in tayga.conf, so it's not as useful.

@gtsiam
Copy link
Contributor Author

gtsiam commented Mar 10, 2026

small enough to review

Yeah, fair enough. For this, would you prefer I split it up, leave it as is, or to pull the rest of the dev branch (which has the static gcfg) and any other changes all here?

We could probably move some of these vars to gcfg

That had been my next planned change. And for user/group/chroot specifically to be added to the configuration file.

move the cmdline parsing to config_init

Can't say I'm a huge fan of that - not with the current setup at least. Actually - maybe if we rewrite config parsing to use a single source of truth for cmd/config parsing? Sounds doable.

mktun/rmtun

Agreed. Just that I'd do it right before 1.0. Not sure what you're release plan is. Breaking changes aren't fun, but people will check twice for a major version change. Probably.

@apalrd
Copy link
Owner

apalrd commented Mar 11, 2026

Agree with most of that.

I'd like to eventually merge the config + cmdline stuff in one area, since there is some overlap between them. Some options are obviously exclusive to one or the other, but we could re-use the parsing code for options which are valid on both. user/group/chroot could be valid in either place I think. Maybe if you have a really simple setup you could write it entirely on the command line, I guess?

We also already have config_validate which validates all of the option combinations, and could be extended to validate the command line options make sense together (and many of these look at both - such as chroot looking for config option data-dir). Currently these validations are mostly done in main(), which is not pretty, and most of the code in main() is not covered by any test cases.

All of your dev branch stuff seems reasonable to me. If you'd like to add it to the PR I can add individual review comments to a few of them.

@apalrd
Copy link
Owner

apalrd commented Mar 14, 2026

Since the change of gcfg from heap to bss caused a massive number of lines to show as 'changed', I pushed that alone as a separate PR in #185. That should help make the reviews here not polluted with the big bulk change.

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

Successfully merging this pull request may close these issues.

2 participants