@cryptotaxi247 / netdata-1 / commits / ae75f49f6

faster execution of external programs (#12759)

* faster invocation of external plugins by eliminating the need for starting /bin/sh and then the command * added missing parameter * prefer the z function * cleanup and clarity - addressed LGTM issue * simplified the popen() interface a bit, to make it more predictable for future uses * removed commented old code * more comments cleanup * mypopen_raw() added for completeness - it is not currently used * simplified the mypopen_raw() interface even further * Update libnetdata/popen/popen.c Co-authored-by: Vladimir Kobal <vlad@prokk.net> * restored 0 flags for netdata_spawn() and cosmetic changes * added more clarity to the code and reverted old behavior of all other execution of commands Co-authored-by: Vladimir Kobal <vlad@prokk.net>

Costa Tsaousis committed Apr 28, 2022 at 18:35 UTC ae75f49f686a73a348396261e1434488f6360bd2
4 files changed +135 -45
collectors/cgroups.plugin/cgroup-network.c
+22 -7
@@ -479,7 +479,17 @@ void call_the_helper(pid_t pid, const char *cgroup) {
479 info("running: %s", command);
480
481 pid_t cgroup_pid;
482 - FILE *fp = mypopene(command, &cgroup_pid, environment);
482 + FILE *fp;
483 +
484 + if(cgroup) {
485 + (void)mypopen_raw_default_flags(&cgroup_pid, environment, &fp, PLUGINS_DIR "/cgroup-network-helper.sh", "--cgroup", cgroup);
486 + }
487 + else {
488 + char buffer[100];
489 + snprintfz(buffer, sizeof(buffer) - 1, "%d", pid);
490 + (void)mypopen_raw_default_flags(&cgroup_pid, environment, &fp, PLUGINS_DIR "/cgroup-network-helper.sh", "--pid", buffer);
491 + }
492 +
493 if(fp) {
494 char buffer[CGROUP_NETWORK_INTERFACE_MAX_LINE + 1];
495 char *s;
@@ -643,8 +653,13 @@ int main(int argc, char **argv) {
653 if(argc != 3)
654 usage();
655
646 - if(!strcmp(argv[1], "-p") || !strcmp(argv[1], "--pid")) {
647 - pid = atoi(argv[2]);
656 + int arg = 1;
657 + int helper = 1;
658 + if (getenv("KUBERNETES_SERVICE_HOST") != NULL && getenv("KUBERNETES_SERVICE_PORT") != NULL)
659 + helper = 0;
660 +
661 + if(!strcmp(argv[arg], "-p") || !strcmp(argv[arg], "--pid")) {
662 + pid = atoi(argv[arg+1]);
663
664 if(pid <= 0) {
665 errno = 0;
@@ -652,17 +667,17 @@ int main(int argc, char **argv) {
667 return 2;
668 }
669
655 - call_the_helper(pid, NULL);
670 + if(helper) call_the_helper(pid, NULL);
671 }
657 - else if(!strcmp(argv[1], "--cgroup")) {
658 - char *cgroup = argv[2];
672 + else if(!strcmp(argv[arg], "--cgroup")) {
673 + char *cgroup = argv[arg+1];
674 if(verify_path(cgroup) == -1) {
675 error("cgroup '%s' does not exist or is not valid.", cgroup);
676 return 1;
677 }
678
679 pid = read_pid_from_cgroup(cgroup);
665 - call_the_helper(pid, cgroup);
680 + if(helper) call_the_helper(pid, cgroup);
681
682 if(pid <= 0 && !detected_devices) {
683 errno = 0;
collectors/cgroups.plugin/sys_fs_cgroup.c
+12 -14
@@ -1475,19 +1475,20 @@ static inline void read_cgroup_network_interfaces(struct cgroup *cg) {
1475 debug(D_CGROUP, "looking for the network interfaces of cgroup '%s' with chart id '%s' and title '%s'", cg->id, cg->chart_id, cg->chart_title);
1476
1477 pid_t cgroup_pid;
1478 - char command[CGROUP_NETWORK_INTERFACE_MAX_LINE + 1];
1478 + char cgroup_identifier[CGROUP_NETWORK_INTERFACE_MAX_LINE + 1];
1479
1480 if(!(cg->options & CGROUP_OPTIONS_IS_UNIFIED)) {
1481 - snprintfz(command, CGROUP_NETWORK_INTERFACE_MAX_LINE, "exec %s --cgroup '%s%s'", cgroups_network_interface_script, cgroup_cpuacct_base, cg->id);
1481 + snprintfz(cgroup_identifier, CGROUP_NETWORK_INTERFACE_MAX_LINE, "%s%s", cgroup_cpuacct_base, cg->id);
1482 }
1483 else {
1484 - snprintfz(command, CGROUP_NETWORK_INTERFACE_MAX_LINE, "exec %s --cgroup '%s%s'", cgroups_network_interface_script, cgroup_unified_base, cg->id);
1484 + snprintfz(cgroup_identifier, CGROUP_NETWORK_INTERFACE_MAX_LINE, "%s%s", cgroup_unified_base, cg->id);
1485 }
1486
1487 - debug(D_CGROUP, "executing command '%s' for cgroup '%s'", command, cg->id);
1488 - FILE *fp = mypopen(command, &cgroup_pid);
1487 + debug(D_CGROUP, "executing cgroup_identifier %s --cgroup '%s' for cgroup '%s'", cgroups_network_interface_script, cgroup_identifier, cg->id);
1488 + FILE *fp;
1489 + (void)mypopen_raw_default_flags_and_environment(&cgroup_pid, &fp, cgroups_network_interface_script, "--cgroup", cgroup_identifier);
1490 if(!fp) {
1490 - error("CGROUP: cannot popen(\"%s\", \"r\").", command);
1491 + error("CGROUP: cannot popen(%s --cgroup \"%s\", \"r\").", cgroups_network_interface_script, cgroup_identifier);
1492 return;
1493 }
1494
@@ -1528,7 +1529,7 @@ static inline void read_cgroup_network_interfaces(struct cgroup *cg) {
1529 }
1530
1531 mypclose(fp, cgroup_pid);
1531 - // debug(D_CGROUP, "closed command for cgroup '%s'", cg->id);
1532 + // debug(D_CGROUP, "closed cgroup_identifier for cgroup '%s'", cg->id);
1533 }
1534
1535 static inline void free_cgroup_network_interfaces(struct cgroup *cg) {
@@ -1614,13 +1615,10 @@ static inline void cgroup_get_chart_name(struct cgroup *cg) {
1615 debug(D_CGROUP, "looking for the name of cgroup '%s' with chart id '%s' and title '%s'", cg->id, cg->chart_id, cg->chart_title);
1616
1617 pid_t cgroup_pid;
1617 - char command[CGROUP_CHARTID_LINE_MAX + 1];
1618 -
1618 // TODO: use cg->id when the renaming script is fixed
1620 - snprintfz(command, CGROUP_CHARTID_LINE_MAX, "exec %s '%s'", cgroups_rename_script, cg->intermediate_id);
1621 -
1622 - debug(D_CGROUP, "executing command \"%s\" for cgroup '%s'", command, cg->chart_id);
1623 - FILE *fp = mypopen(command, &cgroup_pid);
1619 + debug(D_CGROUP, "executing command %s \"%s\" for cgroup '%s'", cgroups_rename_script, cg->intermediate_id, cg->chart_id);
1620 + FILE *fp;
1621 + (void)mypopen_raw_default_flags_and_environment(&cgroup_pid, &fp, cgroups_rename_script, cg->intermediate_id);
1622 if(fp) {
1623 // debug(D_CGROUP, "reading from command '%s' for cgroup '%s'", command, cg->id);
1624 char buffer[CGROUP_CHARTID_LINE_MAX + 1];
@@ -1661,7 +1659,7 @@ static inline void cgroup_get_chart_name(struct cgroup *cg) {
1659 }
1660 }
1661 else
1664 - error("CGROUP: cannot popen(\"%s\", \"r\").", command);
1662 + error("CGROUP: cannot popen(%s \"%s\", \"r\").", cgroups_rename_script, cg->intermediate_id);
1663 }
1664
1665 static inline struct cgroup *cgroup_add(const char *id) {
libnetdata/popen/popen.c
+84 -24
@@ -78,28 +78,34 @@ static void myp_del(pid_t pid) {
78 #define PIPE_READ 0
79 #define PIPE_WRITE 1
80
81 -/* custom_popene flag definitions */
82 -#define FLAG_CREATE_PIPE 1 // Create a pipe like popen() when set, otherwise set stdout to /dev/null
83 -#define FLAG_CLOSE_FD 2 // Close all file descriptors other than STDIN_FILENO, STDOUT_FILENO, STDERR_FILENO
81 +static inline void convert_argv_to_string(char *dst, size_t size, const char *spawn_argv[]) {
82 + int i;
83 + for(i = 0; spawn_argv[i] ;i++) {
84 + if(i == 0) snprintfz(dst, size, "%s", spawn_argv[i]);
85 + else {
86 + size_t len = strlen(dst);
87 + snprintfz(&dst[len], size - len, " '%s'", spawn_argv[i]);
88 + }
89 + }
90 +}
91
92 /*
86 - * Returns -1 on failure, 0 on success. When FLAG_CREATE_PIPE is set, on success set the FILE *fp pointer.
93 + * Returns -1 on failure, 0 on success. When POPEN_FLAG_CREATE_PIPE is set, on success set the FILE *fp pointer.
94 */
88 -static inline int custom_popene(const char *command, volatile pid_t *pidptr, char **env, uint8_t flags, FILE **fpp) {
95 +static int custom_popene(volatile pid_t *pidptr, char **env, uint8_t flags, FILE **fpp, const char *command, const char *spawn_argv[]) {
96 + // create a string to be logged about the command we are running
97 + char command_to_be_logged[2048];
98 + convert_argv_to_string(command_to_be_logged, sizeof(command_to_be_logged), spawn_argv);
99 + // info("custom_popene() running command: %s", command_to_be_logged);
100 +
101 FILE *fp = NULL;
102 int ret = 0; // success by default
103 int pipefd[2], error;
104 pid_t pid;
93 - char *const spawn_argv[] = {
94 - "sh",
95 - "-c",
96 - (char *)command,
97 - NULL
98 - };
105 posix_spawnattr_t attr;
106 posix_spawn_file_actions_t fa;
107
102 - if (flags & FLAG_CREATE_PIPE) {
108 + if (flags & POPEN_FLAG_CREATE_PIPE) {
109 if (pipe(pipefd) == -1)
110 return -1;
111 if ((fp = fdopen(pipefd[PIPE_READ], "r")) == NULL) {
@@ -107,7 +113,7 @@ static inline int custom_popene(const char *command, volatile pid_t *pidptr, cha
113 }
114 }
115
110 - if (flags & FLAG_CLOSE_FD) {
116 + if (flags & POPEN_FLAG_CLOSE_FD) {
117 // Mark all files to be closed by the exec() stage of posix_spawn()
118 int i;
119 for (i = (int) (sysconf(_SC_OPEN_MAX) - 1); i >= 0; i--) {
@@ -117,7 +123,7 @@ static inline int custom_popene(const char *command, volatile pid_t *pidptr, cha
123 }
124
125 if (!posix_spawn_file_actions_init(&fa)) {
120 - if (flags & FLAG_CREATE_PIPE) {
126 + if (flags & POPEN_FLAG_CREATE_PIPE) {
127 // move the pipe to stdout in the child
128 if (posix_spawn_file_actions_adddup2(&fa, pipefd[PIPE_WRITE], STDOUT_FILENO)) {
129 error("posix_spawn_file_actions_adddup2() failed");
@@ -150,22 +156,22 @@ static inline int custom_popene(const char *command, volatile pid_t *pidptr, cha
156 // Take the lock while we fork to ensure we don't race with SIGCHLD
157 // delivery on a process which exits quickly.
158 myp_add_lock();
153 - if (!posix_spawn(&pid, "/bin/sh", &fa, &attr, spawn_argv, env)) {
159 + if (!posix_spawn(&pid, command, &fa, &attr, (char * const*)spawn_argv, env)) {
160 *pidptr = pid;
161 myp_add_locked(pid);
156 - debug(D_CHILDS, "Spawned command: '%s' on pid %d from parent pid %d.", command, pid, getpid());
162 + debug(D_CHILDS, "Spawned command: \"%s\" on pid %d from parent pid %d.", command_to_be_logged, pid, getpid());
163 } else {
164 myp_add_unlock();
159 - error("Failed to spawn command: '%s' from parent pid %d.", command, getpid());
160 - if (flags & FLAG_CREATE_PIPE) {
165 + error("Failed to spawn command: \"%s\" from parent pid %d.", command_to_be_logged, getpid());
166 + if (flags & POPEN_FLAG_CREATE_PIPE) {
167 fclose(fp);
168 }
169 ret = -1;
170 }
165 - if (flags & FLAG_CREATE_PIPE) {
171 + if (flags & POPEN_FLAG_CREATE_PIPE) {
172 close(pipefd[PIPE_WRITE]);
173 if (0 == ret) // on success set FILE * pointer
168 - *fpp = fp;
174 + if(fpp) *fpp = fp;
175 }
176
177 if (!error) {
@@ -181,8 +187,9 @@ static inline int custom_popene(const char *command, volatile pid_t *pidptr, cha
187 error_after_posix_spawn_file_actions_init:
188 if (posix_spawn_file_actions_destroy(&fa))
189 error("posix_spawn_file_actions_destroy");
190 +
191 error_after_pipe:
185 - if (flags & FLAG_CREATE_PIPE) {
192 + if (flags & POPEN_FLAG_CREATE_PIPE) {
193 if (fp)
194 fclose(fp);
195 else
@@ -193,6 +200,41 @@ error_after_pipe:
200 return -1;
201 }
202
203 +int custom_popene_variadic_internal_dont_use_directly(volatile pid_t *pidptr, char **env, uint8_t flags, FILE **fpp, const char *command, ...) {
204 + // convert the variable list arguments into what posix_spawn() needs
205 + // all arguments are expected strings
206 + va_list args;
207 + int args_count;
208 +
209 + // count the number variable parameters
210 + // the variable parameters are expected NULL terminated
211 + {
212 + const char *s;
213 +
214 + va_start(args, command);
215 + args_count = 0;
216 + while ((s = va_arg(args, const char *))) args_count++;
217 + va_end(args);
218 + }
219 +
220 + // create a string pointer array as needed by posix_spawn()
221 + // variable array in the stack
222 + const char *spawn_argv[args_count + 1];
223 + {
224 + const char *s;
225 + va_start(args, command);
226 + int i;
227 + for (i = 0; i < args_count; i++) {
228 + s = va_arg(args, const char *);
229 + spawn_argv[i] = s;
230 + }
231 + spawn_argv[args_count] = NULL;
232 + va_end(args);
233 + }
234 +
235 + return custom_popene(pidptr, env, flags, fpp, command, spawn_argv);
236 +}
237 +
238 // See man environ
239 extern char **environ;
240
@@ -252,19 +294,37 @@ int myp_reap(pid_t pid) {
294
295 FILE *mypopen(const char *command, volatile pid_t *pidptr) {
296 FILE *fp = NULL;
255 - (void)custom_popene(command, pidptr, environ, FLAG_CREATE_PIPE | FLAG_CLOSE_FD, &fp);
297 + const char *spawn_argv[] = {
298 + "sh",
299 + "-c",
300 + command,
301 + NULL
302 + };
303 + (void)custom_popene(pidptr, environ, POPEN_FLAG_CREATE_PIPE|POPEN_FLAG_CLOSE_FD, &fp, "/bin/sh", spawn_argv);
304 return fp;
305 }
306
307 FILE *mypopene(const char *command, volatile pid_t *pidptr, char **env) {
308 FILE *fp = NULL;
261 - (void)custom_popene(command, pidptr, env, FLAG_CREATE_PIPE | FLAG_CLOSE_FD, &fp);
309 + const char *spawn_argv[] = {
310 + "sh",
311 + "-c",
312 + command,
313 + NULL
314 + };
315 + (void)custom_popene( pidptr, env, POPEN_FLAG_CREATE_PIPE|POPEN_FLAG_CLOSE_FD, &fp, "/bin/sh", spawn_argv);
316 return fp;
317 }
318
319 // returns 0 on success, -1 on failure
320 int netdata_spawn(const char *command, volatile pid_t *pidptr) {
267 - return custom_popene(command, pidptr, environ, 0, NULL);
321 + const char *spawn_argv[] = {
322 + "sh",
323 + "-c",
324 + command,
325 + NULL
326 + };
327 + return custom_popene( pidptr, environ, POPEN_FLAG_NONE, NULL, "/bin/sh", spawn_argv);
328 }
329
330 int custom_pclose(FILE *fp, pid_t pid) {
libnetdata/popen/popen.h
+17
@@ -8,6 +8,23 @@
8 #define PIPE_READ 0
9 #define PIPE_WRITE 1
10
11 +/* custom_popene_variadic_internal_dont_use_directly flag definitions */
12 +#define POPEN_FLAG_NONE 0
13 +#define POPEN_FLAG_CREATE_PIPE 1 // Create a pipe like popen() when set, otherwise set stdout to /dev/null
14 +#define POPEN_FLAG_CLOSE_FD 2 // Close all file descriptors other than STDIN_FILENO, STDOUT_FILENO, STDERR_FILENO
15 +
16 +// the flags to be used by default
17 +#define POPEN_FLAGS_DEFAULT (POPEN_FLAG_CREATE_PIPE|POPEN_FLAG_CLOSE_FD)
18 +
19 +// mypopen_raw is the interface to use instead of custom_popene_variadic_internal_dont_use_directly()
20 +// mypopen_raw will add the terminating NULL at the arguments list
21 +// we append the parameter 'command' twice - this is because the underlying call needs the command to execute and the argv[0] to pass to it
22 +#define mypopen_raw_default_flags_and_environment(pidptr, fpp, command, args...) custom_popene_variadic_internal_dont_use_directly(pidptr, environ, POPEN_FLAGS_DEFAULT, fpp, command, command, ##args, NULL)
23 +#define mypopen_raw_default_flags(pidptr, env, fpp, command, args...) custom_popene_variadic_internal_dont_use_directly(pidptr, env, POPEN_FLAGS_DEFAULT, fpp, command, command, ##args, NULL)
24 +#define mypopen_raw(pidptr, env, flags, fpp, command, args...) custom_popene_variadic_internal_dont_use_directly(pidptr, env, flags, fpp, command, command, ##args, NULL)
25 +
26 +extern int custom_popene_variadic_internal_dont_use_directly(volatile pid_t *pidptr, char **env, uint8_t flags, FILE **fpp, const char *command, ...);
27 +
28 extern FILE *mypopen(const char *command, volatile pid_t *pidptr);
29 extern FILE *mypopene(const char *command, volatile pid_t *pidptr, char **env);
30 extern int mypclose(FILE *fp, pid_t pid);