Improve error messages in `pgagroal-cli`

[fluca1978]

While working on #399 I discovered that pgagroal-cli does not provide good error messages. For example.

• pgagroal-cli conf -> pgagroal-cli: unknown command conf while it should be something like command conf requires a subcommand
• pgagroal-cli conf get -> pgagroal-cli: unknown command conf while it should be something like command conf get requires an argument name

Similar behaviour for other commands. The problem is that the handling of command line arguments in pgagroal-cli is done with a catch-all error block https://github.com/agroal/pgagroal/blob/master/src/cli.c#L652 so every rule that cannot fully parse jumps into such block and provides no useful information than a generic error.

My idea is to trap errors near every parsing rule, so that the error message can have context-aware information to send back to the user.

[fluca1978]

Same issue happens in pgagroal-admin:

% pgagroal-admin user
pgagroal-admin: unknown command or subcommand <user>

while it should report something like command user requires a subcommand.

[decarv]

Hello, @fluca1978.

As we have discussed, I am ready to start working on this issue. In the near future, I will outline my proposed approach for us to discuss further, so we're aligned on the solution's direction before I dive deeper into implementation.

[decarv]

While investigating this issue, I encountered a segmentation fault when running pgagroal-cli when no command is specified a command, but a password is specified.

It appears the segfault occurs because the program attempts to free a password that points to a position in *argv.

Steps to reproduce

./pgagroal-cli -h localhost -p 2345 -U test -P test

Proposed Solution: This can be addressed by setting the initial value of do_free to false and then setting it to true only if pgagroal_get_password is called.

Here is the relevant segment of the proposed fix (please notice my comments):

// NOTE(decarv): adjust below
bool do_free = false;

// ...

/* Password */
if (password == NULL)
{
    // NOTE(decarv): This inner check seems unnecessary since `password` is confirmed to be NULL above. Am I missing something?
    if (password != NULL) 
    {
        free(password);
        password = NULL;
    }

    printf("Password : ");
    password = pgagroal_get_password();
    printf("\n");
    
    // NOTE(decarv): add below
    do_free = true;
}
// NOTE(decarv):  Suggested to remove the following else block to avoid resetting `do_free` unnecessarily.
else
{
   do_free = false;
}

Ref.: https://github.com/agroal/pgagroal/blob/3b565188fc619cf0b635147ce0097282cedae156/src/cli.c#L534

Should I open a new issue to address this or would it be more appropriate to include the fix in this issue and corresponding PR?

Edit: Add line ref.

[jesperpedersen]

New issue

[fluca1978]

Yeah, open a new issue. At glance, I would remove do_free completely and memcpy the password into another char* so that free can be called freely (no pun intended). Seems that the inner free at https://github.com/agroal/pgagroal/blob/3b565188fc619cf0b635147ce0097282cedae156/src/cli.c#L538 will never be called in any way, so it is probably a wrong rebase/merge.

I would investigate/support from next monday.

[decarv]

Based on previous discussion, I have considered two approaches for improving the error messages in pgagroal-cli.

1. Modify parse_command so it deals with pgagroal_command_type

We could define each command as a struct containing the command string, the expected number of arguments, and an error message. Enumerating the commands would simplify error mapping during the parsing phase, establishing some sort of a formal grammar for command processing.

2. Define a pgagroal_errno global variable

This approach would involve attributing each program error to a pgagroal_errno value, which could then be checked at strategic points to direct the flow to a done label, returning the error message to the user. Its primary benefit is the capability to handle a wider array of errors beyond just parsing issues, including those related to runtime connections.

I am tending towards the second because it enables an equally robust error message handling, but broader. However, I am unsure if it aligns with your vision for the project. What are your thoughts?

[fluca1978]

1. Modify parse_command so it deals with pgagroal_command_type

We could define each command as a struct containing the command string, the expected number of arguments, and an error message. Enumerating the commands would simplify error mapping during the parsing phase, establishing some sort of a formal grammar for command processing.

This is, according to me, the optimal solution, even if the longest and probably most difficult to implement, so I'm not sure it is worth for such a bunch of commands as pgagroal-cli (and pgagroal-admin) has.

2. Define a pgagroal_errno global variable

This approach would involve attributing each program error to a pgagroal_errno value, which could then be checked at strategic points to direct the flow to a done label, returning the error message to the user. Its primary benefit is the capability to handle a wider array of errors beyond just parsing issues, including those related to runtime connections.

I don't like very much the idea of having a global variable. I was thinking about parse_command to return a few values to describe parsing correct, missing sub command, missing argument and so on, capture the return value and jump to the error handling as soon as possible.

Another idea could be to define different functions, like pgagroal_parse_subcommand, pgagroal_parse_command, pgagroal_parse_single_command and pgagroal_parse_deprecated_command and group each set of parsing stage together, so that conf get must be parsed by pgagroal_parse_command (conf) and then pgagroal_parse_subcommand (get), therefore something like:

if ( pgagroal_parse_command( "conf" ) ) {
  // here we need all the conf xxx commands
  if ( pgagroal_parse_subcommand( "get" ) ) { ... }
  ...
  else {
     errx( "Command get requires a subcommand like get, set, ..." );
  }
}
else if ( pgagroal_parse_single_command( "ping" ) { ... }
else if ( pgagroal_parse_deprecated_command( "conf-get" ) { ... }

and pgagroal_parse_subcommand could directly die if there is the need for an argument that has not been specified.

[decarv]

1. Modify parse_command so it deals with pgagroal_command_type

We could define each command as a struct containing the command string, the expected number of arguments, and an error message. Enumerating the commands would simplify error mapping during the parsing phase, establishing some sort of a formal grammar for command processing.

This is, according to me, the optimal solution, even if the longest and probably most difficult to implement, so I'm not sure it is worth for such a bunch of commands as pgagroal-cli (and pgagroal-admin) has.

Great! I think I have enough to draft a solution for now. I will come back to you should I have further questions.

[fluca1978]

@decarv https://github.com/decarv/pgagroal/commit/e0cb97be04637af6d166ca959ce65ff5383d24e4 looks really promising, give me a few days to fully review your contribution.

[decarv]

@fluca1978 Thank you! I actually changed my code last night and I feel I reached a point where I'm happy with it. I will open a PR so you can review it.

[fluca1978]

Close via 11a1f475631534797aab63d6ad34d3990f0f9e70