[jira] [Commented] (ZOOKEEPER-1400) Allow logging via callback instead of raw FILE pointer

["Stephen Tyree (Commented) (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/ZOOKEEPER-1400?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13230364#comment-13230364 ] 

Stephen Tyree commented on ZOOKEEPER-1400:
------------------------------------------

Sorry for the delay

I think that would make sense to be a separate JIRA. As far as your change goes it looks good, but a couple of points:

+    char* buf = get_format_log_buffer();

This means there is now a limit on log size that wasn't there before. Have you made sure this buffer can handle all possible log messages?

Also, you made a bunch of formatting changes that seem tangential to the change itself, like the following:

-    static const char* dbgLevelStr[]={"ZOO_INVALID","ZOO_ERROR","ZOO_WARN",
-            "ZOO_INFO","ZOO_DEBUG"};
+    char* buf = get_format_log_buffer();
+    if(!buf)
+        return;
+
+    static const char* dbgLevelStr[] = {"ZOO_INVALID",
+                                        "ZOO_ERROR",
+                                        "ZOO_WARN",
+                                        "ZOO_INFO",
+                                        "ZOO_DEBUG"};

Maybe those should be reserved for a separate change just to keep things tidy?
                
> Allow logging via callback instead of raw FILE pointer
> ------------------------------------------------------
>
>                 Key: ZOOKEEPER-1400
>                 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-1400
>             Project: ZooKeeper
>          Issue Type: Improvement
>          Components: c client
>    Affects Versions: 3.4.2
>         Environment: Linux
>            Reporter: Marshall McMullen
>             Fix For: 3.4.2
>
>         Attachments: case-2739.patch
>
>
> The existing logging framework inside the C client uses a raw FILE*. Using a FILE* is very limiting and potentially dangerous. A safer alternative is to just provide a callback that the C client will call for each message. In our environment, we saw some really nasty issues with multiple threads all connecting to zookeeper via the C Client related to the use of a raw FILE*. Specifically, if the FILE * is closed and that file descriptor is reused by the kernel before the C client is notified then the C client will use it's static global logStream pointer for subsequent logging messages. That FILE* is now a loose cannon! In our environment, we saw zookeeper log messages ending up in other sockets and even in our core data path. Clearly this is dangerous. In our particular case, we'd omitted a call to zoo_set_log_stream(NULL) to notify C client that the FILE* has been closed. However, even with that bug fixed, there's still a race condition where log messages in flight may be sent before the C client is notified of the FILE closure, and the same problem can happen.
> Other issues we've seen involved multiple threads, wherein one would close the FILE*, and that's a global change that affects all threads connected within that process. That's a pretty nasty limitation as well.
> My proposed change is to allow setting a callback for log messages. A callback is used in preference to a raw FILE*. If no callback is set, then it will fallback to the existing FILE*. If that's not set, then it falls back to stderr as it always has.
> While refactoring this code, I removed the need for the double parens in all the LOG macros as that wasn't necessary and didn't fit with my new approach.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira