On 9/1/06, Daniel Rall <dlr@collab.net> wrote:
> Any particular reason why one error message complains about the lock
> element "wrong location", while the other complains about an element
> at an "unexpected location"?
Because I was kind of inconsistent ;-)
I'll make one of them consistent with the other.
> ...
> > -/* This implements the `ne_xml_endelm_cb' prototype. */
> > -static int getlocks_end_element(void *userdata, int state,
> > - const char *ns, const char *ln)
> > +/* This implements the `svn_ra_dav__endelm_cb_t' prototype. */
> > +static svn_error_t *
> > +getlocks_end_element(void *userdata, int state,
> > + const char *ns, const char *ln)
> > {
> > get_locks_baton_t *baton = userdata;
> > const svn_ra_dav__xml_elm_t *elm;
> > - svn_error_t *err;
> >
> > elm = svn_ra_dav__lookup_xml_elem(getlocks_report_elements, ns, ln);
> >
> > /* Just skip unknown elements. */
> > if (elm == NULL)
> > - return NE_XML_DECLINE;
> > + return SVN_NO_ERROR;;
>
> This seems like a subtle change in behavior in the information
> returned by this function, and a difference in the API (lacking an
> "int *elem" parameter). I guess the loss of "declined" vs. "no error"
> doesn't matter in this case?
I don't think it matters. NE_XML_DECLINE is a zero return value,
which Neon defines as meaning "do nothing". Non-zero aborts the
parse. We return zero from that wrapper callback in any case where
there's no svn error returned.
> ...
> > @@ -1638,11 +1647,8 @@
> > || (! baton->current_lock->token)
> > || (! baton->current_lock->owner)
> > || (! baton->current_lock->creation_date))
> > - {
> > - baton->err = svn_error_create(SVN_ERR_RA_DAV_MALFORMED_DATA, NULL,
> > - _("Incomplete lock data returned"));
> > - return NE_XML_ABORT;
> > - }
> > + SVN_ERR(svn_error_create(SVN_ERR_RA_DAV_MALFORMED_DATA, NULL,
> > + _("Incomplete lock data returned")));
>
> Ditto.
Should be fine, since if we get an error there it will return
NE_XML_ABORT higher up.
> ...
> > - /* unrecognized encoding! */
> > - return NE_XML_ABORT;
> > + return svn_error_createf(SVN_ERR_RA_DAV_MALFORMED_DATA,
> > + NULL,
> > + _("Got unrecognized encoding '%s'"),
> > + baton->encoding);
>
> Ditto.
>
> (In case any other reviewers are curious, we discontinued use of the
> baton->err field because it was removed from the baton structure.)
Yep.
> ...
> > rc = validate_element(NULL, parent_state, elm->id);
> >
> > if (rc != SVN_RA_DAV__XML_VALID)
> > - return (rc == SVN_RA_DAV__XML_DECLINE) ? NE_XML_DECLINE : NE_XML_ABORT;
> > + {
> > + if (rc == SVN_RA_DAV__XML_DECLINE)
> > + {
> > + *elem = NE_XML_DECLINE;
> > + return SVN_NO_ERROR;
> > + }
> > + else
> > + return svn_error_createf(SVN_ERR_RA_DAV_MALFORMED_DATA, NULL,
> > + _("Got unexpected element '%s:%s'"),
>
> It's nice that we supply the unexpected element's name here. Why not
> do that above also? (And do we need to check the element's name for
> null before using it?)
I think the elt_name has to be non-null, although I suspect the
namespace might be, I'll look into it further. I'll also look at
making the other errors return the element names. Sometimes it wasn't
convenient to figure out what element was actually causing the
problem, but in many cases it should be possible.
> > --- trunk/subversion/libsvn_ra_dav/file_revs.c (original)
> > +++ trunk/subversion/libsvn_ra_dav/file_revs.c Fri Sep 1 13:14:27 2006
> ...
> > switch (parent_state)
> > {
> > case ELEM_root:
> > if (elm->id != ELEM_file_revs_report)
> > - return NE_XML_ABORT;
> > + return svn_error_create(SVN_ERR_RA_DAV_MALFORMED_DATA, NULL,
> > + _("Got unexpected element"));
> > break;
>
> As above -- dropping some error context, and not supplying the element
> name in the error message. More of the same throughout this file.
The error returned will be the same, but yeah, I agree we should use
the name if possible.
> ...
> > switch (elm->id)
> > {
> > case ELEM_rev_prop:
> > case ELEM_set_prop:
> > att = svn_xml_get_attr_value("name", atts);
> > if (!att)
> > - return NE_XML_ABORT;
> > + return svn_error_create(SVN_ERR_RA_DAV_MALFORMED_DATA, NULL,
> > + _("Element is missing name attr"));
>
> Out of context, this error message would probably be a bit tough to
> digest. It would help to quote "name". More of the same below.
Keep in mind, these errors are only going to occur if the server is
broken or something is actually removing stuff from the XML. I agree
it could be made better (most of these errors could), but jumping
through hoops to improve the clarity of errors that almost never
happen may not be the best use of time.
> > --- trunk/subversion/libsvn_ra_dav/ra_dav.h (original)
> > +++ trunk/subversion/libsvn_ra_dav/ra_dav.h Fri Sep 1 13:14:27 2006
> ...
> > +/* Our equivalent of ne_xml_startelm_cb, the difference being that it
> > + * returns errors in a svn_error_t, and returns the element type via
> > + * ELEM. To ignore the element *ELEM should be set to NE_XML_DECLINE
> > + * and SVN_NO_ERROR should be returned.
> > + */
> > +typedef svn_error_t * (*svn_ra_dav__startelm_cb_t)(int *elem,
> > + void *baton,
> > + int parent,
> > + const char *nspace,
> > + const char *name,
> > + const char **atts);
> > +
> > +/* Our equivalent of ne_xml_cdata_cb, the difference being that it returns
> > + * errors in a svn_error_t.
> > + */
> > +typedef svn_error_t * (*svn_ra_dav__cdata_cb_t)(void *baton,
> > + int state,
> > + const char *cdata,
> > + size_t len);
>
> As mentioned above, why do some APIs allow return of the Neon error
> code through the int *elem parameter, and some do not?
Because the start callback needs to be able to return the state entry
for this element, so they need to be able to return arbitrary
integers. The cdata and end element callbacks only need to return
either zero (for no problem at all), which is what the wrapper returns
when you return SVN_NO_ERROR from your callback, or non-zero to break
out of the parse, which is what gets returned whenever you return an
actual error from your callback.
> > --- trunk/subversion/libsvn_ra_dav/replay.c (original)
> > +++ trunk/subversion/libsvn_ra_dav/replay.c Fri Sep 1 13:14:27 2006
>
> I have similar questions/comments as for replay.c as for the other .c
> files in ra_dav.
>
> > --- trunk/subversion/libsvn_ra_dav/util.c (original)
> > +++ trunk/subversion/libsvn_ra_dav/util.c Fri Sep 1 13:14:27 2006
> > @@ -555,6 +555,70 @@
> > }
> >
> >
> > +typedef struct {
> > + svn_error_t *err;
> > +
> > + void *baton;
> > +
> > + svn_ra_dav__startelm_cb_t startelm_cb;
> > + svn_ra_dav__cdata_cb_t cdata_cb;
> > + svn_ra_dav__endelm_cb_t endelm_cb;
> > +} parser_wrapper_baton_t;
>
> This Neon wrapper data structure should be documented per the info
> from the change log message for this commit. Good doc here would
> largely obviate the need to document the implementing functions below.
I'll improve the docs there.
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Sep 5 17:21:21 2006