[Spice-devel] [PATCH spice-xpi 3/4] Switch from log4cpp to glib
Alon Levy
alevy at redhat.com
Fri Feb 24 03:10:11 PST 2012
On Fri, Feb 24, 2012 at 11:54:37AM +0100, Marc-André Lureau wrote:
> GLib is used in the defacto library for desktop projects. It includes
> logging facilities that can be filtered by domain and level, and can
> be redirected or formated in various ways. Instead of pulling an extra
> large depedency for a small plugin, let's use what is common to the
> desktop.
>
> Afaict, nobody else is using log4cpp in Fedora. The configuration file
> is not user friendly. In comparison, you can just run
> "G_MESSAGES_DEBUG=SpiceXPI firefox" and easily modify logging via
> environment variable. There is no logging to file, and there is no
> configuration file. If you want to redirect to file, you can do so via
> redirection, or other mechanisms (ldpreload, environment..).
I was with you until you suggested to use ldpreload. Is there an actual
easy way to do this to redirect error messages somewhere?
So how would we make sure there is a log file for SpiceXPI?
Other then that the patch looks good. (small comment at the end, again
nitpick).
> ---
> SpiceXPI/src/plugin/Makefile.am | 33 ++++-----
> SpiceXPI/src/plugin/controller.cpp | 21 ++----
> SpiceXPI/src/plugin/debug.h | 143 ------------------------------------
> SpiceXPI/src/plugin/plugin.cpp | 64 ++++++----------
> configure.ac | 8 +-
> 5 files changed, 49 insertions(+), 220 deletions(-)
> delete mode 100644 SpiceXPI/src/plugin/debug.h
>
> diff --git a/SpiceXPI/src/plugin/Makefile.am b/SpiceXPI/src/plugin/Makefile.am
> index e409d94..08d89d7 100644
> --- a/SpiceXPI/src/plugin/Makefile.am
> +++ b/SpiceXPI/src/plugin/Makefile.am
> @@ -4,33 +4,31 @@ FIREFOX_APPID = {ec8030f7-c20a-464f-9b0e-13a3a9e97384}
> extensiondir = $(libdir)/mozilla
> SDK_INCLUDE_DIR = `pkg-config --variable=idldir libxul`
>
> -INCLUDES = \
> +plugindir=$(extensiondir)/plugins
> +
> +plugin_LTLIBRARIES = libnsISpicec.la
> +
> +libnsISpicec_la_LDFLAGS = -avoid-version -module
> +libnsISpicec_la_CPPFLAGS = \
> + -I$(XUL_INCLUDEDIR) \
> + -I$(XUL_INCLUDEDIR)/dom \
> + -I$(XUL_INCLUDEDIR)/necko \
> + -I$(XUL_INCLUDEDIR)/plugin \
> + -I$(XUL_INCLUDEDIR)/string \
> -I$(top_srcdir)/common \
> - $(XUL_CFLAGS) \
> + $(GLIB_CFLAGS) \
> $(SPICE_PROTOCOL_CFLAGS) \
> - $(LOG4CPP_CFLAGS) \
> + $(XUL_CFLAGS) \
> -DCAIRO_CANVAS_ACCESS_TEST \
> -DCAIRO_CANVAS_CACHE \
> -DCAIRO_CANVAS_NO_CHUNKS \
> + -DG_LOG_DOMAIN=\"SpiceXPI\" \
> -DMOZILLA_INTERNAL_API \
> -DXP_UNIX \
> $(NULL)
>
> -plugindir=$(extensiondir)/plugins
> -
> -plugin_LTLIBRARIES = libnsISpicec.la
> -
> -libnsISpicec_la_LDFLAGS = -avoid-version -module
> -libnsISpicec_la_CPPFLAGS = \
> - -I $(XUL_INCLUDEDIR) \
> - -I $(XUL_INCLUDEDIR)/dom \
> - -I $(XUL_INCLUDEDIR)/necko \
> - -I $(XUL_INCLUDEDIR)/plugin \
> - -I $(XUL_INCLUDEDIR)/string \
> - $(NULL)
> -
> libnsISpicec_la_LIBADD = \
> - $(LOG4CPP_LIBS) \
> + $(GLIB_LIBS) \
> $(XUL_LIBS) \
> $(NULL)
>
> @@ -39,7 +37,6 @@ libnsISpicec_la_SOURCES = \
> $(top_srcdir)/common/rederrorcodes.h \
> controller.cpp \
> controller.h \
> - debug.h \
> np_entry.cpp \
> npn_gate.cpp \
> npp_gate.cpp \
> diff --git a/SpiceXPI/src/plugin/controller.cpp b/SpiceXPI/src/plugin/controller.cpp
> index 256dbf9..131cfc5 100644
> --- a/SpiceXPI/src/plugin/controller.cpp
> +++ b/SpiceXPI/src/plugin/controller.cpp
> @@ -44,6 +44,7 @@
> #include <cstdlib>
> #include <cstring>
> #include <cerrno>
> +#include <glib.h>
>
> extern "C" {
> # include <stdint.h>
> @@ -53,17 +54,11 @@ extern "C" {
> # include <sys/un.h>
> }
>
> -#include "debug.h"
> #include "rederrorcodes.h"
> #include "controller.h"
>
> const char *channel_names[] = { "dummy", "main", "display", "inputs", "cursor", "playback", "record" };
>
> -void QErrorHandler(int err, const char *custom_string)
> -{
> - LOG_DEBUG("Something went wrong: " << custom_string << ", " << err);
> -}
> -
> SpiceController::SpiceController():
> m_client_socket(-1)
> {
> @@ -77,7 +72,7 @@ SpiceController::SpiceController(const std::string &name):
>
> SpiceController::~SpiceController()
> {
> - LOG_TRACE("");
> + g_debug(G_STRFUNC);
> Disconnect();
> }
>
> @@ -96,7 +91,7 @@ int SpiceController::Connect()
> {
> if ((m_client_socket = socket(AF_UNIX, SOCK_STREAM, 0)) == -1)
> {
> - QErrorHandler(errno, "SpiceController::Connect socket create error");
> + g_critical("controller socket: %s", g_strerror(errno));
> return -1;
> }
> }
> @@ -108,12 +103,11 @@ int SpiceController::Connect()
> int rc = connect(m_client_socket, (struct sockaddr *) &remote, strlen(remote.sun_path) + sizeof(remote.sun_family));
> if (rc == -1)
> {
> - QErrorHandler(errno, "connect error");
> - LOG_DEBUG("Connect Error");
> + g_critical("controller connect: %s", g_strerror(errno));
> }
> else
> {
> - LOG_DEBUG("Connected!");
> + g_debug("controller connected");
> }
>
> return rc;
> @@ -152,9 +146,8 @@ uint32_t SpiceController::Write(const void *lpBuffer, uint32_t nBytesToWrite)
>
> if (len != (ssize_t)nBytesToWrite)
> {
> - LOG_WARN("send error, bytes to write = " << nBytesToWrite <<
> - ", bytes actually written = " << len << ", errno = " << errno);
> - QErrorHandler(1, "send error");
> + g_warning("incomplete send, bytes to write = %lu, bytes written = %d: %s",
> + nBytesToWrite, len, g_strerror(errno));
> }
>
> return len;
> diff --git a/SpiceXPI/src/plugin/debug.h b/SpiceXPI/src/plugin/debug.h
> deleted file mode 100644
> index 4b58491..0000000
> --- a/SpiceXPI/src/plugin/debug.h
> +++ /dev/null
> @@ -1,143 +0,0 @@
> -/* ***** BEGIN LICENSE BLOCK *****
> - * Version: MPL 1.1/GPL 2.0/LGPL 2.1
> - *
> - * The contents of this file are subject to the Mozilla Public License Version
> - * 1.1 (the "License"); you may not use this file except in compliance with
> - * the License. You may obtain a copy of the License at
> - * http://www.mozilla.org/MPL/
> - *
> - * Software distributed under the License is distributed on an "AS IS" basis,
> - * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
> - * for the specific language governing rights and limitations under the
> - * License.
> - *
> - * Copyright 2009-2011, Red Hat Inc.
> - * Based on mozilla.org's scriptable plugin example
> - *
> - * The Original Code is mozilla.org code.
> - *
> - * The Initial Developer of the Original Code is
> - * Netscape Communications Corporation.
> - * Portions created by the Initial Developer are Copyright (C) 1998
> - * the Initial Developer. All Rights Reserved.
> - *
> - * Contributor(s):
> - * Uri Lublin
> - *
> - * Alternatively, the contents of this file may be used under the terms of
> - * either the GNU General Public License Version 2 or later (the "GPL"), or
> - * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
> - * in which case the provisions of the GPL or the LGPL are applicable instead
> - * of those above. If you wish to allow use of your version of this file only
> - * under the terms of either the GPL or the LGPL, and not to allow others to
> - * use your version of this file under the terms of the MPL, indicate your
> - * decision by deleting the provisions above and replace them with the notice
> - * and other provisions required by the GPL or the LGPL. If you do not delete
> - * the provisions above, a recipient may use your version of this file under
> - * the terms of any one of the MPL, the GPL or the LGPL.
> - *
> - * ***** END LICENSE BLOCK ***** */
> -
> -#ifndef DEBUG_H
> -#define DEBUG_H
> -
> -#include <sstream>
> -
> -#include <log4cpp/Category.hh>
> -#include <log4cpp/convenience.h>
> -
> -#define ON_PANIC() ::abort()
> -
> -#ifdef RED_DEBUG
> -
> -# ifdef WIN32
> -# define ASSERTBREAK DebugBreak()
> -# else
> -# define ASSERTBREAK ::abort()
> -# endif
> -
> -# define ASSERT(x) if (!(x)) { \
> - printf("%s: ASSERT %s failed\n", __FUNCTION__, #x); \
> - ASSERTBREAK; \
> -}
> -
> -#else
> -# define ASSERT(cond)
> -#endif
> -
> -#ifdef __GNUC__
> -static inline std::string pretty_func_to_func_name(const std::string& f_name)
> -{
> - std::string name(f_name);
> - std::string::size_type end_pos = f_name.find('(');
> - if (end_pos == std::string::npos) {
> - return f_name;
> - }
> - std::string::size_type start = f_name.rfind(' ', end_pos);
> - if (start == std::string::npos) {
> - return f_name;
> - }
> - end_pos -= ++start;
> - return name.substr(start, end_pos);
> -}
> -# define FUNC_NAME pretty_func_to_func_name(__PRETTY_FUNCTION__).c_str()
> -#else
> -# define FUNC_NAME __FUNCTION__
> -#endif
> -
> -LOG4CPP_LOGGER("spice")
> -
> -#define LOG(func, message) { \
> - std::ostringstream os; \
> - os << FUNC_NAME; \
> - os << ": "; \
> - os << message; \
> - func(logger, os.str().c_str()); \
> -}
> -
> -// Used to log just about everything that might be useful (or not).
> -#define LOG_TRACE(message) LOG(LOG4CPP_DEBUG, message)
> -
> -// Used to log debug information which doesn't inflate the log with useless
> -// and repeated messages.
> -#define LOG_DEBUG(message) LOG(LOG4CPP_DEBUG, message)
> -
> -// Used to log the program flow.
> -#define LOG_INFO(message) LOG(LOG4CPP_INFO, message)
> -
> -// Used to log errors which are handled by the program.
> -#define LOG_WARN(message) LOG(LOG4CPP_WARN, message)
> -
> -// Used to log errors that can't be handled by the program.
> -#define LOG_ERROR(message) LOG(LOG4CPP_ERROR, message)
> -
> -// Used to log errors that can't happen (even if they do!).
> -#define LOG_FATAL(message) { \
> - LOG(LOG4CPP_EMERG, message); \
> - ON_PANIC(); \
> -}
> -
> -#if 0
> -# define DBGLEVEL 1000
> -
> -# define DBG(level, msg) { \
> - if (level <= DBGLEVEL) { \
> - std::ostringstream os; \
> - os << __FUNCTION__; \
> - os << ": "; \
> - os << msg; \
> - os << "\n"; \
> - std::cout << os.str(); \
> - std::cout.flush(); \
> - } \
> -}
> -#endif
> -
> -#define PANIC(msg) LOG_FATAL(msg)
> -#define WARN(msg) LOG_WARN(msg)
> -#define ERR(msg) LOG_ERROR(msg)
> -#define NOTIFY(msg) LOG_INFO(msg)
> -#define INFO(msg) LOG_INFO(msg)
> -#define DBG(level, msg) LOG_DEBUG(msg)
> -
> -#endif // DEBUG_H
> diff --git a/SpiceXPI/src/plugin/plugin.cpp b/SpiceXPI/src/plugin/plugin.cpp
> index c3d2d91..f5c0500 100644
> --- a/SpiceXPI/src/plugin/plugin.cpp
> +++ b/SpiceXPI/src/plugin/plugin.cpp
> @@ -46,6 +46,7 @@
>
> #include <sys/types.h>
> #include <sys/wait.h>
> +#include <sys/stat.h>
>
> #include <stdlib.h>
> #include <dlfcn.h>
> @@ -54,6 +55,7 @@
> #include <string>
> #include <sstream>
> #include <signal.h>
> +#include <glib.h>
>
> #include "nsCOMPtr.h"
>
> @@ -73,15 +75,11 @@ static NS_DEFINE_CID(kPluginManagerCID, NS_PLUGINMANAGER_CID);
> #include <nsIServiceManager.h>
> #include <nsISupportsUtils.h> // some usefule macros are defined here
>
> -
> -#include <log4cpp/PropertyConfigurator.hh>
> -#include <log4cpp/BasicConfigurator.hh>
> -
> #include <fstream>
> #include <set>
> +
> #include "config.h"
> #include "controller.h"
> -#include "debug.h"
> #include "plugin.h"
> #include "nsScriptablePeer.h"
>
> @@ -197,18 +195,6 @@ nsPluginInstance::nsPluginInstance(NPP aInstance):
> char tmp_dir[] = "/tmp/spicec-XXXXXX";
> m_tmp_dir = mkdtemp(tmp_dir);
>
> - // configure log4cpp
> - std::ifstream log_init(LOGGER_CONFIG.c_str());
> - if (log_init.good())
> - {
> - log_init.close();
> - log4cpp::PropertyConfigurator::configure(LOGGER_CONFIG.c_str());
> - }
> - else
> - {
> - log4cpp::BasicConfigurator::configure();
> - }
> -
> m_connected_status = -2;
>
> struct sigaction chld;
> @@ -565,7 +551,7 @@ void nsPluginInstance::Connect()
> socket_file += "/spice-xpi";
> if (setenv("SPICE_XPI_SOCKET", socket_file.c_str(), 1))
> {
> - LOG_ERROR ("could not set SPICE_XPI_SOCKET env variable");
> + g_critical("could not set SPICE_XPI_SOCKET env variable");
> return;
> }
>
> @@ -577,27 +563,27 @@ void nsPluginInstance::Connect()
> }
>
> pid_t child = fork();
> - LOG_DEBUG("child pid: " << child);
> + g_debug("child pid: %llu", (guint64)child);
> if (child == 0)
> {
> - close (pipe_fds[1]);
> + close(pipe_fds[1]);
> pipe_fds[1] = -1;
>
> char c;
> if (read(pipe_fds[0], &c, 1) != 0)
> - LOG_ERROR("Invalid read on pipe");
> + g_critical("Error while reading on pipe: %s", g_strerror(errno));
>
> - close (pipe_fds[0]);
> + close(pipe_fds[0]);
> pipe_fds[0] = -1;
>
> execl("/usr/libexec/spice-xpi-client", "/usr/libexec/spice-xpi-client", NULL);
> - LOG_ERROR("ERROR failed to run spice-xpi-client");
> + g_message("failed to run spice-xpi-client, running spicec instead");
>
> // TODO: temporary fallback for backward compatibility
> execl("/usr/bin/spicec", "/usr/bin/spicec", "--controller", NULL);
> - LOG_ERROR("ERROR failed to run spicec fallback");
> + g_critical("ERROR failed to run spicec fallback");
>
> - exit(1);
> + exit(EXIT_FAILURE);
> }
> else
> {
> @@ -613,7 +599,7 @@ void nsPluginInstance::Connect()
>
> if (m_external_controller.Connect(10) != 0)
> {
> - LOG_ERROR ("could not connect to spice client controller");
> + g_critical("could not connect to spice client controller");
> return;
> }
>
> @@ -636,7 +622,7 @@ void nsPluginInstance::Connect()
> }
> else
> {
> - LOG_ERROR ("could not open truststore temp file");
> + g_critical("could not open truststore temp file");
> close(fd);
> unlink(m_trust_store_file.c_str());
> m_trust_store_file.clear();
> @@ -645,7 +631,7 @@ void nsPluginInstance::Connect()
> }
> else
> {
> - LOG_ERROR ("could not create truststore temp file. errno=" << errno);
> + g_critical("could not create truststore temp file: %s", g_strerror(errno));
> return;
> }
>
> @@ -689,7 +675,7 @@ void nsPluginInstance::Connect()
>
> void nsPluginInstance::Show()
> {
> - LOG_DEBUG("sending show message");
> + g_debug("sending show message");
> SendMsg(CONTROLLER_SHOW);
> }
>
> @@ -733,7 +719,7 @@ void nsPluginInstance::CallOnDisconnected(int code)
> NPObject *window = NULL;
> if (NPN_GetValue(m_instance, NPNVWindowNPObject, &window) != NPERR_NO_ERROR)
> {
> - LOG_ERROR("could not get browser window, when trying to call OnDisconnected");
> + g_critical("could not get browser window, when trying to call OnDisconnected");
> return;
> }
>
> @@ -741,20 +727,20 @@ void nsPluginInstance::CallOnDisconnected(int code)
> NPIdentifier id_on_disconnected = NPN_GetStringIdentifier("OnDisconnected");
> if (!id_on_disconnected)
> {
> - LOG_ERROR("could not find OnDisconnected identifier");
> + g_critical("could not find OnDisconnected identifier");
> return;
> }
>
> NPVariant var_on_disconnected;
> if (!NPN_GetProperty(m_instance, window, id_on_disconnected, &var_on_disconnected))
> {
> - LOG_ERROR("could not get OnDisconnected function");
> + g_critical("could not get OnDisconnected function");
> return;
> }
>
> if (!NPVARIANT_IS_OBJECT(var_on_disconnected))
> {
> - LOG_ERROR("OnDisconnected is not object");
> + g_critical("OnDisconnected is not object");
> return;
> }
>
> @@ -767,13 +753,9 @@ void nsPluginInstance::CallOnDisconnected(int code)
> NPVariant args[] = { arg };
>
> if (NPN_InvokeDefault(m_instance, call_on_disconnected, args, sizeof(args) / sizeof(args[0]), &void_result))
> - {
> - LOG_DEBUG("OnDisconnected successfuly called");
> - }
> + g_debug("OnDisconnected successfuly called");
Why are you dropping the scope?
> else
> - {
> - LOG_ERROR("could not call OnDisconnected");
> - }
> + g_critical("could not call OnDisconnected");
Ditto.
>
> // cleanup
> NPN_ReleaseObject(window);
> @@ -782,14 +764,14 @@ void nsPluginInstance::CallOnDisconnected(int code)
>
> void nsPluginInstance::SigchldRoutine(int sig, siginfo_t *info, void *uap)
> {
> - LOG_DEBUG("child finished, pid: " << info->si_pid);
> + g_debug("child finished, pid: %llu", (guint64)info->si_pid);
> int exit_code;
> waitpid(info->si_pid, &exit_code, 0);
>
> if (!getenv("SPICE_XPI_DEBUG")) {
> nsPluginInstance *fake_this = s_children[info->si_pid];
> if (fake_this == NULL) {
> - LOG_ERROR("Invalid children signal");
> + g_critical("Invalid children signal");
> return;
> }
>
> diff --git a/configure.ac b/configure.ac
> index cb16269..4614083 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -18,14 +18,14 @@ AM_PROG_CC_C_O
> dnl =========================================================================
> dnl Check deps
>
> -PKG_CHECK_MODULES(LOG4CPP, log4cpp)
> -AC_SUBST(LOG4CPP_CFLAGS)
> -AC_SUBST(LOG4CPP_LIBS)
> -
> AC_CONFIG_SUBDIRS([spice-protocol])
> SPICE_PROTOCOL_CFLAGS='-I ${top_srcdir}/spice-protocol'
> AC_SUBST(SPICE_PROTOCOL_CFLAGS)
>
> +PKG_CHECK_MODULES(GLIB, glib-2.0)
> +AC_SUBST(GLIB_CFLAGS)
> +AC_SUBST(GLIB_LIBS)
> +
> # The explicit nspr dep is needed because libxul-embedding
> # in RHEL5 is missing the Requires
> PKG_CHECK_MODULES(XUL, libxul-embedding >= 1.9 nspr >= 4.7.1)
> --
> 1.7.7.6
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
More information about the Spice-devel
mailing list