On Tue, Jul 12, 2011 at 2:49 AM, Mike Frysinger <[email protected]> wrote:
> On Monday, July 11, 2011 14:15:17 Jie Zhang wrote:
>> On Wed, Jul 6, 2011 at 5:48 PM, Mike Frysinger wrote:
>> > if we tweak the name and merge your older idea, we could have:
>> > urj_log_error_describe (level)
>> > now that the message prefix issue is sorted out in the core, we dont need
>> > to worry about that anymore.
>>
>> As I said before, there is no meaning to pass levels other than
>> WARNING or ERROR to urj_log_error_describe. That function describes an
>> error.
>
> urj_error_io() is akin to the C library strerror() and errno. so while they
> usually describe warnings/errors, the fact that they're set/read is not
> always. think of code which uses C library functions where you look for
> multiple files ... some not being found is not an error or warning, but common
> behavior. or perhaps reading from some data source was interrupted
> (signal/ptrace/etc...). the errno would be EINTR, but would not be shown as
> an error (if at all). or the data source is set as nonblocking and so you get
> back EAGAIN since there is currently no data available.
>
> if the user had a noisier debug level set, they might want to see all of those
> hits, but they wouldnt get logged at the warning/error level. they'd get
> logged at "detail" or perhaps "debug".
>
When a user program calls C library to open a file, if the file cannot
be open, it's definitely an error for C library. But how to deal with
this error is up to the user program according to user program's
verbose level, which might be different to the library's verbose
level. So why embed library's log level into the error message? Log
level and error message are two different things. We should not
require user to pass a log level to urj_log_error_describe () to get
the error message. For this sake, this function would be better to be
renamed to urj_error_describe (), as akin to the C library.
>> > also, in the comment above the func, note that it implicitly calls
>> > urj_error_reset().
>> >
>> >> + if (level == URJ_LOG_LEVEL_WARNING || level == URJ_LOG_LEVEL_ERROR)
>> >> + {
>> >> + r += log_printf (p, "%s: ", urj_log_level_string (level));
>> >> + if (urj_log_state.level <= URJ_LOG_LEVEL_DETAIL)
>> >> + r += log_printf (p, "%s:%i %s(): ", file, line, func);
>> >> + }
>> >
>> > i think this part should read:
>> > if (level == URJ_LOG_LEVEL_WARNING || level == URJ_LOG_LEVEL_ERROR ||
>> > urj_log_state.level <= URJ_LOG_LEVEL_DEBUG)
>> > r += log_printf (p, "%s: ", urj_log_level_string (level));
>> > if (urj_log_state.level <= URJ_LOG_LEVEL_DETAIL)
>> > r += log_printf (p, "%s:%i %s(): ", file, line, func);
>>
>> Yes, that's your idea in your previous email. I tried that. But I
>> found always adding a level prefix would make output ugly and unclear
>> to read. There is little use to indicate the level of a message when
>> they are not errors or warnings. For the same reason, I chose to only
>> output source line information for WARNING and ERROR.
>
> i dont understand ... the prefix isnt normally seen, only when the user
> manually increases the debug level. and when you increase the debug level,
> you want the extra output. this snippet i actually tested and the output from
> doing like "cable probe; detect" was unchanged at the default debug level of
> "normal". only when i did "debug detail" did i see the extra prefixes.
>
Did you test by programing a flash or update the firmware for ICE-100B
cable under detail log level.
jtag> debug debug
debug: parse.c:167 urj_parse_line(): Return in urj_parse_line r=0
line={debug debug}
jtag> cable ICE-100B firmware=/home/jie/Desktop/firmware_1_6.hex
cable/ice100.c:880 ice_init(): ICE-100B firmware version is 2.0.0
cable/ice100.c:722 ice_update_firmware(): Updating to firmware
/home/jie/Desktop/firmware_1_6.hex
cable/ice100.c:658 ice_send_flash_data(): .cable/ice100.c:658
ice_send_flash_data(): .cable/ice100.c:658 ice_send_flash_data():
.cable/ice100.c:658 ice_send_flash_data(): .cable/ice100.c:658
ice_send_flash_data(): .cable/ice100.c:658 ice_send_flash_data():
.cable/ice100.c:658 ice_send_flash_data(): .cable/ice100.c:658
ice_send_flash_data(): .cable/ice100.c:658 ice_send_flash_data():
.cable/ice100.c:658 ice_send_flash_data(): .cable/ice100.c:658
ice_send_flash_data(): .cable/ice100.c:658 ice_send_flash_data():
.cable/ice100.c:730 ice_update_firmware():
cable/ice100.c:897 ice_init(): The firmware has been updated
successfully. Please unplug
the ICE-100B cable and reconnect it to finish the update process.
debug: state.c:59 urj_tap_state_dump(): tap_state: UNKNOWN_STATE
debug: parse.c:167 urj_parse_line(): Return in urj_parse_line r=1
line={cable ICE-100B firmware=/home/jie/Desktop/firmware_1_6.hex}
jtag>
vs with my patch
jtag> debug debug
Return in urj_parse_line r=0 line={debug debug}
jtag> cable ICE-100B firmware=/home/jie/Desktop/firmware_1_6.hex
ICE-100B firmware version is 1.0.6
error: cable/ice100.c:885 ice_init(): The firmware on the ICE-100B
needs to be updated. Please go to:
error: cable/ice100.c:886 ice_init():
http://docs.blackfin.uclinux.org/doku.php?id=hw:jtag:ice100b
error: cable/ice100.c:887 ice_init(): to learn how to update the firmware.
Updating to firmware /home/jie/Desktop/firmware_1_6.hex
............
The firmware has been updated successfully. Please unplug
the ICE-100B cable and reconnect it to finish the update process.
tap_state: UNKNOWN_STATE
Return in urj_parse_line r=1 line={cable ICE-100B
firmware=/home/jie/Desktop/firmware_1_6.hex}
jtag>
Jie
------------------------------------------------------------------------------
AppSumo Presents a FREE Video for the SourceForge Community by Eric
Ries, the creator of the Lean Startup Methodology on "Lean Startup
Secrets Revealed." This video shows you how to validate your ideas,
optimize your ideas and identify your business strategy.
http://p.sf.net/sfu/appsumosfdev2dev
_______________________________________________
UrJTAG-development mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/urjtag-development
