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@xxxxxxxxxx>
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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel