Willy,
Am 05.03.20 um 19:48 schrieb Willy Tarreau:
> On Thu, Mar 05, 2020 at 05:56:35PM +0100, Tim Duesterhus wrote:
>> @@ -3484,13 +3491,21 @@ stats_error_parsing:
>> }
>>
>> else if (strcmp(args[0], "unique-id-header") == 0) {
>> + char *copy;
>> if (!*(args[1])) {
>> ha_alert("parsing [%s:%d] : %s expects an argument.\n",
>> file, linenum, args[0]);
>> err_code |= ERR_ALERT | ERR_FATAL;
>> goto out;
>> }
>> - free(curproxy->header_unique_id);
>> - curproxy->header_unique_id = strdup(args[1]);
>> + copy = strdup(args[1]);
>> + if (copy == NULL) {
>> + ha_alert("parsing [%s:%d] : failed to allocate memory
>> for unique-id-header\n", file, linenum);
>> + err_code |= ERR_ALERT | ERR_FATAL;
>> + goto out;
>> + }
>> +
>> + istfree(&curproxy->header_unique_id);
>> + curproxy->header_unique_id = ist(copy);
>> }
>
> Not important at all and I'll take it like this, but since you now
> have istdup() you could have simplified it further:
>
> copy = istdup(ist(args[1]));
> if (!isttest(copy) {
> alert();
> }
> istfree(&curproxy->header_unique_id);
> curproxy->header_unique_id = copy;
I was thinking about this, but disliked round-tripping it through a
temporary `ist`. Seeing this now it definitely looks cleaner, though.
Feel free to adjust the patch (unless it's too late already :-) ).
> By the way this pointer swap pattern is particularly interesting because
> it's used a lot in the config parser. It makes me think we could later
> improve that by having :
>
> /* frees ist and replaces it with a copy of str. Returns non-0 on success
> * success or zero on allocation failure with the original left intact.
> */
> int istreplace(struct ist *ist, const char *str)
> {
> struct ist new = istdup(ist(str));
> if (!isttest(new))
> return 0;
> istfree(*ist);
It would be `istfree(ist)`. It takes a pointer after all.
> *ist = new;
> return 1;
> }
>
> Then at various places we could simply do :
>
> if (!istreplace(&curproxy->foo, argv[1])) {
> alert();
> }
>
> I just don't know how many candidate places we have for this :-)
I guess it will become more common the more you convert to `ist`s.
Best regards
Tim Düsterhus
