Re: git commit: TS-2088: Change TSRecordType enum values to powers of two

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

On Jan 20, 2014, at 11:56 AM, sorber@apache.org wrote:

> Updated Branches:
>  refs/heads/5.0.x db67432fc -> f32e013d3
> 
> 
> TS-2088: Change TSRecordType enum values to powers of two

This should be documented, preferably in a man page, at minimum in the header file. It would be helpful for the commit message to also describe the reason for this change, and the change to RecDumpRecords().

> 
> Project: http://git-wip-us.apache.org/repos/asf/trafficserver/repo
> Commit: http://git-wip-us.apache.org/repos/asf/trafficserver/commit/f32e013d
> Tree: http://git-wip-us.apache.org/repos/asf/trafficserver/tree/f32e013d
> Diff: http://git-wip-us.apache.org/repos/asf/trafficserver/diff/f32e013d
> 
> Branch: refs/heads/5.0.x
> Commit: f32e013d3b13f84177db8de18bc4710fd1eb033e
> Parents: db67432
> Author: Phil Sorber <so...@apache.org>
> Authored: Mon Jan 20 12:51:05 2014 -0700
> Committer: Phil Sorber <so...@apache.org>
> Committed: Mon Jan 20 12:55:08 2014 -0700
> 
> ----------------------------------------------------------------------
> CHANGES                 |  4 ++++
> lib/records/I_RecDefs.h | 13 +++++++------
> lib/records/RecCore.cc  |  2 +-
> proxy/api/ts/ts.h.in    | 13 +++++++------
> 4 files changed, 19 insertions(+), 13 deletions(-)
> ----------------------------------------------------------------------
> 
> 
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/f32e013d/CHANGES
> ----------------------------------------------------------------------
> diff --git a/CHANGES b/CHANGES
> index 2a0771c..a5d6c3f 100644
> --- a/CHANGES
> +++ b/CHANGES
> @@ -1,4 +1,8 @@
>                                                          -*- coding: utf-8 -*-
> +Changes with Apache Traffic Server 5.0.0
> +
> +  *) [TS-2088] Change TSRecordType enum values to powers of two
> +
> Changes with Apache Traffic Server 4.2.0
> 
>   *) [TS-2467] traffic_shell doesn't work with tcl 8.6.
> 
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/f32e013d/lib/records/I_RecDefs.h
> ----------------------------------------------------------------------
> diff --git a/lib/records/I_RecDefs.h b/lib/records/I_RecDefs.h
> index 5a9c121..de45d0d 100644
> --- a/lib/records/I_RecDefs.h
> +++ b/lib/records/I_RecDefs.h
> @@ -57,12 +57,13 @@ typedef int8_t RecByte;
> enum RecT
> {
>   RECT_NULL = 0,
> -  RECT_CONFIG,
> -  RECT_PROCESS,
> -  RECT_NODE,
> -  RECT_CLUSTER,
> -  RECT_LOCAL,
> -  RECT_PLUGIN,
> +  RECT_CONFIG = 1,
> +  RECT_PROCESS = 2,
> +  RECT_NODE = 4,
> +  RECT_CLUSTER = 8,
> +  RECT_LOCAL = 16,
> +  RECT_PLUGIN = 32,
> +  RECT_ALL = 63,
>   RECT_MAX
> };
> 
> 
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/f32e013d/lib/records/RecCore.cc
> ----------------------------------------------------------------------
> diff --git a/lib/records/RecCore.cc b/lib/records/RecCore.cc
> index 532aecc..2430416 100644
> --- a/lib/records/RecCore.cc
> +++ b/lib/records/RecCore.cc
> @@ -853,7 +853,7 @@ RecDumpRecords(RecT rec_type, RecDumpEntryCb callback, void *edata)

This change makes the use of RecT ambiguous; probably it should be "unsigned rec_type".

>   num_records = g_num_records;
>   for (i = 0; i < num_records; i++) {
>     RecRecord *r = &(g_records[i]);
> -    if ((rec_type == RECT_NULL) || (rec_type == r->rec_type)) {
> +    if ((rec_type == RECT_NULL) || (rec_type & r->rec_type)) {
>       rec_mutex_acquire(&(r->lock));
>       callback(rec_type, edata, r->registered, r->name, r->data_type, &r->data);
>       rec_mutex_release(&(r->lock));
> 
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/f32e013d/proxy/api/ts/ts.h.in
> ----------------------------------------------------------------------
> diff --git a/proxy/api/ts/ts.h.in b/proxy/api/ts/ts.h.in
> index ff025d5..ffaedfb 100644
> --- a/proxy/api/ts/ts.h.in
> +++ b/proxy/api/ts/ts.h.in
> @@ -526,12 +526,13 @@ extern "C"
>   typedef enum
>     {
>       TS_RECORDTYPE_NULL = 0,
> -      TS_RECORDTYPE_CONFIG,
> -      TS_RECORDTYPE_PROCESS,
> -      TS_RECORDTYPE_NODE,
> -      TS_RECORDTYPE_CLUSTER,
> -      TS_RECORDTYPE_LOCAL,
> -      TS_RECORDTYPE_PLUGIN,
> +      TS_RECORDTYPE_CONFIG = 1,
> +      TS_RECORDTYPE_PROCESS = 2,
> +      TS_RECORDTYPE_NODE = 4,
> +      TS_RECORDTYPE_CLUSTER = 8,
> +      TS_RECORDTYPE_LOCAL = 16,
> +      TS_RECORDTYPE_PLUGIN = 32,
> +      TS_RECORDTYPE_ALL = 63,
>       TS_RECORDTYPE_MAX
>     } TSRecordType;

I think that you should bump the SDK version number for this, since it is binary incompatible. Also, please consider whether this change is worth making everyone change their plugins :)

J