4.4.1: session client cleanup

[Danny Milosavljevic]

Hi,

I just read the SessionClient of Xfce 4.4.0 and unfortunately it doesn't do much good with memory management and, of course, there are no getters/setters.

Unfortunately the structure is public, thereby bringing these disadvantages:
1. cannot add or remove or change field definitions of the structure without breaking every client application (no encapsulation)
2. caller needs to be aware when and if he's allowed to modify which field's data
3. caller needs to free fields himself (using his crystal ball to find out when it's safe to free - i. e. probably never)

Oh boy.

Point 1 is unfixable without breaking ABI and a constraint on any changes.

Point 3 is fixable because after adding a "client_session_free", new clients can call "client_session_free" in the "die" callback (although the "die" callback doesn't get passed a session_client, it gets passed a userdata structure into which the user would just put the session_client pointer)

Point 2 is fixable by adding setters that brag when he's trying to do something weird:

Index: session-client.c
===================================================================
--- session-client.c	(Revision 24812)
+++ session-client.c	(Arbeitskopie)
@@ -831,3 +831,369 @@
 
     return session_client;
 }
+
+void client_session_free(SessionClient * session_client)
+{
+    /* session_client->data; */
+    /* session_client->session_connection; */
+    
+    if (session_client->current_state != SESSION_CLIENT_DISCONNECTED) {
+        disconnect(session_client);
+    }
+
+    g_free(session_client->client_id);
+    g_free(session_client->given_client_id);
+
+    g_free(session_client->current_directory);
+    g_free(session_client->program);
+    g_strfreev(session_client->clone_command);
+    g_strfreev(session_client->resign_command);
+    g_strfreev(session_client->restart_command);
+    g_strfreev(session_client->discard_command);
+    g_strfreev(session_client->shutdown_command);
+    g_free(session_client);
+}
+
+gchar* safe_strdup(gchar const* source)
+{
+    if (source != NULL) {
+        return g_strdup(source);
+    } else {
+        return NULL;
+    }
+}
+
+gchar** safe_strvdup(gchar** const source)
+{
+    int     count;
+    int     i;
+    gchar** result;
+    
+    count = g_strv_length(source);
+    result = g_new0(gchar*, count + 1);
+    
+    for (i = 0; i < count; ++i) {
+        result[i] = safe_strdup(source[i]);
+    }
+
+    result[count] = NULL;
+    return result;
+}
+
+/* messes with session_connection:
+SessionClient *client_session_copy(SessionClient * session_client)
+{
+    SessionClient* result;
+    result = g_new0(SessionClient, 1);
+
+    result->data = session_client->data;
+    result->session_connection = session_client->session_connection; 
+
+    result->current_state = session_client->current_state;
+    result->restart_style = session_client->restart_style;
+    result->interact_style = session_client->interact_style;
+
+    result->priority = session_client->priority;
+
+    result->client_id = safe_strdup(session_client->client_id);
+    result->given_client_id = safe_strdup(session_client->given_client_id);
+
+    result->current_directory = safe_strdup(session_client->current_directory);
+    result->program = safe_strdup(session_client->program);
+    result->clone_command = safe_strvdup(session_client->clone_command);
+    result->resign_command = safe_strvdup(session_client->resign_command);
+    result->restart_command = safe_strvdup(session_client->restart_command);
+    result->discard_command = safe_strvdup(session_client->discard_command);
+    result->shutdown_command = safe_strvdup(session_client->shutdown_command);
+
+    result->shutdown = session_client->shutdown;
+}
+*/
+
+void client_session_set_save_phase_2_callback(SessionClient * session_client, save_phase_2_callback value)
+{
+    session_client->save_phase_2 = value;
+}
+
+void client_session_set_interact_callback(SessionClient * session_client, interact_callback value)
+{
+    session_client->interact = value;
+}
+
+void client_session_set_shutdown_cancelled_callback(SessionClient * session_client, shutdown_cancelled_callback value)
+{
+    session_client->shutdown_cancelled = value;
+}
+
+void client_session_set_save_complete_callback(SessionClient * session_client, save_complete_callback value)
+{
+    session_client->save_complete = value;
+}
+
+void client_session_set_die_callback(SessionClient * session_client, die_callback value)
+{
+    session_client->die = value;
+}
+
+void client_session_set_save_yourself_callback(SessionClient * session_client, save_yourself_callback value)
+{
+    session_client->save_yourself = value;
+}
+
+void client_session_set_data(SessionClient * session_client, gpointer value)
+{
+    session_client->data = value;
+}
+
+/*void client_session_set_session_connection(SessionClient * session_client, gpointer value)*/
+
+/*void client_session_set_current_state(SessionClient * session_client, SessionClientState value)*/
+
+void client_session_set_restart_style(SessionClient * session_client, SessionRestartStyle value)
+{
+    if (session_client->session_connection != NULL) {
+        g_error("SessionClient: changing \"restart_style\" while connected to session manager is not implemented.");
+        return;
+    }
+    
+    session_client->restart_style = value;
+}
+
+/* void client_session_set_interact_style(SessionClient * session_client, SessionInteractStyle value) */
+
+void client_session_set_priority(SessionClient * session_client, gchar value)
+{
+    if (session_client->session_connection != NULL) {
+        g_error("SessionClient: changing \"priority\" while connected to session manager is not implemented.");
+        return;
+    }
+    
+    session_client->priority = value;
+}
+
+void client_session_set_client_id(SessionClient * session_client, gchar const* value)
+{
+    if (session_client->session_connection != NULL) {
+        g_error("SessionClient: changing \"client_id\" while connected to session manager is not possible.");
+        return;
+    }
+    
+    if (session_client->client_id != value) {
+        g_free(session_client->client_id);
+        session_client->client_id = safe_strdup(value);
+    }
+}
+
+/*void client_session_set_given_client_id(SessionClient * session_client, gchar const* value) */
+
+void client_session_set_current_directory(SessionClient * session_client, gchar const* value)
+{
+    if (session_client->session_connection != NULL) {
+        g_error("SessionClient: changing \"current_directory\" while connected to session manager is not implemented.");
+        return;
+    }
+    
+    if (session_client->current_directory != value) {
+        g_free(session_client->current_directory);
+        session_client->current_directory = safe_strdup(value);
+    }
+}
+
+void client_session_set_program(SessionClient * session_client, gchar const* value)
+{
+    if (session_client->session_connection != NULL) {
+        g_error("SessionClient: changing \"program\" while connected to session manager is not implemented.");
+        return;
+    }
+    
+    if (session_client->program != value) {
+        g_free(session_client->program);
+        session_client->program = safe_strdup(value);
+    }
+}
+
+void client_session_set_clone_command(SessionClient * session_client, gchar** const value)
+{
+    if (session_client->clone_command == value) {
+        return;
+    }
+    
+    if (session_client->clone_command != NULL) {
+        g_strfreev(session_client->clone_command);
+        session_client->clone_command = NULL;
+    }
+
+    if (value != NULL) {
+        session_client->clone_command = safe_strvdup(value);
+    }
+}
+
+void client_session_set_resign_command(SessionClient * session_client, gchar** const value)
+{
+    if (session_client->resign_command == value) {
+        return;
+    }
+    
+    if (session_client->resign_command != NULL) {
+        g_strfreev(session_client->resign_command);
+        session_client->resign_command = NULL;
+    }
+
+    if (value != NULL) {
+        session_client->resign_command = safe_strvdup(value);
+    }
+}
+
+void client_session_set_restart_command(SessionClient * session_client, gchar** const value)
+{
+    if (session_client->restart_command == value) {
+        return;
+    }
+    
+    if (session_client->restart_command != NULL) {
+        g_strfreev(session_client->restart_command);
+        session_client->restart_command = NULL;
+    }
+
+    if (value != NULL) {
+        session_client->restart_command = safe_strvdup(value);
+    }
+}
+
+void client_session_set_discard_command(SessionClient * session_client, gchar** const value)
+{
+    if (session_client->discard_command == value) {
+        return;
+    }
+    
+    if (session_client->discard_command != NULL) {
+        g_strfreev(session_client->discard_command);
+        session_client->discard_command = NULL;
+    }
+
+    if (value != NULL) {
+        session_client->discard_command = safe_strvdup(value);
+    }
+}
+
+void client_session_set_shutdown_command(SessionClient * session_client, gchar** const value)
+{
+    if (session_client->shutdown_command == value) {
+        return;
+    }
+    
+    if (session_client->shutdown_command != NULL) {
+        g_strfreev(session_client->shutdown_command);
+        session_client->shutdown_command = NULL;
+    }
+
+    if (value != NULL) {
+        session_client->shutdown_command = safe_strvdup(value);
+    }
+}
+
+save_phase_2_callback client_session_get_save_phase_2_callback(SessionClient * session_client)
+{
+    return session_client->save_phase_2;
+}
+
+interact_callback client_session_get_interact_callback(SessionClient * session_client)
+{
+    return session_client->interact;
+}
+
+shutdown_cancelled_callback client_session_get_shutdown_cancelled_callback(SessionClient * session_client)
+{
+    return session_client->shutdown_cancelled;
+}
+
+save_complete_callback client_session_get_save_complete_callback(SessionClient * session_client)
+{
+    return session_client->save_complete;
+}
+
+die_callback client_session_get_die_callback(SessionClient * session_client)
+{
+    return session_client->die;
+}
+
+save_yourself_callback client_session_get_save_yourself_callback(SessionClient * session_client)
+{
+    return session_client->save_yourself;
+}
+
+gpointer client_session_get_data(SessionClient * session_client)
+{
+    return session_client->data;
+}
+
+gpointer client_session_get_session_connection(SessionClient * session_client)
+{
+    return session_client->session_connection;
+}
+
+SessionClientState client_session_get_current_state(SessionClient * session_client)
+{
+    return session_client->current_state;
+}
+
+SessionRestartStyle client_session_get_restart_style(SessionClient * session_client)
+{
+    return session_client->restart_style;
+}
+
+SessionInteractStyle client_session_get_interact_style(SessionClient * session_client)
+{
+    return session_client->interact_style;
+}
+
+gchar client_session_get_priority(SessionClient * session_client)
+{
+    return session_client->priority;
+}
+
+gchar const* client_session_get_client_id(SessionClient * session_client)
+{
+    return session_client->client_id;
+}
+
+gchar const* client_session_get_given_client_id(SessionClient * session_client)
+{
+    return session_client->given_client_id;
+}
+
+gchar const* client_session_get_current_directory(SessionClient * session_client)
+{
+    return session_client->current_directory;
+}
+
+gchar const* client_session_get_program(SessionClient * session_client)
+{
+    return session_client->program;
+}
+
+G_CONST_RETURN gchar* G_CONST_RETURN * client_session_get_clone_command(SessionClient * session_client)
+{
+    return (const gchar * const *) session_client->clone_command;
+}
+
+G_CONST_RETURN gchar* G_CONST_RETURN * client_session_get_resign_command(SessionClient * session_client)
+{
+    return (const gchar * const *) session_client->resign_command;
+}
+
+G_CONST_RETURN gchar* G_CONST_RETURN * client_session_get_restart_command(SessionClient * session_client)
+{
+    return (const gchar * const *) session_client->restart_command;
+}
+
+G_CONST_RETURN gchar* G_CONST_RETURN * client_session_get_discard_command(SessionClient * session_client)
+{
+    return (const gchar * const *) session_client->discard_command;
+}
+
+G_CONST_RETURN gchar* G_CONST_RETURN * client_session_get_shutdown_command(SessionClient * session_client)
+{
+    return (const gchar * const *) session_client->shutdown_command;
+}
+
Index: session-client.h
===================================================================
--- session-client.h	(Revision 24812)
+++ session-client.h	(Arbeitskopie)
@@ -118,8 +118,49 @@
 				   SessionRestartStyle restart_style,
 				   gchar priority);
 
+void client_session_free(SessionClient * session_client);
+/* SessionClient *client_session_copy(SessionClient * session_client); */
+
 gboolean session_init (SessionClient * session_client);
 void session_shutdown (SessionClient * session_client);
 void logout_session (SessionClient * session_client);
 
+void client_session_set_save_phase_2_callback(SessionClient * session_client, save_phase_2_callback value);
+void client_session_set_interact_callback(SessionClient * session_client, interact_callback value);
+void client_session_set_shutdown_cancelled_callback(SessionClient * session_client, shutdown_cancelled_callback value);
+void client_session_set_save_complete_callback(SessionClient * session_client, save_complete_callback value);
+void client_session_set_die_callback(SessionClient * session_client, die_callback value);
+void client_session_set_save_yourself_callback(SessionClient * session_client, save_yourself_callback value);
+void client_session_set_data(SessionClient * session_client, gpointer value);
+void client_session_set_restart_style(SessionClient * session_client, SessionRestartStyle value);
+void client_session_set_priority(SessionClient * session_client, gchar value);
+void client_session_set_client_id(SessionClient * session_client, gchar const* value);
+void client_session_set_current_directory(SessionClient * session_client, gchar const* value);
+void client_session_set_program(SessionClient * session_client, gchar const* value);
+void client_session_set_clone_command(SessionClient * session_client, gchar** const value);
+void client_session_set_resign_command(SessionClient * session_client, gchar** const value);
+void client_session_set_restart_command(SessionClient * session_client, gchar** const value);
+void client_session_set_discard_command(SessionClient * session_client, gchar** const value);
+void client_session_set_shutdown_command(SessionClient * session_client, gchar** const value);
+save_phase_2_callback client_session_get_save_phase_2_callback(SessionClient * session_client);
+interact_callback client_session_get_interact_callback(SessionClient * session_client);
+shutdown_cancelled_callback client_session_get_shutdown_cancelled_callback(SessionClient * session_client);
+save_complete_callback client_session_get_save_complete_callback(SessionClient * session_client);
+die_callback client_session_get_die_callback(SessionClient * session_client);
+save_yourself_callback client_session_get_save_yourself_callback(SessionClient * session_client);
+gpointer client_session_get_data(SessionClient * session_client);
+gpointer client_session_get_session_connection(SessionClient * session_client);
+SessionClientState client_session_get_current_state(SessionClient * session_client);
+SessionRestartStyle client_session_get_restart_style(SessionClient * session_client);
+SessionInteractStyle client_session_get_interact_style(SessionClient * session_client);
+gchar client_session_get_priority(SessionClient * session_client);
+gchar const* client_session_get_client_id(SessionClient * session_client);
+gchar const* client_session_get_given_client_id(SessionClient * session_client);
+gchar const* client_session_get_current_directory(SessionClient * session_client);
+gchar const* client_session_get_program(SessionClient * session_client);
+G_CONST_RETURN gchar* G_CONST_RETURN * client_session_get_clone_command(SessionClient * session_client);
+G_CONST_RETURN gchar* G_CONST_RETURN * client_session_get_resign_command(SessionClient * session_client);
+G_CONST_RETURN gchar* G_CONST_RETURN * client_session_get_restart_command(SessionClient * session_client);
+G_CONST_RETURN gchar* G_CONST_RETURN * client_session_get_discard_command(SessionClient * session_client);
+G_CONST_RETURN gchar* G_CONST_RETURN * client_session_get_shutdown_command(SessionClient * session_client);
 #endif

This is a huge change in terms of number of lines, but it doesn't change anything at all in terms of existing functionality.

Then again, it fixes point 2 and 3. (Point 1 is unfixable in principle)

What do you think?

cheers,
  Danny