Re: [08/12] git commit: TS-2323: separate remap.config parsing from UrlRewrite

[Igor Galić <i....@brainsware.org>]

----- Original Message -----
> TS-2323: separate remap.config parsing from UrlRewrite
> 
> 
> Project: http://git-wip-us.apache.org/repos/asf/trafficserver/repo
> Commit: http://git-wip-us.apache.org/repos/asf/trafficserver/commit/90f923ef
> Tree: http://git-wip-us.apache.org/repos/asf/trafficserver/tree/90f923ef
> Diff: http://git-wip-us.apache.org/repos/asf/trafficserver/diff/90f923ef
> 
> Branch: refs/heads/master
> Commit: 90f923ef36313f74241f9ebe55ceea17de5caac7
> Parents: 3e8668d
> Author: James Peach <jp...@apache.org>
> Authored: Fri Nov 1 10:28:14 2013 -0700
> Committer: James Peach <jp...@apache.org>
> Committed: Tue Nov 5 16:46:35 2013 -0800
> 
> ----------------------------------------------------------------------
>  proxy/http/remap/RemapConfig.cc | 765 ++++++++++++++++++++++++++++++-
>  proxy/http/remap/RemapConfig.h  |  11 +-
>  proxy/http/remap/UrlRewrite.cc  | 860 ++++-------------------------------
>  proxy/http/remap/UrlRewrite.h   |  11 +-
>  4 files changed, 848 insertions(+), 799 deletions(-)
> ----------------------------------------------------------------------
> 
> 
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/90f923ef/proxy/http/remap/RemapConfig.cc
> ----------------------------------------------------------------------
> diff --git a/proxy/http/remap/RemapConfig.cc
> b/proxy/http/remap/RemapConfig.cc
> index 0c6442b..a4d41f2 100644
> --- a/proxy/http/remap/RemapConfig.cc
> +++ b/proxy/http/remap/RemapConfig.cc
> @@ -22,8 +22,39 @@
>   */
>  
>  #include "RemapConfig.h"
> -#include "libts.h"
> +#include "UrlRewrite.h"
> +#include "ReverseProxy.h"
>  #include "HTTP.h"
> +#include "libts.h"
> +#include "ink_cap.h"
> +
> +#define modulePrefix "[ReverseProxy]"
> +
> +/**
> +  Returns the length of the URL.
> +
> +  Will replace the terminator with a '/' if this is a full URL and
> +  there are no '/' in it after the the host.  This ensures that class
> +  URL parses the URL correctly.
> +
> +*/
> +static int
> +UrlWhack(char *toWhack, int *origLength)
> +{
> +  int length = strlen(toWhack);
> +  char *tmp;
> +  *origLength = length;
> +
> +  // Check to see if this a full URL
> +  tmp = strstr(toWhack, "://");
> +  if (tmp != NULL) {
> +    if (strchr(tmp + 3, '/') == NULL) {
> +      toWhack[length] = '/';
> +      length++;
> +    }
> +  }
> +  return length;
> +}
>  
>  /**
>    Cleanup *char[] array - each item in array must be allocated via
> @@ -58,6 +89,33 @@ BUILD_TABLE_INFO::reset()
>    clear_xstr_array(this->argv, sizeof(this->argv) / sizeof(char *));
>  }
>  
> +static const char *
> +process_filter_opt(url_mapping * mp, const BUILD_TABLE_INFO * bti, char *
> errStrBuf, int errStrBufSize)
> +{
> +  acl_filter_rule *rp, **rpp;
> +  const char *errStr = NULL;
> +
> +  if (unlikely(!mp || !bti || !errStrBuf || errStrBufSize <= 0)) {
> +    Debug("url_rewrite", "[process_filter_opt] Invalid argument(s)");
> +    return (const char *) "[process_filter_opt] Invalid argument(s)";
> +  }
> +  for (rp = bti->rules_list; rp; rp = rp->next) {
> +    if (rp->active_queue_flag) {
> +      Debug("url_rewrite", "[process_filter_opt] Add active main filter
> \"%s\" (argc=%d)",
> +            rp->filter_name ? rp->filter_name : "<NULL>", rp->argc);
> +      for (rpp = &mp->filter; *rpp; rpp = &((*rpp)->next));
> +      if ((errStr = remap_validate_filter_args(rpp, (const char **)rp->argv,
> rp->argc, errStrBuf, errStrBufSize)) != NULL)
> +        break;
> +    }
> +  }
> +  if (!errStr && (bti->remap_optflg & REMAP_OPTFLG_ALL_FILTERS) != 0) {
> +    Debug("url_rewrite", "[process_filter_opt] Add per remap filter");
> +    for (rpp = &mp->filter; *rpp; rpp = &((*rpp)->next));
> +    errStr = remap_validate_filter_args(rpp, (const char **)bti->argv,
> bti->argc, errStrBuf, errStrBufSize);
> +  }
> +  return errStr;
> +}
> +
>  static bool
>  is_inkeylist(const char * key, ...)
>  {
> @@ -104,7 +162,7 @@ parse_define_directive(const char * directive,
> BUILD_TABLE_INFO * bti, char * er
>  
>    flg = ((rp = acl_filter_rule::find_byname(bti->rules_list, (const char *)
>    bti->paramv[1])) == NULL) ? true : false;
>    // coverity[alloc_arg]
> -  if ((cstr = remap_validate_filter_args(&rp, bti->argv, bti->argc, errbuf,
> errbufsize)) == NULL && rp) {
> +  if ((cstr = remap_validate_filter_args(&rp, (const char **)bti->argv,
> bti->argc, errbuf, errbufsize)) == NULL && rp) {
>      if (flg) {                  // new filter - add to list
>        Debug("url_rewrite", "[parse_directive] new rule \"%s\" was created",
>        bti->paramv[1]);
>        for (rpp = &bti->rules_list; *rpp; rpp = &((*rpp)->next));
> @@ -223,11 +281,12 @@ remap_parse_directive(BUILD_TABLE_INFO *bti, char *
> errbuf, size_t errbufsize)
>  }
>  
>  const char *
> -remap_validate_filter_args(acl_filter_rule ** rule_pp, char ** argv, int
> argc, char * errStrBuf, size_t errStrBufSize)
> +remap_validate_filter_args(acl_filter_rule ** rule_pp, const char ** argv,
> int argc, char * errStrBuf, size_t errStrBufSize)
>  {
>    acl_filter_rule *rule;
>    unsigned long ul;
> -  char *argptr, tmpbuf[1024];
> +  const char *argptr;
> +  char tmpbuf[1024];
>    src_ip_info_t *ipi;
>    int i, j, m;
>    bool new_rule_flg = false;
> @@ -255,7 +314,7 @@ remap_validate_filter_args(acl_filter_rule ** rule_pp,
> char ** argv, int argc, c
>    }
>  
>    for (i = 0; i < argc; i++) {
> -    if ((ul = remap_check_option(&argv[i], 1, 0, NULL, &argptr)) == 0) {
> +    if ((ul = remap_check_option((const char **)&argv[i], 1, 0, NULL,
> &argptr)) == 0) {
>        Debug("url_rewrite", "[validate_filter_args] Unknow remap option -
>        %s", argv[i]);
>        snprintf(errStrBuf, errStrBufSize, "Unknown option - \"%s\"",
>        argv[i]);
>        errStrBuf[errStrBufSize - 1] = 0;
> @@ -405,7 +464,7 @@ remap_validate_filter_args(acl_filter_rule ** rule_pp,
> char ** argv, int argc, c
>  }
>  
>  unsigned long
> -remap_check_option(char *argv[], int argc, unsigned long findmode, int
> *_ret_idx, char **argptr)
> +remap_check_option(const char ** argv, int argc, unsigned long findmode, int
> *_ret_idx, const char **argptr)
>  {
>    unsigned long ret_flags = 0;
>    int idx = 0;
> @@ -474,3 +533,697 @@ remap_check_option(char *argv[], int argc, unsigned
> long findmode, int *_ret_idx
>      *_ret_idx = idx;
>    return ret_flags;
>  }
> +
> +int
> +remap_load_plugin(const char ** argv, int argc, url_mapping *mp, char
> *errbuf, int errbufsize,
> +    int jump_to_argc, int *plugin_found_at)
> +{
> +  TSRemapInterface ri;
> +  struct stat stat_buf;
> +  remap_plugin_info *pi;
> +  char *c, *err, tmpbuf[2048], default_path[PATH_NAME_MAX];
> +  const char *new_argv[1024];
> +  char * parv[1024];
> +  int idx = 0, retcode = 0;
> +  int parc = 0;
> +
> +  *plugin_found_at = 0;
> +
> +  memset(parv, 0, sizeof(parv));
> +  memset(new_argv, 0, sizeof(new_argv));
> +  tmpbuf[0] = 0;
> +
> +  ink_assert(argc < countof(new_argv));
> +
> +  if (jump_to_argc != 0) {
> +    argc -= jump_to_argc;
> +    int i = 0;
> +    while (argv[i + jump_to_argc]) {
> +      new_argv[i] = argv[i + jump_to_argc];
> +      i++;
> +    }
> +    argv = &new_argv[0];
> +    if (!remap_check_option(argv, argc, REMAP_OPTFLG_PLUGIN, &idx)) {
> +      return -1;
> +    }
> +  } else {
> +    if (unlikely(!mp || (remap_check_option(argv, argc, REMAP_OPTFLG_PLUGIN,
> &idx) & REMAP_OPTFLG_PLUGIN) == 0)) {
> +      snprintf(errbuf, errbufsize, "Can't find remap plugin keyword or
> \"url_mapping\" is NULL");
> +      return -1;                /* incorrect input data - almost impossible
> case */
> +    }
> +  }
> +
> +  if (unlikely((c = strchr(argv[idx], (int) '=')) == NULL || !(*(++c)))) {
> +    snprintf(errbuf, errbufsize, "Can't find remap plugin file name in
> \"@%s\"", argv[idx]);
> +    return -2;                  /* incorrect input data */
> +  }
> +
> +  if (stat(c, &stat_buf) != 0) {
> +    const char *plugin_default_path = TSPluginDirGet();
> +
> +    // Try with the plugin path instead
> +    if (strlen(c) + strlen(plugin_default_path) > (PATH_NAME_MAX - 1)) {
> +      Debug("remap_plugin", "way too large a path specified for remap
> plugin");
> +      return -3;

not a huge fan of these return values (would be nice if they had names)
How do we know what they mean? What's the difference between -1 and -2 in
this instance? etc...

Also, this debug message seems strange.

[snip]

++ i
Igor Galić

Tel: +43 (0) 664 886 22 883
Mail: i.galic@brainsware.org
URL: http://brainsware.org/
GPG: 8716 7A9F 989B ABD5 100F  4008 F266 55D6 2998 1641