[#403][#410] Improve error messages for pgagroal-cli

[decarv]

This commit improves the error messages for pgagroal-cli and pgagroal-admin commands by modifying the parse_command function.

It also tries to simplify adding new commands. Now adding a command with the structure " [subcommand] [arg] [arg] ..." requires the addition of the command enum and its struct in the command_table.

This means that the command parsing now involves the interpretation of a command_table defined in each cli.c and admin.c files.

[decarv]

Thank you for your feedback, @fluca1978.

My main goal with this PR was to seek your insights regarding my implementation and identify areas for improvement based on your vision.

Given your response suggests I'm on the right track, I'd like to discuss additional ideas (below) aimed at simplifying our new command handling approach.

As for the issues you highlighted above, I will address them later this evening.

1. Refining Command Execution through Callbacks I propose shifting from directly assigning action, mode, and log traces to utilizing a callback function activated upon command execution. This function would be the already existing implementation of the actions flush, enabledb, ..., but with a different signature, thereby streamlining the command structure and execution process. The callback could be defined as:

int (*action)(struct pgagroal_parsed_command* cmd) // returns exit_error

The pgagroal_parsed_command struct would then include all necessary information for action execution, which currently varies in signature but shares common elements like SSL* ssl, int socket, bool verbose, char output_format, etc.

With this, eliminating the pgagroal_{cli|action}_command_code seems beneficial, as it appears to add complexity without clear value. We could determine the command count using sizeof(array)/sizeof(array[0]).

Initially, I hesitated to implement these changes as they would necessitate further modifications to the existing code. What do you think?

2. Simplifying Error Handling I suggest removing the error_message field from the command struct, as I believe directly outputting error messages to stderr would be more straightforward and efficient. Is retaining error_message essential in your view?

[fluca1978]

Thank you for your feedback, @fluca1978.

My main goal with this PR was to seek your insights regarding my implementation and identify areas for improvement based on your vision.

Given your response suggests I'm on the right track, I'd like to discuss additional ideas (below) aimed at simplifying our new command handling approach.

As for the issues you highlighted above, I will address them later this evening.

1. Refining Command Execution through Callbacks I propose shifting from directly assigning action, mode, and log traces to utilizing a callback function activated upon command execution. This function would be the already existing implementation of the actions flush, enabledb, ..., but with a different signature, thereby streamlining the command structure and execution process. The callback could be defined as:

int (*action)(struct pgagroal_parsed_command* cmd) // returns exit_error

The pgagroal_parsed_command struct would then include all necessary information for action execution, which currently varies in signature but shares common elements like SSL* ssl, int socket, bool verbose, char output_format, etc.

This is clearly an optimal solution, even if I think it is probably too much for the needs of the project. I would suffice with this first patch, but if you have time and will to implement a callback based structure, I'm fine with it.

2. Simplifying Error Handling I suggest removing the error_message field from the command struct, as I believe directly outputting error messages to stderr would be more straightforward and efficient. Is retaining error_message essential in your view?

It's not essential, it could be beneficial only if we are not sure at which branch the error is raised and we need to try another branch, otherwise it is fine to output it directly having clear that this also means exiting from the parsing loop, which may be too early (?). Again, I'm almost fine with the status you reached in https://github.com/agroal/pgagroal/pull/411/commits/6bb59bb308b4fc743652a8f239c735fda34f3c6d but if you have more time and will, let's try to improve your implementation!

[decarv]

While attempting to implement the callback based structure, I found myself redesigning the entire file, which wasn't the goal, so I decided to abort that approach.

Could you check the PR and let me know if it looks good to you? I'm here to help with any adjustments you need.

[fluca1978]

@decarv I added a few comments on https://github.com/agroal/pgagroal/commit/e57675d0654f73a1af86510787465cf2b938bc09, please do the review. Also, could you please reword the commit message to include a little more details about the changes? Not a complete description but something more verbose.

[fluca1978]

@decarv I think the last commit is fine. Thanks.

@jesperpedersen PTAL commit https://github.com/agroal/pgagroal/commit/19f30dc5809449d22288a2c6cab1a57128f3a791

[jesperpedersen]

Otherwise it looks good

[jesperpedersen]

Merged.

Thanks for your contribution !

[decarv]

Thank you @fluca1978 and @jesperpedersen for the help!