On Mon, Dec 24, 2012 at 07:58:13AM +0100, Willy Tarreau wrote: > Hi Simon, > > I have some minor comments below for this patch : > > On Mon, Dec 24, 2012 at 10:33:54AM +0900, Simon Horman wrote: > > +static int stats_sock_parse_weight_change_request(struct stream_interface > > *si, > > + struct server *sv, const char > > *weight_str) > > Since this function will be processing weights from various sources, better > place it into server.c and name it "server_parse_weight_change_request" (for > example).
Sure, that sounds reasonable. > > +{ > > + const char *warning; > > + unsigned int status; > > + struct proxy *px; > > + int w; > > + > > + px = sv->proxy; > > + > > + /* if the weight is terminated with '%', it is set relative to > > + * the initial weight, otherwise it is absolute. > > + */ > > + if (!*weight_str) { > > + warning = "Require <weight> or <weight%>.\n"; > > + status = STAT_CLI_PRINT; > > + goto err; > > + } > > + > > + w = atoi(weight_str); > > + if (strchr(weight_str, '%') != NULL) { > > + if (w < 0 || w > 100) { > > + warning = "Relative weight must be positive.\n"; > > + status = STAT_CLI_PRINT; > > + goto err; > > + } > > + w = sv->iweight * w / 100; > > + } > > + else { > > + if (w < 0 || w > 256) { > > + warning = "Absolute weight can only be between 0 and > > 256 inclusive.\n"; > > + status = STAT_CLI_PRINT; > > + goto err; > > + } > > + } > > + > > + if (w && w != sv->iweight && !(px->lbprm.algo & BE_LB_PROP_DYN)) { > > + warning = "Backend is using a static LB algorithm and only > > accepts weights '0%' and '100%'.\n"; > > + status = STAT_CLI_PRINT; > > + goto err; > > + } > > + > > + sv->uweight = w; > > + > > + if (px->lbprm.algo & BE_LB_PROP_DYN) { > > + /* we must take care of not pushing the server to full throttle during > > slow starts */ > > + if ((sv->state & SRV_WARMINGUP) && (px->lbprm.algo & > > BE_LB_PROP_DYN)) > > + sv->eweight = (BE_WEIGHT_SCALE * (now.tv_sec - > > sv->last_change) + sv->slowstart - 1) / sv->slowstart; > > + else > > + sv->eweight = BE_WEIGHT_SCALE; > > + sv->eweight *= sv->uweight; > > + } else { > > + sv->eweight = sv->uweight; > > + } > > + > > + /* static LB algorithms are a bit harder to update */ > > + if (px->lbprm.update_server_eweight) > > + px->lbprm.update_server_eweight(sv); > > + else if (sv->eweight) { > > + if (px->lbprm.set_server_status_up) > > + px->lbprm.set_server_status_up(sv); > > + } > > + else { > > + if (px->lbprm.set_server_status_down) > > + px->lbprm.set_server_status_down(sv); > > + } > > + > > + return 0; > > + > > +err: > > + if (si) { > > + si->applet.ctx.cli.msg = warning; > > + si->applet.st0 = status; > > + } else { > > + Warning("%s\n", warning); > > It would be better not to emit a warning here, because it's not very > appropriate to do that when that can be controlled by possibly untrusted > servers, and it doesn't indicate what server caused the issue. > > In the end, in order for the function not to depend on its source, > just have it return a pointer to the warning message in case of error, > or NULL if no error was encountered. Thus the caller can do whatever > it wants with it. dumpstats will set the state to STAT_CLI_PRINT and > the health check code will probably emit a log which will clearly > indicate the proxy and server's name. I think that should work out quite well. I'll update the code accordingly. > > + } > > + return 1; > > + > > +} > > + > > +int process_weight_change_request(struct server *sv, const char > > *weight_str) > > +{ > > + stats_sock_parse_weight_change_request(NULL, sv, weight_str); > > +} > > Warning, missing return in a non-void function. Thanks, I'll fix that up. > > /* Processes the stats interpreter on the statistics socket. This function > > is > > * called from an applet running in a stream interface. The function > > returns 1 > > * if the request was understood, otherwise zero. It sets si->applet.st0 > > to a value > > @@ -1089,72 +1174,13 @@ static int stats_sock_parse_request(struct > > stream_interface *si, char *line) > > } > > else if (strcmp(args[0], "set") == 0) { > > if (strcmp(args[1], "weight") == 0) { > > - struct proxy *px; > > struct server *sv; > > - int w; > > > > sv = expect_server_admin(s, si, args[2]); > > if (!sv) > > return 1; > > - px = sv->proxy; > > - > > - /* if the weight is terminated with '%', it is set > > relative to > > - * the initial weight, otherwise it is absolute. > > - */ > > - if (!*args[3]) { > > - si->applet.ctx.cli.msg = "Require <weight> or > > <weight%>.\n"; > > - si->applet.st0 = STAT_CLI_PRINT; > > - return 1; > > - } > > - > > - w = atoi(args[3]); > > - if (strchr(args[3], '%') != NULL) { > > - if (w < 0 || w > 100) { > > - si->applet.ctx.cli.msg = "Relative > > weight can only be set between 0 and 100% inclusive.\n"; > > - si->applet.st0 = STAT_CLI_PRINT; > > - return 1; > > - } > > - w = sv->iweight * w / 100; > > - } > > - else { > > - if (w < 0 || w > 256) { > > - si->applet.ctx.cli.msg = "Absolute > > weight can only be between 0 and 256 inclusive.\n"; > > - si->applet.st0 = STAT_CLI_PRINT; > > - return 1; > > - } > > - } > > - > > - if (w && w != sv->iweight && !(px->lbprm.algo & > > BE_LB_PROP_DYN)) { > > - si->applet.ctx.cli.msg = "Backend is using a > > static LB algorithm and only accepts weights '0%' and '100%'.\n"; > > - si->applet.st0 = STAT_CLI_PRINT; > > - return 1; > > - } > > - > > - sv->uweight = w; > > - > > - if (px->lbprm.algo & BE_LB_PROP_DYN) { > > - /* we must take care of not pushing the server to full > > throttle during slow starts */ > > - if ((sv->state & SRV_WARMINGUP) && > > (px->lbprm.algo & BE_LB_PROP_DYN)) > > - sv->eweight = (BE_WEIGHT_SCALE * > > (now.tv_sec - sv->last_change) + sv->slowstart - 1) / sv->slowstart; > > - else > > - sv->eweight = BE_WEIGHT_SCALE; > > - sv->eweight *= sv->uweight; > > - } else { > > - sv->eweight = sv->uweight; > > - } > > - > > - /* static LB algorithms are a bit harder to update */ > > - if (px->lbprm.update_server_eweight) > > - px->lbprm.update_server_eweight(sv); > > - else if (sv->eweight) { > > - if (px->lbprm.set_server_status_up) > > - px->lbprm.set_server_status_up(sv); > > - } > > - else { > > - if (px->lbprm.set_server_status_down) > > - px->lbprm.set_server_status_down(sv); > > - } > > > > + stats_sock_parse_weight_change_request(si, sv, args[3]); > > return 1; > > } > > else if (strcmp(args[1], "timeout") == 0) { > > -- > > 1.7.10.4 > > Regards, > Willy > >