Re: [PATCH vdagent-linux 1/6] move to GLib memory functions

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

On Tue, Sep 04, 2018 at 06:40:46PM +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_clear_pointer where possible
> for simplification.
> 
> Note: this commit only partially touches
> src/vdagent/x11.c as the clipboard-handling
> code here should be removed in the future.

I wouldn't mind to change it all for consistency. But a note in
commit log also works for me.

> Signed-off-by: Jakub Janků <jjanku@xxxxxxxxxx>
> ---
>  src/udscs.c                  | 60 ++++++++++--------------------------
>  src/vdagent/x11-randr.c      | 54 ++++++++++----------------------
>  src/vdagent/x11.c            | 15 +++------
>  src/vdagentd/console-kit.c   | 28 ++++++-----------
>  src/vdagentd/systemd-login.c | 19 +++++-------
>  src/vdagentd/uinput.c        |  9 ++----
>  src/vdagentd/vdagentd.c      | 55 +++++++++------------------------
>  src/vdagentd/virtio-port.c   | 42 ++++++++-----------------
>  8 files changed, 86 insertions(+), 196 deletions(-)
> 
> diff --git a/src/udscs.c b/src/udscs.c
> index a77da99..31643e9 100644
> --- a/src/udscs.c
> +++ b/src/udscs.c
> @@ -87,10 +87,7 @@ struct udscs_connection *udscs_connect(const char *socketname,
>      struct sockaddr_un address;
>      struct udscs_connection *conn;
>  
> -    conn = calloc(1, sizeof(*conn));
> -    if (!conn)
> -        return NULL;
> -
> +    conn = g_new0(struct udscs_connection, 1);
>      conn->type_to_string = type_to_string;
>      conn->no_types = no_types;
>      conn->debug = debug;
> @@ -98,7 +95,7 @@ struct udscs_connection *udscs_connect(const char *socketname,
>      conn->fd = socket(PF_UNIX, SOCK_STREAM, 0);
>      if (conn->fd == -1) {
>          syslog(LOG_ERR, "creating unix domain socket: %m");
> -        free(conn);
> +        g_free(conn);
>          return NULL;
>      }
>  
> @@ -109,7 +106,7 @@ struct udscs_connection *udscs_connect(const char *socketname,
>          if (conn->debug) {
>              syslog(LOG_DEBUG, "connect %s: %m", socketname);
>          }
> -        free(conn);
> +        g_free(conn);
>          return NULL;
>      }
>  
> @@ -147,13 +144,12 @@ void udscs_destroy_connection(struct udscs_connection **connp)
>      wbuf = conn->write_buf;
>      while (wbuf) {
>          next_wbuf = wbuf->next;
> -        free(wbuf->buf);
> -        free(wbuf);
> +        g_free(wbuf->buf);
> +        g_free(wbuf);
>          wbuf = next_wbuf;
>      }
>  
> -    free(conn->data.buf);
> -    conn->data.buf = NULL;
> +    g_clear_pointer(&conn->data.buf, g_free);
>  
>      if (conn->next)
>          conn->next->prev = conn->prev;
> @@ -171,8 +167,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)
> @@ -194,18 +189,11 @@ int udscs_write(struct udscs_connection *conn, uint32_t type, uint32_t arg1,
>      struct udscs_buf *wbuf, *new_wbuf;
>      struct udscs_message_header header;
>  
> -    new_wbuf = malloc(sizeof(*new_wbuf));
> -    if (!new_wbuf)
> -        return -1;
> -
> +    new_wbuf = g_new(struct udscs_buf, 1);
>      new_wbuf->pos = 0;
>      new_wbuf->size = sizeof(header) + size;
>      new_wbuf->next = NULL;
> -    new_wbuf->buf = malloc(new_wbuf->size);
> -    if (!new_wbuf->buf) {
> -        free(new_wbuf);
> -        return -1;
> -    }
> +    new_wbuf->buf = g_malloc(new_wbuf->size);
>  
>      header.type = type;
>      header.arg1 = arg1;
> @@ -271,7 +259,7 @@ static void udscs_read_complete(struct udscs_connection **connp)
>              return;
>      }
>  
> -    free(conn->data.buf);
> +    g_free(conn->data.buf);
>      memset(&conn->data, 0, sizeof(conn->data)); /* data.buf = NULL */
>      conn->header_read = 0;
>  }
> @@ -312,12 +300,7 @@ static void udscs_do_read(struct udscs_connection **connp)
>              }
>              conn->data.pos = 0;
>              conn->data.size = conn->header.size;
> -            conn->data.buf = malloc(conn->data.size);
> -            if (!conn->data.buf) {
> -                syslog(LOG_ERR, "out of memory, disconnecting %p", conn);
> -                udscs_destroy_connection(connp);
> -                return;
> -            }
> +            conn->data.buf = g_malloc(conn->data.size);
>          }
>      } else {
>          conn->data.pos += n;
> @@ -354,8 +337,8 @@ static void udscs_do_write(struct udscs_connection **connp)
>      wbuf->pos += n;
>      if (wbuf->pos == wbuf->size) {
>          conn->write_buf = wbuf->next;
> -        free(wbuf->buf);
> -        free(wbuf);
> +        g_free(wbuf->buf);
> +        g_free(wbuf);
>      }
>  }
>  
> @@ -414,10 +397,7 @@ struct udscs_server *udscs_create_server_for_fd(int fd,
>          return NULL;
>      }
>  
> -    server = calloc(1, sizeof(*server));
> -    if (!server)
> -        return NULL;
> -
> +    server = g_new0(struct udscs_server, 1);
>      server->type_to_string = type_to_string;
>      server->no_types = no_types;
>      server->debug = debug;
> @@ -487,7 +467,7 @@ void udscs_destroy_server(struct udscs_server *server)
>          conn = next_conn;
>      }
>      close(server->fd);
> -    free(server);
> +    g_free(server);
>  }
>  
>  int udscs_get_peer_pid(struct udscs_connection *conn)
> @@ -509,13 +489,7 @@ static void udscs_server_accept(struct udscs_server *server) {
>          return;
>      }
>  
> -    new_conn = calloc(1, sizeof(*conn));
> -    if (!new_conn) {
> -        syslog(LOG_ERR, "out of memory, disconnecting new client");
> -        close(fd);
> -        return;
> -    }
> -
> +    new_conn = g_new0(struct udscs_connection, 1);
>      new_conn->fd = fd;
>      new_conn->type_to_string = server->type_to_string;
>      new_conn->no_types = server->no_types;
> @@ -528,7 +502,7 @@ static void udscs_server_accept(struct udscs_server *server) {
>      if (r != 0) {
>          syslog(LOG_ERR, "Could not get peercred, disconnecting new client");
>          close(fd);
> -        free(new_conn);
> +        g_free(new_conn);
>          return;
>      }
>  
> diff --git a/src/vdagent/x11-randr.c b/src/vdagent/x11-randr.c
> index bfa1a02..fa52fba 100644
> --- a/src/vdagent/x11-randr.c
> +++ b/src/vdagent/x11-randr.c
> @@ -81,18 +81,15 @@ 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_clear_pointer(&x11->randr.outputs, g_free);
>      }
>      if (x11->randr.crtcs != NULL) {
>          for (i = 0 ; i < x11->randr.res->ncrtc; ++i) {
>              XRRFreeCrtcInfo(x11->randr.crtcs[i]);
>          }
> -        free(x11->randr.crtcs);
> +        g_clear_pointer(&x11->randr.crtcs, g_free);
>      }
> -    XRRFreeScreenResources(x11->randr.res);
> -    x11->randr.res = NULL;
> -    x11->randr.outputs = NULL;
> -    x11->randr.crtcs = NULL;
> +    g_clear_pointer(&x11->randr.res, XRRFreeScreenResources);
>      x11->randr.num_monitors = 0;
>  }
>  
> @@ -105,8 +102,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));

g_new() ?

>      for (i = 0 ; i < x11->randr.res->noutput; ++i) {
>          x11->randr.outputs[i] = XRRGetOutputInfo(x11->display, x11->randr.res,
>                                                   x11->randr.res->outputs[i]);
> @@ -659,11 +656,7 @@ static VDAgentMonitorsConfig *get_current_mon_config(struct vdagent_x11 *x11)
>      XRRScreenResources *res = x11->randr.res;
>      VDAgentMonitorsConfig *mon_config;
>  
> -    mon_config = calloc(1, config_size(res->noutput));
> -    if (!mon_config) {
> -        syslog(LOG_ERR, "out of memory allocating current monitor config");
> -        return NULL;
> -    }
> +    mon_config = g_malloc0(config_size(res->noutput));
>  
>      for (i = 0 ; i < res->noutput; i++) {
>          int j;
> @@ -696,7 +689,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;
>  }
>  
> @@ -843,12 +836,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));

This can be replaced by g_memdup()

> @@ -900,7 +893,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)
> @@ -919,11 +912,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;
> @@ -931,7 +920,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) {
> @@ -940,17 +929,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;
> @@ -964,9 +949,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];
> @@ -989,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/vdagent/x11.c b/src/vdagent/x11.c
> index 0ea5668..c3c7a65 100644
> --- a/src/vdagent/x11.c
> +++ b/src/vdagent/x11.c
> @@ -210,19 +210,14 @@ struct vdagent_x11 *vdagent_x11_create(struct udscs_connection *vdagentd,
>  #endif
>      gchar *net_wm_name = NULL;
>  
> -    x11 = calloc(1, sizeof(*x11));
> -    if (!x11) {
> -        syslog(LOG_ERR, "out of memory allocating vdagent_x11 struct");
> -        return NULL;
> -    }
> -
> +    x11 = g_new0(struct vdagent_x11, 1);
>      x11->vdagentd = vdagentd;
>      x11->debug = debug;
>  
>      x11->display = XOpenDisplay(NULL);
>      if (!x11->display) {
>          syslog(LOG_ERR, "could not connect to X-server");
> -        free(x11);
> +        g_free(x11);
>          return NULL;
>      }
>  
> @@ -231,7 +226,7 @@ struct vdagent_x11 *vdagent_x11_create(struct udscs_connection *vdagentd,
>          syslog(LOG_ERR, "Error too much screens: %d > %d",
>                 x11->screen_count, MAX_SCREENS);
>          XCloseDisplay(x11->display);
> -        free(x11);
> +        g_free(x11);
>          return NULL;
>      }
>  
> @@ -345,8 +340,8 @@ void vdagent_x11_destroy(struct vdagent_x11 *x11, int vdagentd_disconnected)
>  #endif
>  
>      XCloseDisplay(x11->display);
> -    free(x11->randr.failed_conf);
> -    free(x11);
> +    g_free(x11->randr.failed_conf);
> +    g_free(x11);
>  }
>  
>  int vdagent_x11_get_fd(struct vdagent_x11 *x11)
> diff --git a/src/vdagentd/console-kit.c b/src/vdagentd/console-kit.c
> index 381b97e..c9da0b9 100644
> --- a/src/vdagentd/console-kit.c
> +++ b/src/vdagentd/console-kit.c
> @@ -71,8 +71,7 @@ static void si_dbus_match_remove(struct session_info *info)
>          if (info->verbose)
>              syslog(LOG_DEBUG, "(console-kit) seat match removed: %s",
>                     info->match_seat_signals);
> -        g_free(info->match_seat_signals);
> -        info->match_seat_signals = NULL;
> +        g_clear_pointer(&info->match_seat_signals, g_free);
>      }
>  
>      if (info->match_session_signals != NULL) {
> @@ -84,8 +83,7 @@ static void si_dbus_match_remove(struct session_info *info)
>          if (info->verbose)
>              syslog(LOG_DEBUG, "(console-kit) session match removed: %s",
>                     info->match_session_signals);
> -        g_free(info->match_session_signals);
> -        info->match_session_signals = NULL;
> +        g_clear_pointer(&info->match_session_signals, g_free);
>      }
>  }
>  
> @@ -117,8 +115,7 @@ static void si_dbus_match_rule_update(struct session_info *info)
>              syslog(LOG_WARNING, "Unable to add dbus rule match: %s",
>                     error.message);
>              dbus_error_free(&error);
> -            g_free(info->match_seat_signals);
> -            info->match_seat_signals = NULL;
> +            g_clear_pointer(&info->match_seat_signals, g_free);
>          }
>      }
>  
> @@ -140,8 +137,7 @@ static void si_dbus_match_rule_update(struct session_info *info)
>              syslog(LOG_WARNING, "Unable to add dbus rule match: %s",
>                     error.message);
>              dbus_error_free(&error);
> -            g_free(info->match_session_signals);
> -            info->match_session_signals = NULL;
> +            g_clear_pointer(&info->match_session_signals, g_free);
>          }
>      }
>  }
> @@ -162,8 +158,7 @@ si_dbus_read_signals(struct session_info *info)
>              gint type;
>              gchar *session;
>  
> -            free(info->active_session);
> -            info->active_session = NULL;
> +            g_clear_pointer(&info->active_session, g_free);
>  
>              dbus_message_iter_init(message, &iter);
>              type = dbus_message_iter_get_arg_type(&iter);
> @@ -222,10 +217,7 @@ struct session_info *session_info_create(int verbose)
>      struct session_info *info;
>      DBusError error;
>  
> -    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;
> @@ -239,7 +231,7 @@ struct session_info *session_info_create(int verbose)
>               dbus_error_free(&error);
>          } else
>               syslog(LOG_ERR, "Unable to connect to system bus");
> -        free(info);
> +        g_free(info);
>          return NULL;
>      }
>  
> @@ -265,9 +257,9 @@ void session_info_destroy(struct session_info *info)
>  
>      si_dbus_match_remove(info);
>      dbus_connection_close(info->connection);
> -    free(info->seat);
> -    free(info->active_session);
> -    free(info);
> +    g_free(info->seat);
> +    g_free(info->active_session);
> +    g_free(info);
>  }
>  
>  int session_info_get_fd(struct session_info *info)
> diff --git a/src/vdagentd/systemd-login.c b/src/vdagentd/systemd-login.c
> index 9719c0b..a20cd58 100644
> --- a/src/vdagentd/systemd-login.c
> +++ b/src/vdagentd/systemd-login.c
> @@ -83,8 +83,7 @@ static void si_dbus_match_remove(struct session_info *si)
>                            si->dbus.match_session_signals,
>                            &error);
>  
> -    g_free(si->dbus.match_session_signals);
> -    si->dbus.match_session_signals = NULL;
> +    g_clear_pointer(&si->dbus.match_session_signals, g_free);
>  }
>  
>  static void si_dbus_match_rule_update(struct session_info *si)
> @@ -113,8 +112,7 @@ static void si_dbus_match_rule_update(struct session_info *si)
>          syslog(LOG_WARNING, "Unable to add dbus rule match: %s",
>                 error.message);
>          dbus_error_free(&error);
> -        g_free(si->dbus.match_session_signals);
> -        si->dbus.match_session_signals = NULL;
> +        g_clear_pointer(&si->dbus.match_session_signals, g_free);
>      }
>  }
>  
> @@ -230,17 +228,14 @@ struct session_info *session_info_create(int verbose)
>      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;
>  
>      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;
>      }
>  
> @@ -256,8 +251,8 @@ void session_info_destroy(struct session_info *si)
>      si_dbus_match_remove(si);
>      dbus_connection_close(si->dbus.system_connection);
>      sd_login_monitor_unref(si->mon);
> -    free(si->session);
> -    free(si);
> +    g_free(si->session);
> +    g_free(si);
>  }
>  
>  int session_info_get_fd(struct session_info *si)
> @@ -282,7 +277,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_match_rule_update(si);
>      return si->session;
> diff --git a/src/vdagentd/uinput.c b/src/vdagentd/uinput.c
> index 6d04322..3172891 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 adfaf39..1ac30ee 100644
> --- a/src/vdagentd/vdagentd.c
> +++ b/src/vdagentd/vdagentd.c
> @@ -124,12 +124,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);
> @@ -145,7 +140,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)
> @@ -198,12 +193,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);

I don't see much point for the if() above. That's just to save
alloc operation but this is not hot path.

Should be fine to replace the if() and memcpy() with

    g_free(mon_config);
    mon_config = g_memdup(new_monitors, size);

Feel free to do the suggestion above in a separate patch or in
this one as it kinda relate to moving to g_memdup()

> @@ -240,13 +231,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));

Ditto :)

>      if (caps->request) {
> @@ -328,7 +314,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)
> @@ -342,7 +328,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,
> @@ -849,13 +835,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);
> @@ -874,12 +854,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,
> @@ -908,12 +887,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));

g_memdup() too


Looks fine otherwise,
Reviewed-by: Victor Toso <victortoso@xxxxxxxxxx>

>          agent_data->width  = header->arg1;
>          agent_data->height = header->arg2;
> @@ -1177,8 +1152,6 @@ int main(int argc, char *argv[])
>          if (errno == EADDRINUSE) {
>              syslog(LOG_CRIT, "Fatal the server socket %s exists already. Delete it?",
>                     vdagentd_socket);
> -        } else if (errno == ENOMEM) {
> -            syslog(LOG_CRIT, "Fatal could not allocate memory for udscs server");
>          } else {
>              syslog(LOG_CRIT, "Fatal could not create the server socket %s: %m",
>                     vdagentd_socket);
> diff --git a/src/vdagentd/virtio-port.c b/src/vdagentd/virtio-port.c
> index 3dc6f44..1fac1db 100644
> --- a/src/vdagentd/virtio-port.c
> +++ b/src/vdagentd/virtio-port.c
> @@ -85,9 +85,7 @@ struct vdagent_virtio_port *vdagent_virtio_port_create(const char *portname,
>      struct sockaddr_un address;
>      int c;
>  
> -    vport = calloc(1, sizeof(*vport));
> -    if (!vport)
> -        return 0;
> +    vport = g_new0(struct vdagent_virtio_port, 1);
>  
>      vport->fd = open(portname, O_RDWR);
>      if (vport->fd == -1) {
> @@ -118,7 +116,7 @@ error:
>      if (vport->fd != -1) {
>          close(vport->fd);
>      }
> -    free(vport);
> +    g_free(vport);
>      return NULL;
>  }
>  
> @@ -137,18 +135,17 @@ void vdagent_virtio_port_destroy(struct vdagent_virtio_port **vportp)
>      wbuf = vport->write_buf;
>      while (wbuf) {
>          next_wbuf = wbuf->next;
> -        free(wbuf->buf);
> -        free(wbuf);
> +        g_free(wbuf->buf);
> +        g_free(wbuf);
>          wbuf = next_wbuf;
>      }
>  
>      for (i = 0; i < VDP_END_PORT; i++) {
> -        free(vport->port_data[i].message_data);
> +        g_free(vport->port_data[i].message_data);
>      }
>  
>      close(vport->fd);
> -    free(vport);
> -    *vportp = NULL;
> +    g_clear_pointer(vportp, g_free);
>  }
>  
>  int vdagent_virtio_port_fill_fds(struct vdagent_virtio_port *vport,
> @@ -203,19 +200,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->pos = 0;
>      new_wbuf->write_pos = 0;
>      new_wbuf->size = sizeof(chunk_header) + sizeof(message_header) + data_size;
>      new_wbuf->next = NULL;
> -    new_wbuf->buf = malloc(new_wbuf->size);
> -    if (!new_wbuf->buf) {
> -        free(new_wbuf);
> -        return -1;
> -    }
> +    new_wbuf->buf = g_malloc(new_wbuf->size);
>  
>      chunk_header.port = GUINT32_TO_LE(port_nr);
>      chunk_header.size = GUINT32_TO_LE(sizeof(message_header) + data_size);
> @@ -291,7 +281,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]));
>  }
>  
> @@ -318,12 +308,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");
> -                    vdagent_virtio_port_destroy(vportp);
> -                    return;
> -                }
> +                port->message_data = g_malloc(port->message_header.size);
>              }
>          }
>          pos = read;
> @@ -359,8 +344,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);
>          }
>      }
>  }
> @@ -496,7 +480,7 @@ static void vdagent_virtio_port_do_write(struct vdagent_virtio_port **vportp)
>      wbuf->pos += n;
>      if (wbuf->pos == wbuf->size) {
>          vport->write_buf = wbuf->next;
> -        free(wbuf->buf);
> -        free(wbuf);
> +        g_free(wbuf->buf);
> +        g_free(wbuf);
>      }
>  }
> -- 
> 2.17.1
> 

Attachment: signature.asc
Description: PGP signature

_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Index of Archives]     [Linux Virtualization]     [Linux Virtualization]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]