httpd-apreq-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Max Kellermann <>
Subject today's code review..
Date Wed, 09 Mar 2005 23:07:26 GMT

I spent this evening with a code review of util.c and param.c
(multi-env). I have collected several issues/questions:

- we can't guarantee that X == apreq_encode(apreq_decode(X)) when X
  contains unicode escapes.

- we should define what apreq_decode() does when it sees 8 bit ASCII
  escapes and unicode escapes in one string; the current behaviour
  (ignoring that) isn't a good solution. Maybe mark with that utf8
  flag? (could also solve the previous item on my list)

- do you think the simple heuristic algorithm in apreq_quote_once()
  to determine if a value is quoted is enough? should we really export
  this function?

- should apreq_join() return NULL or "" when n==0?
- should apreq_params_as_string() return NULL or "" when n==0?
  (currently NULL, both undocumented!)

- apreq_join() documentation says you can upgrade the result to
  apreq_value_t; this is wrong, isn't it? Get rid of all that
  "upgrading" before 2.05 please - it's unsafe! ;)

- apreq_join() returns NULL when a sequence cannot be decoded; this is
  the same return value as for "arr->nelts == 0" (which is not an
  error); should we better return an apr_status_t here?

- util.c/apreq_fwritev() can go into endless recursion when
  apr_file_writev() returns zero length (the write manpage says this
  can happen without an error condition; writev says nothing about
  this case, but write is the fallback for writev); if we catch that,
  apreq_brigade_fwrite() cannot assume anymore that *nelts will always
  be decreased by apr_file_writev(), or it will segfault. Ideas to
  resolve that?

- what is apreq_param_size() for? the same for apreq_cookie_size()

- let's change apreq_params() to:
  apr_status_t apreq_params(apreq_handle_t *req, const apr_table_t **t)
  .. like apreq_body() and apreq_args() - if a user wants to mess with
  the table, he can use apr_table_copy() (like the old apreq_params()
  does currently internally)

- parameter order: what is the APR policy for placing an apr_pool_t
  parameter? we should decide whether we put it first or last.


View raw message