Hi,
I intend to commit the attached patch. It re-adds the syslog
appender (fixes LOGCXX-66).
The memory leak which was reported some days ago should be fixed.
I dont really like the memory pools being passed from one
method to another (and probably this would not have helped here
because it would probably still have been the same pool then).
After asking on the dev@apr.apache.org mailing list and
reading http://subversion.tigris.org/hacking.html#apr-pools
and http://www.apachetutor.org/dev/pools, I think a better
solution is to use the object-specific memory pool for the
socket only (because the lifetime of the APR socket and the
lifetime of the DatagramSocket object are aligned), and use
local pools for the apr_sockaddr_info_get() call (the
apr_sockaddr_t data is not needed anymore when the method
returns).
Apart from that, the build.xml always configures the local
syslog() function to be available on UNIX and to be not
available on Windows, while the autoconf configure script
checks if the function is actually available.
Thanks & Best regards,
Andreas
Index: include/log4cxx/helpers/datagramsocket.h
===================================================================
--- include/log4cxx/helpers/datagramsocket.h (Revision 394078)
+++ include/log4cxx/helpers/datagramsocket.h (Arbeitskopie)
@@ -20,6 +20,7 @@
#include <log4cxx/helpers/objectimpl.h>
#include <log4cxx/helpers/objectptr.h>
#include <log4cxx/helpers/inetaddress.h>
+#include <log4cxx/helpers/pool.h>
namespace log4cxx
{
@@ -91,7 +92,7 @@
/** Returns wether the socket is closed or not. */
inline bool isClosed() const
- { return fd != 0; }
+ { return socket != 0; }
/** Returns the connection state of the socket. */
inline bool isConnected() const
@@ -104,11 +105,16 @@
void send(DatagramPacketPtr& p);
protected:
- /** The file descriptor object for this socket. */
- int fd;
+ /** The APR socket */
+ void *socket;
+ /** The memory pool for the socket */
+ Pool socketPool;
+
InetAddress address;
+
InetAddress localAddress;
+
int port;
/** The local port number to which this socket is
connected. */
Index: include/log4cxx/private/log4cxx_private.h.in
===================================================================
--- include/log4cxx/private/log4cxx_private.h.in (Revision 394078)
+++ include/log4cxx/private/log4cxx_private.h.in (Arbeitskopie)
@@ -54,6 +54,6 @@
// attempting creation of the corresponding appender.
//
#define LOG4CXX_HAVE_SMTP 0
-#define LOG4CXX_HAVE_SYSLOG 1
+#define LOG4CXX_HAVE_SYSLOG @HAS_SYSLOG@
#endif
Index: configure.in
===================================================================
--- configure.in (Revision 394078)
+++ configure.in (Arbeitskopie)
@@ -154,10 +154,16 @@
AC_CHECK_FUNCS(swprintf)
-# for SysLogAppender
+# for local syslog() function for SyslogAppender
+AC_CHECK_FUNCS(syslog, [have_syslog=yes], [have_syslog=no])
+if test "$have_syslog" = "yes"
+then
+ AC_SUBST(HAS_SYSLOG, 1)
+else
+ AC_SUBST(HAS_SYSLOG, 0)
+fi
+
-AC_CHECK_FUNCS(syslog)
-
# Checks for libraries
# ----------------------------------------------------------------------------
Index: src/syslogappender.cpp
===================================================================
--- src/syslogappender.cpp (Revision 394078)
+++ src/syslogappender.cpp (Arbeitskopie)
@@ -21,6 +21,7 @@
#include <log4cxx/spi/loggingevent.h>
#include <log4cxx/level.h>
#include <log4cxx/helpers/transcoder.h>
+#include <log4cxx/private/log4cxx_private.h>
#ifdef LOG4CXX_HAVE_SYSLOG
#include <syslog.h>
Index: src/syslogwriter.cpp
===================================================================
--- src/syslogwriter.cpp (Revision 394078)
+++ src/syslogwriter.cpp (Arbeitskopie)
@@ -20,6 +20,7 @@
#include <log4cxx/helpers/datagramsocket.h>
#include <log4cxx/helpers/datagrampacket.h>
#include <log4cxx/helpers/socketimpl.h>
+#include <log4cxx/helpers/transcoder.h>
#define SYSLOG_PORT 514
@@ -50,18 +51,14 @@
}
}
-void SyslogWriter::write(const LogString&)
-{
-#if 0
-// TODO
- USES_CONVERSION;
- const char * bytes = T2A(string.c_str());
- DatagramPacketPtr packet = new DatagramPacket((void *)bytes,
string.length() + 1,
- address, SYSLOG_PORT);
+void SyslogWriter::write(const LogString& source) {
+ LOG4CXX_ENCODE_CHAR(data, source);
- if(this->ds != 0)
- {
+ DatagramPacketPtr packet =
+ new DatagramPacket((void*) data.c_str(), data.length() + 1,
+ address, SYSLOG_PORT);
+
+ if(this->ds != 0) {
ds->send(packet);
}
-#endif
}
Index: src/datagramsocket.cpp
===================================================================
--- src/datagramsocket.cpp (Revision 394078)
+++ src/datagramsocket.cpp (Arbeitskopie)
@@ -14,38 +14,27 @@
* limitations under the License.
*/
-
-#if defined(_WIN32)
-#include <windows.h>
-#include <winsock.h>
-#else
-#include <sys/types.h>
-#include <sys/socket.h>
-#include <netinet/in.h>
-#include <arpa/inet.h>
-#include <unistd.h>
-#include <netdb.h>
-#include <sys/time.h>
-#include <sys/types.h>
-#endif
-
#include <log4cxx/helpers/datagrampacket.h>
#include <log4cxx/helpers/datagramsocket.h>
#include <log4cxx/helpers/loglog.h>
+#include <log4cxx/helpers/transcoder.h>
#include <log4cxx/helpers/socketimpl.h>
+#include "apr_network_io.h"
+#include "apr_lib.h"
+
using namespace log4cxx::helpers;
IMPLEMENT_LOG4CXX_OBJECT(DatagramSocket);
DatagramSocket::DatagramSocket()
- : fd(0), address(), localAddress(), port(0), localPort(0)
+ : socket(0), address(), localAddress(), port(0), localPort(0)
{
create();
}
DatagramSocket::DatagramSocket(int localPort)
- : fd(0), address(), localAddress(), port(0), localPort(0)
+ : socket(0), address(), localAddress(), port(0), localPort(0)
{
InetAddress bindAddr;
bindAddr.address = INADDR_ANY;
@@ -55,7 +44,7 @@
}
DatagramSocket::DatagramSocket(int localPort, InetAddress localAddress)
- : fd(0), address(), localAddress(), port(0), localPort(0)
+ : socket(0), address(), localAddress(), port(0), localPort(0)
{
create();
bind(localPort, localAddress);
@@ -75,16 +64,22 @@
/** Binds a datagram socket to a local port and address.*/
void DatagramSocket::bind(int localPort, InetAddress localAddress)
{
- struct sockaddr_in server_addr;
- int server_len = sizeof(server_addr);
+ Pool addrPool;
- server_addr.sin_family = AF_INET;
- server_addr.sin_addr.s_addr = htonl(localAddress.address);
- server_addr.sin_port = htons(localPort);
+ // Create server socket address
+ LOG4CXX_ENCODE_CHAR(hostAddr, localAddress.getHostAddress());
+ apr_sockaddr_t *server_addr;
+ apr_status_t status =
+ apr_sockaddr_info_get(&server_addr, hostAddr.c_str(), APR_INET,
+ localPort, 0, (apr_pool_t*)
addrPool.getAPRPool());
+ if (status != APR_SUCCESS) {
+ throw BindException(status);
+ }
- if (::bind(fd, (sockaddr *)&server_addr, server_len) == -1)
- {
- throw BindException();
+ // bind the socket to the address
+ status = apr_socket_bind((apr_socket_t*) socket, server_addr);
+ if (status != APR_SUCCESS) {
+ throw BindException(status);
}
this->localPort = localPort;
@@ -94,97 +89,100 @@
/** Close the socket.*/
void DatagramSocket::close()
{
- if (fd != 0)
- {
+ if (socket != 0) {
LOGLOG_DEBUG(LOG4CXX_STR("closing socket"));
-#if defined(WIN32) || defined(_WIN32)
- if (::closesocket(fd) == -1)
-#else
- if (::close(fd) == -1)
-#endif
- {
- throw SocketException();
+ apr_status_t status = apr_socket_close((apr_socket_t*) socket);
+ if (status != APR_SUCCESS) {
+ throw SocketException(status);
}
- fd = 0;
+ socket = 0;
localPort = 0;
}
}
void DatagramSocket::connect(InetAddress address, int port)
{
- sockaddr_in client_addr;
- int client_len = sizeof(client_addr);
- client_addr.sin_family = AF_INET;
- client_addr.sin_addr.s_addr = htonl(address.address);
- client_addr.sin_port = htons(port);
+ this->address = address;
+ this->port = port;
- if (::connect(fd, (sockaddr *)&client_addr, client_len) == -1)
- {
- throw ConnectException();
+ Pool addrPool;
+
+ // create socket address
+ LOG4CXX_ENCODE_CHAR(hostAddr, address.getHostAddress());
+ apr_sockaddr_t *client_addr;
+ apr_status_t status =
+ apr_sockaddr_info_get(&client_addr, hostAddr.c_str(), APR_INET,
+ port, 0, (apr_pool_t*) addrPool.getAPRPool());
+ if (status != APR_SUCCESS) {
+ throw ConnectException(status);
}
- this->address = address;
- this->port = port;
+ // connect the socket
+ status = apr_socket_connect((apr_socket_t*) socket, client_addr);
+ if (status != APR_SUCCESS) {
+ throw ConnectException();
+ }
}
/** Creates a datagram socket.*/
void DatagramSocket::create()
{
- if ((fd = ::socket(AF_INET, SOCK_DGRAM, 0)) == -1)
- {
- throw SocketException();
- }
+ apr_socket_t* newSocket;
+ apr_status_t status =
+ apr_socket_create(&newSocket, APR_INET, SOCK_DGRAM,
+ APR_PROTO_UDP, (apr_pool_t*) socketPool.getAPRPool());
+ socket = newSocket;
+ if (status != APR_SUCCESS) {
+ throw SocketException(status);
+ }
}
/** Receive the datagram packet.*/
void DatagramSocket::receive(DatagramPacketPtr& p)
{
- sockaddr_in addr;
- int addr_len = sizeof(addr);
+ Pool addrPool;
- addr.sin_family = AF_INET;
- addr.sin_addr.s_addr = htonl(p->getAddress().address);
- addr.sin_port = htons(p->getPort());
-
-#if defined(WIN32) || defined(_WIN32)
- if (::recvfrom(fd, (char *)p->getData(), p->getLength(), 0,
- (sockaddr *)&addr, &addr_len) == -1)
-#elif defined(__hpux)
- if (::recvfrom(fd, p->getData(), p->getLength(), 0,
- (sockaddr *)&addr, &addr_len) == -1)
-#else
- if (::recvfrom(fd, p->getData(), p->getLength(), 0,
- (sockaddr *)&addr, (socklen_t *)&addr_len) == -1)
-#endif
- {
- throw IOException();
+ // Create the address from which to receive the datagram packet
+ LOG4CXX_ENCODE_CHAR(hostAddr, p->getAddress().getHostAddress());
+ apr_sockaddr_t *addr;
+ apr_status_t status =
+ apr_sockaddr_info_get(&addr, hostAddr.c_str(), APR_INET,
+ p->getPort(), 0, (apr_pool_t*)
addrPool.getAPRPool());
+ if (status != APR_SUCCESS) {
+ throw SocketException(status);
}
+ // receive the datagram packet
+ apr_size_t len = p->getLength();
+ status = apr_socket_recvfrom(addr, (apr_socket_t*) socket, 0,
+ (char *)p->getData(), &len);
+ if (status != APR_SUCCESS) {
+ throw IOException(status);
+ }
}
/** Sends a datagram packet.*/
void DatagramSocket::send(DatagramPacketPtr& p)
{
- sockaddr_in addr;
- int addr_len = sizeof(addr);
+ Pool addrPool;
- addr.sin_family = AF_INET;
- addr.sin_addr.s_addr = htonl(p->getAddress().address);
- addr.sin_port = htons(p->getPort());
+ // create the adress to which to send the datagram packet
+ LOG4CXX_ENCODE_CHAR(hostAddr, p->getAddress().getHostAddress());
+ apr_sockaddr_t *addr;
+ apr_status_t status =
+ apr_sockaddr_info_get(&addr, hostAddr.c_str(), APR_INET, p->getPort(),
+ 0, (apr_pool_t*) addrPool.getAPRPool());
+ if (status != APR_SUCCESS) {
+ throw SocketException(status);
+ }
-#if defined(WIN32) || defined(_WIN32)
- if (::sendto(fd, (const char *)p->getData(), p->getLength(), 0,
- (sockaddr *)&addr, addr_len) == -1)
-#else
- if (::sendto(fd, p->getData(), p->getLength(), 0,
- (sockaddr *)&addr, addr_len) == -1)
-#endif
- {
- throw IOException();
+ // send the datagram packet
+ apr_size_t len = p->getLength();
+ status = apr_socket_sendto((apr_socket_t*) socket, addr, 0,
+ (char *)p->getData(), &len);
+ if (status != APR_SUCCESS) {
+ throw IOException(status);
}
}
-
-
-
Index: build.xml
===================================================================
--- build.xml (Revision 394078)
+++ build.xml (Arbeitskopie)
@@ -420,8 +420,15 @@
replace="${logchar_is_wchar}"/>
<replaceregexp file="${include.dir}/log4cxx/log4cxx.h"
- match="#define LOG4CXX_HAS_WCHAR_T.*"
- replace="#define LOG4CXX_HAS_WCHAR_T ${has.wchar_t}"/>
+ match="@HAS_WCHAR_T@"
+ replace="${has.wchar_t}"/>
+
+ <!-- The MS Windows template for log4cxx_private.h contains a hard coded
+ "0" for LOG4CXX_HAVE_SYSLOG. Therefore the following replacement
+ only applies to the UNIX template. -->
+ <replaceregexp file="${include.dir}/log4cxx/private/log4cxx_private.h"
+ match="@HAS_SYSLOG@"
+ replace="1"/>
</target>
<target name="get-apr-module">
