[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [PATCH] Error checking in authz

From: Greg Hudson <ghudson_at_MIT.EDU>
Date: 2005-07-14 16:24:04 CEST

Here's my review notes:

  * Use two spaces after periods in the log messages.
  * I think we normally use "err", not "error", to name variables or
fields containing errors, but I might be making up the consistency of
that convention.
  * You missed space-before-paren in a number of places.
  * "Dependancy" isn't strictly 100% incorrect, but "dependency" is the
normal way it's written.
  * You have "wether" in a number of places; it's spelled "whether".
(Including some places in existing code. Didn't we cover this last time
around?)
  * Your design of authz file validation is a little dissatisfactory.
By performing the checking in authz_group_contains_user() and using that
function in authz_validate_group(), you've presented a structural
obstacle to taking advantage of the validation by not doing
error-checking during actual authz lookup.
  * In fact, if the second patch were designed properly, the first patch
would be essentially unnecessary.

+ svn_config_get (b->config, &val, "groups",
+ &name[1], NULL);

Fits nicely on one line. Why split it?

+/** A data type specific to authz configurations. Consider as
+ opaque. */
+typedef svn_config_t svn_authz_t;

I'm not sure that this really allows us to change the representation of
svn_authz_t in the future. I'd really like to see:

  typedef struct svn_authz_t svn_authz_t;

In authz.c, you can either define:

  struct svn_authz_t {
    svn_config_t *cfg;
  };

Or, you can cast svn_authz_t * pointers to svn_config_t * pointers and
vice versa, and never actually make struct svn_authz_t a complete type.
ANSI C guarantees that pointer to struct foo can be casted to pointer to
struct bar without losing information, I believe, so this is correct if
perhaps a little weird. You'd only need to cast in two places, I think
(once when returning an svn_authz_t * in the read function, and once
when accepting it in the query function).

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Jul 14 16:25:49 2005

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.