Hi Willy,
Thanks for the review!!!
I fixed most of the problems, but I have a 3 points I'd like to discuss:
> + If an IP address can be found, it is stored into <var>. If any kind of
> > + error occurs, then <var> is not set.
>
> Just to be sure, it is not set or not modified ? I guess the latter, which
> is fine.
>
Yes, not set. So '-m found' can be used.
> > + struct sample *smp;
> > +
> > + conn_get_from_addr(cli_conn);
> > +
> > + smp = sample_fetch_as_type(px, sess, s,
> SMP_OPT_DIR_REQ|SMP_OPT_FINAL, rule->arg.dns.expr, SMP_T_STR);
> > + if (smp) {
> > + char *fqdn;
> > +
> > + fqdn = smp->data.u.str.area;
> > + if (action_prepare_for_resolution(s, fqdn) == -1) {
> > + ha_alert("Can't create DNS resolution for
> server 'http request action'\n");
>
> Please don't send runtime alerts. We've tried hard to clean them up so
> that they remain only during startup.
>
In this function, I have a proxy structure. Should I use send_log() on it
to report the error?
> > + case ACT_HTTP_DO_RESOLVE:
> > case ACT_CUSTOM:
> > if ((s->req.flags & CF_READ_ERROR) ||
> > ((s->req.flags & (CF_SHUTR|CF_READ_NULL)) &&
>
> Suddenly that makes me wonder : why is it needed to have a dedicated
> action since it uses the generic infrastructure with ACT_CUSTOM ?
>
I think this must have been one of the first thing I did during my
development phase so I would be able to "isolate" my code when needed.
Now you said it, and I step back a bit, I also consider there is no value
in this action, appart being clear on the action name and gives us the
ability to be very cautious if we update the behavior of ACT_CUSTOM in the
future.
I can remove ACT_HTTP_DO_RESOLVE and add a comment in ACT_CUSTOM saying
that the do-resolve action relies on this code, just in case.
Baptiste
