[PATCH] [BUG] stats: handle POST request params in any order

When enabling/disabling a server with POST to the stats page, the
order of the required params is important: the server name had to be
first. This patch allows to handle those parameters in any order.
---
src/proto_http.c | 45 +++++++++++++++++++++++++--------------------
1 files changed, 25 insertions(+), 20 deletions(-)

diff --git a/src/proto_http.c b/src/proto_http.c
index c71b839..068498f 100644
--- a/src/proto_http.c
+++ b/src/proto_http.c
@@ -2947,6 +2947,7 @@ int http_process_req_stat_post(struct stream_interface *si, struct http_txn *txn
struct proxy *px;
struct server *sv;

+ char *server = NULL;
char *backend = NULL;
int action = 0;

@@ -3017,30 +3018,34 @@ int http_process_req_stat_post(struct stream_interface *si, struct http_txn *txn
}
}
else if (strcmp(key, "s") == 0) {
- if (backend && action && get_backend_server(backend, value, &px, &sv)) {
- switch (action) {
- case 1:
- if ((px->state != PR_STSTOPPED) && !(sv->state & SRV_MAINTAIN)) {
- /* Not already in maintenance, we can change the server state */
- sv->state |= SRV_MAINTAIN;
- set_server_down(sv);
- si->applet.ctx.stats.st_code = STAT_STATUS_DONE;
- }
- break;
- case 2:
- if ((px->state != PR_STSTOPPED) && (sv->state & SRV_MAINTAIN)) {
- /* Already in maintenance, we can change the server state */
- set_server_up(sv);
- sv->health = sv->rise; /* up, but will fall down at first failure */
- si->applet.ctx.stats.st_code = STAT_STATUS_DONE;
- }
- break;
- }
- }
+ server = value;
}
next_param = cur_param;
}
}
+
+ /* Check if we have everything needed to execute an action */
+ if (backend && server && action && get_backend_server(backend, server, &px, &sv)) {
+ switch (action) {
+ case 1:
+ if ((px->state != PR_STSTOPPED) && !(sv->state & SRV_MAINTAIN)) {
+ /* Not already in maintenance, we can change the server state */
+ sv->state |= SRV_MAINTAIN;
+ set_server_down(sv);
+ si->applet.ctx.stats.st_code = STAT_STATUS_DONE;
+ }
+ break;
+ case 2:
+ if ((px->state != PR_STSTOPPED) && (sv->state & SRV_MAINTAIN)) {
+ /* Already in maintenance, we can change the server state */
+ set_server_up(sv);
+ sv->health = sv->rise; /* up, but will fall down at first failure */
+ si->applet.ctx.stats.st_code = STAT_STATUS_DONE;
+ }
+ break;
+ }
+ }
+
return 1;
}

--
1.7.9.1

Hi Vincent,

On Wed, Mar 07, 2012 at 06:16:32PM +0100, Vincent Bernat wrote:
> When enabling/disabling a server with POST to the stats page, the
> order of the required params is important: the server name had to be
> first. This patch allows to handle those parameters in any order.

looks good, will check this tomorrow when I have my eyeballs wider
opened.

Thanks!
Willy

Hi Vincent and Willy,

Le 09/03/2012 01:51, Willy Tarreau a écrit :
> Hi Vincent,
>
> On Wed, Mar 07, 2012 at 06:16:32PM +0100, Vincent Bernat wrote:
>> When enabling/disabling a server with POST to the stats page, the
>> order of the required params is important: the server name had to be
>> first. This patch allows to handle those parameters in any order.
>
> looks good, will check this tomorrow when I have my eyeballs wider
> opened.

I didn't have time to test the patch yet, but for me this will keep some
side effects : depending on the parameters order, this will skip some
servers.

Example :
?s=server1&b=backend&action=disable&s=server2&s=server3

With the patch, I think "server3" will be ignored.

When writing this part of code, the order dependency was not considered
as a bug but as an "optimization" instead :
- the stats page form provides the parameters in that order.
- the query string is parsed from right to left to only remember the
last value for parameters "action" and "b" (in case someone provides
several ones).

If we want to break the order dependency, we should think of a more
complete patch, for example using a 2 pass parsing (1st pass to find
"action" and "b" parameters, 2nd pass to apply them to the servers) :
- to prevent memory allocation
- to take into account every "s" parameters
- of course, it can be optimized to prevent the 2nd pass if the
parameters are already ordered.


--
Cyril Bonté

OoO En ce doux début de matinée du vendredi 09 mars 2012, vers 08:28,
Cyril Bonté <[email protected]> disait :

>> looks good, will check this tomorrow when I have my eyeballs wider
>> opened.

> I didn't have time to test the patch yet, but for me this will keep
> some side effects : depending on the parameters order, this will skip
> some servers.

> Example :
> ?s=server1&b=backend&action=disable&s=server2&s=server3

> With the patch, I think "server3" will be ignored.

Yes, sorry, I didn't consider multiple servers.

> When writing this part of code, the order dependency was not
> considered as a bug but as an "optimization" instead :
> - the stats page form provides the parameters in that order.
> - the query string is parsed from right to left to only remember the
> last value for parameters "action" and "b" (in case someone provides
> several ones).

> If we want to break the order dependency, we should think of a more
> complete patch, for example using a 2 pass parsing (1st pass to find
> "action" and "b" parameters, 2nd pass to apply them to the servers) :
> - to prevent memory allocation
> - to take into account every "s" parameters
> - of course, it can be optimized to prevent the 2nd pass if the
> parameters are already ordered.

Maybe, it's not worth it.
--
Vincent Bernat ☯ http://vincent.bernat.im

die_if_kernel("Whee... Hello Mr. Penguin", current->tss.kregs);
2.2.16 /usr/src/linux/arch/sparc/kernel/traps.c

Hi Vincent and Willy,

Le 09/03/2012 09:07, Vincent Bernat a écrit :
> OoO En ce doux début de matinée du vendredi 09 mars 2012, vers 08:28,
> Cyril Bonté<[email protected]> disait :
>> (...)
>> If we want to break the order dependency, we should think of a more
>> complete patch, for example using a 2 pass parsing (1st pass to find
>> "action" and "b" parameters, 2nd pass to apply them to the servers) :
>> - to prevent memory allocation
>> - to take into account every "s" parameters
>> - of course, it can be optimized to prevent the 2nd pass if the
>> parameters are already ordered.
>
> Maybe, it's not worth it.

I'm taking the action. The code is ready but it still requires some
cleanups before being merged.

I think I will be able to send a patch those next days.

--
Cyril Bonté

Hi Cyril,

On Wed, Mar 28, 2012 at 03:30:12PM +0200, Cyril Bonté wrote:
> Hi Vincent and Willy,
>
> Le 09/03/2012 09:07, Vincent Bernat a écrit :
> >OoO En ce doux début de matinée du vendredi 09 mars 2012, vers 08:28,
> >Cyril Bonté<[email protected]> disait :
> >>(...)
> >>If we want to break the order dependency, we should think of a more
> >>complete patch, for example using a 2 pass parsing (1st pass to find
> >>"action" and "b" parameters, 2nd pass to apply them to the servers) :
> >>- to prevent memory allocation
> >>- to take into account every "s" parameters
> >>- of course, it can be optimized to prevent the 2nd pass if the
> >>parameters are already ordered.
> >
> >Maybe, it's not worth it.
>
> I'm taking the action. The code is ready but it still requires some
> cleanups before being merged.
>
> I think I will be able to send a patch those next days.

Take your time (unless you want some peer review), all this week I'm at
the IETF and won't have time to read it.

Thanks!
Willy