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

multi-tun support for windows #1020

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

multi-tun support for windows #1020

wants to merge 8 commits into from

Conversation

dovholuknf
Copy link
Member

still can debate the name "ipc-discriminator". Nothing new has come to mind yet but there is probably something better. If no discriminator is passed the legacy path is used. This will keep things backward compatible when only one tun is running and the UI should also "just work".

@dovholuknf dovholuknf requested a review from a team as a code owner October 15, 2024 10:57
@@ -561,9 +567,9 @@ static bool process_tunnel_commands(const tunnel_command *tnl_cmd, command_cb cb
add_id_req->identifier = strdup(new_identifier);
add_id_req->identifier_file_name = strdup(new_identifier_name);
add_id_req->jwt_content = strdup(tunnel_add_identity_cmd.jwtContent);
add_id_req->use_keychain = tunnel_add_identity_cmd.useKeychain;

add_id_req->use_keychain = true;
Copy link
Member

Choose a reason for hiding this comment

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

this should not be changed

size_t sockfilebase_len = strlen(sockfilebase);
size_t eventsockfilebase_len = strlen(eventsockfilebase);

sockfile = calloc(socket_path_len + sockfilebase_len + ipc_discriminator_len + 1, sizeof(char));
Copy link
Member

Choose a reason for hiding this comment

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

why allocate just to print it below? however it is populated it can be logged directly

also this is a memory leak

f = model_list_it_element(it);
it = model_list_it_remove(it);
ZITI_LOG(INFO, "zet [%d] IPC found at: %s%c%s", idx++, SOCKET_PATH, PATH_SEP, f);
char *ipc = calloc(100, sizeof(char));
Copy link
Member

Choose a reason for hiding this comment

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

not used?

Copy link
Member

Choose a reason for hiding this comment

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

seems only use to format and log, just log info directly

@@ -2125,12 +2299,15 @@ static int version_opts(int argc, char *argv[]) {
int c, option_index, errors = 0;
optind = 0;

while ((c = getopt_long(argc, argv, "v",
while ((c = getopt_long(argc, argv, "v:P:",
Copy link
Member

Choose a reason for hiding this comment

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

v does not take arg does not need : after it

@@ -43,6 +43,8 @@ extern "C" {
#define MAXPATHLEN PATH_MAX
#endif

#define DEFAULT_EXECUTABLE_NAME "ziti-edge-tunnel"
Copy link
Member

Choose a reason for hiding this comment

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

wrong place for it

@@ -201,6 +203,7 @@ extern void ziti_tunnel_set_log_level(int lvl);
typedef void (*ziti_tunnel_async_fn)(uv_loop_t *loop, void *ctx);
extern void ziti_tunnel_async_send(tunneler_context tctx, ziti_tunnel_async_fn f, void *arg);

size_t find_other_zets(model_list* ipcs, const char* base, const char* prefix);
Copy link
Member

Choose a reason for hiding this comment

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

wrong place -- should not be in the library code


char* get_config_file_name() {
if (config_dir != NULL) {
char* config_file_name = calloc(FILENAME_MAX, sizeof(char));
Copy link
Member

Choose a reason for hiding this comment

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

leak

Copy link
Member

Choose a reason for hiding this comment

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

this is a bad practice -- since it's sometimes allocated that caller has no idea if it can be freed

LIST_INSERT_HEAD(&load_list, inst, _next);
}
exit_loop:
Copy link
Member

Choose a reason for hiding this comment

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

exit_loop is not exiting loop? misleading

@@ -1102,36 +1112,39 @@ static void load_identities(uv_work_t *wr) {

uv_dirent_t file;
while (uv_fs_scandir_next(&fs, &file) == 0) {
ZITI_LOG(TRACE, "processing file: %s %d", file.name, rc);
char* file_as_identifier = malloc(MAXPATHLEN);
Copy link
Member

Choose a reason for hiding this comment

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

why not use a stack buffer?

}

char actual_config_dir[FILENAME_MAX];
realpath(path, actual_config_dir);
Copy link
Member

Choose a reason for hiding this comment

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

should we check the return value of realpath?

#elif __unix__ || unix || ( __APPLE__ && __MACH__ )
#include <grp.h>
#include <sys/un.h>
#define SOCKET_PATH "/tmp/.ziti"
Copy link
Member

Choose a reason for hiding this comment

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

SOCKET_PATH is used in at least one #ifdef. Changing it from a macro to a variable inadvertently eviscerates make_socket_path()

uv_fs_t fs;
int rc = uv_fs_scandir(uv_default_loop(), &fs, SOCKET_PATH, 0, NULL);
if (rc < 0) {
ZITI_LOG(ERROR, "failed to scan dir[%s]: %d/%s", ipc_base, rc, uv_strerror(rc));
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be debug or lower

initialize_instance_config(config_dir);

model_list ipc_list = {0};
size_t other_zets = find_other_zets(&ipc_list, SOCKET_PATH, sockfilebase);
Copy link
Member

Choose a reason for hiding this comment

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

find_other_zets ends up being called twice. First in configure_ipc, and then again here.

model_list_append(ipcs, strdup(file.name));
}
}
return model_list_size(ipcs);
Copy link
Member

Choose a reason for hiding this comment

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

The default socket files in /tmp/.ziti are not removed when zet exits on linux/darwin, which makes the list size one even if there is no running zet.

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.

3 participants