On Tue, Jul 12, 2011 at 5:34 PM, Mike Frysinger <[email protected]> wrote:
> On Tue, Jul 12, 2011 at 14:07, Jie Zhang wrote:
>> On Tue, Jul 12, 2011 at 2:49 AM, Mike Frysinger 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.
>
> exactly right
>
>> 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.
>
> not quite. there is only one verbose level (urj_log_state.level), and
> while the library holds onto it, it's under the user's control. this
> is the level the user has to control what messages they care to
> actually see.
>
> the leaf functions of liburjtag set the error (i.e. "errno = xxx").
> the functions that call those errors get to determine how much of an
> "error" those errors actually are. perhaps it's fatal (so we log at
> "error" level), or perhaps it's recoverable but generally not a good
> thing (so we log at "warning" level), or perhaps it's generally
> harmless and can be ignored (so we log at "detail" level).
>
> urj_log_state.level has no bearing at what level the library uses when
> it calls urj_log(). that comes into play internally as a check
> internally to see whether it actually gets logged.
>
Sorry I didn't make myself clear. My point is user program can have
its own verbose level, which might be different to library. User
program will output the error description according to its log
convention. So urj_error_describe should just return the error
description without the urjtag log level prefix.
>> So why embed library's log level into the error message? Log
>> level and error message are two different things.
>
> correct
>
>> 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.
>
> the point of urj_log_error_describe(level) is simply an optimization
> to the common idiom:
> urj_log (level, "%s\n", urj_error_describe());
> urj_error_reset ();
> and to make to keep people from doing the wrong thing (as some of our
> code shows now):
> urj_log (URJ_LOG_LEVEL_NORMAL, "Error: %s\n", urj_error_describe());
> logging an "error" at the "normal" level is incorrect imo.
>
> so keeping urj_error_describe() makes sense for the reasons you
> highlight, but we still want urj_log_error_describe(level) for the
> reasons i mention above.
>
Good point. So in this version I generalize urj_log_error_1 from the
last version to urj_log_error_describe.
>>>> > 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.
>>
>> cable/ice100.c:658 ice_send_flash_data(): .cable/ice100.c:658
>> ice_send_flash_data(): .cable/ice100.c:658 ice_send_flash_data():
>>
>> vs with my patch
>>
>> ............
>
> so your issue is with progress bars. all the other lines looked fine to me.
>
> i'd say that this is "logging" vs "user output" and is a general
> problem. on one hand, we have the library code (the ice100b driver)
> which only exists to perform operations. on the other hand, we have
> an application (`urjtag`) which is merely an interface to the library.
> progress bars are part of the app, not the library.
>
> in this regard, i think we want to update urj_log_state_t to take
> another func pointer for progress bars. that way the app can add a
> hook for properly updating things itself (whether it be a gui bar that
> fills up, or a spiny wheel, or something else). then we'd have
> helpers that the ice100b firmware code would use to post updates about
> its progress. either it's a 0% to 100% completion, or it's a simple
> "yes, i'm still alive, please wait".
>
> for now, i know this has ballooned way beyond what Chad expected, so
> i'm fine with the display that mine proposal ends up with at more
> verbose levels. and i can take care of implementing the progress bar
> idea so you can move on :). but you'll have to hook me up with some
> firmware files so i can flash my ice100b cable.
>
Thanks for your offer. I think I will put the latest firmware on the
wiki when the firmware updater is in.
btw, I change the progress bar to several lines of "updating ...". So
even on debug level, the output still looks not so bad. I will post
the updated patch in its thread soon.
I also make a tweak about when prefix and line info will be printed
out. I think it will be better to start outputting the prefix from
detail level and source line info from the debug level.
Regards,
Jie
* include/urjtag/parse.h (urj_parse_stream): Update declaration.
(urj_parse_file): Likewise.
* include/urjtag/log.h (urj_do_log): Add file, line, func parameters.
(urj_log): Pass __FILE__, __LINE__, __func__ to urj_do_log.
(urj_warning): Use urj_log.
(urj_log_error_describe): Declare.
* src/tap/detect.c (urj_tap_detect_parts): Use urj_log_error_describe.
* src/global/log-error.c: Include <stdbool.h>.
(log_printf): New.
(urj_do_log): Print log level prefix for error, warning, or when
log level is URJ_LOG_LEVEL_DETAIL or lower.
Print file, line, func when log level is URJ_LOG_LEVEL_DEBUG
or lower.
(urj_error_describe): Don't include file, line, or function.
(urj_log_error_describe): New.
* src/global/parse.c (urj_parse_stream): Remove log level argument.
Use urj_log_error_describe.
(urj_parse_file): Remove log level argument.
(urj_parse_include): Update use of urj_parse_file.
* src/apps/jtag/jtag.c (jtag_readline_multiple_commands_support):
Use urj_log_error_describe.
(jtag_parse_rc): Update use of urj_parse_file.
(main): Update use of urj_parse_file and urj_parse_stream.
Use urj_log_warning.
* src/apps/bsdl2jtag/bsdl2jtag.c (main): Use urj_log_error.
Index: include/urjtag/parse.h
===================================================================
--- include/urjtag/parse.h.orig 2011-07-13 11:44:43.000000000 -0400
+++ include/urjtag/parse.h 2011-07-13 11:50:58.000000000 -0400
@@ -63,7 +63,7 @@
* URJ_STATUS_ERROR on error
* URJ_STATUS_QUIT on quit command
*/
-int urj_parse_stream (urj_log_level_t ll, urj_chain_t *chain, FILE *f);
+int urj_parse_stream (urj_chain_t *chain, FILE *f);
/**
* Open the specified file and run through urj_parse_stream().
@@ -73,8 +73,7 @@
* URJ_STATUS_ERROR on error
* URJ_STATUS_QUIT on quit command
*/
-int urj_parse_file (urj_log_level_t ll, urj_chain_t *chain,
- const char *filename);
+int urj_parse_file (urj_chain_t *chain, const char *filename);
/**
* Include a file. Autodetects whether it is a bsdl file or a UrJTAG command
Index: include/urjtag/log.h
===================================================================
--- include/urjtag/log.h.orig 2011-07-13 11:44:43.000000000 -0400
+++ include/urjtag/log.h 2011-07-13 11:52:04.000000000 -0400
@@ -40,16 +40,17 @@
extern urj_log_state_t urj_log_state;
-int urj_do_log (urj_log_level_t level, const char *fmt, ...)
+int urj_do_log (urj_log_level_t level, const char *file, size_t line,
+ const char *func, const char *fmt, ...)
#ifdef __GNUC__
- __attribute__ ((format (printf, 2, 3)))
+ __attribute__ ((format (printf, 5, 6)))
#endif
;
#define urj_log(lvl, ...) \
do { \
if ((lvl) >= urj_log_state.level) \
- urj_do_log (lvl, __VA_ARGS__); \
+ urj_do_log (lvl, __FILE__, __LINE__, __func__, __VA_ARGS__); \
} while (0)
/**
@@ -59,12 +60,12 @@
* @param ... consists of a printf argument set. It needs to start with a
* const char *fmt, followed by arguments used by fmt.
*/
-#define urj_warning(...) \
- do { \
- urj_log (URJ_LOG_LEVEL_WARNING, "%s:%d %s() Warning: ", \
- __FILE__, __LINE__, __func__); \
- urj_log (URJ_LOG_LEVEL_WARNING, __VA_ARGS__); \
- } while (0)
+#define urj_warning(...) urj_log (URJ_LOG_LEVEL_WARNING, __VA_ARGS__)
+
+/**
+ * Print the error set by urj_error_set and call urj_error_reset.
+ */
+void urj_log_error_describe (urj_log_level_t level);
/**
* Convert the named level into the corresponding urj_log_level_t.
Index: src/tap/detect.c
===================================================================
--- src/tap/detect.c.orig 2011-07-13 11:44:43.000000000 -0400
+++ src/tap/detect.c 2011-07-13 11:50:58.000000000 -0400
@@ -426,11 +426,7 @@
strcpy (part->part, partname);
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_error_reset();
- }
+ urj_log_error_describe (URJ_LOG_LEVEL_ERROR);
#ifdef ENABLE_BSDL
}
#endif
Index: src/global/log-error.c
===================================================================
--- src/global/log-error.c.orig 2011-07-13 11:44:43.000000000 -0400
+++ src/global/log-error.c 2011-07-13 11:58:38.000000000 -0400
@@ -26,6 +26,7 @@
#include <stdarg.h>
#include <errno.h>
#include <string.h>
+#include <stdbool.h>
#include <urjtag/log.h>
#include <urjtag/error.h>
@@ -59,20 +60,44 @@
return r;
}
-int
-urj_do_log (urj_log_level_t level, const char *fmt, ...)
+static int
+log_printf (int (*p) (const char *, va_list), const char *fmt, ...)
{
va_list ap;
int r;
+ va_start (ap, fmt);
+ r = (*p) (fmt, ap);
+ va_end (ap);
+
+ return r;
+}
+
+int
+urj_do_log (urj_log_level_t level, const char *file, size_t line,
+ const char *func, const char *fmt, ...)
+{
+ int (*p) (const char *fmt, va_list ap);
+ va_list ap;
+ int r = 0;
+
if (level < urj_log_state.level)
return 0;
- va_start (ap, fmt);
if (level < URJ_LOG_LEVEL_WARNING)
- r = urj_log_state.out_vprintf (fmt, ap);
+ p = urj_log_state.out_vprintf;
else
- r = urj_log_state.err_vprintf (fmt, ap);
+ p = urj_log_state.err_vprintf;
+
+ if (level == URJ_LOG_LEVEL_WARNING || level == URJ_LOG_LEVEL_ERROR
+ || level <= URJ_LOG_LEVEL_DETAIL)
+ r += log_printf (p, "%s: ", urj_log_level_string (level));
+
+ if (urj_log_state.level <= URJ_LOG_LEVEL_DEBUG)
+ r += log_printf (p, "%s:%i %s(): ", file, line, func);
+
+ va_start (ap, fmt);
+ r += (*p) (fmt, ap);
va_end (ap);
return r;
@@ -187,20 +212,31 @@
if (urj_error_state.errnum == URJ_ERROR_IO)
{
- snprintf (msg, sizeof msg, "%s:%d %s() %s: %s %s",
- urj_error_state.file, urj_error_state.line,
- urj_error_state.function,
- "System error", strerror(urj_error_state.sys_errno),
+ snprintf (msg, sizeof msg, "%s: %s %s",
+ "system error", strerror(urj_error_state.sys_errno),
urj_error_state.msg);
}
else
{
- snprintf (msg, sizeof msg, "%s:%d %s() %s: %s",
- urj_error_state.file, urj_error_state.line,
- urj_error_state.function,
+ snprintf (msg, sizeof msg, "%s: %s",
urj_error_string (urj_error_state.errnum),
urj_error_state.msg);
}
return msg;
}
+
+void
+urj_log_error_describe (urj_log_level_t level)
+{
+ if (urj_error_get () == URJ_ERROR_OK)
+ return;
+
+ if (level >= urj_log_state.level)
+ urj_do_log (level,
+ urj_error_state.file, urj_error_state.line,
+ urj_error_state.function,
+ "%s\n", urj_error_describe ());
+
+ urj_error_reset ();
+}
Index: src/global/parse.c
===================================================================
--- src/global/parse.c.orig 2011-07-13 11:44:43.000000000 -0400
+++ src/global/parse.c 2011-07-13 11:50:58.000000000 -0400
@@ -213,7 +213,7 @@
#endif
int
-urj_parse_stream (urj_log_level_t ll, urj_chain_t *chain, FILE *f)
+urj_parse_stream (urj_chain_t *chain, FILE *f)
{
char *inputline, *p;
size_t len;
@@ -248,8 +248,8 @@
go = urj_parse_line (chain, inputline);
if (go == URJ_STATUS_FAIL)
{
- urj_log (ll, "Error: %s; command '%s'\n", urj_error_describe(), inputline);
- urj_error_reset ();
+ urj_log (URJ_LOG_LEVEL_ERROR, "when parsing command '%s'\n", inputline);
+ urj_log_error_describe (URJ_LOG_LEVEL_ERROR);
}
urj_tap_chain_flush (chain);
}
@@ -261,7 +261,7 @@
}
int
-urj_parse_file (urj_log_level_t ll, urj_chain_t *chain, const char *filename)
+urj_parse_file (urj_chain_t *chain, const char *filename)
{
FILE *f;
int go;
@@ -273,7 +273,7 @@
return URJ_STATUS_FAIL;
}
- go = urj_parse_stream (ll, chain, f);
+ go = urj_parse_stream (chain, f);
fclose (f);
urj_log (URJ_LOG_LEVEL_DEBUG, "File Closed go=%d\n", go);
@@ -332,7 +332,7 @@
else
#endif
{
- r = urj_parse_file (URJ_LOG_LEVEL_NORMAL, chain, filename);
+ r = urj_parse_file (chain, filename);
}
free (path);
Index: src/apps/jtag/jtag.c
===================================================================
--- src/apps/jtag/jtag.c.orig 2011-07-13 11:44:43.000000000 -0400
+++ src/apps/jtag/jtag.c 2011-07-13 11:50:58.000000000 -0400
@@ -247,10 +247,7 @@
r = urj_parse_line (chain, line);
if (r == URJ_STATUS_FAIL)
- {
- urj_log (URJ_LOG_LEVEL_NORMAL, "Error: %s\n", urj_error_describe());
- urj_error_reset ();
- }
+ urj_log_error_describe (URJ_LOG_LEVEL_ERROR);
urj_tap_chain_flush (chain);
@@ -336,7 +333,7 @@
if (!file)
return URJ_STATUS_FAIL;
- go = urj_parse_file (URJ_LOG_LEVEL_DETAIL, chain, file);
+ go = urj_parse_file (chain, file);
free (file);
@@ -485,7 +482,7 @@
return -1;
}
- go = urj_parse_file (URJ_LOG_LEVEL_NORMAL, chain, argv[i]);
+ go = urj_parse_file (chain, argv[i]);
cleanup (chain);
if (go < 0 && go != URJ_STATUS_MUST_QUIT)
{
@@ -507,7 +504,7 @@
printf (_("Out of memory\n"));
return -1;
}
- urj_parse_stream (URJ_LOG_LEVEL_NORMAL, chain, stdin);
+ urj_parse_stream (chain, stdin);
cleanup (chain);
@@ -540,10 +537,7 @@
/* Create ~/.jtag */
if (jtag_create_jtagdir () != URJ_STATUS_OK)
- {
- urj_warning ("%s\n", urj_error_describe());
- urj_error_reset();
- }
+ urj_log_error_describe (URJ_LOG_LEVEL_WARNING);
/* Parse and execute the RC file */
if (!norc)
@@ -553,10 +547,10 @@
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_error_describe (URJ_LOG_LEVEL_ERROR);
}
- urj_error_reset();
+ else
+ urj_error_reset();
}
}
Index: src/apps/bsdl2jtag/bsdl2jtag.c
===================================================================
--- src/apps/bsdl2jtag/bsdl2jtag.c.orig 2011-07-13 11:44:43.000000000 -0400
+++ src/apps/bsdl2jtag/bsdl2jtag.c 2011-07-13 11:50:58.000000000 -0400
@@ -68,8 +68,7 @@
chain = urj_tap_chain_alloc ();
if (chain == NULL)
{
- urj_log (URJ_LOG_LEVEL_NORMAL, "Error: %s\n",
- urj_error_describe());
+ urj_log_error_describe (URJ_LOG_LEVEL_ERROR);
return 1;
}
@@ -92,10 +91,7 @@
urj_log_state.out_vprintf = log_to_file;
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_error_reset ();
- }
+ urj_log_error_describe (URJ_LOG_LEVEL_ERROR);
fclose (jtag_file);
cleanup (chain);
------------------------------------------------------------------------------
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
