Karl Fogel wrote on Sun, 11 May 2008 at 22:57 -0400:
> Daniel Shahaf <d.s_at_daniel.shahaf.co.il> writes:
> > [[[
> > Validate svn:date property values in libsvn_repos.
> >
> > Suggested by: sussman
> >
> > * subversion/libsvn_repos/fs-wrap.c
> > (svn_time.h): Include.
> > (validate_prop): Add optional 'value' and 'pool' parameters. Extend to
> > validate the value for svn:* properties.
> > (svn_repos_fs_change_node_prop, svn_repos_fs_change_txn_props,
> > svn_repos_fs_change_rev_prop3):
> > Pass pool and property value to validate_prop().
> >
> > * subversion/tests/cmdline/prop_tests.py
> > (invalid_propvalues): New test.
> > (test_list): Run new test.
> > ]]]
> >
> > --- subversion/libsvn_repos/fs-wrap.c (revision 31118)
> > +++ subversion/libsvn_repos/fs-wrap.c (working copy)
> > @@ -146,17 +147,38 @@ svn_repos_fs_begin_txn_for_update(svn_fs_txn_t **t
> > /*** Property wrappers ***/
> >
> > /* Validate that property NAME is valid for use in a Subversion
> > - repository. */
> > + repository. If given, validate VALUE as well (for "svn:" properties only).
> > + Use POOL for temporary allocations.
> > + */
>
> As long as you're adjusting the doc string, you might as well say what
> error is returned if the property value is not valid.
Okay. I suppose the existing SVN_ERR_REPOS_BAD_ARGS should be untouched
for compatibility, though.
> Also, we're not
> currently validating all "svn:" properties, only some of them -- well,
> only one of them, in fact. You don't have to list them all in the doc
> string, but at least it should say that validation *may* happen, rather
> than that it will definitely happen.
>
Okay.
> > static svn_error_t *
> > -validate_prop(const char *name)
> > +validate_prop(const char *name, const svn_string_t *value, apr_pool_t *pool)
> > {
> > svn_prop_kind_t kind = svn_property_kind(NULL, name);
> > +
> > + /* Disallow setting non-regular properties. */
> > if (kind != svn_prop_regular_kind)
> > return svn_error_createf
> > (SVN_ERR_REPOS_BAD_ARGS, NULL,
> > _("Storage of non-regular property '%s' is disallowed through the "
> > "repository interface, and could indicate a bug in your client"),
> > name);
> > +
> > + /* Validate "svn:" properties. */
> > + if (svn_prop_is_svn_prop(name) && value != NULL)
> > + {
> > + /* "svn:date" should a valid date. */
> > + if (! strcmp(name, SVN_PROP_REVISION_DATE))
> > + {
>
> We generally use this idiom:
>
> if (strcmp(name, SVN_PROP_REVISION_DATE) == 0)
>
> (so that equality is signified by "==" rather than "!").
>
Done.
> > + apr_time_t temp;
> > + svn_error_t *err;
> > +
> > + err = svn_time_from_cstring(&temp, value->data, pool);
> > + if (err)
> > + return svn_error_create(SVN_ERR_BAD_PROPERTY_VALUE,
> > + err, NULL);
> > + }
> > + }
> > +
> > return SVN_NO_ERROR;
> > }
> >
> > @@ -169,7 +191,7 @@ svn_repos_fs_change_node_prop(svn_fs_root_t *root,
> > apr_pool_t *pool)
> > {
> > /* Validate the property, then call the wrapped function. */
> > - SVN_ERR(validate_prop(name));
> > + SVN_ERR(validate_prop(name, value, pool));
> > return svn_fs_change_node_prop(root, path, name, value, pool);
> > }
>
> Hmmm, doesn't this imply a change to svn_repos_fs_change_node_prop()'s
> doc string? :-)
>
What would you like to change?
/** Validating wrapper for svn_fs_change_node_prop() (which see for
* argument descriptions).
*/
svn_error_t *
svn_repos_fs_change_node_prop(svn_fs_root_t *root,
const char *path,
const char *name,
const svn_string_t *value,
apr_pool_t *pool);
> > @@ -182,7 +204,10 @@ svn_repos_fs_change_txn_props(svn_fs_txn_t *txn,
> > int i;
> >
> > for (i = 0; i < txnprops->nelts; i++)
> > - SVN_ERR(validate_prop(APR_ARRAY_IDX(txnprops, i, svn_prop_t).name));
> > + {
> > + svn_prop_t *prop = &APR_ARRAY_IDX(txnprops, i, svn_prop_t);
> > + SVN_ERR(validate_prop(prop->name, prop->value, pool));
> > + }
> >
> > return svn_fs_change_txn_props(txn, txnprops, pool);
> > }
>
> ...and this a change to svn_repos_fs_change_txn_props()'s doc string?
>
What would you like to change?
/** Validating wrapper for svn_fs_change_txn_props() (which see for
* argument descriptions).
*
* @since New in 1.5.
*/
svn_error_t *
svn_repos_fs_change_txn_props(svn_fs_txn_t *txn,
apr_array_header_t *props,
apr_pool_t *pool);
> > @@ -227,8 +252,9 @@ svn_repos_fs_change_rev_prop3(svn_repos_t *repos,
> >
> > if (readability == svn_repos_revision_access_full)
> > {
> > - SVN_ERR(validate_prop(name));
> > + SVN_ERR(validate_prop(name, new_value, pool));
> > SVN_ERR(svn_fs_revision_prop(&old_value, repos->fs, rev, name, pool));
> > +
> > if (! new_value)
> > action = 'D';
> > else if (! old_value)
>
> ...and this a change to svn_repos_fs_change_rev_prop3()'s doc string?
>
Yes, but only because it doesn't already state that it was validating
the property.
> -Karl
>
Thanks for the review,
Daniel
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-05-12 09:38:20 CEST