Fixed per your review and committed in r31138. If more tweaks are
necessary I'll make them in a followup commit, of course.
Thanks,
Daniel
Daniel Shahaf wrote on Mon, 12 May 2008 at 10:38 +0300:
> 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 19:08:58 CEST