[RFC,spice-vdagent,09/18] udscs: simplify logging

Submitted by Jakub Janku on Aug. 14, 2018, 6:53 p.m.

Details

Message ID 20180814185352.6080-10-jjanku@redhat.com
State New
Headers show
Series "GLib integration" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Jakub Janku Aug. 14, 2018, 6:53 p.m.
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().
---
 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(-)

Patch hide | download patch | download mbox

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;
 };
 
+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";
+
+    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;

Comments

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@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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel