On Tue, 8 Feb 2005, Ben Collins-Sussman wrote:
>
> On Feb 8, 2005, at 8:22 AM, Peter N. Lundblad wrote:
> >>
> >> + switch (elm->id)
> >> + {
> >> + case ELEM_lock:
> >> + /* finish the whole svn_lock_t. */
> >> + apr_hash_set(baton->lock_hash, baton->current_lock->path,
> >> + APR_HASH_KEY_STRING, baton->current_lock);
> >> + break;
> >> +
> > I think you want to validate the lock here. Else, misbehaving servers
> > can
> > crash the client. This means checking that mandatory fields really were
> > set.
>
> Hm, maybe we should create some svn_lock_validate() function in
> libsvn_subr? I imagine that other RA layers might want to validate
> locks coming from the network too, yes?
>
Not that I know of. libsvn_ra_svn's praser will validate the input in the
sense that it won't allow a missing path field fr example.
> >> const char *final_val;
> >> +
> >> + if (baton->encoding)
> >> + {
> >> + /* ### possibly recognize other encodings someday */
> >
> > What other encodings would that be? Wouldn't it be simple to just use a
> > boolean in the baton for base64?
>
> kfogel and cmpilato wanted to leave the doors open for other possible
> encodings in the future. Nothing specific in mind, just wanted to keep
> it flexible.
>
I don't feel strongly about it, but I think that is premature. You can't
add the new encoding to a server in 1.x without breaking compatibility.
And, to be honest, it is not that there are a lot of encodings that we
would need to support. I think it's just confusing. But, I leave it up to
you.
> > err is leaked. I think you need to store the error in the baton and
> > return
> > it after the parse.
>
> What err??
>
You have a variable named err in that function. It is set during date
parsing but never cleared, nor returned AFAICT.
Bye,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Feb 8 16:25:54 2005