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

Simpler IPC #907

Merged
merged 5 commits into from
Jul 22, 2024
Merged

Simpler IPC #907

merged 5 commits into from
Jul 22, 2024

Conversation

ekoby
Copy link
Member

@ekoby ekoby commented Jul 19, 2024

simplifies IPC client cli subcommands (tunnel_status, etc) -- simple command/response client does not need an event loop
improves handling of incoming commands - use JSON to determine command boundaries

more robust incoming command parsing

get getopt via vcpkg
@ekoby ekoby requested a review from a team as a code owner July 19, 2024 20:13
json_tokener *parser = s->data;

size_t processed = 0;
while (processed < len) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this have some kind of escape-hatch max value? Something like a 1mb or 10 mb? something 'crazy' but not too crazy?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ipc_cmd_buffered used to use MAXIPCCOMMANDLEN (4096 * 4)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

previous implementation was not correct and would definitely crash if you send over 16K of data

if (cmd_soc == INVALID_HANDLE_VALUE) {
DWORD err = GetLastError();
fprintf(stderr, "failed to connect to pipe: %lu", err);
exit(1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are you certain about this particular change with respect to exit-ing? Why is windows the only one that would end up in this situation? Shouldn't the other OS's all exit too in these situations?

This function is much harder to review with all these defines. the old code appears much simpler (just looking at it here, without the IDE) why were these changes needed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

other OS's would also exit -- just a bit later -- after the write attempt. I'll add the exit on connect

there were multiple callback needed for a simple request/response pattern. which is definitely not simpler

also almost all ziti-edge-tunnel <subcmd> were not working on windows because command parsing was relying on the '\0'/'\n'

Copy link
Member

@dovholuknf dovholuknf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i still think we should prevent reading from the IPC 'forever' but other than that, lgtm

@ekoby ekoby merged commit eaf6f9f into main Jul 22, 2024
20 checks passed
@ekoby ekoby deleted the simpler-ipc branch July 22, 2024 18:55
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