A new version of the patch is attached.
On Fri, Jun 17, 2011 at 3:35 PM, Jie Zhang <[email protected]> wrote:
> Thank you for your comments!
>
> On Fri, Jun 17, 2011 at 1:02 PM, Mike Frysinger <[email protected]> wrote:
>> On Friday, June 17, 2011 08:41:35 Jie Zhang wrote:
>>> Any comments? If no one objects, I will commit it at the beginning of
>>> the next week.
>>
>> the basic idea is fine, but some implementation issues ...
>>
>>> --- src/global/log-error.c (revision 1907)
>>> +++ src/global/log-error.c (working copy)
>>>
>>> + char *msg_header;
>>
>> const
>>
> OK.
>
Done.
>>> + 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 just added an assert so we don't need the "" case.
>>> - snprintf (msg, sizeof msg, "%s:%d %s() %s: %s %s",
>>> + snprintf (msg, sizeof msg, "%s: %s:%d %s() %s: %s %s",
>>> + msg_header,
>>>
>>> + snprintf (msg, sizeof msg, "%s: %s:%d %s() %s: %s",
>>> + msg_header,
>>> + urj_error_state.file, urj_error_state.line,
>>
>> but you always use "%s: " with msg_header, even when it's "". so perhaps the
>> ": " should be moved into msg_header and the format string changed to just
>> "%s" rather than "%s: ".
>>
> Maybe. Depending on the way we choose for the former question. If we
> use an assert, leaving ':' here makes the pattern a little more
> structural.
>
I still prefer ':' in the pattern string.
>>> - urj_log (URJ_LOG_LEVEL_NORMAL, "Error: %s\n", urj_error_describe());
>>> + urj_log (URJ_LOG_LEVEL_ERROR, "%s\n",
>>> + urj_error_describe(URJ_LOG_LEVEL_ERROR));
>>
>> there seems to be a lot of this style now. this brings up two questions ...
>>
>> do we ever use urj_log() without a trailing new line ? seems like we should
>> move that into urj_log() itself.
>>
> I don't know the answer.
>
>> many (most? all?) of our logs are of the form:
>> "xxxxxx: %s\n", ..., urj_error_describe()
>> so perhaps it'd make sense to add a bit to the first parameter of urj_log()
>> so
>> that it'll automatically append the error description. think perror().
>>
>> and along those lines, if many times we are simply displaying the error, then
>> perhaps add a helper func like urj_log_simple(level) which expands into:
>> urj_log(level | URJ_LOG_APPEND_ERROR, "");
>>
> Actually, only a small portion of our logs are of that form. Most just
> look like urj_log (level, "log message", ...) and urj_warning to
> immediately print out the message when the error happens.
>
> 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?
>
> 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.
>
> 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.
>
> Maybe we should take some efforts to simplify it and make it more
> consistent throughout UrJTAG.
>
All above are exceeding the intent of my patch. So I don't touch those
with this patch.
Regards,
Jie
* src/global/log-error.c: Include <assert.h>.
(urj_error_describe): Add an argument to indicate if it's a
warning or an error.
* include/urjtag/error.h (urj_error_describe): Update declaration.
* include/urjtag/log.h (urj_warning): Make the message start with
"Warning".
* src/tap/detect.c (urj_tap_detect_parts): Update calling of
urj_error_describe.
* src/global/parse.c (urj_parse_stream): Likewise.
* src/apps/jtag/jtag.c (jtag_readline_multiple_commands_support):
Likewise.
(main): Likewise.
* src/apps/bsdl2jtag/bsdl2jtag.c (main): Likewise.
Index: include/urjtag/error.h
===================================================================
--- include/urjtag/error.h.orig 2011-06-20 08:54:32.000000000 -0400
+++ include/urjtag/error.h 2011-06-22 15:48:26.000000000 -0400
@@ -149,6 +149,6 @@
*
* @return a pointer to a static area.
*/
-const char *urj_error_describe (void);
+const char *urj_error_describe (urj_log_level_t);
#endif /* URJ_ERROR_H */
Index: include/urjtag/log.h
===================================================================
--- include/urjtag/log.h.orig 2011-06-20 08:54:32.000000000 -0400
+++ include/urjtag/log.h 2011-06-22 15:48:26.000000000 -0400
@@ -61,7 +61,7 @@
*/
#define urj_warning(...) \
do { \
- urj_log (URJ_LOG_LEVEL_WARNING, "%s:%d %s() Warning: ", \
+ urj_log (URJ_LOG_LEVEL_WARNING, "Warning: %s:%d %s(): ", \
__FILE__, __LINE__, __func__); \
urj_log (URJ_LOG_LEVEL_WARNING, __VA_ARGS__); \
} while (0)
Index: src/tap/detect.c
===================================================================
--- src/tap/detect.c.orig 2011-06-20 08:54:32.000000000 -0400
+++ src/tap/detect.c 2011-06-22 15:48:26.000000000 -0400
@@ -427,8 +427,8 @@
strcpy (part->stepping, stepping);
if (urj_parse_include (chain, data_path, 0) == URJ_STATUS_FAIL)
{
- urj_log (URJ_LOG_LEVEL_NORMAL, "Error: %s\n",
- urj_error_describe());
+ urj_log (URJ_LOG_LEVEL_ERROR, "%s\n",
+ urj_error_describe(URJ_LOG_LEVEL_ERROR));
urj_error_reset();
}
#ifdef ENABLE_BSDL
Index: src/global/log-error.c
===================================================================
--- src/global/log-error.c.orig 2011-06-20 08:54:32.000000000 -0400
+++ src/global/log-error.c 2011-06-23 10:31:04.000000000 -0400
@@ -26,6 +26,7 @@
#include <stdarg.h>
#include <errno.h>
#include <string.h>
+#include <assert.h>
#include <urjtag/log.h>
#include <urjtag/error.h>
@@ -141,13 +142,27 @@
}
const char *
-urj_error_describe (void)
+urj_error_describe (urj_log_level_t level)
{
static char msg[URJ_ERROR_MSG_LEN + 1024 + 256 + 20];
+ const char *msg_header;
- if (urj_error_state.errnum == URJ_ERROR_IO)
+ /* urj_error_describe should only be called for error or warning. */
+ assert (level == URJ_LOG_LEVEL_WARNING || level == URJ_LOG_LEVEL_ERROR);
+
+ if (level == URJ_LOG_LEVEL_WARNING)
+ msg_header = "Warning";
+ else /* level == URJ_LOG_LEVEL_ERROR */
+ msg_header = "Error";
+
+ if (urj_error_state.errnum == URJ_ERROR_OK)
+ {
+ msg[0] = '\0';
+ }
+ else if (urj_error_state.errnum == URJ_ERROR_IO)
{
- snprintf (msg, sizeof msg, "%s:%d %s() %s: %s %s",
+ snprintf (msg, sizeof msg, "%s: %s:%d %s() %s: %s %s",
+ msg_header,
urj_error_state.file, urj_error_state.line,
urj_error_state.function,
"System error", strerror(urj_error_state.sys_errno),
@@ -155,7 +170,8 @@
}
else
{
- snprintf (msg, sizeof msg, "%s:%d %s() %s: %s",
+ snprintf (msg, sizeof msg, "%s: %s:%d %s() %s: %s",
+ msg_header,
urj_error_state.file, urj_error_state.line,
urj_error_state.function,
urj_error_string (urj_error_state.errnum),
Index: src/global/parse.c
===================================================================
--- src/global/parse.c.orig 2011-06-20 08:54:32.000000000 -0400
+++ src/global/parse.c 2011-06-22 15:48:26.000000000 -0400
@@ -248,7 +248,7 @@
go = urj_parse_line (chain, inputline);
if (go == URJ_STATUS_FAIL)
{
- urj_log (ll, "Error: %s; command '%s'\n", urj_error_describe(), inputline);
+ urj_log (ll, "%s; command '%s'\n", urj_error_describe(URJ_LOG_LEVEL_ERROR), inputline);
urj_error_reset ();
}
urj_tap_chain_flush (chain);
Index: src/apps/jtag/jtag.c
===================================================================
--- src/apps/jtag/jtag.c.orig 2011-06-20 08:54:32.000000000 -0400
+++ src/apps/jtag/jtag.c 2011-06-22 15:48:26.000000000 -0400
@@ -248,7 +248,8 @@
r = urj_parse_line (chain, line);
if (r == URJ_STATUS_FAIL)
{
- urj_log (URJ_LOG_LEVEL_NORMAL, "Error: %s\n", urj_error_describe());
+ urj_log (URJ_LOG_LEVEL_ERROR, "%s\n",
+ urj_error_describe(URJ_LOG_LEVEL_ERROR));
urj_error_reset ();
}
@@ -541,7 +542,8 @@
/* Create ~/.jtag */
if (jtag_create_jtagdir () != URJ_STATUS_OK)
{
- urj_warning ("%s\n", urj_error_describe());
+ urj_log (URJ_LOG_LEVEL_WARNING, "%s\n",
+ urj_error_describe(URJ_LOG_LEVEL_WARNING));
urj_error_reset();
}
@@ -553,8 +555,8 @@
if (urj_error_get() != URJ_ERROR_IO)
{
/* Only warn about RC problems; don't prevent running */
- urj_log (URJ_LOG_LEVEL_NORMAL, "Error: %s\n",
- urj_error_describe());
+ urj_log (URJ_LOG_LEVEL_WARNING, "%s\n",
+ urj_error_describe(URJ_LOG_LEVEL_WARNING));
}
urj_error_reset();
}
Index: src/apps/bsdl2jtag/bsdl2jtag.c
===================================================================
--- src/apps/bsdl2jtag/bsdl2jtag.c.orig 2011-06-20 08:54:32.000000000 -0400
+++ src/apps/bsdl2jtag/bsdl2jtag.c 2011-06-22 15:48:26.000000000 -0400
@@ -68,8 +68,8 @@
chain = urj_tap_chain_alloc ();
if (chain == NULL)
{
- urj_log (URJ_LOG_LEVEL_NORMAL, "Error: %s\n",
- urj_error_describe());
+ urj_log (URJ_LOG_LEVEL_ERROR, "%s\n",
+ urj_error_describe(URJ_LOG_LEVEL_ERROR));
return 1;
}
@@ -93,7 +93,8 @@
result = urj_bsdl_read_file (chain, argv[1], URJ_BSDL_MODE_DUMP, NULL);
if (result < 0)
{
- urj_log (URJ_LOG_LEVEL_ERROR, "Error: %s\n", urj_error_describe());
+ urj_log (URJ_LOG_LEVEL_ERROR, "%s\n",
+ urj_error_describe(URJ_LOG_LEVEL_ERROR));
urj_error_reset ();
}
------------------------------------------------------------------------------
Simplify data backup and recovery for your virtual environment with vRanger.
Installation's a snap, and flexible recovery options mean your data is safe,
secure and there when you need it. Data protection magic?
Nope - It's vRanger. Get your free trial download today.
http://p.sf.net/sfu/quest-sfdev2dev
_______________________________________________
UrJTAG-development mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/urjtag-development
