On Wed, Jun 29, 2011 at 6:17 PM, Mike Frysinger <[email protected]> wrote:
> On Friday, June 17, 2011 15:35:51 Jie Zhang wrote:
>> On Fri, Jun 17, 2011 at 1:02 PM, Mike Frysinger wrote:
>> > On Friday, June 17, 2011 08:41:35 Jie Zhang wrote:
>> >> + if (level == URJ_LOG_LEVEL_WARNING)
>> >> + msg_header = "Warning";
>> >> + else if (level == URJ_LOG_LEVEL_ERROR)
>> >> + msg_header = "Error";
>> >> + else
>> >> + msg_header = "";
>> >
>> > so the msg_header is one of {"Warning", "Error", ""} ...
>>
>> I thought this piece of code when I'm writing it but has no good idea
>> how to deal with it. I think urj_error_describe should only be called
>> for error messages. But there is one and only one place in UrJTAG
>> calling it for warning message. I don't know how to not do that for
>> that warning message. So urj_error_describe should only be called for
>> warning or error. Maybe I should use an assert under the else branch?
>> What do you think?
>
> i dont like assert's in libraries. the current code works fine when called at
> any time.
>
> this translating of level into a string is already handled in
> src/cmd/cmd_debug.c:log_level_to_string(). so why dont we rip those to helper
> funcs out into src/global/log-error.c, make them into proper urj_* funcs, and
> then we can have urj_error_describe() use that. that way we always get a
> proper prefixed message (since the helper returns "unknown" in the default
> case).
>
The problem is what we want here as a parameter is something to tell
what we describe is a warning or an error. urj_log_level_t is not
suitable here. I was lazy and tried to avoid introducing a new type
something like:
typedef enum
{
ERROR,
WARNING,
}
urj_error_or_warning_t;
or define urj_error_describe as
const char *
urj_error_describe (urj_log_level_t level, bool is_error)
{}
But I'm afraid this is not intuitive when using.
or define two interfaces:
urj_error_describe () and urj_warning_describe ()
both will call an internal urj_error_warning_describe (urj_log_level_t
level, bool is_error)
Which way do you like more?
>> I'm not in favor of the error reporting system currently used in
>> UrJTAG. It's too complicated for me. For example,
>>
>> 1. I cannot find a suitable errnum for some error messages in ICE-100B
>> cable. I thought adding a new errnum like URJ_ERROR_ICE_100B. But does
>> that means we need a number for each cable driver?
>
> we can add a general URJ_ERROR_CABLE if there's something missing. what
> exactly is the case you're handling that the current URJ_ERROR_* dont cover ?
>
For example error when parsing HEX firmware file. Now I use a new one
URJ_ERROR_FIRMWARE, which I think is better than the cable one since
other cables can also use this.
>> 2. urj_error_set/urj_error_describe is difficult to use. Sometimes I
>> don't want my error message to show file/line/function before it. It's
>> not useful to user and makes the error message read strange. But it's
>> not easy to disable this.
>
> i'm fine with changing the file/line/function to only show when the debug
> level is set to detail or debug levels. the default is normal.
> jtag> debug
> Current log level is 'normal'
>
I will see if I will find time to do this later.
>> 3. currently the error messages are handled using two ways. One is
>> output immediately. The other is to store it and output late. I don't
>> know when to use which, or we should always use the latter, but there
>> are some many code using the former now.
>
> my understanding is that core pieces log while cmd pieces output. that means
> cable drivers should log only.
>
When working on the latest version of the ICE-100B cable firmware
updater, I felt this.
Jie
------------------------------------------------------------------------------
All of the data generated in your IT infrastructure is seriously valuable.
Why? It contains a definitive record of application performance, security
threats, fraudulent activity, and more. Splunk takes this data and makes
sense of it. IT sense. And common sense.
http://p.sf.net/sfu/splunk-d2d-c2
_______________________________________________
UrJTAG-development mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/urjtag-development
