add option to detach the client process from the terminal instance #397

Manually merged
dnkl merged 3 commits from felipetrz/foot:option-no-wait into master 9 months ago

I made this PR to add the option to detach the client process from the terminal instance, following #395, by factoring out the ownership of the terminal to an intermediate structure that can be owned by either the client or the server.

Closes #395.

I made this PR to add the option to detach the client process from the terminal instance, following https://codeberg.org/dnkl/foot/issues/395, by factoring out the ownership of the terminal to an intermediate structure that can be owned by either the client or the server. Closes #395.
felipetrz force-pushed option-no-wait from 8ecbdc897f to 3ed9314266 9 months ago
dnkl requested changes 9 months ago
dnkl left a comment

Overall, I think the idea, and the design is ok. And, while it's a fairly large refactoring, the added code is small.

There are a couple of minor issues described below. In addition to fixing them, I would also like you to:

  • Add a changelog entry (and if you want, add yourself to the "contributors" list)
  • Shell completions for the new --no-wait option (completions/**/*client*).
client_destroy(struct client *client);
static void
client_send_exit_code(struct client *client, int exit_code);
dnkl commented 9 months ago
Poster
Owner

Nit: forward declarations should be single line (the idea is to be able to grep for ^function)

Nit: forward declarations should be single line (the idea is to be able to grep for `^function`)
dnkl marked this conversation as resolved
server.c Outdated
struct terminal *term;
};
static void
instance_destroy(struct terminal_instance *instance, int exit_code);
dnkl commented 9 months ago
Poster
Owner

nit: forward declaration should be single line

nit: forward declaration should be single line
dnkl marked this conversation as resolved
{
if (instance->terminal != NULL) {
term_destroy(instance->terminal);
}
dnkl commented 9 months ago
Poster
Owner

Style: single-line if-statements don't use braces.

Style: single-line if-statements don't use braces.
dnkl marked this conversation as resolved
}
if (instance->client != NULL) {
LOG_WARN("client FD=%d: is still alive", instance->client->fd);
dnkl commented 9 months ago
Poster
Owner

I don't think we ever want this warning. When the client owns the instance (i.e. when the user did not use --no-wait), the client connection is typically always alive when the terminal shuts down.

I don't think we ever want this warning. When the client owns the instance (i.e. when the user did **not** use `--no-wait`), the client connection is typically _always_ alive when the terminal shuts down.
dnkl marked this conversation as resolved
free(argv);
if (instance->conf.no_wait) {
// the server owns the instance
tll_push_back(server->terminals, instance);
dnkl commented 9 months ago
Poster
Owner

This code path never initializes instance->client.

This code path never initializes `instance->client`.
dnkl marked this conversation as resolved
server.c Outdated
} else {
// the instance is attached to the client
instance->client = client;
client->instance = instance;
dnkl commented 9 months ago
Poster
Owner

This code path; i.e. starting a client without --no-wait, leaks memory:

=================================================================
==6041==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 8 byte(s) in 1 object(s) allocated from:
    #0 0x7f411bcd4639 in __interceptor_calloc /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cpp:154
    #1 0x560fcf87f49f in xcalloc ../../xmalloc.c:32
    #2 0x560fcf782aff in fdm_client ../../server.c:270
    #3 0x560fcf6fb7e3 in fdm_poll ../../fdm.c:459
    #4 0x560fcf72693b in main ../../main.c:524
    #5 0x7f411acaab24 in __libc_start_main (/usr/lib/libc.so.6+0x27b24)

SUMMARY: AddressSanitizer: 8 byte(s) leaked in 1 allocation(s).
This code path; i.e. starting a client **without** `--no-wait`, leaks memory: ``` ================================================================= ==6041==ERROR: LeakSanitizer: detected memory leaks Direct leak of 8 byte(s) in 1 object(s) allocated from: #0 0x7f411bcd4639 in __interceptor_calloc /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cpp:154 #1 0x560fcf87f49f in xcalloc ../../xmalloc.c:32 #2 0x560fcf782aff in fdm_client ../../server.c:270 #3 0x560fcf6fb7e3 in fdm_poll ../../fdm.c:459 #4 0x560fcf72693b in main ../../main.c:524 #5 0x7f411acaab24 in __libc_start_main (/usr/lib/libc.so.6+0x27b24) SUMMARY: AddressSanitizer: 8 byte(s) leaked in 1 allocation(s). ```
Poster

Forgot to readd the free(argv); there, fixed

Forgot to readd the `free(argv);` there, fixed
dnkl marked this conversation as resolved
}
void
dnkl commented 9 months ago
Poster
Owner

Super nitpick: only a single line between functions, please :)

Super nitpick: only a single line between functions, please :)
dnkl marked this conversation as resolved
server.c Outdated
tll_foreach(server->terminals, it) {
instance_destroy(it->item, 1);
}
dnkl commented 9 months ago
Poster
Owner

Style: single-line for-statements don't use braces.

Style: single-line for-statements don't use braces.
dnkl marked this conversation as resolved
dnkl added the
enhancement
label 9 months ago
Owner

Forgot: please also update the man page: doc/footclient.1.scd.

Forgot: please also update the man page: `doc/footclient.1.scd`.
felipetrz force-pushed option-no-wait from 43d69c5a68 to 760c073ba6 9 months ago
felipetrz force-pushed option-no-wait from 760c073ba6 to 4b64d52e70 9 months ago
felipetrz force-pushed option-no-wait from 4b64d52e70 to 11e403651f 9 months ago
felipetrz force-pushed option-no-wait from 11e403651f to 1422ee55c7 9 months ago
Poster

Just made 2 commits fixing the mentioned issues.

Just made 2 commits fixing the mentioned issues.
felipetrz requested review from dnkl 9 months ago
felipetrz force-pushed option-no-wait from 1422ee55c7 to 452830bd08 9 months ago
dnkl reviewed 9 months ago
struct terminal_instance *instance = malloc(sizeof(struct terminal_instance));
*instance = (struct terminal_instance) {
.client = NULL,
dnkl commented 9 months ago
Poster
Owner

I just noticed that I read the initial PR a bit too quickly; I missed the fact that you initialized the entire struct here. Hence explicitly setting .client = NULL is strictly speaking not necessary.

I.e my comment about instance->client not being set was wrong. Sorry!

But, lets not change things around again. We'll keep it as it is - it doesn't hurt anything.

I just noticed that I read the initial PR a bit too quickly; I missed the fact that you initialized the entire struct here. Hence explicitly setting `.client = NULL` is strictly speaking not necessary. I.e my comment about `instance->client` not being set was wrong. Sorry! But, lets not change things around again. We'll keep it as it is - it doesn't hurt anything.
dnkl marked this conversation as resolved
Owner

This looks good to me now. But, let me run this for a while before I merge, just to make sure nothing unexpected pops up.

This looks good to me now. But, let me run this for a while before I merge, just to make sure nothing unexpected pops up.
dnkl added this to the 1.7.0 milestone 9 months ago
dnkl merged commit e7a4378f18 into master manually 9 months ago

Reviewers

dnkl was requested for review 9 months ago
The pull request has been manually merged as e7a4378f18.
Sign in to join this conversation.
Loading…
There is no content yet.