Skip to content
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

fix: build log_proxy command in wrapper as an argv array, don't use gshell parser #38

Merged
merged 1 commit into from
Feb 9, 2024
Merged

Conversation

mk-fg
Copy link
Contributor

@mk-fg mk-fg commented Feb 8, 2024

This should fix passing -T "(null)" to log_proxy introduced by #35 in a hopefully slightly more obvious/cleaner way, and fix #37.

g_spawn_command_line_async() was replaced by g_spawn_async(), so that instead of earlier workflow of:

  • sprintf-template a command line of arguments.
  • Run g_spawn_command_line_async on it.
  • Which runs g_shell_parse_argv to parse command line of arguments to gchar **argv.
  • Run g_spawn_async with this argv.

It's just "build argv, run it with g_spawn_async", which eliminates unnecessary template-and-parse-it-back round-trip, and fixes string-escaping issues introduced by it cleanly.

Alternative can be to build a dynamic string, using g_shell_quote where string arguments are passed-through, but that seems unnecessary when it's just immediately parsed back to argv array anyway, and more error-prone in general.

Also removed glib/* component includes at the top, as these are redundant with #include <glib.h> which loads all of them, and are insufficient/incorrect otherwise anyway.

…shell parser

Should address earlier "(null)"-prefix bug from #35 in a more obvious way, and fix #37.
@thebaptiste
Copy link
Contributor

Many thanks for this nice improvement. I'm happy to remove my ugly patch about "(null)" (which I wasn't very proud of)

@thebaptiste thebaptiste merged commit 0eb49b5 into metwork-framework:master Feb 9, 2024
2 checks passed
@mk-fg
Copy link
Contributor Author

mk-fg commented Feb 9, 2024

Thanks for merging.

Oh, and also, can you hold off release for one more PR?

While going over the code again, I've noticed that my slapdash implementation of timestamp prefix in #35 was hugging lock in a loop unnecessarily ("continue" on EAGAIN without releasing the lock), so I made a patch to make it simpler and fix the issue, but didn't test it, which I can probably do and submit as a PR in the next hour or two.

@mk-fg
Copy link
Contributor Author

mk-fg commented Feb 9, 2024

Specific fix that I meant above: https://github.com/mk-fg/log_proxy/commit/ed0e3d7

@mk-fg
Copy link
Contributor Author

mk-fg commented Feb 9, 2024

Tested it just now, seem to work with/without prefix, added PR as #40.
I think everything is freed properly, and it's actually much less changes and code from pre-PR-35 than #35 itself is, which I think is always a plus too.
util.c can probably benefit from a cleanup to use more glib primitives, and is kinda redundant with what glib itself is.

@thebaptiste
Copy link
Contributor

Thanks for merging.

Oh, and also, can you hold off release for one more PR?

While going over the code again, I've noticed that my slapdash implementation of timestamp prefix in #35 was hugging lock in a loop unnecessarily ("continue" on EAGAIN without releasing the lock), so I made a patch to make it simpler and fix the issue, but didn't test it, which I can probably do and submit as a PR in the next hour or two.

Yes it's probably simpler, safer and faster to have only one write per line.

@mk-fg mk-fg deleted the use_argv_array_in_wrapper branch February 9, 2024 11:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handling of double-quotes in command-line arguments of log_proxy_wrapper
2 participants