Hi Olivier,
On Mon, Aug 07, 2017 at 07:29:23PM +0200, Olivier Houchard wrote:
> > What makes the distinction between an SRV record and a real hostname here ?
> > Just the leading underscore, or the plain "_http." maybe ? I'm not expecting
> > any problem with this given that the underscore is not allowed as a regular
> > hostname character (except under windows). But at least this will deserve
> > a mention in the doc where the server's address is described, so that anyone
> > experiencing trouble could spot this easily.
> >
>
> Yes, whatever starts with an underscore, I didn't want to rely on _http, as
> it may not be http. As underscores aren't supposed to be present in hostnames,
> and I certainly hope even people using them don't have hostname starting with
> one, I thought it was OK.
> It is documented in the updated patches attached.
OK fine, thanks!
> > > - for (dns_answer_record_id = 0; dns_answer_record_id <
> > > dns_p->header.ancount; dns_answer_record_id++) {
> > > + for (int i = 0; i < dns_p->header.ancount; i++) {
> >
> > "for (int i ...)" will not build here, it's only C99 and we don't enforce
> > this (and personally I hate this form as it's the only exception I know
> > of where a variable declaration is used outside of the scope it's declared
> > in.
> >
>
> Bah, I happen to like it, but I understand C99 is still a bit experimental,
Not experimental, it's in the future... on some of my machines :-)
> so it's gone in the updated patch set.
Thank you.
> > > + char weight[9];
> > > +
> > > + snprintf(weight,
> > > sizeof(weight),
> > > + "%d",
> > > item1->weight);
> > > +
> > > server_parse_weight_change_request(srv, weight);
> >
> >
> > Next time, you can use ultoa() as a replacement for snprintf(str, size,
> > "%d"),
> > which also ensures you'll use a large enough string for the largest
> > supported
> > integer types, there's also U2A() which uses a rotating buffer and which can
> > be used multiple times in a single function call.
> >
>
> Hmm, if we ever want to move to a multithreaded version, I'd rather not use
> ultoa() :)
Good point but no in fact. It's used a lot (via U2A and I don't remember
which one as well) for stats dump because it's convenient. I understood
that the buffers were simply made thread-local instead of global. They're
very small, nobody will notice.
> > I think we'll have to add some easier conversion functions, like unaligned
> > reads using something like this to make such manipulation less of a pain :
> >
> > static inline uint16_t readu16(const void *p)
> > {
> > union { const void *p; uint16_t u16; } __attribute__((packed)) u = {
> > .p = p };
> > return u.u16;
> > }
> >
> > static inline uint16_t readbe16(const void *p)
> > {
> > return ((unsigned char *)p)[0] << 8 +
> > ((unsigned char *)p)[1];
> > }
> >
> > The the code above simply becomes :
> >
> > reader += sizeof(uint16_t);
> > dns_answer_record->weight = readbe16(reader);
> >
> > and for cases where native-endian word is needed instead :
> >
> > reader += sizeof(uint16_t);
> > dns_answer_record->weight = readu16(reader);
> >
>
> Ok you got me convinced gcc can do the right thing for architectures that
> have hard alignment requirements, so as we discussed, the updated patch
> provides those kind of helper functions.
Thank you, this way we may even think about doing some cleanup passes
on other areas before the release :-)
> > > From 899413f3a965feeaa96ffb5b284b504dbdb01fa1 Mon Sep 17 00:00:00 2001
> > > From: Olivier Houchard <ohouch...@haproxy.com>
> > > Date: Fri, 4 Aug 2017 18:39:01 +0200
> > > Subject: [PATCH 4/4] MINOR: check: Fix checks when using SRV records.
> > >
> > > When started, a server may not yet have an associated protocol, so don't
> > > bother trying to run the checks until it is there.
> > > ---
> > > src/checks.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/src/checks.c b/src/checks.c
> > > index 7938b873..fc92a243 100644
> > > --- a/src/checks.c
> > > +++ b/src/checks.c
> > > @@ -1557,7 +1557,7 @@ static int connect_conn_chk(struct task *t)
> > > }
> > >
> > > ret = SF_ERR_INTERNAL;
> > > - if (proto->connect)
> > > + if (proto && proto->connect)
> > > ret = proto->connect(conn, check->type, quickack ? 2 : 0);
> > > if (s->check.send_proxy && !(check->state & CHK_ST_AGENT)) {
> > > conn->send_proxy_ofs = 1;
> >
> > Do you think this one might be triggered before SRV records ? If so
> > we'd need to tag it for backporting. If you're unsure we should also
> > backport it, of course :-)
> >
>
> That is a good question, I think not, but I wouldn't swear it :)
OK, let's not backport it then.
Thanks. I'll wait for Baptiste's ack and will take it if OK.
Willy
