[
https://issues.apache.org/jira/browse/ZOOKEEPER-1400?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13657844#comment-13657844
]
Marshall McMullen commented on ZOOKEEPER-1400:
----------------------------------------------
Stephen - thanks for the quick review. As to your questions:
1.Why in create_completion_entry do you LOG_ERROR to LOG_STREAM instead of
LOGCALLBACK? Sometimes logging to the callback and sometimes not seems like
confusing behavior for an end user.
That was an oversight. Didn't have a zhandle* in create_competion_entry so that
was probably the easy solution at the time. I've updated the patch to pass the
zhandle* through to create_completion_entry so we can use LOGCALLBACK like
everywhere else consistently.
2.Why did you change resolve_hosts to be static? If it works with it being
static, then it probably should be static, just curious of the reasoning.
It's not a publicly documented function and does not appear in zookeeper.h and
is not called anywhere outside of zookeeper.c. And there's no rational reason
for that to be a public function that I could think of as it's very much
internal. That said, it's definitely a tangental change so I will revert that
part if that's a deal breaker for anyone. But I don't think it should be.
3.You use variadic macros with log_message, and a GCC extension of them at
that. Assuming you switch to the C99 way of doing them, does the Zookeeper
library target C99? Would this work on Windows?
Not sure what you mean here... I'm not using any variadic macros. Perhaps
you're referring to my use of vsnprintf? Frankly I don't do anything on windows
so I often forget about it :). I am far from an expert in the _windows_ way to
do this, but a quick google turned up:
http://msdn.microsoft.com/en-us/library/w05tbk72%28VS.71%29.aspx
_vscprintf returns the number of characters that would be generated if the
string pointed to by the list of arguments was printed or sent to a file or
buffer using the specified formatting codes. The value returned does not
include the terminating null character.
Wonder why they went to the trouble to implement this instead of just
implementing vsnprintf and comply with C99.
Anyhow, clarify for me if that is what you're referring to. As I don't have any
access to a windows machine or compiler would need someone interested in that
to assist testing out my proposed solution.
> 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.5.0
> Environment: Linux
> Reporter: Marshall McMullen
> Assignee: Marshall McMullen
> Fix For: 3.5.0
>
> Attachments: ZOOKEEPER-1400.patch, ZOOKEEPER-1400.patch,
> ZOOKEEPER-1400.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
For more information on JIRA, see: http://www.atlassian.com/software/jira
