578496 - PRLog should use the android log system when a log file isn't specified

Status: RESOLVED FIXED

(NSPR :: NSPR, defect)

Description

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

generally stdout and stderr are pretty useless to us

Comment 1

[Wan-Teh Chang]

Comment on attachment 457160 [details] [diff] [review]
patch

In prlog.c:

>+#define _PUT_LOG(fd, buf, nb)                                   \
>+  PR_BEGIN_MACRO                                                \
>+  if (fd == PR_STDERR || fd == PR_STDOUT)                       \
>+    __android_log_print(ANDROID_LOG_INFO, "PRLog" , buf, nb);   \
>+  else                                                          \
>+    PR_Write(fd, buf, nb);                                      \
>+  PR_END_MACRO

In NSPR, the indentation level is 4, not 2.
I can fix that for you when I check this in.

Two questions:

1. Could you change PR_STDERR to _pr_stderr
and PR_STDOUT to _pr_stdout?

PR_STDERR and PR_STDOUT expand to function calls,
so they're more expensive than referring to the
global variables _pr_stderr and _pr_stdout directly.

2. Please search for "WinDebug" in prlog.c and
see if an approach like that (setting the
environment variable NSPR_LOG_FILE to some
magic string, say "AndroidLog", to coerce
log output to use the Android log) would
work for you.

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch that tests _pr_stderr

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch that tests for AndroidLog

both those options work. Take your pick.

Comment 4

[Wan-Teh Chang]

Comment on attachment 459707 [details] [diff] [review]
patch that tests _pr_stderr

Brad, thanks a lot for implementing both approaches.
Let's go with this patch for its simplicity.
                                          \
>+        __android_log_print(ANDROID_LOG_INFO, "PRLog" , buf, nb); \

I believe that the 'nb' argument is extraneous because
__android_log_print treats 'buf' as a printf format
string, which doesn't have any % format specifier in it.
So we should be able to use __android_log_write instead.

Does <android/log.h> have any function that prints a
buffer with the length specified?  That'll allow us
to get rid of the savebyte business.  Using web search,
I found a __android_log_bwrite function, but I'm not
sure if it's appropriate to use it.

Comment 5

[Brad Lassey [:blassey] (use needinfo?)]

>                                           \
> >+        __android_log_print(ANDROID_LOG_INFO, "PRLog" , buf, nb); \
> 

yea, sorry nb shouldn't be there.

> I believe that the 'nb' argument is extraneous because
> __android_log_print treats 'buf' as a printf format
> string, which doesn't have any % format specifier in it.
> So we should be able to use __android_log_write instead.
> 
> Does <android/log.h> have any function that prints a
> buffer with the length specified?  That'll allow us
> to get rid of the savebyte business.  Using web search,
> I found a __android_log_bwrite function, but I'm not
> sure if it's appropriate to use it.

log.h has __android_log_write, __android_log_printf and __android_log_vprintf. Of these __android_log_write seems most appropriate.  Where did you find __android_log_bwrite?

Comment 6

[Wan-Teh Chang]

I found it here:
http://www.netmite.com/android/mydroid/system/core/include/cutils/log.h
http://www.netmite.com/android/mydroid/system/core/liblog/logd_write.c

Comment 7

[Wan-Teh Chang]

Attachment: patch that tests _pr_stderr, v2, by Brad Lassey

I edited Brad's patch and checked it in on the NSPR trunk
(NSPR 4.8.7).

Checking in Makefile.in;
/cvsroot/mozilla/nsprpub/pr/src/Makefile.in,v  <--  Makefile.in
new revision: 1.58; previous revision: 1.57
done
Checking in io/prlog.c;
/cvsroot/mozilla/nsprpub/pr/src/io/prlog.c,v  <--  prlog.c
new revision: 3.52; previous revision: 3.51
done