[RFC,spice-vdagent,16/18] move to GLib memory functions

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

Details

Message ID 20180814185352.6080-17-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.
Some older parts of the code currently use
memory functions defined in stdlib.h
and usually handle allocation errors.

On the other hand, newer parts of the code
and GLib/GTK+ functions themselves commonly use
wrappers provided by GLib that terminate
the program when there isn't enough memory.

So it doesn't make much sense to check for
ENOMEM somewhere, while the program can be
terminated at any time elsewhere.

Unify the code and use GLib wrappers only.

malloc --> g_malloc, g_new
calloc --> g_malloc0, g_new0
free   --> g_free

Use g_try_malloc only when allocating
message buffers, where the agent could
theoretically continue functioning even
after the allocation fails.
(e.g. when trying to send large
      clipboard data buffer)

Use g_clear_pointer where possible
for simplification.
---
 src/udscs.c                  | 23 ++++-----------
 src/vdagent/x11-randr.c      | 43 ++++++++++-----------------
 src/vdagentd/console-kit.c   | 13 ++++----
 src/vdagentd/systemd-login.c | 13 ++++----
 src/vdagentd/uinput.c        |  9 ++----
 src/vdagentd/vdagentd.c      | 57 +++++++++---------------------------
 src/vdagentd/virtio-port.c   | 37 ++++++++---------------
 7 files changed, 60 insertions(+), 135 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/udscs.c b/src/udscs.c
index 0b80317..681e80f 100644
--- a/src/udscs.c
+++ b/src/udscs.c
@@ -101,9 +101,7 @@  struct udscs_connection *udscs_connect(const char *socketname,
     if (io_stream == NULL)
         return NULL;
 
-    conn = calloc(1, sizeof(*conn));
-    if (!conn)
-        return NULL;
+    conn = g_new0(struct udscs_connection, 1);
     conn->debug = debug;
     conn->conn = vdagent_connection_new(io_stream,
                                         FALSE,
@@ -141,8 +139,7 @@  void udscs_destroy_connection(struct udscs_connection **connp)
     if (conn->debug)
         syslog(LOG_DEBUG, "%p disconnected", conn);
 
-    free(conn);
-    *connp = NULL;
+    g_clear_pointer(connp, g_free);
 }
 
 void udscs_set_user_data(struct udscs_connection *conn, void *data)
@@ -166,7 +163,7 @@  int udscs_write(struct udscs_connection *conn, uint32_t type, uint32_t arg1,
     struct udscs_message_header header;
 
     buff_size = sizeof(header) + size;
-    buff = malloc(buff_size);
+    buff = g_try_malloc(buff_size);
     if (buff == NULL)
         return -1;
 
@@ -212,10 +209,7 @@  struct udscs_server *udscs_server_new(
 {
     struct udscs_server *server;
 
-    server = calloc(1, sizeof(*server));
-    if (!server)
-        return NULL;
-
+    server = g_new0(struct udscs_server, 1);
     server->debug = debug;
     server->connect_callback = connect_callback;
     server->read_callback = read_callback;
@@ -282,7 +276,7 @@  void udscs_destroy_server(struct udscs_server *server)
         conn = next_conn;
     }
     g_object_unref(server->service);
-    free(server);
+    g_free(server);
 }
 
 int udscs_get_peer_pid(struct udscs_connection *conn)
@@ -299,12 +293,7 @@  static gboolean udscs_server_accept_cb(GSocketService    *service,
     struct udscs_server *server = user_data;
     struct udscs_connection *new_conn, *conn;
 
-    new_conn = calloc(1, sizeof(*conn));
-    if (!new_conn) {
-        syslog(LOG_ERR, "out of memory, disconnecting new client");
-        return TRUE;
-    }
-
+    new_conn = g_new0(struct udscs_connection, 1);
     new_conn->debug = server->debug;
     new_conn->read_callback = server->read_callback;
     new_conn->disconnect_callback = server->disconnect_callback;
diff --git a/src/vdagent/x11-randr.c b/src/vdagent/x11-randr.c
index 803cf73..29ce6fc 100644
--- a/src/vdagent/x11-randr.c
+++ b/src/vdagent/x11-randr.c
@@ -81,13 +81,13 @@  static void free_randr_resources(struct vdagent_x11 *x11)
         for (i = 0 ; i < x11->randr.res->noutput; ++i) {
             XRRFreeOutputInfo(x11->randr.outputs[i]);
         }
-        free(x11->randr.outputs);
+        g_free(x11->randr.outputs);
     }
     if (x11->randr.crtcs != NULL) {
         for (i = 0 ; i < x11->randr.res->ncrtc; ++i) {
             XRRFreeCrtcInfo(x11->randr.crtcs[i]);
         }
-        free(x11->randr.crtcs);
+        g_free(x11->randr.crtcs);
     }
     XRRFreeScreenResources(x11->randr.res);
     x11->randr.res = NULL;
@@ -105,8 +105,8 @@  static void update_randr_res(struct vdagent_x11 *x11, int poll)
         x11->randr.res = XRRGetScreenResources(x11->display, x11->root_window[0]);
     else
         x11->randr.res = XRRGetScreenResourcesCurrent(x11->display, x11->root_window[0]);
-    x11->randr.outputs = malloc(x11->randr.res->noutput * sizeof(*x11->randr.outputs));
-    x11->randr.crtcs = malloc(x11->randr.res->ncrtc * sizeof(*x11->randr.crtcs));
+    x11->randr.outputs = g_malloc(x11->randr.res->noutput * sizeof(*x11->randr.outputs));
+    x11->randr.crtcs = g_malloc(x11->randr.res->ncrtc * sizeof(*x11->randr.crtcs));
     for (i = 0 ; i < x11->randr.res->noutput; ++i) {
         x11->randr.outputs[i] = XRRGetOutputInfo(x11->display, x11->randr.res,
                                                  x11->randr.res->outputs[i]);
@@ -696,7 +696,7 @@  static VDAgentMonitorsConfig *get_current_mon_config(struct vdagent_x11 *x11)
 
 error:
     syslog(LOG_ERR, "error: inconsistent or stale data from X");
-    free(mon_config);
+    g_free(mon_config);
     return NULL;
 }
 
@@ -844,12 +844,12 @@  void vdagent_x11_set_monitor_config(struct vdagent_x11 *x11,
             if (!fallback) {
                 syslog(LOG_WARNING, "Restoring previous config");
                 vdagent_x11_set_monitor_config(x11, curr, 1);
-                free(curr);
+                g_free(curr);
                 /* Remember this config failed, if the client is maximized or
                    fullscreen it will keep sending the failing config. */
-                free(x11->randr.failed_conf);
+                g_free(x11->randr.failed_conf);
                 x11->randr.failed_conf =
-                    malloc(config_size(mon_config->num_of_monitors));
+                    g_malloc(config_size(mon_config->num_of_monitors));
                 if (x11->randr.failed_conf)
                     memcpy(x11->randr.failed_conf, mon_config,
                            config_size(mon_config->num_of_monitors));
@@ -895,7 +895,7 @@  exit:
 
     /* Flush output buffers and consume any pending events */
     vdagent_x11_do_read(x11);
-    free(curr);
+    g_free(curr);
 }
 
 void vdagent_x11_send_daemon_guest_xorg_res(struct vdagent_x11 *x11, int update)
@@ -914,11 +914,7 @@  void vdagent_x11_send_daemon_guest_xorg_res(struct vdagent_x11 *x11, int update)
             goto no_info;
 
         screen_count = curr->num_of_monitors;
-        res = malloc(screen_count * sizeof(*res));
-        if (!res) {
-            free(curr);
-            goto no_mem;
-        }
+        res = g_malloc(screen_count * sizeof(*res));
 
         for (i = 0; i < screen_count; i++) {
             res[i].width  = curr->monitors[i].width;
@@ -926,7 +922,7 @@  void vdagent_x11_send_daemon_guest_xorg_res(struct vdagent_x11 *x11, int update)
             res[i].x = curr->monitors[i].x;
             res[i].y = curr->monitors[i].y;
         }
-        free(curr);
+        g_free(curr);
         width  = x11->width[0];
         height = x11->height[0];
     } else if (x11->has_xinerama) {
@@ -935,17 +931,13 @@  void vdagent_x11_send_daemon_guest_xorg_res(struct vdagent_x11 *x11, int update)
         screen_info = XineramaQueryScreens(x11->display, &screen_count);
         if (!screen_info)
             goto no_info;
-        res = malloc(screen_count * sizeof(*res));
-        if (!res) {
-            XFree(screen_info);
-            goto no_mem;
-        }
+        res = g_malloc(screen_count * sizeof(*res));
         for (i = 0; i < screen_count; i++) {
             if (screen_info[i].screen_number >= screen_count) {
                 syslog(LOG_ERR, "Invalid screen number in xinerama screen info (%d >= %d)",
                        screen_info[i].screen_number, screen_count);
                 XFree(screen_info);
-                free(res);
+                g_free(res);
                 return;
             }
             res[screen_info[i].screen_number].width = screen_info[i].width;
@@ -959,9 +951,7 @@  void vdagent_x11_send_daemon_guest_xorg_res(struct vdagent_x11 *x11, int update)
     } else {
 no_info:
         screen_count = x11->screen_count;
-        res = malloc(screen_count * sizeof(*res));
-        if (!res)
-            goto no_mem;
+        res = g_malloc(screen_count * sizeof(*res));
         for (i = 0; i < screen_count; i++) {
             res[i].width  = x11->width[i];
             res[i].height = x11->height[i];
@@ -982,8 +972,5 @@  no_info:
 
     udscs_write(x11->vdagentd, VDAGENTD_GUEST_XORG_RESOLUTION, width, height,
                 (uint8_t *)res, screen_count * sizeof(*res));
-    free(res);
-    return;
-no_mem:
-    syslog(LOG_ERR, "out of memory while trying to send resolutions, not sending resolutions.");
+    g_free(res);
 }
diff --git a/src/vdagentd/console-kit.c b/src/vdagentd/console-kit.c
index 390491e..8094fa9 100644
--- a/src/vdagentd/console-kit.c
+++ b/src/vdagentd/console-kit.c
@@ -162,10 +162,7 @@  struct session_info *session_info_create(int verbose, ActiveSessionChangeCb cb)
     struct session_info *info;
     GError *err = NULL;
 
-    info = calloc(1, sizeof(*info));
-    if (!info)
-        return NULL;
-
+    info = g_new0(struct session_info, 1);
     info->verbose = verbose;
     info->session_is_locked = FALSE;
     info->session_idle_hint = FALSE;
@@ -174,7 +171,7 @@  struct session_info *session_info_create(int verbose, ActiveSessionChangeCb cb)
     if (err) {
         syslog(LOG_ERR, "%s: %s", __func__, err->message);
         g_error_free(err);
-        free(info);
+        g_free(info);
         return NULL;
     }
 
@@ -194,9 +191,9 @@  void session_info_destroy(struct session_info *info)
 
     si_dbus_signals_unsubscribe(info);
     g_object_unref(info->dbus);
-    free(info->seat);
-    free(info->active_session);
-    free(info);
+    g_free(info->seat);
+    g_free(info->active_session);
+    g_free(info);
 }
 
 /* Invoke a method on a remote object through DBus and wait for reply.
diff --git a/src/vdagentd/systemd-login.c b/src/vdagentd/systemd-login.c
index 0940230..7a9a13f 100644
--- a/src/vdagentd/systemd-login.c
+++ b/src/vdagentd/systemd-login.c
@@ -109,10 +109,7 @@  struct session_info *session_info_create(int verbose, ActiveSessionChangeCb cb)
     struct session_info *si;
     int r;
 
-    si = calloc(1, sizeof(*si));
-    if (!si)
-        return NULL;
-
+    si = g_new0(struct session_info, 1);
     si->verbose = verbose;
     si->session_is_locked = FALSE;
     si->session_change_cb = cb;
@@ -120,7 +117,7 @@  struct session_info *session_info_create(int verbose, ActiveSessionChangeCb cb)
     r = sd_login_monitor_new("session", &si->mon);
     if (r < 0) {
         syslog(LOG_ERR, "Error creating login monitor: %s", strerror(-r));
-        free(si);
+        g_free(si);
         return NULL;
     }
 
@@ -139,8 +136,8 @@  void session_info_destroy(struct session_info *si)
     g_source_remove(si->io_watch_id);
     g_io_channel_unref(si->io_channel);
     sd_login_monitor_unref(si->mon);
-    free(si->session);
-    free(si);
+    g_free(si->session);
+    g_free(si);
 }
 
 const char *session_info_get_active_session(struct session_info *si)
@@ -160,7 +157,7 @@  const char *session_info_get_active_session(struct session_info *si)
         syslog(LOG_INFO, "Active session: %s", si->session);
 
     sd_login_monitor_flush(si->mon);
-    free(old_session);
+    g_free(old_session);
 
     si_dbus_proxy_update(si);
     return si->session;
diff --git a/src/vdagentd/uinput.c b/src/vdagentd/uinput.c
index e2966c4..880a679 100644
--- a/src/vdagentd/uinput.c
+++ b/src/vdagentd/uinput.c
@@ -31,6 +31,7 @@ 
 #include <linux/input.h>
 #include <linux/uinput.h>
 #include <spice/vd_agent.h>
+#include <glib.h>
 #include "uinput.h"
 
 struct vdagentd_uinput {
@@ -52,10 +53,7 @@  struct vdagentd_uinput *vdagentd_uinput_create(const char *devname,
 {
     struct vdagentd_uinput *uinput;
 
-    uinput = calloc(1, sizeof(*uinput));
-    if (!uinput)
-        return NULL;
-
+    uinput = g_new0(struct vdagentd_uinput, 1);
     uinput->devname = devname;
     uinput->fd      = -1; /* Gets opened by vdagentd_uinput_update_size() */
     uinput->debug   = debug;
@@ -76,8 +74,7 @@  void vdagentd_uinput_destroy(struct vdagentd_uinput **uinputp)
 
     if (uinput->fd != -1)
         close(uinput->fd);
-    free(uinput);
-    *uinputp = NULL;
+    g_clear_pointer(uinputp, g_free);
 }
 
 void vdagentd_uinput_update_size(struct vdagentd_uinput **uinputp,
diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c
index 6d117a8..b8f617c 100644
--- a/src/vdagentd/vdagentd.c
+++ b/src/vdagentd/vdagentd.c
@@ -123,12 +123,7 @@  static void send_capabilities(struct vdagent_virtio_port *vport,
     uint32_t size;
 
     size = sizeof(*caps) + VD_AGENT_CAPS_BYTES;
-    caps = calloc(1, size);
-    if (!caps) {
-        syslog(LOG_ERR, "out of memory allocating capabilities array (write)");
-        return;
-    }
-
+    caps = g_malloc0(size);
     caps->request = request;
     VD_AGENT_SET_CAPABILITY(caps->caps, VD_AGENT_CAP_MOUSE_STATE);
     VD_AGENT_SET_CAPABILITY(caps->caps, VD_AGENT_CAP_MONITORS_CONFIG);
@@ -144,7 +139,7 @@  static void send_capabilities(struct vdagent_virtio_port *vport,
     vdagent_virtio_port_write(vport, VDP_CLIENT_PORT,
                               VD_AGENT_ANNOUNCE_CAPABILITIES, 0,
                               (uint8_t *)caps, size);
-    free(caps);
+    g_free(caps);
 }
 
 static void do_client_disconnect(void)
@@ -197,12 +192,8 @@  static void do_client_monitors(struct vdagent_virtio_port *vport, int port_nr,
 
     if (!mon_config ||
             mon_config->num_of_monitors != new_monitors->num_of_monitors) {
-        free(mon_config);
-        mon_config = malloc(size);
-        if (!mon_config) {
-            syslog(LOG_ERR, "out of memory allocating monitors config");
-            return;
-        }
+        g_free(mon_config);
+        mon_config = g_malloc(size);
     }
     memcpy(mon_config, new_monitors, size);
 
@@ -239,13 +230,8 @@  static void do_client_capabilities(struct vdagent_virtio_port *vport,
 
     if (capabilities_size != new_size) {
         capabilities_size = new_size;
-        free(capabilities);
-        capabilities = malloc(capabilities_size * sizeof(uint32_t));
-        if (!capabilities) {
-            syslog(LOG_ERR, "oom allocating capabilities array (read)");
-            capabilities_size = 0;
-            return;
-        }
+        g_free(capabilities);
+        capabilities = g_malloc(capabilities_size * sizeof(uint32_t));
     }
     memcpy(capabilities, caps->caps, capabilities_size * sizeof(uint32_t));
     if (caps->request) {
@@ -327,7 +313,7 @@  static void send_file_xfer_status(struct vdagent_virtio_port *vport,
         data_size = 0;
     }
 
-    status = malloc(sizeof(*status) + data_size);
+    status = g_malloc(sizeof(*status) + data_size);
     status->id = GUINT32_TO_LE(id);
     status->result = GUINT32_TO_LE(xfer_status);
     if (data)
@@ -341,7 +327,7 @@  static void send_file_xfer_status(struct vdagent_virtio_port *vport,
                                   VD_AGENT_FILE_XFER_STATUS, 0,
                                   (uint8_t *)status, sizeof(*status) + data_size);
 
-    free(status);
+    g_free(status);
 }
 
 static void do_client_file_xfer(struct vdagent_virtio_port *vport,
@@ -875,13 +861,7 @@  static gboolean remove_active_xfers(gpointer key, gpointer value, gpointer conn)
 static void agent_connect(struct udscs_connection *conn)
 {
     struct agent_data *agent_data;
-
-    agent_data = calloc(1, sizeof(*agent_data));
-    if (!agent_data) {
-        syslog(LOG_ERR, "Out of memory allocating agent data, disconnecting");
-        udscs_destroy_connection(&conn);
-        return;
-    }
+    agent_data = g_new0(struct agent_data, 1);
 
     if (session_info) {
         uint32_t pid = udscs_get_peer_pid(conn);
@@ -900,12 +880,11 @@  static void agent_disconnect(struct udscs_connection *conn)
 
     g_hash_table_foreach_remove(active_xfers, remove_active_xfers, conn);
 
-    free(agent_data->session);
-    agent_data->session = NULL;
+    g_clear_pointer(&agent_data->session, g_free);
     update_active_session_connection(NULL);
 
-    free(agent_data->screen_info);
-    free(agent_data);
+    g_free(agent_data->screen_info);
+    g_free(agent_data);
 }
 
 static void agent_read_complete(struct udscs_connection **connp,
@@ -934,12 +913,8 @@  static void agent_read_complete(struct udscs_connection **connp,
             return;
         }
 
-        free(agent_data->screen_info);
-        res = malloc(n * sizeof(*res));
-        if (!res) {
-            syslog(LOG_ERR, "out of memory allocating screen info");
-            n = 0;
-        }
+        g_free(agent_data->screen_info);
+        res = g_malloc(n * sizeof(*res));
         memcpy(res, data, n * sizeof(*res));
         agent_data->width  = header->arg1;
         agent_data->height = header->arg2;
@@ -1112,10 +1087,6 @@  int main(int argc, char *argv[])
     /* Setup communication with vdagent process(es) */
     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;
-    }
 #ifdef WITH_SYSTEMD_SOCKET_ACTIVATION
     int n_fds;
     /* try to retrieve pre-configured sockets from systemd */
diff --git a/src/vdagentd/virtio-port.c b/src/vdagentd/virtio-port.c
index 9731086..723d9ad 100644
--- a/src/vdagentd/virtio-port.c
+++ b/src/vdagentd/virtio-port.c
@@ -116,10 +116,7 @@  struct vdagent_virtio_port *vdagent_virtio_port_create(const char *portname,
             return NULL;
     }
 
-    vport = calloc(1, sizeof(*vport));
-    if (!vport)
-        return 0;
-
+    vport = g_new0(struct vdagent_virtio_port, 1);
     /* When calling vdagent_connection_new(),
      * @wait_on_opening MUST be set to TRUE:
      *
@@ -166,17 +163,16 @@  static void virtio_port_destroy(struct vdagent_virtio_port **vportp,
         vport->disconnect_callback(vport, by_user);
 
     if (vport->write_buf) {
-        free(vport->write_buf->buf);
-        free(vport->write_buf);
+        g_free(vport->write_buf->buf);
+        g_free(vport->write_buf);
     }
 
     for (i = 0; i < VDP_END_PORT; i++) {
-        free(vport->port_data[i].message_data);
+        g_free(vport->port_data[i].message_data);
     }
 
     vdagent_connection_destroy(vport->conn);
-    free(vport);
-    *vportp = NULL;
+    g_clear_pointer(vportp, g_free);
 }
 
 void vdagent_virtio_port_destroy(struct vdagent_virtio_port **vportp)
@@ -195,15 +191,12 @@  int vdagent_virtio_port_write_start(
     VDIChunkHeader chunk_header;
     VDAgentMessage message_header;
 
-    new_wbuf = malloc(sizeof(*new_wbuf));
-    if (!new_wbuf)
-        return -1;
-
+    new_wbuf = g_new(struct vdagent_virtio_port_buf, 1);
     new_wbuf->write_pos = 0;
     new_wbuf->size = sizeof(chunk_header) + sizeof(message_header) + data_size;
-    new_wbuf->buf = malloc(new_wbuf->size);
+    new_wbuf->buf =g_try_malloc(new_wbuf->size);
     if (!new_wbuf->buf) {
-        free(new_wbuf);
+        g_free(new_wbuf);
         return -1;
     }
 
@@ -249,7 +242,7 @@  int vdagent_virtio_port_write_append(struct vdagent_virtio_port *vport,
 
     if (wbuf->write_pos == wbuf->size) {
         vdagent_connection_write(vport->conn, wbuf->buf, wbuf->size);
-        g_clear_pointer(&vport->write_buf, free);
+        g_clear_pointer(&vport->write_buf, g_free);
     }
     return 0;
 }
@@ -282,7 +275,7 @@  void vdagent_virtio_port_reset(struct vdagent_virtio_port *vport, int port)
         syslog(LOG_ERR, "vdagent_virtio_port_reset port out of range");
         return;
     }
-    free(vport->port_data[port].message_data);
+    g_free(vport->port_data[port].message_data);
     memset(&vport->port_data[port], 0, sizeof(vport->port_data[0]));
 }
 
@@ -311,12 +304,7 @@  static void vdagent_virtio_port_do_chunk(struct vdagent_virtio_port **vportp,
             port->message_header.size = GUINT32_FROM_LE(port->message_header.size);
 
             if (port->message_header.size) {
-                port->message_data = malloc(port->message_header.size);
-                if (!port->message_data) {
-                    syslog(LOG_ERR, "out of memory, disconnecting virtio");
-                    virtio_port_destroy(vportp, FALSE);
-                    return;
-                }
+                port->message_data = g_malloc(port->message_header.size);
             }
         }
         pos = read;
@@ -352,8 +340,7 @@  static void vdagent_virtio_port_do_chunk(struct vdagent_virtio_port **vportp,
             }
             port->message_header_read = 0;
             port->message_data_pos = 0;
-            free(port->message_data);
-            port->message_data = NULL;
+            g_clear_pointer(&port->message_data, g_free);
         }
     }
 }

Comments

Hi,

This one I would also take, together with Patch 0001, outside of
this series in order to reduce the patches a bit and focus more
on the Glib integration.

The memory functions in glib are not bounded to GMainLoop so it
should be fine.

On Tue, Aug 14, 2018 at 08:53:50PM +0200, Jakub Janků wrote:
> Some older parts of the code currently use
> memory functions defined in stdlib.h
> and usually handle allocation errors.
> 
> On the other hand, newer parts of the code
> and GLib/GTK+ functions themselves commonly use
> wrappers provided by GLib that terminate
> the program when there isn't enough memory.
> 
> So it doesn't make much sense to check for
> ENOMEM somewhere, while the program can be
> terminated at any time elsewhere.
> 
> Unify the code and use GLib wrappers only.
> 
> malloc --> g_malloc, g_new
> calloc --> g_malloc0, g_new0
> free   --> g_free
> 
> Use g_try_malloc only when allocating
> message buffers, where the agent could
> theoretically continue functioning even
> after the allocation fails.
> (e.g. when trying to send large
>       clipboard data buffer)

No, I don't think we should. For the clipboard example, the best
would be to either have a clipboard-size-limit (pretty sure there
is one) or we should use some limit in the spice-protocol itself.

The use case of g-try-malloc is for systems with small amount of
resources, such as embedded systems (IMHO).

> Use g_clear_pointer where possible
> for simplification.

Yep. If possible, S-o-B too.

Small comment below.

> ---
>  src/udscs.c                  | 23 ++++-----------
>  src/vdagent/x11-randr.c      | 43 ++++++++++-----------------
>  src/vdagentd/console-kit.c   | 13 ++++----
>  src/vdagentd/systemd-login.c | 13 ++++----
>  src/vdagentd/uinput.c        |  9 ++----
>  src/vdagentd/vdagentd.c      | 57 +++++++++---------------------------
>  src/vdagentd/virtio-port.c   | 37 ++++++++---------------
>  7 files changed, 60 insertions(+), 135 deletions(-)
> 
> diff --git a/src/udscs.c b/src/udscs.c
> index 0b80317..681e80f 100644
> --- a/src/udscs.c
> +++ b/src/udscs.c
> @@ -101,9 +101,7 @@ struct udscs_connection *udscs_connect(const char *socketname,
>      if (io_stream == NULL)
>          return NULL;
>  
> -    conn = calloc(1, sizeof(*conn));
> -    if (!conn)
> -        return NULL;
> +    conn = g_new0(struct udscs_connection, 1);
>      conn->debug = debug;
>      conn->conn = vdagent_connection_new(io_stream,
>                                          FALSE,
> @@ -141,8 +139,7 @@ void udscs_destroy_connection(struct udscs_connection **connp)
>      if (conn->debug)
>          syslog(LOG_DEBUG, "%p disconnected", conn);
>  
> -    free(conn);
> -    *connp = NULL;
> +    g_clear_pointer(connp, g_free);
>  }
>  
>  void udscs_set_user_data(struct udscs_connection *conn, void *data)
> @@ -166,7 +163,7 @@ int udscs_write(struct udscs_connection *conn, uint32_t type, uint32_t arg1,
>      struct udscs_message_header header;
>  
>      buff_size = sizeof(header) + size;
> -    buff = malloc(buff_size);
> +    buff = g_try_malloc(buff_size);
>      if (buff == NULL)
>          return -1;
>  
> @@ -212,10 +209,7 @@ struct udscs_server *udscs_server_new(
>  {
>      struct udscs_server *server;
>  
> -    server = calloc(1, sizeof(*server));
> -    if (!server)
> -        return NULL;
> -
> +    server = g_new0(struct udscs_server, 1);
>      server->debug = debug;
>      server->connect_callback = connect_callback;
>      server->read_callback = read_callback;
> @@ -282,7 +276,7 @@ void udscs_destroy_server(struct udscs_server *server)
>          conn = next_conn;
>      }
>      g_object_unref(server->service);
> -    free(server);
> +    g_free(server);
>  }
>  
>  int udscs_get_peer_pid(struct udscs_connection *conn)
> @@ -299,12 +293,7 @@ static gboolean udscs_server_accept_cb(GSocketService    *service,
>      struct udscs_server *server = user_data;
>      struct udscs_connection *new_conn, *conn;
>  
> -    new_conn = calloc(1, sizeof(*conn));
> -    if (!new_conn) {
> -        syslog(LOG_ERR, "out of memory, disconnecting new client");
> -        return TRUE;
> -    }
> -
> +    new_conn = g_new0(struct udscs_connection, 1);
>      new_conn->debug = server->debug;
>      new_conn->read_callback = server->read_callback;
>      new_conn->disconnect_callback = server->disconnect_callback;
> diff --git a/src/vdagent/x11-randr.c b/src/vdagent/x11-randr.c
> index 803cf73..29ce6fc 100644
> --- a/src/vdagent/x11-randr.c
> +++ b/src/vdagent/x11-randr.c
> @@ -81,13 +81,13 @@ static void free_randr_resources(struct vdagent_x11 *x11)
>          for (i = 0 ; i < x11->randr.res->noutput; ++i) {
>              XRRFreeOutputInfo(x11->randr.outputs[i]);
>          }
> -        free(x11->randr.outputs);
> +        g_free(x11->randr.outputs);
>      }
>      if (x11->randr.crtcs != NULL) {
>          for (i = 0 ; i < x11->randr.res->ncrtc; ++i) {
>              XRRFreeCrtcInfo(x11->randr.crtcs[i]);
>          }
> -        free(x11->randr.crtcs);
> +        g_free(x11->randr.crtcs);
>      }
>      XRRFreeScreenResources(x11->randr.res);
>      x11->randr.res = NULL;
> @@ -105,8 +105,8 @@ static void update_randr_res(struct vdagent_x11 *x11, int poll)
>          x11->randr.res = XRRGetScreenResources(x11->display, x11->root_window[0]);
>      else
>          x11->randr.res = XRRGetScreenResourcesCurrent(x11->display, x11->root_window[0]);
> -    x11->randr.outputs = malloc(x11->randr.res->noutput * sizeof(*x11->randr.outputs));
> -    x11->randr.crtcs = malloc(x11->randr.res->ncrtc * sizeof(*x11->randr.crtcs));
> +    x11->randr.outputs = g_malloc(x11->randr.res->noutput * sizeof(*x11->randr.outputs));
> +    x11->randr.crtcs = g_malloc(x11->randr.res->ncrtc * sizeof(*x11->randr.crtcs));
>      for (i = 0 ; i < x11->randr.res->noutput; ++i) {
>          x11->randr.outputs[i] = XRRGetOutputInfo(x11->display, x11->randr.res,
>                                                   x11->randr.res->outputs[i]);
> @@ -696,7 +696,7 @@ static VDAgentMonitorsConfig *get_current_mon_config(struct vdagent_x11 *x11)
>  
>  error:
>      syslog(LOG_ERR, "error: inconsistent or stale data from X");
> -    free(mon_config);
> +    g_free(mon_config);
>      return NULL;
>  }
>  
> @@ -844,12 +844,12 @@ void vdagent_x11_set_monitor_config(struct vdagent_x11 *x11,
>              if (!fallback) {
>                  syslog(LOG_WARNING, "Restoring previous config");
>                  vdagent_x11_set_monitor_config(x11, curr, 1);
> -                free(curr);
> +                g_free(curr);
>                  /* Remember this config failed, if the client is maximized or
>                     fullscreen it will keep sending the failing config. */
> -                free(x11->randr.failed_conf);
> +                g_free(x11->randr.failed_conf);
>                  x11->randr.failed_conf =
> -                    malloc(config_size(mon_config->num_of_monitors));
> +                    g_malloc(config_size(mon_config->num_of_monitors));
>                  if (x11->randr.failed_conf)
>                      memcpy(x11->randr.failed_conf, mon_config,
>                             config_size(mon_config->num_of_monitors));
> @@ -895,7 +895,7 @@ exit:
>  
>      /* Flush output buffers and consume any pending events */
>      vdagent_x11_do_read(x11);
> -    free(curr);
> +    g_free(curr);
>  }
>  
>  void vdagent_x11_send_daemon_guest_xorg_res(struct vdagent_x11 *x11, int update)
> @@ -914,11 +914,7 @@ void vdagent_x11_send_daemon_guest_xorg_res(struct vdagent_x11 *x11, int update)
>              goto no_info;
>  
>          screen_count = curr->num_of_monitors;
> -        res = malloc(screen_count * sizeof(*res));
> -        if (!res) {
> -            free(curr);
> -            goto no_mem;
> -        }
> +        res = g_malloc(screen_count * sizeof(*res));
>  
>          for (i = 0; i < screen_count; i++) {
>              res[i].width  = curr->monitors[i].width;
> @@ -926,7 +922,7 @@ void vdagent_x11_send_daemon_guest_xorg_res(struct vdagent_x11 *x11, int update)
>              res[i].x = curr->monitors[i].x;
>              res[i].y = curr->monitors[i].y;
>          }
> -        free(curr);
> +        g_free(curr);
>          width  = x11->width[0];
>          height = x11->height[0];
>      } else if (x11->has_xinerama) {
> @@ -935,17 +931,13 @@ void vdagent_x11_send_daemon_guest_xorg_res(struct vdagent_x11 *x11, int update)
>          screen_info = XineramaQueryScreens(x11->display, &screen_count);
>          if (!screen_info)
>              goto no_info;
> -        res = malloc(screen_count * sizeof(*res));
> -        if (!res) {
> -            XFree(screen_info);
> -            goto no_mem;
> -        }
> +        res = g_malloc(screen_count * sizeof(*res));
>          for (i = 0; i < screen_count; i++) {
>              if (screen_info[i].screen_number >= screen_count) {
>                  syslog(LOG_ERR, "Invalid screen number in xinerama screen info (%d >= %d)",
>                         screen_info[i].screen_number, screen_count);
>                  XFree(screen_info);
> -                free(res);
> +                g_free(res);
>                  return;
>              }
>              res[screen_info[i].screen_number].width = screen_info[i].width;
> @@ -959,9 +951,7 @@ void vdagent_x11_send_daemon_guest_xorg_res(struct vdagent_x11 *x11, int update)
>      } else {
>  no_info:
>          screen_count = x11->screen_count;
> -        res = malloc(screen_count * sizeof(*res));
> -        if (!res)
> -            goto no_mem;
> +        res = g_malloc(screen_count * sizeof(*res));
>          for (i = 0; i < screen_count; i++) {
>              res[i].width  = x11->width[i];
>              res[i].height = x11->height[i];
> @@ -982,8 +972,5 @@ no_info:
>  
>      udscs_write(x11->vdagentd, VDAGENTD_GUEST_XORG_RESOLUTION, width, height,
>                  (uint8_t *)res, screen_count * sizeof(*res));
> -    free(res);
> -    return;
> -no_mem:
> -    syslog(LOG_ERR, "out of memory while trying to send resolutions, not sending resolutions.");
> +    g_free(res);
>  }
> diff --git a/src/vdagentd/console-kit.c b/src/vdagentd/console-kit.c
> index 390491e..8094fa9 100644
> --- a/src/vdagentd/console-kit.c
> +++ b/src/vdagentd/console-kit.c
> @@ -162,10 +162,7 @@ struct session_info *session_info_create(int verbose, ActiveSessionChangeCb cb)
>      struct session_info *info;
>      GError *err = NULL;
>  
> -    info = calloc(1, sizeof(*info));
> -    if (!info)
> -        return NULL;
> -
> +    info = g_new0(struct session_info, 1);
>      info->verbose = verbose;
>      info->session_is_locked = FALSE;
>      info->session_idle_hint = FALSE;
> @@ -174,7 +171,7 @@ struct session_info *session_info_create(int verbose, ActiveSessionChangeCb cb)
>      if (err) {
>          syslog(LOG_ERR, "%s: %s", __func__, err->message);
>          g_error_free(err);
> -        free(info);
> +        g_free(info);
>          return NULL;
>      }
>  
> @@ -194,9 +191,9 @@ void session_info_destroy(struct session_info *info)
>  
>      si_dbus_signals_unsubscribe(info);
>      g_object_unref(info->dbus);
> -    free(info->seat);
> -    free(info->active_session);
> -    free(info);
> +    g_free(info->seat);
> +    g_free(info->active_session);
> +    g_free(info);
>  }
>  
>  /* Invoke a method on a remote object through DBus and wait for reply.
> diff --git a/src/vdagentd/systemd-login.c b/src/vdagentd/systemd-login.c
> index 0940230..7a9a13f 100644
> --- a/src/vdagentd/systemd-login.c
> +++ b/src/vdagentd/systemd-login.c
> @@ -109,10 +109,7 @@ struct session_info *session_info_create(int verbose, ActiveSessionChangeCb cb)
>      struct session_info *si;
>      int r;
>  
> -    si = calloc(1, sizeof(*si));
> -    if (!si)
> -        return NULL;
> -
> +    si = g_new0(struct session_info, 1);
>      si->verbose = verbose;
>      si->session_is_locked = FALSE;
>      si->session_change_cb = cb;
> @@ -120,7 +117,7 @@ struct session_info *session_info_create(int verbose, ActiveSessionChangeCb cb)
>      r = sd_login_monitor_new("session", &si->mon);
>      if (r < 0) {
>          syslog(LOG_ERR, "Error creating login monitor: %s", strerror(-r));
> -        free(si);
> +        g_free(si);
>          return NULL;
>      }
>  
> @@ -139,8 +136,8 @@ void session_info_destroy(struct session_info *si)
>      g_source_remove(si->io_watch_id);
>      g_io_channel_unref(si->io_channel);
>      sd_login_monitor_unref(si->mon);
> -    free(si->session);
> -    free(si);
> +    g_free(si->session);
> +    g_free(si);
>  }
>  
>  const char *session_info_get_active_session(struct session_info *si)
> @@ -160,7 +157,7 @@ const char *session_info_get_active_session(struct session_info *si)
>          syslog(LOG_INFO, "Active session: %s", si->session);
>  
>      sd_login_monitor_flush(si->mon);
> -    free(old_session);
> +    g_free(old_session);
>  
>      si_dbus_proxy_update(si);
>      return si->session;
> diff --git a/src/vdagentd/uinput.c b/src/vdagentd/uinput.c
> index e2966c4..880a679 100644
> --- a/src/vdagentd/uinput.c
> +++ b/src/vdagentd/uinput.c
> @@ -31,6 +31,7 @@
>  #include <linux/input.h>
>  #include <linux/uinput.h>
>  #include <spice/vd_agent.h>
> +#include <glib.h>
>  #include "uinput.h"
>  
>  struct vdagentd_uinput {
> @@ -52,10 +53,7 @@ struct vdagentd_uinput *vdagentd_uinput_create(const char *devname,
>  {
>      struct vdagentd_uinput *uinput;
>  
> -    uinput = calloc(1, sizeof(*uinput));
> -    if (!uinput)
> -        return NULL;
> -
> +    uinput = g_new0(struct vdagentd_uinput, 1);
>      uinput->devname = devname;
>      uinput->fd      = -1; /* Gets opened by vdagentd_uinput_update_size() */
>      uinput->debug   = debug;
> @@ -76,8 +74,7 @@ void vdagentd_uinput_destroy(struct vdagentd_uinput **uinputp)
>  
>      if (uinput->fd != -1)
>          close(uinput->fd);
> -    free(uinput);
> -    *uinputp = NULL;
> +    g_clear_pointer(uinputp, g_free);
>  }
>  
>  void vdagentd_uinput_update_size(struct vdagentd_uinput **uinputp,
> diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c
> index 6d117a8..b8f617c 100644
> --- a/src/vdagentd/vdagentd.c
> +++ b/src/vdagentd/vdagentd.c
> @@ -123,12 +123,7 @@ static void send_capabilities(struct vdagent_virtio_port *vport,
>      uint32_t size;
>  
>      size = sizeof(*caps) + VD_AGENT_CAPS_BYTES;
> -    caps = calloc(1, size);
> -    if (!caps) {
> -        syslog(LOG_ERR, "out of memory allocating capabilities array (write)");
> -        return;
> -    }
> -
> +    caps = g_malloc0(size);
>      caps->request = request;
>      VD_AGENT_SET_CAPABILITY(caps->caps, VD_AGENT_CAP_MOUSE_STATE);
>      VD_AGENT_SET_CAPABILITY(caps->caps, VD_AGENT_CAP_MONITORS_CONFIG);
> @@ -144,7 +139,7 @@ static void send_capabilities(struct vdagent_virtio_port *vport,
>      vdagent_virtio_port_write(vport, VDP_CLIENT_PORT,
>                                VD_AGENT_ANNOUNCE_CAPABILITIES, 0,
>                                (uint8_t *)caps, size);
> -    free(caps);
> +    g_free(caps);
>  }
>  
>  static void do_client_disconnect(void)
> @@ -197,12 +192,8 @@ static void do_client_monitors(struct vdagent_virtio_port *vport, int port_nr,
>  
>      if (!mon_config ||
>              mon_config->num_of_monitors != new_monitors->num_of_monitors) {
> -        free(mon_config);
> -        mon_config = malloc(size);
> -        if (!mon_config) {
> -            syslog(LOG_ERR, "out of memory allocating monitors config");
> -            return;
> -        }
> +        g_free(mon_config);
> +        mon_config = g_malloc(size);
>      }
>      memcpy(mon_config, new_monitors, size);
>  
> @@ -239,13 +230,8 @@ static void do_client_capabilities(struct vdagent_virtio_port *vport,
>  
>      if (capabilities_size != new_size) {
>          capabilities_size = new_size;
> -        free(capabilities);
> -        capabilities = malloc(capabilities_size * sizeof(uint32_t));
> -        if (!capabilities) {
> -            syslog(LOG_ERR, "oom allocating capabilities array (read)");
> -            capabilities_size = 0;
> -            return;
> -        }
> +        g_free(capabilities);
> +        capabilities = g_malloc(capabilities_size * sizeof(uint32_t));
>      }
>      memcpy(capabilities, caps->caps, capabilities_size * sizeof(uint32_t));
>      if (caps->request) {
> @@ -327,7 +313,7 @@ static void send_file_xfer_status(struct vdagent_virtio_port *vport,
>          data_size = 0;
>      }
>  
> -    status = malloc(sizeof(*status) + data_size);
> +    status = g_malloc(sizeof(*status) + data_size);
>      status->id = GUINT32_TO_LE(id);
>      status->result = GUINT32_TO_LE(xfer_status);
>      if (data)
> @@ -341,7 +327,7 @@ static void send_file_xfer_status(struct vdagent_virtio_port *vport,
>                                    VD_AGENT_FILE_XFER_STATUS, 0,
>                                    (uint8_t *)status, sizeof(*status) + data_size);
>  
> -    free(status);
> +    g_free(status);
>  }
>  
>  static void do_client_file_xfer(struct vdagent_virtio_port *vport,
> @@ -875,13 +861,7 @@ static gboolean remove_active_xfers(gpointer key, gpointer value, gpointer conn)
>  static void agent_connect(struct udscs_connection *conn)
>  {
>      struct agent_data *agent_data;
> -
> -    agent_data = calloc(1, sizeof(*agent_data));
> -    if (!agent_data) {
> -        syslog(LOG_ERR, "Out of memory allocating agent data, disconnecting");
> -        udscs_destroy_connection(&conn);
> -        return;
> -    }
> +    agent_data = g_new0(struct agent_data, 1);
>  
>      if (session_info) {
>          uint32_t pid = udscs_get_peer_pid(conn);
> @@ -900,12 +880,11 @@ static void agent_disconnect(struct udscs_connection *conn)
>  
>      g_hash_table_foreach_remove(active_xfers, remove_active_xfers, conn);
>  
> -    free(agent_data->session);
> -    agent_data->session = NULL;
> +    g_clear_pointer(&agent_data->session, g_free);
>      update_active_session_connection(NULL);
>  
> -    free(agent_data->screen_info);
> -    free(agent_data);
> +    g_free(agent_data->screen_info);
> +    g_free(agent_data);
>  }
>  
>  static void agent_read_complete(struct udscs_connection **connp,
> @@ -934,12 +913,8 @@ static void agent_read_complete(struct udscs_connection **connp,
>              return;
>          }
>  
> -        free(agent_data->screen_info);
> -        res = malloc(n * sizeof(*res));
> -        if (!res) {
> -            syslog(LOG_ERR, "out of memory allocating screen info");
> -            n = 0;
> -        }
> +        g_free(agent_data->screen_info);
> +        res = g_malloc(n * sizeof(*res));
>          memcpy(res, data, n * sizeof(*res));
>          agent_data->width  = header->arg1;
>          agent_data->height = header->arg2;
> @@ -1112,10 +1087,6 @@ int main(int argc, char *argv[])
>      /* Setup communication with vdagent process(es) */
>      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;
> -    }
>  #ifdef WITH_SYSTEMD_SOCKET_ACTIVATION
>      int n_fds;
>      /* try to retrieve pre-configured sockets from systemd */
> diff --git a/src/vdagentd/virtio-port.c b/src/vdagentd/virtio-port.c
> index 9731086..723d9ad 100644
> --- a/src/vdagentd/virtio-port.c
> +++ b/src/vdagentd/virtio-port.c
> @@ -116,10 +116,7 @@ struct vdagent_virtio_port *vdagent_virtio_port_create(const char *portname,
>              return NULL;
>      }
>  
> -    vport = calloc(1, sizeof(*vport));
> -    if (!vport)
> -        return 0;
> -
> +    vport = g_new0(struct vdagent_virtio_port, 1);
>      /* When calling vdagent_connection_new(),
>       * @wait_on_opening MUST be set to TRUE:
>       *
> @@ -166,17 +163,16 @@ static void virtio_port_destroy(struct vdagent_virtio_port **vportp,
>          vport->disconnect_callback(vport, by_user);
>  
>      if (vport->write_buf) {
> -        free(vport->write_buf->buf);
> -        free(vport->write_buf);
> +        g_free(vport->write_buf->buf);
> +        g_free(vport->write_buf);
>      }
>  
>      for (i = 0; i < VDP_END_PORT; i++) {
> -        free(vport->port_data[i].message_data);
> +        g_free(vport->port_data[i].message_data);
>      }
>  
>      vdagent_connection_destroy(vport->conn);
> -    free(vport);
> -    *vportp = NULL;
> +    g_clear_pointer(vportp, g_free);
>  }
>  
>  void vdagent_virtio_port_destroy(struct vdagent_virtio_port **vportp)
> @@ -195,15 +191,12 @@ int vdagent_virtio_port_write_start(
>      VDIChunkHeader chunk_header;
>      VDAgentMessage message_header;
>  
> -    new_wbuf = malloc(sizeof(*new_wbuf));
> -    if (!new_wbuf)
> -        return -1;
> -
> +    new_wbuf = g_new(struct vdagent_virtio_port_buf, 1);
>      new_wbuf->write_pos = 0;
>      new_wbuf->size = sizeof(chunk_header) + sizeof(message_header) + data_size;
> -    new_wbuf->buf = malloc(new_wbuf->size);
> +    new_wbuf->buf =g_try_malloc(new_wbuf->size);
>      if (!new_wbuf->buf) {
> -        free(new_wbuf);
> +        g_free(new_wbuf);
>          return -1;

Functions like this one, we can remove the return value after
using g_malloc() as that would abort() in case of failure
(ENOMEM). In that case, a patch for that function only would be
easier to review/revert if necessary.

Reviewed-by: Victor Toso <victortoso@redhat.com>

Cheers,
Victor

>      }
>  
> @@ -249,7 +242,7 @@ int vdagent_virtio_port_write_append(struct vdagent_virtio_port *vport,
>  
>      if (wbuf->write_pos == wbuf->size) {
>          vdagent_connection_write(vport->conn, wbuf->buf, wbuf->size);
> -        g_clear_pointer(&vport->write_buf, free);
> +        g_clear_pointer(&vport->write_buf, g_free);
>      }
>      return 0;
>  }
> @@ -282,7 +275,7 @@ void vdagent_virtio_port_reset(struct vdagent_virtio_port *vport, int port)
>          syslog(LOG_ERR, "vdagent_virtio_port_reset port out of range");
>          return;
>      }
> -    free(vport->port_data[port].message_data);
> +    g_free(vport->port_data[port].message_data);
>      memset(&vport->port_data[port], 0, sizeof(vport->port_data[0]));
>  }
>  
> @@ -311,12 +304,7 @@ static void vdagent_virtio_port_do_chunk(struct vdagent_virtio_port **vportp,
>              port->message_header.size = GUINT32_FROM_LE(port->message_header.size);
>  
>              if (port->message_header.size) {
> -                port->message_data = malloc(port->message_header.size);
> -                if (!port->message_data) {
> -                    syslog(LOG_ERR, "out of memory, disconnecting virtio");
> -                    virtio_port_destroy(vportp, FALSE);
> -                    return;
> -                }
> +                port->message_data = g_malloc(port->message_header.size);
>              }
>          }
>          pos = read;
> @@ -352,8 +340,7 @@ static void vdagent_virtio_port_do_chunk(struct vdagent_virtio_port **vportp,
>              }
>              port->message_header_read = 0;
>              port->message_data_pos = 0;
> -            free(port->message_data);
> -            port->message_data = NULL;
> +            g_clear_pointer(&port->message_data, g_free);
>          }
>      }
>  }
> -- 
> 2.17.1
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel