Re: [Monotone-devel] making I(), E(), N() throw bad_decode for network data

[Timothy Brownawell]

On Mon, 2008-11-10 at 13:15 +0100, Thomas Keller wrote:
> Stephen Leake schrieb:
> > Timothy Brownawell <address@hidden> writes:
> > 
> >> I've just pushed a revision that makes the sanity macros look at a
> >> 'made_from' value, and throw bad_decode instead of their normal behavior
> >> if that value is set to made_from_network. ATOMIC and ENCODING types and
> >> revision_t have a member like this, and
> >>    revision_t rev;
> >>    rev.made_from = made_from_network;
> >>    read_revision(garbage_data, rev);
> >> does actually throw bad_decode now for at least some kinds of garbage.
> > 
> > I gather the network sync code handles bad_decode gracefully.
> > 
> >> I don't particularly like having the macros take a "hidden" argument
> >> like this <snip>
> > 
> > We could define a new set of macros to use in the network sync code.
> > 
> > But I guess that doesn't help if the network code calls a function
> > from another file that uses the standard macros.
> 
> I haven't looked at the code, but couldn't we use exceptions for things
> like this? I.e. if a malformed packet is given to read_revision, it
> throws an MalformedRevisionPacketException which is catched and handled
> gracefully in the netsync place and otherwise leads to an I() or E()
> somewhere else?

Maybe, but there are some complications.

One would be that we probably don't want the server to catch *all* cases
of a malformed revision, just those that came from the network. If we
read a garbage revision from the db we probably want to crash with a
debug log regardless of what code ran to get us there. I suppose this
could probably be handled with a flag in the thrown exception to
indicate the source of the bad data.

Another is that checks would suddenly be a lot less convenient to write
than they are now. Instead of just "I(condition)" you'd also have to
give the proper exception type and probably an origin flag. But if the
origin flag could also be "user", hmm, we effectively get to choose
between I(), E(), and N() at runtime based on the source of the bad
data...


How about two sanity macros, and probably two or three exception types.
One that's like I() is now, but only for verifying things that could not
possibly have come from "outside" (which would include coming from the
database). And one to use like E() and N(), but with another argument
E(condition, *origin*, description), that would replace our current E()
and N() and many uses of I(). "origin" could be network (exception will
be handled by netsync event loop), user (result like N()), system (like
E(), would be good for a lot of file_path errors), database (like I(),
but probably with a different message), and internal (I() would become a
shortcut for this). Probably this would mean to give informative_failure
an origin flag and have subclasses for recoverable (network, user,
system) and non-recoverable (internal, database) errors.


-- 
Timothy

Free (experimental) public monotone hosting: http://mtn-host.prjek.net