httpd-apreq-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Joe Schaefer <joe+gm...@sunstarsys.com>
Subject Re: today's code review..
Date Thu, 10 Mar 2005 00:21:39 GMT
Max Kellermann <max@duempel.org> writes:

> Hi,
>
> I spent this evening with a code review of util.c and param.c
> (multi-env).

++Max!

> I have collected several issues/questions:
>
> - we can't guarantee that X == apreq_encode(apreq_decode(X)) when X
>   contains unicode escapes.

I'm not sure that's relevant.  As far as encoding/decoding goes,
I think we're in line with RFC 3986 Section 2, no?

> - 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)

Hmm, tough question.  If we start marking things as utf8, we need to do it
accurately.  Can you be sure the unicode escapes don't come from some
other UTF encoding?

> - 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?

Probably not, I suppose.

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

I think the "right thing" is to return "" when n ==0, and reserve
NULL for errors.

> - 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! ;)

Agreed.

> - 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?

Not worth an apr_status_t return, IMO- see above.

> - util.c/apreq_fwritev() can go into endless recursion when
>   apr_file_writev() returns zero length

Return zero on a single call, or always?

> (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?

I don't see a problem.  A system that consistently returns zero
on a file write() isn't one that I care to worry too much about.  
If it concens you, I don't mind promoting that condition (apr_file_writev
returns APR_SUCCESS, but writes zero bytes to the file) to an 
APREQ_ERROR_GENERAL.

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

To get a quick O(1) count on how big each request object is.
+1 to remove them.

> - 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)

I like that, but I need to think a bit more about how this 
translates into the perl glue.

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

+1 for consistent treatment of pool args;  but I'm not sure what APR's 
preference is here.  Looking at their file constructors, they put the 
pool last; but string, table and brigade constructors have it in the
first slot.  If we had to choose between them, I'd probably follow
the apr_file pattern and put it last.  But I don't feel strongly about
it, so whatever folks come up with is ok with me.

-- 
Joe Schaefer


Mime
View raw message