[Spice-devel] [RFC spice-vdagent 09/18] udscs: simplify logging
Victor Toso
victortoso at redhat.com
Wed Sep 19 07:23:19 UTC 2018
Hi,
Forgot to reply to this one, small suggestions below.
On Tue, Aug 14, 2018 at 08:53:43PM +0200, Jakub Janků wrote:
> Remove type_to_string, no_types arguments from
> udscs_connect() and udscs_server_new().
> udscs is used only in vdagent.c and vdagentd.c
> and in both cases the args are the same
> (vdagentd_messages, VDAGENTD_NO_MESSAGES).
>
> Add debug_print_message_header().
If not a problem, please add SoB
> ---
> src/udscs.c | 52 ++++++++++++++---------------------------
> src/udscs.h | 14 ++++-------
> src/vdagent/vdagent.c | 3 +--
> src/vdagentd/vdagentd.c | 5 ++--
> 4 files changed, 25 insertions(+), 49 deletions(-)
>
> diff --git a/src/udscs.c b/src/udscs.c
> index 4a657c9..0b80317 100644
> --- a/src/udscs.c
> +++ b/src/udscs.c
> @@ -31,10 +31,9 @@
>
> #include "udscs.h"
> #include "vdagent-connection.h"
> +#include "vdagentd-proto-strings.h"
>
> struct udscs_connection {
> - const char * const *type_to_string;
> - int no_types;
> int debug;
> void *user_data;
>
> @@ -48,6 +47,17 @@ struct udscs_connection {
> struct udscs_connection *prev;
> };
>
debug_print_message_header() will be called only if conn->debug
is set. We could add that check here and remove where this is
called? something like:
if (conn == NULL || !conn->debug)
return;
> +static void debug_print_message_header(struct udscs_connection *conn,
> + struct udscs_message_header *header,
> + const gchar *direction)
> +{
> + const gchar *type = header->type < G_N_ELEMENTS(vdagentd_messages) ?
> + vdagentd_messages[header->type] : "invalid message";
I find more readable to start type with "invalid message" and use
a if to set it, eg:
const gchar *type = "invalid message";
if (header->type < G_N_ELEMENTS(vdagentd_messages) {
type = vdagentd_messages[header->type];
}
Reviewed-by: Victor Toso <victortoso at redhat.com>
> +
> + syslog(LOG_DEBUG, "%p %s %s, arg1: %u, arg2: %u, size %u",
> + conn, direction, type, header->arg1, header->arg2, header->size);
> +}
> +
> static gboolean conn_header_read_cb(gpointer header_buff,
> gsize *body_size,
> gpointer user_data)
> @@ -64,18 +74,8 @@ static gboolean conn_read_cb(gpointer header_buff,
> struct udscs_connection *conn = user_data;
> struct udscs_message_header *header = header_buff;
>
> - if (conn->debug) {
> - if (header->type < conn->no_types)
> - syslog(LOG_DEBUG,
> - "%p received %s, arg1: %u, arg2: %u, size %u",
> - conn, conn->type_to_string[header->type],
> - header->arg1, header->arg2, header->size);
> - else
> - syslog(LOG_DEBUG,
> - "%p received invalid message %u, arg1: %u, arg2: %u, size %u",
> - conn, header->type, header->arg1, header->arg2,
> - header->size);
> - }
> + if (conn->debug)
> + debug_print_message_header(conn, header, "received");
>
> if (conn->read_callback) {
> conn->read_callback(&conn, header, data);
> @@ -92,7 +92,7 @@ static void conn_error_cb(gpointer user_data)
> struct udscs_connection *udscs_connect(const char *socketname,
> udscs_read_callback read_callback,
> udscs_disconnect_callback disconnect_callback,
> - const char * const type_to_string[], int no_types, int debug)
> + int debug)
> {
> GIOStream *io_stream;
> struct udscs_connection *conn;
> @@ -104,9 +104,6 @@ struct udscs_connection *udscs_connect(const char *socketname,
> conn = calloc(1, sizeof(*conn));
> if (!conn)
> return NULL;
> -
> - conn->type_to_string = type_to_string;
> - conn->no_types = no_types;
> conn->debug = debug;
> conn->conn = vdagent_connection_new(io_stream,
> FALSE,
> @@ -181,15 +178,8 @@ int udscs_write(struct udscs_connection *conn, uint32_t type, uint32_t arg1,
> memcpy(buff, &header, sizeof(header));
> memcpy(buff + sizeof(header), data, size);
>
> - if (conn->debug) {
> - if (type < conn->no_types)
> - syslog(LOG_DEBUG, "%p sent %s, arg1: %u, arg2: %u, size %u",
> - conn, conn->type_to_string[type], arg1, arg2, size);
> - else
> - syslog(LOG_DEBUG,
> - "%p sent invalid message %u, arg1: %u, arg2: %u, size %u",
> - conn, type, arg1, arg2, size);
> - }
> + if (conn->debug)
> + debug_print_message_header(conn, &header, "sent");
>
> vdagent_connection_write(conn->conn, buff, buff_size);
> return 0;
> @@ -202,8 +192,6 @@ int udscs_write(struct udscs_connection *conn, uint32_t type, uint32_t arg1,
> struct udscs_server {
> GSocketService *service;
>
> - const char * const *type_to_string;
> - int no_types;
> int debug;
> struct udscs_connection connections_head;
> udscs_connect_callback connect_callback;
> @@ -220,7 +208,7 @@ struct udscs_server *udscs_server_new(
> udscs_connect_callback connect_callback,
> udscs_read_callback read_callback,
> udscs_disconnect_callback disconnect_callback,
> - const char * const type_to_string[], int no_types, int debug)
> + int debug)
> {
> struct udscs_server *server;
>
> @@ -228,8 +216,6 @@ struct udscs_server *udscs_server_new(
> if (!server)
> return NULL;
>
> - server->type_to_string = type_to_string;
> - server->no_types = no_types;
> server->debug = debug;
> server->connect_callback = connect_callback;
> server->read_callback = read_callback;
> @@ -319,8 +305,6 @@ static gboolean udscs_server_accept_cb(GSocketService *service,
> return TRUE;
> }
>
> - new_conn->type_to_string = server->type_to_string;
> - new_conn->no_types = server->no_types;
> new_conn->debug = server->debug;
> new_conn->read_callback = server->read_callback;
> new_conn->disconnect_callback = server->disconnect_callback;
> diff --git a/src/udscs.h b/src/udscs.h
> index d02d5b4..0a1bad4 100644
> --- a/src/udscs.h
> +++ b/src/udscs.h
> @@ -59,15 +59,12 @@ typedef void (*udscs_disconnect_callback)(struct udscs_connection *conn);
> * Only sockets bound to a pathname are supported.
> *
> * If debug is true then the events on this connection will be traced.
> - * This includes the incoming and outgoing message names. So when debug is true
> - * no_types must be set to the value of the highest valid message id + 1,
> - * and type_to_string must point to a string array of size no_types for
> - * converting the message ids to their names.
> + * This includes the incoming and outgoing message names.
> */
> struct udscs_connection *udscs_connect(const char *socketname,
> udscs_read_callback read_callback,
> udscs_disconnect_callback disconnect_callback,
> - const char * const type_to_string[], int no_types, int debug);
> + int debug);
>
> /* Close the connection, releases the corresponding resources and
> * sets *connp to NULL.
> @@ -106,16 +103,13 @@ typedef void (*udscs_connect_callback)(struct udscs_connection *conn);
> *
> * If debug is true then the events on this socket and related individual
> * connections will be traced.
> - * This includes the incoming and outgoing message names. So when debug is true
> - * no_types must be set to the value of the highest valid message id + 1,
> - * and type_to_string must point to a string array of size no_types for
> - * converting the message ids to their names.
> + * This includes the incoming and outgoing message names.
> */
> struct udscs_server *udscs_server_new(
> udscs_connect_callback connect_callback,
> udscs_read_callback read_callback,
> udscs_disconnect_callback disconnect_callback,
> - const char * const type_to_string[], int no_types, int debug);
> + int debug);
>
> /* Start listening on a pre-configured socket specified by the given @fd.
> * This can be used with systemd socket activation, etc. */
> diff --git a/src/vdagent/vdagent.c b/src/vdagent/vdagent.c
> index 3f8ef31..cb66749 100644
> --- a/src/vdagent/vdagent.c
> +++ b/src/vdagent/vdagent.c
> @@ -42,7 +42,6 @@
>
> #include "udscs.h"
> #include "vdagentd-proto.h"
> -#include "vdagentd-proto-strings.h"
> #include "audio.h"
> #include "x11.h"
> #include "file-xfers.h"
> @@ -369,7 +368,7 @@ static gboolean vdagent_init_async_cb(gpointer user_data)
>
> agent->conn = udscs_connect(vdagentd_socket,
> daemon_read_complete, daemon_disconnect_cb,
> - vdagentd_messages, VDAGENTD_NO_MESSAGES, debug);
> + debug);
> if (agent->conn == NULL) {
> g_timeout_add_seconds(1, vdagent_init_async_cb, agent);
> return G_SOURCE_REMOVE;
> diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c
> index fd54723..8893f3b 100644
> --- a/src/vdagentd/vdagentd.c
> +++ b/src/vdagentd/vdagentd.c
> @@ -41,7 +41,6 @@
>
> #include "udscs.h"
> #include "vdagentd-proto.h"
> -#include "vdagentd-proto-strings.h"
> #include "uinput.h"
> #include "xorg-conf.h"
> #include "virtio-port.h"
> @@ -1105,8 +1104,8 @@ int main(int argc, char *argv[])
> openlog("spice-vdagentd", do_daemonize ? 0 : LOG_PERROR, LOG_USER);
>
> /* Setup communication with vdagent process(es) */
> - server = udscs_server_new(agent_connect, agent_read_complete, agent_disconnect,
> - vdagentd_messages, VDAGENTD_NO_MESSAGES, debug);
> + server = udscs_server_new(agent_connect, agent_read_complete,
> + agent_disconnect, debug);
> if (server == NULL) {
> syslog(LOG_CRIT, "Fatal could not allocate memory for udscs server");
> return 1;
> --
> 2.17.1
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20180919/acf9abc9/attachment.sig>
More information about the Spice-devel
mailing list