Re: git commit: TS-2863: Enable FQDN selection for shared sessions.

[James Peach <jp...@apache.org>]

Still reviewing, but I wish this had been a number of smaller patches. For example, const'ing the MD5 stuff and adding ~PluginIdentity() could have been independent patches ...

On Jul 24, 2014, at 7:20 PM, amc@apache.org wrote:

> Repository: trafficserver
> Updated Branches:
>  refs/heads/master be428d6f4 -> 0c358a4b1
> 
> 
> TS-2863: Enable FQDN selection for shared sessions.

[snip]

> +bool
> +ServerSessionPool::match(HttpServerSession* ss, sockaddr const* addr, INK_MD5 const& hostname_hash, TSServerSessionSharingMatchType match_style)
> +{
> +  return TS_SERVER_SESSION_SHARING_MATCH_NONE != match_style && // if no matching allowed, fail immediately.
> +    // The hostname matches if we're not checking it or it (and the port!) is a match.
> +    (TS_SERVER_SESSION_SHARING_MATCH_IP == match_style ||  (ats_ip_port_cast(addr) == ats_ip_port_cast(ss->server_ip) && ss->hostname_hash == hostname_hash)) &&
> +    // The IP address matches if we're not checking it or it is a match.
> +    (TS_SERVER_SESSION_SHARING_MATCH_HOST == match_style || ats_ip_addr_port_eq(ss->server_ip, addr))
> +    ;
> +}

The comments help, but IMHO this would still be easier to read in a more declarative style:

switch (match_style) {
case TS_SERVER_SESSION_SHARING_MATCH_IP:
	return ats_ip_port_cast(addr) == ats_ip_port_cast(ss->server_ip) && ss->hostname_hash == hostname_hash;
case TS_SERVER_SESSION_SHARING_MATCH_HOST:
	return ats_ip_addr_port_eq(ss->server_ip, addr);
default:
	ink_assert(match_style == TS_SERVER_SESSION_SHARING_MATCH_NONE);
	return false;
}

Re: git commit: TS-2863: Enable FQDN selection for shared sessions.

["Alan M. Carroll" <am...@network-geographics.com>]

I would like to note that another purpose of the FQDN change is to provide better encapsulation for server session management with an eye to future work in making that more accessible to plugins and other core code.

Re: git commit: TS-2863: Enable FQDN selection for shared sessions.

["Alan M. Carroll" <am...@network-geographics.com>]

James,

Friday, July 25, 2014, 3:59:18 PM, you wrote:

> Still reviewing, but I wish this had been a number of smaller patches. For example, const'ing the MD5 stuff and adding ~PluginIdentity() could have been independent patches ...

Yes, the ~PluginIdentity() is a leak from some other work I missed before I committed. I needed the const on the MD5 stuff, though, to make thing compile.

But, just for fun, I'm about to add another big change that messes around with the MD5 setup anyway.