On Mon, Sep 13, 2010 at 5:40 PM, Mike Dunn <[email protected]> wrote: > Hi Øyvind, > > On 09/12/2010 12:23 PM, Øyvind Harboe wrote: >> A general observation: wouldn't it be better to properly report >> the error at the failure site rather than try to interpret errors? > I always assumed that for a project like OpenOCD, which manages and performs > operations on a set of similiar objects (processors and debug interfaces), > consolidation of code and avoidance of duplication would be a fundamental > goal. > Enumerating a list of error conditions and handling them in common > infrastructure code seems consistent with that goal.
Precisely. And reporting and explaining the error *only* where it arises is part of that centralization. Occasionally there are some messages that elaborate on the contact that check retval != ERROR_OK => "while loading" or some such. >> Much less error prone, the error propagation is less than 100%, >> better info and less code. > Wouldn't it be a better goal to fix poor coding practices (not propagating > errors) than to just work around them? We do that too. Relying on the actual values is another bad coding practice that we're getting rid of. > And is it less code if a few lines are > eliminated from breakpoints.c, but every target checks the return code itself > and generates different messages that all say the same basic thing? This only > confuses the conversation. Error messages are roughly modeled on exceptions. The right place to throw an exception is where the exception occurs. Same thing in OpenOCD. return retval != ERROR_OK and LOG_ERROR() at the error site. >> To try to reconstruct some meaningful information about what >> happened from a 32 bit value yields pretty nonsensical error >> messages in general... > In general it can. If you generalize too much across disparate objects, > abstraction, inheritance, and all that becomes impractical and a numerical > error > code loses meaning. But is that the case here? Looking at the error codes in > target.h, all seem rather specific and have the same meaning on all targets. > Probably the most vague is RESOURCE_NOT_AVAILABLE, but it's enough to let a > user > know that if he wants another wp or hw bp, he needs to remove an existing one. > (And currently some of the targets do log their own error in this case: "no > free > breakpoint reg", "can't find an available comparator", etc). The different ERROR_ messages in OpenOCD pretty much reported willy-nilly. Checking for a specific ERROR_ message is like catching a specific exception. That specific ERROR_ message needs to be part of the *documented* API. The number of places in OpenOCD where code executes different code paths based on specific error messages are less than a dozen and any time I encounter one, I usually find that it can be eliminated. > Amyway, that's my two cents. Wasn't thinking these deep thoughts when I > submitted the patch :-) Just trying to fit xcale changes into the broader > framework. We've been deleting this sort of code problem from OpenOCD for a while, but this is a case that we didn't get around to yet. Will you resubmit patch with checking on specific error messages removed? I can do it, but the advantage of you doing it is obviously that you are currently testing those patches... -- Øyvind Harboe US toll free 1-866-980-3434 / International +47 51 63 25 00 http://www.zylin.com/zy1000.html ARM7 ARM9 ARM11 XScale Cortex JTAG debugger and flash programmer _______________________________________________ Openocd-development mailing list [email protected] https://lists.berlios.de/mailman/listinfo/openocd-development
