Command-line error handling is incoherent (and misses certain issues)

[fako1024]

When testing #43 I encountered that when providing -resolve to goQuery nothing happens (as in: the program returns immediately, without printing anything). When calling without that option all works fine.

DoD

• Assess / reproduce
• Fix underlying issue and review DNS resolution
• Improve logging

[fako1024]

Looking closer, the problem is not actually with the DNS resolution but rather missing error handling / logging. When manually adding an error message this is what actually happens:

└─ $ ▶ ./goQuery --query.server.addr 127.0.0.1:8888 -i eth0 -f -10m -q fw-1,fw-2 -resolve -n 10 -c "dip = 1.1.1.1" talk_conv
unknown shorthand flag: 'r' in -resolve

The error simply isn't shown because in cmd/goQuery/commands/root.go there is no handling for the error:

// Execute is the main entrypoint and runs the CLI tool
func Execute() {
        err := rootCmd.Execute()
        if err != nil {
                os.Exit(1)
        }
        return
}

[fako1024]

OK, this is much more complicated than I anticipated. We now have three different ways of communicating errors to the user:

• Our logging (which isn't active yet when rootCmd.Execute() is called)
• The cobra inherent error logging (which is disabled via SilenceErrors: true, see below why that's a problem)
• Several "manual" fmt.FPrintf() statements inside the command preparation / execution

In this particular scenario (argument / command line parameter parsing error) none of the aforementioned methods trigger because:

• The flag parsing is cobra-internal, i.e. we can't use our logger (plus, it's not ready yet)
• The cobra-internal logging is disabled, and enabling it would lead to duplicate errors in other situations (due to the "manual" statements and / or our logger)

IMHO the way we handle logging in conjunction with cobra is a bit whack right now. In particular I don't quite understand the rationale behind the fmt.FPrintf() statements when we could just trickle up the error via the entrypoint() error call up to the caller and then use our logger with a Fatal() instead of the manual os.Exit(1) call (of course we'd have to make sure a logger is available).

@els0r What's your take on this? Is this just a relic from the past or am I missing something?

[els0r]

OK, this is much more complicated than I anticipated. We now have three different ways of communicating errors to the user:

• Our logging (which isn't active yet when rootCmd.Execute() is called)
• The cobra inherent error logging (which is disabled via SilenceErrors: true, see below why that's a problem)
• Several "manual" fmt.FPrintf() statements inside the command preparation / execution

In this particular scenario (argument / command line parameter parsing error) none of the aforementioned methods trigger because:

• The flag parsing is cobra-internal, i.e. we can't use our logger (plus, it's not ready yet)
• The cobra-internal logging is disabled, and enabling it would lead to duplicate errors in other situations (due to the "manual" statements and / or our logger)

IMHO the way we handle logging in conjunction with cobra is a bit whack right now. In particular I don't quite understand the rationale behind the fmt.FPrintf() statements when we could just trickle up the error via the entrypoint() error call up to the caller and then use our logger with a Fatal() instead of the manual os.Exit(1) call (of course we'd have to make sure a logger is available).

@els0r What's your take on this? Is this just a relic from the past or am I missing something?

Incoherent is spot on. As for why we don't log the error in the main routine is because it would then print it twice in case we also wanted to print the usage, the behavior being: print error, show usage.

As for just logging with Fatalf: let's give it a shot. There is exactly one fmt.Fprintf, which is when the logger itself failed to initialize. After that, we should be able to use it at our disgression.

[els0r]

See #110 for a plain CLI logger that can be used from init().