We would like to propose the attached patch.
It is inspired by the several iterations of changing the respective checks.
use macros for the code that checks for command and virtual host sameness
That code consists of several lines which were duplicated in several
places. Now the code is slightly more compact and all the logic is
in a single place.
We use macros as opposed to inline functions because we use them with
different argument types. The only requirement on the types is that
the field names must follow a common convention.
Possibly the next logical step could be to introduce a new struct that would
hold all the ID fields and then use that struct as a member in all other structs
that currently keep the ID information as an assortment of the fields.
In other words, to make the following public (and with a better name):
+ struct last_id {
+ apr_ino_t inode;
+ apr_dev_t deviceid;
+ const char *cmdline;
+ gid_t gid;
+ uid_t uid;
+ int vhost_id;
+ } last_id;
--
Andriy Gapon
commit 60b1c2d2662da6ecc04c03b99c485292f4c3df50
Author: Andriy Gapon <[email protected]>
Date: Mon Sep 2 14:54:11 2013 +0300
use macros for the code that checks for command and virtual host sameness
That code consists of several lines which were duplicated in several
places. Now the code is slightly more compact and all the logic is
in a single place.
We use macros as opposed to inline functions because we use them with
different argument types. The only requirement on the types is that
the field names must follow a common convention.
diff --git a/modules/fcgid/fcgid_bridge.c b/modules/fcgid/fcgid_bridge.c
index f863008..adf355b 100644
--- a/modules/fcgid/fcgid_bridge.c
+++ b/modules/fcgid/fcgid_bridge.c
@@ -56,11 +56,8 @@ static fcgid_procnode *apply_free_procnode(request_rec *r,
while (current_node != proc_table) {
next_node = &proc_table[current_node->next_index];
- if (current_node->inode == inode
- && current_node->deviceid == deviceid
- && !strcmp(current_node->cmdline, cmdline)
- && current_node->vhost_id == command->vhost_id
- && current_node->uid == uid && current_node->gid == gid) {
+ if (IS_SAME_COMMAND(current_node, command)
+ && IS_SAME_VHOST(current_node, command)) {
/* Unlink from idle list */
previous_node->next_index = current_node->next_index;
@@ -136,12 +133,8 @@ static int count_busy_processes(request_rec *r,
fcgid_command *command)
previous_node = busy_list_header;
current_node = &proc_table[previous_node->next_index];
while (current_node != proc_table) {
- if (current_node->inode == command->inode
- && current_node->deviceid == command->deviceid
- && !strcmp(current_node->cmdline, command->cmdline)
- && current_node->vhost_id == command->vhost_id
- && current_node->uid == command->uid
- && current_node->gid == command->gid) {
+ if (IS_SAME_COMMAND(current_node, command)
+ && IS_SAME_VHOST(current_node, command)) {
result++;
}
next_node = &proc_table[current_node->next_index];
diff --git a/modules/fcgid/fcgid_global.h b/modules/fcgid/fcgid_global.h
index d52a2fb..1ae0745 100644
--- a/modules/fcgid/fcgid_global.h
+++ b/modules/fcgid/fcgid_global.h
@@ -57,4 +57,14 @@ APLOG_USE_MODULE(fcgid);
#define fcgid_min(a,b) (((a) < (b)) ? (a) : (b))
+#define IS_SAME_COMMAND(x, y) \
+ ((x)->inode == (y)->inode \
+ && (x)->deviceid == (y)->deviceid \
+ && !strcmp((x)->cmdline, (y)->cmdline) \
+ && (x)->uid == (y)->uid \
+ && (x)->gid == (y)->gid)
+
+#define IS_SAME_VHOST(x, y) \
+ ((x)->vhost_id == (y)->vhost_id)
+
#endif
diff --git a/modules/fcgid/fcgid_spawn_ctl.c b/modules/fcgid/fcgid_spawn_ctl.c
index 2d0b39c..1b561dc 100644
--- a/modules/fcgid/fcgid_spawn_ctl.c
+++ b/modules/fcgid/fcgid_spawn_ctl.c
@@ -58,12 +58,8 @@ register_life_death(server_rec * main_server,
previous_node = g_stat_list_header;
for (current_node = previous_node;
current_node != NULL; current_node = current_node->next) {
- if (current_node->inode == procnode->inode
- && current_node->deviceid == procnode->deviceid
- && !strcmp(current_node->cmdline, procnode->cmdline)
- && current_node->vhost_id == procnode->vhost_id
- && current_node->uid == procnode->uid
- && current_node->gid == procnode->gid)
+ if (IS_SAME_COMMAND(current_node, procnode)
+ && IS_SAME_VHOST(current_node, procnode))
break;
previous_node = current_node;
}
@@ -175,12 +171,8 @@ int is_spawn_allowed(server_rec * main_server,
fcgid_command * command)
/* Can I find the node base on inode, device id and cmdline? */
for (current_node = g_stat_list_header;
current_node != NULL; current_node = current_node->next) {
- if (current_node->inode == command->inode
- && current_node->deviceid == command->deviceid
- && !strcmp(current_node->cmdline, command->cmdline)
- && current_node->vhost_id == command->vhost_id
- && current_node->uid == command->uid
- && current_node->gid == command->gid)
+ if (IS_SAME_COMMAND(current_node, command)
+ && IS_SAME_VHOST(current_node, command))
break;
}
@@ -234,12 +226,8 @@ int is_kill_allowed(server_rec * main_server,
fcgid_procnode * procnode)
/* Can I find the node base on inode, device id and cmdline? */
for (current_node = g_stat_list_header;
current_node != NULL; current_node = current_node->next) {
- if (current_node->inode == procnode->inode
- && current_node->deviceid == procnode->deviceid
- && !strcmp(current_node->cmdline, procnode->cmdline)
- && current_node->vhost_id == procnode->vhost_id
- && current_node->uid == procnode->uid
- && current_node->gid == procnode->gid)
+ if (IS_SAME_COMMAND(current_node, procnode)
+ && IS_SAME_VHOST(current_node, procnode))
break;
}
diff --git a/modules/fcgid/mod_fcgid.c b/modules/fcgid/mod_fcgid.c
index 0b3d26c..3bc9a41 100644
--- a/modules/fcgid/mod_fcgid.c
+++ b/modules/fcgid/mod_fcgid.c
@@ -340,13 +340,15 @@ static int fcgid_status_hook(request_rec *r, int flags)
{
fcgid_procnode **ar = NULL, *current_node;
int num_ent, index;
- apr_ino_t last_inode = 0;
- apr_dev_t last_deviceid = 0;
- gid_t last_gid = 0;
- uid_t last_uid = 0;
- const char *last_cmdline = "";
+ struct last_id {
+ apr_ino_t inode;
+ apr_dev_t deviceid;
+ const char *cmdline;
+ gid_t gid;
+ uid_t uid;
+ int vhost_id;
+ } last_id;
apr_time_t now;
- int last_vhost_id = -1;
const char *basename, *tmpbasename;
fcgid_procnode *proc_table = proctable_get_table_array();
fcgid_procnode *error_list_header = proctable_get_error_list();
@@ -411,15 +413,18 @@ static int fcgid_status_hook(request_rec *r, int flags)
qsort((void *)ar, num_ent, sizeof(fcgid_procnode *),
(int (*)(const void *, const void *))fcgidsort);
+ /* Ensure that last_id doesn't match the first entry */
+ last_id.inode = 0;
+ last_id.cmdline = "";
+ last_id.vhost_id = -1;
+
/* Output */
ap_rputs("<hr />\n<h1>mod_fcgid status:</h1>\n", r);
ap_rprintf(r, "Total FastCGI processes: %d\n", num_ent);
for (index = 0; index < num_ent; index++) {
current_node = ar[index];
- if (current_node->inode != last_inode || current_node->deviceid !=
last_deviceid
- || current_node->gid != last_gid || current_node->uid != last_uid
- || strcmp(current_node->cmdline, last_cmdline)
- || current_node->vhost_id != last_vhost_id) {
+ if (!IS_SAME_VHOST(current_node, &last_id)
+ || !IS_SAME_COMMAND(current_node, &last_id)) {
if (index != 0)
ap_rputs("</table>\n\n", r);
@@ -441,12 +446,12 @@ static int fcgid_status_hook(request_rec *r, int flags)
"<th>Accesses</th><th>State</th>"
"</tr>\n", r);
- last_inode = current_node->inode;
- last_deviceid = current_node->deviceid;
- last_gid = current_node->gid;
- last_uid = current_node->uid;
- last_cmdline = current_node->cmdline;
- last_vhost_id = current_node->vhost_id;
+ last_id.inode = current_node->inode;
+ last_id.deviceid = current_node->deviceid;
+ last_id.gid = current_node->gid;
+ last_id.uid = current_node->uid;
+ last_id.cmdline = current_node->cmdline;
+ last_id.vhost_id = current_node->vhost_id;
}
ap_rprintf(r, "<tr><td>%" APR_PID_T_FMT "</td><td>%" APR_TIME_T_FMT
"</td><td>%" APR_TIME_T_FMT "</td><td>%d</td><td>%s</td></tr>",
