[Spice-devel] spice-xpi
Christophe Fergeau
cfergeau at redhat.com
Tue Mar 13 05:26:00 PDT 2012
Hey,
On Tue, Mar 13, 2012 at 12:29:01PM +0100, Peter Hatina wrote:
> Hi,
>
> as Marc-Andre has decided to drop log4cpp, I would like to at least
> revert the part of the message format. The reason is simple, Spice-QA
> guys are developing a test framework, which uses the log4cpp format we had.
>
> Opinions?
There seems to have several things mixed in this patch, this (re?)adds
logging in some places, this wraps use of g_debug/... in LOG_DEBUG/..., and
there is another change replacing execl use with execv.
If I understand things correctly, your main goal is to use the same message
formatting as before to avoid breaking the parsing QE is doing on these
messages, right?
Could you split the log additions in a separate patch?
Apart from this, no strong opposition from me if this doesn't spam
stdin/stdout but only log a few useful messages during spice-xpi runtime.
Christophe
>
> --
> Peter Hatina
> EMEA ENG-Desktop Development
> Red Hat Czech, Brno
> From 276c4dea6ec24d859714c901e1f6505d8bd6d188 Mon Sep 17 00:00:00 2001
> From: Peter Hatina <phatina at redhat.com>
> Date: Tue, 13 Mar 2012 11:02:52 +0100
> Subject: [PATCH] revert log messages format
>
> ---
> SpiceXPI/src/plugin/controller.cpp | 18 +++---
> SpiceXPI/src/plugin/debug.h | 75 ++++++++++++++++++++++++
> SpiceXPI/src/plugin/plugin.cpp | 112 ++++++++++++++++++++++++++++++------
> 3 files changed, 176 insertions(+), 29 deletions(-)
> create mode 100644 SpiceXPI/src/plugin/debug.h
>
> diff --git a/SpiceXPI/src/plugin/controller.cpp b/SpiceXPI/src/plugin/controller.cpp
> index 23c853b..14de666 100644
> --- a/SpiceXPI/src/plugin/controller.cpp
> +++ b/SpiceXPI/src/plugin/controller.cpp
> @@ -54,6 +54,7 @@ extern "C" {
> # include <sys/un.h>
> }
>
> +#include "debug.h"
> #include "rederrorcodes.h"
> #include "controller.h"
>
> @@ -70,7 +71,6 @@ SpiceController::SpiceController(const std::string &name):
>
> SpiceController::~SpiceController()
> {
> - g_debug(G_STRFUNC);
> Disconnect();
> }
>
> @@ -89,7 +89,7 @@ int SpiceController::Connect()
> {
> if ((m_client_socket = socket(AF_UNIX, SOCK_STREAM, 0)) == -1)
> {
> - g_critical("controller socket: %s", g_strerror(errno));
> + LOG_CRITICAL("controller socket: " << g_strerror(errno));
> return -1;
> }
> }
> @@ -100,13 +100,9 @@ int SpiceController::Connect()
>
> int rc = connect(m_client_socket, (struct sockaddr *) &remote, strlen(remote.sun_path) + sizeof(remote.sun_family));
> if (rc == -1)
> - {
> - g_critical("controller connect: %s", g_strerror(errno));
> - }
> + LOG_CRITICAL("controller connect: " << g_strerror(errno));
> else
> - {
> - g_debug("controller connected");
> - }
> + LOG_DEBUG("controller connected");
>
> return rc;
> }
> @@ -136,6 +132,8 @@ void SpiceController::Disconnect()
> // delete the temporary file, which is used for the socket
> unlink(m_name.c_str());
> m_name.clear();
> +
> + LOG_DEBUG("disconnecting");
> }
>
> uint32_t SpiceController::Write(const void *lpBuffer, uint32_t nBytesToWrite)
> @@ -144,8 +142,8 @@ uint32_t SpiceController::Write(const void *lpBuffer, uint32_t nBytesToWrite)
>
> if (len != (ssize_t)nBytesToWrite)
> {
> - g_warning("incomplete send, bytes to write = %lu, bytes written = %d: %s",
> - nBytesToWrite, len, g_strerror(errno));
> + LOG_WARNING("incomplete send, bytes to write = " << nBytesToWrite
> + << ", bytes written = " << len << ": " << g_strerror(errno));
> }
>
> return len;
> diff --git a/SpiceXPI/src/plugin/debug.h b/SpiceXPI/src/plugin/debug.h
> new file mode 100644
> index 0000000..97735cf
> --- /dev/null
> +++ b/SpiceXPI/src/plugin/debug.h
> @@ -0,0 +1,75 @@
> +/* ***** 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 2012, 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):
> + * Peter Hatina
> + *
> + * 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>
> +
> +#if __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
> +
> +#define LOG(func, msg) \
> + func(dynamic_cast<std::stringstream&>(std::stringstream() \
> + << std::flush << FUNC_NAME << ": " << msg).str().c_str())
> +
> +#define LOG_DEBUG(msg) LOG(g_debug, msg)
> +#define LOG_WARNING(msg) LOG(g_warning, msg)
> +#define LOG_CRITICAL(msg) LOG(g_critical, msg)
> +#define LOG_MESSAGE(msg) LOG(g_message, msg)
> +
> +#endif // DEBUG_H
> diff --git a/SpiceXPI/src/plugin/plugin.cpp b/SpiceXPI/src/plugin/plugin.cpp
> index 39f2e81..d9f20bd 100644
> --- a/SpiceXPI/src/plugin/plugin.cpp
> +++ b/SpiceXPI/src/plugin/plugin.cpp
> @@ -85,6 +85,7 @@ static NS_DEFINE_CID(kPluginManagerCID, NS_PLUGINMANAGER_CID);
>
> #include "config.h"
> #include "controller.h"
> +#include "debug.h"
> #include "plugin.h"
> #include "nsScriptablePeer.h"
>
> @@ -260,199 +261,243 @@ NPBool nsPluginInstance::isInitialized()
> /* attribute string hostIP; */
> char *nsPluginInstance::GetHostIP() const
> {
> + LOG_DEBUG(m_host_ip);
> return stringCopy(m_host_ip);
> }
>
> void nsPluginInstance::SetHostIP(const char *aHostIP)
> {
> m_host_ip = aHostIP;
> + LOG_DEBUG(m_host_ip);
> }
>
> /* attribute string port; */
> char *nsPluginInstance::GetPort() const
> {
> + LOG_DEBUG(m_port);
> return stringCopy(m_port);
> }
>
> void nsPluginInstance::SetPort(const char *aPort)
> {
> m_port = aPort;
> + LOG_DEBUG(m_port);
> }
>
> /* attribute string SecurePort; */
> char *nsPluginInstance::GetSecurePort() const
> {
> + LOG_DEBUG(m_secure_port);
> return stringCopy(m_secure_port);
> }
>
> void nsPluginInstance::SetSecurePort(const char *aSecurePort)
> {
> m_secure_port = aSecurePort;
> + LOG_DEBUG(m_secure_port);
> }
>
> /* attribute string Password; */
> char *nsPluginInstance::GetPassword() const
> {
> + LOG_DEBUG("password requested");
> return stringCopy(m_password);
> }
>
> void nsPluginInstance::SetPassword(const char *aPassword)
> {
> m_password = aPassword;
> + LOG_DEBUG("password set");
> }
>
> /* attribute string CipherSuite; */
> char *nsPluginInstance::GetCipherSuite() const
> {
> + LOG_DEBUG(m_cipher_suite);
> return stringCopy(m_cipher_suite);
> }
>
> void nsPluginInstance::SetCipherSuite(const char *aCipherSuite)
> {
> m_cipher_suite = aCipherSuite;
> + LOG_DEBUG(m_cipher_suite);
> }
>
> /* attribute string SSLChannels; */
> char *nsPluginInstance::GetSSLChannels() const
> {
> + LOG_DEBUG(m_ssl_channels);
> return stringCopy(m_ssl_channels);
> }
>
> void nsPluginInstance::SetSSLChannels(const char *aSSLChannels)
> {
> m_ssl_channels = aSSLChannels;
> + LOG_DEBUG(m_ssl_channels);
> }
>
> //* attribute string TrustStore; */
> char *nsPluginInstance::GetTrustStore() const
> {
> + LOG_DEBUG(m_trust_store);
> return stringCopy(m_trust_store);
> }
>
> void nsPluginInstance::SetTrustStore(const char *aTrustStore)
> {
> m_trust_store = aTrustStore;
> + LOG_DEBUG(m_trust_store);
> }
>
> /* attribute string HostSubject; */
> char *nsPluginInstance::GetHostSubject() const
> {
> + LOG_DEBUG(m_host_subject);
> return stringCopy(m_host_subject);
> }
>
> void nsPluginInstance::SetHostSubject(const char *aHostSubject)
> {
> m_host_subject = aHostSubject;
> + LOG_DEBUG(m_host_subject);
> }
>
> /* attribute boolean fullScreen; */
> PRBool nsPluginInstance::GetFullScreen() const
> {
> + LOG_DEBUG(m_fullscreen);
> return m_fullscreen;
> }
>
> void nsPluginInstance::SetFullScreen(PRBool aFullScreen)
> {
> m_fullscreen = aFullScreen;
> + LOG_DEBUG(m_fullscreen);
> }
>
> /* attribute boolean Smartcard; */
> PRBool nsPluginInstance::GetSmartcard() const
> {
> + LOG_DEBUG(m_smartcard);
> return m_smartcard;
> }
>
> void nsPluginInstance::SetSmartcard(PRBool aSmartcard)
> {
> m_smartcard = aSmartcard;
> + LOG_DEBUG(m_smartcard);
> }
>
> /* attribute string Title; */
> char *nsPluginInstance::GetTitle() const
> {
> + std::string title(m_title);
> + size_t found = -2;
> + while ((found = title.find("%", found + 2)) != std::string::npos)
> + title.replace(found, 1, "%%");
> + LOG_DEBUG(title);
> return stringCopy(m_title);
> }
>
> void nsPluginInstance::SetTitle(const char *aTitle)
> {
> m_title = aTitle;
> + std::string title(m_title);
> + size_t found = -2;
> + while ((found = title.find("%", found + 2)) != std::string::npos)
> + title.replace(found, 1, "%%");
> + LOG_DEBUG(title);
> }
>
> /* attribute string dynamicMenu; */
> char *nsPluginInstance::GetDynamicMenu() const
> {
> + LOG_DEBUG(m_dynamic_menu);
> return stringCopy(m_dynamic_menu);
> }
>
> void nsPluginInstance::SetDynamicMenu(const char *aDynamicMenu)
> {
> m_dynamic_menu = aDynamicMenu;
> + LOG_DEBUG(m_dynamic_menu);
> }
>
> /* attribute string NumberOfMonitors; */
> char *nsPluginInstance::GetNumberOfMonitors() const
> {
> + LOG_DEBUG(m_number_of_monitors);
> return stringCopy(m_number_of_monitors);
> }
>
> void nsPluginInstance::SetNumberOfMonitors(const char *aNumberOfMonitors)
> {
> m_number_of_monitors = aNumberOfMonitors;
> + LOG_DEBUG(m_number_of_monitors);
> }
>
> /* attribute boolean AdminConsole; */
> PRBool nsPluginInstance::GetAdminConsole() const
> {
> + LOG_DEBUG(m_admin_console);
> return m_admin_console;
> }
>
> void nsPluginInstance::SetAdminConsole(PRBool aAdminConsole)
> {
> m_admin_console = aAdminConsole;
> + LOG_DEBUG(m_admin_console);
> }
>
> /* attribute string GuestHostName; */
> char *nsPluginInstance::GetGuestHostName() const
> {
> + LOG_DEBUG(m_guest_host_name);
> return stringCopy(m_guest_host_name);
> }
>
> void nsPluginInstance::SetGuestHostName(const char *aGuestHostName)
> {
> m_guest_host_name = aGuestHostName;
> + LOG_DEBUG(m_guest_host_name);
> }
>
> /* attribute string HotKey; */
> char *nsPluginInstance::GetHotKeys() const
> {
> + LOG_DEBUG(m_hot_keys);
> return stringCopy(m_hot_keys);
> }
>
> void nsPluginInstance::SetHotKeys(const char *aHotKeys)
> {
> m_hot_keys = aHotKeys;
> + LOG_DEBUG(m_hot_keys);
> }
>
> /* attribute boolean NoTaskMgrExecution; */
> PRBool nsPluginInstance::GetNoTaskMgrExecution() const
> {
> + LOG_DEBUG(m_no_taskmgr_execution);
> return m_no_taskmgr_execution;
> }
>
> void nsPluginInstance::SetNoTaskMgrExecution(PRBool aNoTaskMgrExecution)
> {
> m_no_taskmgr_execution = aNoTaskMgrExecution;
> + LOG_DEBUG(m_no_taskmgr_execution);
> }
>
> /* attribute boolean SendCtrlAltDelete; */
> PRBool nsPluginInstance::GetSendCtrlAltDelete() const
> {
> + LOG_DEBUG(m_send_ctrlaltdel);
> return m_send_ctrlaltdel;
> }
>
> void nsPluginInstance::SetSendCtrlAltDelete(PRBool aSendCtrlAltDelete)
> {
> m_send_ctrlaltdel = aSendCtrlAltDelete;
> + LOG_DEBUG(m_send_ctrlaltdel);
> }
>
> /* attribute unsigned short UsbListenPort; */
> @@ -461,6 +506,7 @@ unsigned short nsPluginInstance::GetUsbListenPort() const
> // this method exists due to RHEVM 2.2
> // and should be removed some time in future,
> // when fixed in RHEVM
> + LOG_WARNING("this should not be called");
> return 0;
> }
>
> @@ -469,6 +515,7 @@ void nsPluginInstance::SetUsbListenPort(unsigned short aUsbPort)
> // this method exists due to RHEVM 2.2
> // and should be removed some time in future,
> // when fixed in RHEVM
> + LOG_WARNING("this should not be called");
> }
>
> /* attribute boolean UsbAutoShare; */
> @@ -477,6 +524,7 @@ PRBool nsPluginInstance::GetUsbAutoShare() const
> // this method exists due to RHEVM 2.2
> // and should be removed some time in future,
> // when fixed in RHEVM
> + LOG_WARNING("this should not be called");
> return false;
> }
>
> @@ -485,6 +533,7 @@ void nsPluginInstance::SetUsbAutoShare(PRBool aUsbAutoShare)
> // this method exists due to RHEVM 2.2
> // and should be removed some time in future,
> // when fixed in RHEVM
> + LOG_WARNING("this should not be called");
> }
>
> void nsPluginInstance::WriteToPipe(const void *data, uint32_t size)
> @@ -555,7 +604,7 @@ void nsPluginInstance::Connect()
> socket_file += "/spice-xpi";
> if (setenv("SPICE_XPI_SOCKET", socket_file.c_str(), 1))
> {
> - g_critical("could not set SPICE_XPI_SOCKET env variable");
> + LOG_CRITICAL("could not set SPICE_XPI_SOCKET env variable");
> return;
> }
>
> @@ -574,23 +623,43 @@ void nsPluginInstance::Connect()
>
> char c;
> if (read(pipe_fds[0], &c, 1) != 0)
> - g_critical("Error while reading on pipe: %s", g_strerror(errno));
> + LOG_CRITICAL("Error while reading on pipe: " << g_strerror(errno));
>
> close(pipe_fds[0]);
> pipe_fds[0] = -1;
>
> - execl("/usr/libexec/spice-xpi-client", "/usr/libexec/spice-xpi-client", NULL);
> - g_message("failed to run spice-xpi-client, running spicec instead");
> + char *spice_xpi_client_args[] = {
> + const_cast<char*>("/usr/libexec/spice-xpi-client"),
> + NULL
> + };
> + std::stringstream ss;
> + int cnt = sizeof(spice_xpi_client_args) / sizeof(spice_xpi_client_args[0]);
> + for (int i = 0; i < cnt; ++i)
> + ss << spice_xpi_client_args[i] << " ";
> + LOG_MESSAGE("Launching " << ss.str());
> + execv(spice_xpi_client_args[0], spice_xpi_client_args);
> + LOG_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);
> - g_critical("ERROR failed to run spicec fallback");
> + char *spicec_args[] = {
> + const_cast<char*>("/usr/bin/spicec"),
> + const_cast<char*>("--controller"),
> + NULL
> + };
> + ss.str(std::string());
> + ss.clear();
> + cnt = sizeof(spicec_args) / sizeof(spicec_args[0]);
> + for (int i = 0; i < cnt; ++i)
> + ss << spicec_args[i] << " ";
> + LOG_MESSAGE("Launching " << ss.str());
> + execv(spicec_args[0], spicec_args);
> + LOG_CRITICAL("ERROR failed to run spicec fallback");
>
> exit(EXIT_FAILURE);
> }
> else
> {
> - g_debug("child pid: %"G_GUINT64_FORMAT, (guint64)m_pid_controller);
> + LOG_DEBUG("child pid: " << m_pid_controller);
>
> close(pipe_fds[0]);
> pipe_fds[0] = -1;
> @@ -606,7 +675,7 @@ void nsPluginInstance::Connect()
>
> if (m_external_controller.Connect(10) != 0)
> {
> - g_critical("could not connect to spice client controller");
> + LOG_CRITICAL("could not connect to spice client controller");
> return;
> }
>
> @@ -629,7 +698,7 @@ void nsPluginInstance::Connect()
> }
> else
> {
> - g_critical("could not open truststore temp file");
> + LOG_CRITICAL("could not open truststore temp file");
> close(fd);
> unlink(m_trust_store_file.c_str());
> m_trust_store_file.clear();
> @@ -638,10 +707,11 @@ void nsPluginInstance::Connect()
> }
> else
> {
> - g_critical("could not create truststore temp file: %s", g_strerror(errno));
> + LOG_CRITICAL("could not create truststore temp file: " << g_strerror(errno));
> return;
> }
>
> + LOG_MESSAGE("Initiating connection with controller");
> SendInit();
> SendStr(CONTROLLER_HOST, m_host_ip.c_str());
> SendValue(CONTROLLER_PORT, atoi(m_port.c_str()));
> @@ -683,12 +753,13 @@ void nsPluginInstance::Connect()
>
> void nsPluginInstance::Show()
> {
> - g_debug("sending show message");
> + LOG_DEBUG("sending show message");
> SendMsg(CONTROLLER_SHOW);
> }
>
> void nsPluginInstance::Disconnect()
> {
> + LOG_MESSAGE("Disconnecting");
> kill(m_pid_controller, SIGTERM);
> }
>
> @@ -702,8 +773,10 @@ void nsPluginInstance::SetLanguageStrings(const char *aSection, const char *aLan
> if (aSection != NULL && aLanguage != NULL)
> {
> if (strlen(aSection) > 0 && strlen(aLanguage) > 0)
> + {
> m_language[aSection] = aLanguage;
> -
> + LOG_DEBUG(m_language[aSection]);
> + }
> }
> }
>
> @@ -712,6 +785,7 @@ void nsPluginInstance::SetUsbFilter(const char *aUsbFilter)
> // this method exists due to RHEVM 2.2
> // and should be removed some time in future,
> // when fixed in RHEVM
> + LOG_WARNING("this should not be called");
> }
>
> void nsPluginInstance::CallOnDisconnected(int code)
> @@ -719,7 +793,7 @@ void nsPluginInstance::CallOnDisconnected(int code)
> NPObject *window = NULL;
> if (NPN_GetValue(m_instance, NPNVWindowNPObject, &window) != NPERR_NO_ERROR)
> {
> - g_critical("could not get browser window, when trying to call OnDisconnected");
> + LOG_CRITICAL("could not get browser window, when trying to call OnDisconnected");
> return;
> }
>
> @@ -727,20 +801,20 @@ void nsPluginInstance::CallOnDisconnected(int code)
> NPIdentifier id_on_disconnected = NPN_GetStringIdentifier("OnDisconnected");
> if (!id_on_disconnected)
> {
> - g_critical("could not find OnDisconnected identifier");
> + LOG_CRITICAL("could not find OnDisconnected identifier");
> return;
> }
>
> NPVariant var_on_disconnected;
> if (!NPN_GetProperty(m_instance, window, id_on_disconnected, &var_on_disconnected))
> {
> - g_critical("could not get OnDisconnected function");
> + LOG_CRITICAL("could not get OnDisconnected function");
> return;
> }
>
> if (!NPVARIANT_IS_OBJECT(var_on_disconnected))
> {
> - g_critical("OnDisconnected is not object");
> + LOG_CRITICAL("OnDisconnected is not object");
> return;
> }
>
> @@ -753,9 +827,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))
> - g_debug("OnDisconnected successfuly called");
> + LOG_DEBUG("OnDisconnected successfuly called");
> else
> - g_critical("could not call OnDisconnected");
> + LOG_CRITICAL("could not call OnDisconnected");
>
> // cleanup
> NPN_ReleaseObject(window);
> @@ -770,7 +844,7 @@ void *nsPluginInstance::ControllerWaitHelper(void *opaque)
>
> int exit_code;
> waitpid(fake_this->m_pid_controller, &exit_code, 0);
> - g_debug("child finished, pid: %"G_GUINT64_FORMAT, (guint64)exit_code);
> + LOG_DEBUG("child finished, pid: " << exit_code);
>
> fake_this->m_connected_status = fake_this->m_external_controller.TranslateRC(exit_code);
> if (!getenv("SPICE_XPI_DEBUG"))
> --
> 1.7.7.6
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20120313/6b70e848/attachment.pgp>
More information about the Spice-devel
mailing list