[libvirt] PATCH: Switch remote daemon to use memory alloc APIs

THis patch switches over the remote daemon to use the memory allocation APIs. This required exporting the 4 apis in the .so. I discovered along the way that none of the remote RPC dispatcher impls checked for malloc failure, so I've had to add that in too. qemud/event.c | 49 +++-------- qemud/mdns.c | 53 ++++++------ qemud/qemud.c | 43 +++++----- qemud/remote.c | 200 ++++++++++++++++++++++++++++++------------------ src/libvirt_sym.version | 5 + src/memory.c | 8 - src/memory.h | 16 +-- 7 files changed, 207 insertions(+), 167 deletions(-) Regards, Daniel diff -r 06acc2c5c1fb qemud/event.c --- a/qemud/event.c Thu May 29 16:05:55 2008 -0400 +++ b/qemud/event.c Fri May 30 10:36:42 2008 -0400 @@ -31,6 +31,7 @@ #include "qemud.h" #include "event.h" +#include "memory.h" #define EVENT_DEBUG(fmt, ...) qemudDebug("EVENT: " fmt, __VA_ARGS__) @@ -82,16 +83,11 @@ int virEventAddHandleImpl(int fd, int events, virEventHandleCallback cb, void *opaque) { EVENT_DEBUG("Add handle %d %d %p %p", fd, events, cb, opaque); if (eventLoop.handlesCount == eventLoop.handlesAlloc) { - struct virEventHandle *tmp; EVENT_DEBUG("Used %d handle slots, adding %d more", eventLoop.handlesAlloc, EVENT_ALLOC_EXTENT); - tmp = realloc(eventLoop.handles, - sizeof(struct virEventHandle) * - (eventLoop.handlesAlloc + EVENT_ALLOC_EXTENT)); - if (!tmp) { + if (VIR_REALLOC_N(eventLoop.handles, + (eventLoop.handlesAlloc + EVENT_ALLOC_EXTENT)) < 0) return -1; - } - eventLoop.handles = tmp; eventLoop.handlesAlloc += EVENT_ALLOC_EXTENT; } @@ -152,16 +148,11 @@ } if (eventLoop.timeoutsCount == eventLoop.timeoutsAlloc) { - struct virEventTimeout *tmp; EVENT_DEBUG("Used %d timeout slots, adding %d more", eventLoop.timeoutsAlloc, EVENT_ALLOC_EXTENT); - tmp = realloc(eventLoop.timeouts, - sizeof(struct virEventTimeout) * - (eventLoop.timeoutsAlloc + EVENT_ALLOC_EXTENT)); - if (!tmp) { + if (VIR_REALLOC_N(eventLoop.timeouts, + (eventLoop.timeoutsAlloc + EVENT_ALLOC_EXTENT)) < 0) return -1; - } - eventLoop.timeouts = tmp; eventLoop.timeoutsAlloc += EVENT_ALLOC_EXTENT; } @@ -281,7 +272,7 @@ } *retfds = NULL; /* Setup the poll file handle data structs */ - if (!(fds = malloc(sizeof(*fds) * nfds))) + if (VIR_ALLOC_N(fds, nfds) < 0) return -1; for (i = 0, nfds = 0 ; i < eventLoop.handlesCount ; i++) { @@ -398,16 +389,11 @@ /* Release some memory if we've got a big chunk free */ if ((eventLoop.timeoutsAlloc - EVENT_ALLOC_EXTENT) > eventLoop.timeoutsCount) { - struct virEventTimeout *tmp; EVENT_DEBUG("Releasing %d out of %d timeout slots used, releasing %d", eventLoop.timeoutsCount, eventLoop.timeoutsAlloc, EVENT_ALLOC_EXTENT); - tmp = realloc(eventLoop.timeouts, - sizeof(struct virEventTimeout) * - (eventLoop.timeoutsAlloc - EVENT_ALLOC_EXTENT)); - if (!tmp) { + if (VIR_REALLOC_N(eventLoop.timeouts, + (eventLoop.timeoutsAlloc - EVENT_ALLOC_EXTENT)) < 0) return -1; - } - eventLoop.timeouts = tmp; eventLoop.timeoutsAlloc -= EVENT_ALLOC_EXTENT; } return 0; @@ -439,16 +425,11 @@ /* Release some memory if we've got a big chunk free */ if ((eventLoop.handlesAlloc - EVENT_ALLOC_EXTENT) > eventLoop.handlesCount) { - struct virEventHandle *tmp; EVENT_DEBUG("Releasing %d out of %d handles slots used, releasing %d", eventLoop.handlesCount, eventLoop.handlesAlloc, EVENT_ALLOC_EXTENT); - tmp = realloc(eventLoop.handles, - sizeof(struct virEventHandle) * - (eventLoop.handlesAlloc - EVENT_ALLOC_EXTENT)); - if (!tmp) { + if (VIR_REALLOC_N(eventLoop.handles, + (eventLoop.handlesAlloc - EVENT_ALLOC_EXTENT)) < 0) return -1; - } - eventLoop.handles = tmp; eventLoop.handlesAlloc -= EVENT_ALLOC_EXTENT; } return 0; @@ -466,7 +447,7 @@ return -1; if (virEventCalculateTimeout(&timeout) < 0) { - free(fds); + VIR_FREE(fds); return -1; } @@ -478,20 +459,20 @@ if (errno == EINTR) { goto retry; } - free(fds); + VIR_FREE(fds); return -1; } if (virEventDispatchTimeouts() < 0) { - free(fds); + VIR_FREE(fds); return -1; } if (ret > 0 && virEventDispatchHandles(fds) < 0) { - free(fds); + VIR_FREE(fds); return -1; } - free(fds); + VIR_FREE(fds); if (virEventCleanupTimeouts() < 0) return -1; diff -r 06acc2c5c1fb qemud/mdns.c --- a/qemud/mdns.c Thu May 29 16:05:55 2008 -0400 +++ b/qemud/mdns.c Fri May 30 10:36:42 2008 -0400 @@ -42,6 +42,7 @@ #include "mdns.h" #include "event.h" #include "remote_internal.h" +#include "memory.h" #define AVAHI_DEBUG(fmt, ...) qemudDebug("AVAHI: " fmt, __VA_ARGS__) @@ -99,7 +100,7 @@ /* A service name collision happened. Let's pick a new name */ n = avahi_alternative_service_name(group->name); - free(group->name); + VIR_FREE(group->name); group->name = n; AVAHI_DEBUG("Group name collision, renaming service to '%s'", group->name); @@ -237,8 +238,8 @@ static AvahiWatch *libvirtd_mdns_watch_new(const AvahiPoll *api ATTRIBUTE_UNUSED, int fd, AvahiWatchEvent event, AvahiWatchCallback cb, void *userdata) { - AvahiWatch *w = malloc(sizeof(*w)); - if (!w) + AvahiWatch *w; + if (VIR_ALLOC(w) < 0) return NULL; w->fd = fd; @@ -248,7 +249,7 @@ AVAHI_DEBUG("New handle %p FD %d Event %d", w, w->fd, event); if (virEventAddHandleImpl(fd, event, libvirtd_mdns_watch_dispatch, w) < 0) { - free(w); + VIR_FREE(w); return NULL; } @@ -271,7 +272,7 @@ { AVAHI_DEBUG("Free handle %p %d", w, w->fd); virEventRemoveHandleImpl(w->fd); - free(w); + VIR_FREE(w); } static void libvirtd_mdns_timeout_dispatch(int timer ATTRIBUTE_UNUSED, void *opaque) @@ -287,15 +288,15 @@ AvahiTimeoutCallback cb, void *userdata) { - AvahiTimeout *t = malloc(sizeof(*t)); + AvahiTimeout *t; struct timeval now; long long nowms, thenms, timeout; AVAHI_DEBUG("Add timeout %p TV %p", t, tv); - if (!t) + if (VIR_ALLOC(t) < 0) return NULL; if (gettimeofday(&now, NULL) < 0) { - free(t); + VIR_FREE(t); return NULL; } @@ -317,7 +318,7 @@ t->userdata = userdata; if (t->timer < 0) { - free(t); + VIR_FREE(t); return NULL; } @@ -330,7 +331,7 @@ long long nowms, thenms, timeout; AVAHI_DEBUG("Update timeout %p TV %p", t, tv); if (gettimeofday(&now, NULL) < 0) { - free(t); + VIR_FREE(t); return; } @@ -351,14 +352,14 @@ { AVAHI_DEBUG("Free timeout %p", t); virEventRemoveTimeoutImpl(t->timer); - free(t); + VIR_FREE(t); } static AvahiPoll *libvirtd_create_poll(void) { - AvahiPoll *p = malloc(sizeof(*p)); - if (!p) + AvahiPoll *p; + if (VIR_ALLOC(p) < 0) return NULL; p->userdata = NULL; @@ -377,14 +378,13 @@ struct libvirtd_mdns *libvirtd_mdns_new(void) { - struct libvirtd_mdns *mdns = malloc(sizeof(*mdns)); - if (!mdns) + struct libvirtd_mdns *mdns; + if (VIR_ALLOC(mdns) < 0) return NULL; - memset(mdns, 0, sizeof(*mdns)); /* Allocate main loop object */ if (!(mdns->poller = libvirtd_create_poll())) { - free(mdns); + VIR_FREE(mdns); return NULL; } @@ -406,15 +406,14 @@ } struct libvirtd_mdns_group *libvirtd_mdns_add_group(struct libvirtd_mdns *mdns, const char *name) { - struct libvirtd_mdns_group *group = malloc(sizeof(*group)); + struct libvirtd_mdns_group *group; AVAHI_DEBUG("Adding group '%s'", name); - if (!group) + if (VIR_ALLOC(group) < 0) return NULL; - memset(group, 0, sizeof(*group)); if (!(group->name = strdup(name))) { - free(group); + VIR_FREE(group); return NULL; } group->mdns = mdns; @@ -428,12 +427,12 @@ while (tmp) { if (tmp == group) { - free(group->name); + VIR_FREE(group->name); if (prev) prev->next = group->next; else group->mdns->group = group->next; - free(group); + VIR_FREE(group); return; } prev = tmp; @@ -442,15 +441,15 @@ } struct libvirtd_mdns_entry *libvirtd_mdns_add_entry(struct libvirtd_mdns_group *group, const char *type, int port) { - struct libvirtd_mdns_entry *entry = malloc(sizeof(*entry)); + struct libvirtd_mdns_entry *entry; AVAHI_DEBUG("Adding entry %s %d to group %s", type, port, group->name); - if (!entry) + if (VIR_ALLOC(entry) < 0) return NULL; entry->port = port; if (!(entry->type = strdup(type))) { - free(entry); + VIR_FREE(entry); return NULL; } entry->next = group->entry; @@ -463,7 +462,7 @@ while (tmp) { if (tmp == entry) { - free(entry->type); + VIR_FREE(entry->type); if (prev) prev->next = entry->next; else diff -r 06acc2c5c1fb qemud/qemud.c --- a/qemud/qemud.c Thu May 29 16:05:55 2008 -0400 +++ b/qemud/qemud.c Fri May 30 10:36:42 2008 -0400 @@ -56,6 +56,7 @@ #include "remote_internal.h" #include "conf.h" #include "event.h" +#include "memory.h" #ifdef HAVE_AVAHI #include "mdns.h" #endif @@ -466,12 +467,12 @@ static int qemudListenUnix(struct qemud_server *server, const char *path, int readonly, int auth) { - struct qemud_socket *sock = calloc(1, sizeof(*sock)); + struct qemud_socket *sock; struct sockaddr_un addr; mode_t oldmask; gid_t oldgrp; - if (!sock) { + if (VIR_ALLOC(sock) < 0) { qemudLog(QEMUD_ERR, "%s", _("Failed to allocate memory for struct qemud_socket")); return -1; @@ -611,12 +612,10 @@ struct sockaddr_storage sa; socklen_t salen = sizeof(sa); - sock = calloc (1, sizeof *sock); - - if (!sock) { + if (VIR_ALLOC(sock) < 0) { qemudLog (QEMUD_ERR, _("remoteListenTCP: calloc: %s"), strerror (errno)); - return -1; + goto cleanup; } sock->readonly = 0; @@ -629,7 +628,7 @@ sock->auth = auth; if (getsockname(sock->fd, (struct sockaddr *)(&sa), &salen) < 0) - return -1; + goto cleanup; if (sa.ss_family == AF_INET) sock->port = htons(((struct sockaddr_in*)&sa)->sin_port); @@ -642,12 +641,12 @@ if (qemudSetCloseExec(sock->fd) < 0 || qemudSetNonBlock(sock->fd) < 0) - return -1; + goto cleanup; if (listen (sock->fd, 30) < 0) { qemudLog (QEMUD_ERR, _("remoteListenTCP: listen: %s"), strerror (errno)); - return -1; + goto cleanup; } if (virEventAddHandleImpl(sock->fd, @@ -655,12 +654,17 @@ qemudDispatchServerEvent, server) < 0) { qemudLog(QEMUD_ERR, "%s", _("Failed to add server event callback")); - return -1; + goto cleanup; } } return 0; + +cleanup: + for (i = 0; i < nfds; ++i) + close(fds[0]); + return -1; } static int qemudInitPaths(struct qemud_server *server, @@ -712,7 +716,7 @@ static struct qemud_server *qemudInitialize(int sigread) { struct qemud_server *server; - if (!(server = calloc(1, sizeof(*server)))) { + if (VIR_ALLOC(server) < 0) { qemudLog(QEMUD_ERR, "%s", _("Failed to allocate struct qemud_server")); return NULL; } @@ -1092,8 +1096,7 @@ return -1; } - client = calloc(1, sizeof(*client)); - if (client == NULL) + if (VIR_ALLOC(client) < 0) goto cleanup; client->magic = QEMUD_CLIENT_MAGIC; client->fd = fd; @@ -1733,8 +1736,7 @@ switch (p->type) { case VIR_CONF_STRING: - list = malloc (2 * sizeof (*list)); - if (list == NULL) { + if (VIR_ALLOC_N(list, 2) < 0) { qemudLog (QEMUD_ERR, _("failed to allocate memory for %s config list"), key); return -1; @@ -1745,7 +1747,7 @@ qemudLog (QEMUD_ERR, _("failed to allocate memory for %s config list value"), key); - free (list); + VIR_FREE(list); return -1; } break; @@ -1755,8 +1757,7 @@ virConfValuePtr pp; for (pp = p->list; pp; pp = pp->next) len++; - list = calloc (1+len, sizeof (*list)); - if (list == NULL) { + if (VIR_ALLOC_N(list, 1+len) < 0) { qemudLog (QEMUD_ERR, _("failed to allocate memory for %s config list"), key); return -1; @@ -1766,15 +1767,15 @@ qemudLog (QEMUD_ERR, _("remoteReadConfigFile: %s: %s:" " must be a string or list of strings\n"), filename, key); - free (list); + VIR_FREE(list); return -1; } list[i] = strdup (pp->str); if (list[i] == NULL) { int j; for (j = 0 ; j < i ; j++) - free (list[j]); - free (list); + VIR_FREE(list[j]); + VIR_FREE(list); qemudLog (QEMUD_ERR, _("failed to allocate memory" " for %s config list value"), key); return -1; diff -r 06acc2c5c1fb qemud/remote.c --- a/qemud/remote.c Thu May 29 16:05:55 2008 -0400 +++ b/qemud/remote.c Fri May 30 10:36:42 2008 -0400 @@ -50,6 +50,7 @@ #include "internal.h" #include "qemud.h" +#include "memory.h" #define DEBUG 0 @@ -609,14 +610,19 @@ } /* Allocate return buffer. */ - ret->freeMems.freeMems_val = calloc (args->maxCells, sizeof (*(ret->freeMems.freeMems_val))); + if (VIR_ALLOC_N(ret->freeMems.freeMems_val, args->maxCells) < 0) { + remoteDispatchSendError(client, req, VIR_ERR_NO_MEMORY, NULL); + return -2; + } ret->freeMems.freeMems_len = virNodeGetCellsFreeMemory(client->conn, (unsigned long long *)ret->freeMems.freeMems_val, args->startCell, args->maxCells); - if (ret->freeMems.freeMems_len == 0) + if (ret->freeMems.freeMems_len == 0) { + VIR_FREE(ret->freeMems.freeMems_val); return -1; + } return 0; } @@ -688,15 +694,14 @@ remoteDispatchError (client, req, "%s", _("nparams too large")); return -2; } - params = malloc (sizeof (*params) * nparams); - if (params == NULL) { - remoteDispatchError (client, req, "%s", _("out of memory allocating array")); + if (VIR_ALLOC_N(params, nparams) < 0) { + remoteDispatchSendError(client, req, VIR_ERR_NO_MEMORY, NULL); return -2; } dom = get_nonnull_domain (client->conn, args->dom); if (dom == NULL) { - free (params); + VIR_FREE(params); remoteDispatchError (client, req, "%s", _("domain not found")); return -2; } @@ -704,32 +709,21 @@ r = virDomainGetSchedulerParameters (dom, params, &nparams); if (r == -1) { virDomainFree(dom); - free (params); + VIR_FREE(params); return -1; } /* Serialise the scheduler parameters. */ ret->params.params_len = nparams; - ret->params.params_val = malloc (sizeof (*(ret->params.params_val)) - * nparams); - if (ret->params.params_val == NULL) { - virDomainFree(dom); - free (params); - remoteDispatchError (client, req, - "%s", _("out of memory allocating return array")); - return -2; - } + if (VIR_ALLOC_N(ret->params.params_val, nparams) < 0) + goto oom; for (i = 0; i < nparams; ++i) { // remoteDispatchClientRequest will free this: ret->params.params_val[i].field = strdup (params[i].field); - if (ret->params.params_val[i].field == NULL) { - virDomainFree(dom); - free (params); - remoteDispatchError (client, req, - "%s", _("out of memory allocating return array")); - return -2; - } + if (ret->params.params_val[i].field == NULL) + goto oom; + ret->params.params_val[i].value.type = params[i].type; switch (params[i].type) { case VIR_DOMAIN_SCHED_FIELD_INT: @@ -745,16 +739,23 @@ case VIR_DOMAIN_SCHED_FIELD_BOOLEAN: ret->params.params_val[i].value.remote_sched_param_value_u.b = params[i].value.b; break; default: - virDomainFree(dom); - free (params); remoteDispatchError (client, req, "%s", _("unknown type")); - return -2; + goto cleanup; } } virDomainFree(dom); - free (params); + VIR_FREE(params); return 0; + +oom: + remoteDispatchSendError(client, req, VIR_ERR_NO_MEMORY, NULL); +cleanup: + virDomainFree(dom); + for (i = 0 ; i < nparams ; i++) + VIR_FREE(ret->params.params_val[i].field); + VIR_FREE(params); + return -2; } static int @@ -775,9 +776,8 @@ remoteDispatchError (client, req, "%s", _("nparams too large")); return -2; } - params = malloc (sizeof (*params) * nparams); - if (params == NULL) { - remoteDispatchError (client, req, "%s", _("out of memory allocating array")); + if (VIR_ALLOC_N(params, nparams)) { + remoteDispatchSendError(client, req, VIR_ERR_NO_MEMORY, NULL); return -2; } @@ -805,14 +805,14 @@ dom = get_nonnull_domain (client->conn, args->dom); if (dom == NULL) { - free (params); + VIR_FREE(params); remoteDispatchError (client, req, "%s", _("domain not found")); return -2; } r = virDomainSetSchedulerParameters (dom, params, nparams); virDomainFree(dom); - free (params); + VIR_FREE(params); if (r == -1) return -1; return 0; @@ -1190,9 +1190,9 @@ remote_domain_get_vcpus_args *args, remote_domain_get_vcpus_ret *ret) { - virDomainPtr dom; - virVcpuInfoPtr info; - unsigned char *cpumaps; + virDomainPtr dom = NULL; + virVcpuInfoPtr info = NULL; + unsigned char *cpumaps = NULL; int info_len, i; CHECK_CONN(client); @@ -1215,20 +1215,25 @@ } /* Allocate buffers to take the results. */ - info = calloc (args->maxinfo, sizeof (*info)); - cpumaps = calloc (args->maxinfo * args->maplen, sizeof (*cpumaps)); + if (VIR_ALLOC_N(info, args->maxinfo) < 0) + goto oom; + if (VIR_ALLOC_N(cpumaps, args->maxinfo) < 0) + goto oom; info_len = virDomainGetVcpus (dom, info, args->maxinfo, cpumaps, args->maplen); if (info_len == -1) { + VIR_FREE(info); + VIR_FREE(cpumaps); virDomainFree(dom); return -1; } /* Allocate the return buffer for info. */ ret->info.info_len = info_len; - ret->info.info_val = calloc (info_len, sizeof (*(ret->info.info_val))); + if (VIR_ALLOC_N(ret->info.info_val, info_len) < 0) + goto oom; for (i = 0; i < info_len; ++i) { ret->info.info_val[i].number = info[i].number; @@ -1244,8 +1249,16 @@ ret->cpumaps.cpumaps_len = args->maxinfo * args->maplen; ret->cpumaps.cpumaps_val = (char *) cpumaps; + VIR_FREE(info); virDomainFree(dom); return 0; + +oom: + VIR_FREE(info); + VIR_FREE(cpumaps); + virDomainFree(dom); + remoteDispatchSendError(client, req, VIR_ERR_NO_MEMORY, NULL); + return -2; } static int @@ -1267,13 +1280,16 @@ dname = args->dname == NULL ? NULL : *args->dname; /* Wacky world of XDR ... */ - uri_out = calloc (1, sizeof (*uri_out)); + if (VIR_ALLOC(uri_out) < 0) { + remoteDispatchSendError(client, req, VIR_ERR_NO_MEMORY, NULL); + return -2; + } r = __virDomainMigratePrepare (client->conn, &cookie, &cookielen, uri_in, uri_out, args->flags, dname, args->resource); if (r == -1) { - free(uri_out); + VIR_FREE(uri_out); return -1; } @@ -1284,7 +1300,7 @@ ret->cookie.cookie_val = cookie; if (*uri_out == NULL) { ret->uri_out = NULL; - free(uri_out); + VIR_FREE(uri_out); } else { ret->uri_out = uri_out; } @@ -1361,12 +1377,18 @@ } /* Allocate return buffer. */ - ret->names.names_val = calloc (args->maxnames, sizeof (*(ret->names.names_val))); + if (VIR_ALLOC_N(ret->names.names_val, args->maxnames) < 0) { + remoteDispatchSendError(client, req, VIR_ERR_NO_MEMORY, NULL); + return -2; + } ret->names.names_len = virConnectListDefinedDomains (client->conn, ret->names.names_val, args->maxnames); - if (ret->names.names_len == -1) return -1; + if (ret->names.names_len == -1) { + VIR_FREE(ret->names.names_val); + return -1; + } return 0; } @@ -1769,12 +1791,18 @@ } /* Allocate return buffer. */ - ret->names.names_val = calloc (args->maxnames, sizeof (*(ret->names.names_val))); + if (VIR_ALLOC_N(ret->names.names_val, args->maxnames) < 0) { + remoteDispatchSendError(client, req, VIR_ERR_NO_MEMORY, NULL); + return -2; + } ret->names.names_len = virConnectListDefinedNetworks (client->conn, ret->names.names_val, args->maxnames); - if (ret->names.names_len == -1) return -1; + if (ret->names.names_len == -1) { + VIR_FREE(ret->names.names_val); + return -1; + } return 0; } @@ -1795,11 +1823,17 @@ } /* Allocate return buffer. */ - ret->ids.ids_val = calloc (args->maxids, sizeof (*(ret->ids.ids_val))); + if (VIR_ALLOC_N(ret->ids.ids_val, args->maxids) < 0) { + remoteDispatchSendError(client, req, VIR_ERR_NO_MEMORY, NULL); + return -2; + } ret->ids.ids_len = virConnectListDomains (client->conn, ret->ids.ids_val, args->maxids); - if (ret->ids.ids_len == -1) return -1; + if (ret->ids.ids_len == -1) { + VIR_FREE(ret->ids.ids_val); + return -1; + } return 0; } @@ -1820,12 +1854,18 @@ } /* Allocate return buffer. */ - ret->names.names_val = calloc (args->maxnames, sizeof (*(ret->names.names_val))); + if (VIR_ALLOC_N(ret->names.names_val, args->maxnames) < 0) { + remoteDispatchSendError(client, req, VIR_ERR_NO_MEMORY, NULL); + return -2; + } ret->names.names_len = virConnectListNetworks (client->conn, ret->names.names_val, args->maxnames); - if (ret->names.names_len == -1) return -1; + if (ret->names.names_len == -1) { + VIR_FREE(ret->names.names_len); + return -1; + } return 0; } @@ -2128,8 +2168,8 @@ remote_auth_list_ret *ret) { ret->types.types_len = 1; - if ((ret->types.types_val = calloc (ret->types.types_len, sizeof (*(ret->types.types_val)))) == NULL) { - remoteDispatchSendError(client, req, VIR_ERR_NO_MEMORY, "auth types"); + if (VIR_ALLOC_N(ret->types.types_val, ret->types.types_len) < 0) { + remoteDispatchSendError(client, req, VIR_ERR_NO_MEMORY, NULL); return -2; } ret->types.types_val[0] = client->auth; @@ -2158,9 +2198,8 @@ return NULL; } - addr = malloc(strlen(host) + 1 + strlen(port) + 1); - if (!addr) { - remoteDispatchError(client, req, "%s", _("cannot allocate address")); + if (VIR_ALLOC_N(addr, strlen(host) + 1 + strlen(port) + 1) < 0) { + remoteDispatchSendError(client, req, VIR_ERR_NO_MEMORY, NULL); return NULL; } @@ -2216,11 +2255,11 @@ if (getpeername(client->fd, (struct sockaddr*)&sa, &salen) < 0) { remoteDispatchError(client, req, _("failed to get peer address %d (%s)"), errno, strerror(errno)); - free(localAddr); + VIR_FREE(localAddr); return -2; } if ((remoteAddr = addrToString(client, req, &sa, salen)) == NULL) { - free(localAddr); + VIR_FREE(localAddr); return -2; } @@ -2232,8 +2271,8 @@ NULL, /* XXX Callbacks */ SASL_SUCCESS_DATA, &client->saslconn); - free(localAddr); - free(remoteAddr); + VIR_FREE(localAddr); + VIR_FREE(remoteAddr); if (err != SASL_OK) { qemudLog(QEMUD_ERR, _("sasl context setup failed %d (%s)"), err, sasl_errstring(err, NULL, NULL)); @@ -2477,10 +2516,8 @@ /* NB, distinction of NULL vs "" is *critical* in SASL */ if (serverout) { - ret->data.data_val = malloc(serveroutlen); - if (!ret->data.data_val) { - remoteDispatchError (client, req, - "%s", _("out of memory allocating array")); + if (VIR_ALLOC_N(ret->data.data_val, serveroutlen) < 0) { + remoteDispatchSendError(client, req, VIR_ERR_NO_MEMORY, NULL); return -2; } memcpy(ret->data.data_val, serverout, serveroutlen); @@ -2558,10 +2595,8 @@ /* NB, distinction of NULL vs "" is *critical* in SASL */ if (serverout) { - ret->data.data_val = malloc(serveroutlen); - if (!ret->data.data_val) { - remoteDispatchError (client, req, - "%s", _("out of memory allocating array")); + if (VIR_ALLOC_N(ret->data.data_val, serveroutlen) < 0) { + remoteDispatchSendError(client, req, VIR_ERR_NO_MEMORY, NULL); return -2; } memcpy(ret->data.data_val, serverout, serveroutlen); @@ -2778,12 +2813,18 @@ } /* Allocate return buffer. */ - ret->names.names_val = calloc (args->maxnames, sizeof (char *)); + if (VIR_ALLOC_N(ret->names.names_val, args->maxnames) < 0) { + remoteDispatchSendError(client, req, VIR_ERR_NO_MEMORY, NULL); + return -2; + } ret->names.names_len = virConnectListDefinedStoragePools (client->conn, - ret->names.names_val, args->maxnames); - if (ret->names.names_len == -1) return -1; + ret->names.names_val, args->maxnames); + if (ret->names.names_len == -1) { + VIR_FREE(ret->names.names_val); + return -1; + } return 0; } @@ -2804,12 +2845,18 @@ } /* Allocate return buffer. */ - ret->names.names_val = calloc (args->maxnames, sizeof (char *)); + if (VIR_ALLOC_N(ret->names.names_val, args->maxnames) < 0) { + remoteDispatchSendError(client, req, VIR_ERR_NO_MEMORY, NULL); + return -2; + } ret->names.names_len = virConnectListStoragePools (client->conn, ret->names.names_val, args->maxnames); - if (ret->names.names_len == -1) return -1; + if (ret->names.names_len == -1) { + VIR_FREE(ret->names.names_val); + return -1; + } return 0; } @@ -3213,13 +3260,20 @@ } /* Allocate return buffer. */ - ret->names.names_val = calloc (args->maxnames, sizeof (char *)); + if (VIR_ALLOC_N(ret->names.names_val, args->maxnames) < 0) { + virStoragePoolFree(pool); + remoteDispatchSendError(client, req, VIR_ERR_NO_MEMORY, NULL); + return -2; + } ret->names.names_len = virStoragePoolListVolumes (pool, ret->names.names_val, args->maxnames); virStoragePoolFree(pool); - if (ret->names.names_len == -1) return -1; + if (ret->names.names_len == -1) { + VIR_FREE(ret->names.names_val); + return -1; + } return 0; } diff -r 06acc2c5c1fb src/libvirt_sym.version --- a/src/libvirt_sym.version Thu May 29 16:05:55 2008 -0400 +++ b/src/libvirt_sym.version Fri May 30 10:36:42 2008 -0400 @@ -190,5 +190,10 @@ __virMacAddrCompare; + __virAlloc; + __virAllocN; + __virReallocN; + __virFree; + local: *; }; diff -r 06acc2c5c1fb src/memory.c --- a/src/memory.c Thu May 29 16:05:55 2008 -0400 +++ b/src/memory.c Fri May 30 10:36:42 2008 -0400 @@ -104,7 +104,7 @@ * * Returns -1 on failure to allocate, zero on success */ -int virAlloc(void *ptrptr, size_t size) +int __virAlloc(void *ptrptr, size_t size) { #if TEST_OOM if (virAllocTestFail()) { @@ -137,7 +137,7 @@ * * Returns -1 on failure to allocate, zero on success */ -int virAllocN(void *ptrptr, size_t size, size_t count) +int __virAllocN(void *ptrptr, size_t size, size_t count) { #if TEST_OOM if (virAllocTestFail()) { @@ -171,7 +171,7 @@ * * Returns -1 on failure to allocate, zero on success */ -int virReallocN(void *ptrptr, size_t size, size_t count) +int __virReallocN(void *ptrptr, size_t size, size_t count) { void *tmp; #if TEST_OOM @@ -203,7 +203,7 @@ * the 'ptrptr' variable. After release, 'ptrptr' will be * updated to point to NULL. */ -void virFree(void *ptrptr) +void __virFree(void *ptrptr) { free(*(void**)ptrptr); *(void**)ptrptr = NULL; diff -r 06acc2c5c1fb src/memory.h --- a/src/memory.h Thu May 29 16:05:55 2008 -0400 +++ b/src/memory.h Fri May 30 10:36:42 2008 -0400 @@ -26,10 +26,10 @@ #include "internal.h" /* Don't call these directly - use the macros below */ -int virAlloc(void *ptrptr, size_t size) ATTRIBUTE_RETURN_CHECK; -int virAllocN(void *ptrptr, size_t size, size_t count) ATTRIBUTE_RETURN_CHECK; -int virReallocN(void *ptrptr, size_t size, size_t count) ATTRIBUTE_RETURN_CHECK; -void virFree(void *ptrptr); +int __virAlloc(void *ptrptr, size_t size) ATTRIBUTE_RETURN_CHECK; +int __virAllocN(void *ptrptr, size_t size, size_t count) ATTRIBUTE_RETURN_CHECK; +int __virReallocN(void *ptrptr, size_t size, size_t count) ATTRIBUTE_RETURN_CHECK; +void __virFree(void *ptrptr); /** * VIR_ALLOC: @@ -41,7 +41,7 @@ * * Returns -1 on failure, 0 on success */ -#define VIR_ALLOC(ptr) virAlloc(&(ptr), sizeof(*(ptr))) +#define VIR_ALLOC(ptr) __virAlloc(&(ptr), sizeof(*(ptr))) /** * VIR_ALLOC_N: @@ -54,7 +54,7 @@ * * Returns -1 on failure, 0 on success */ -#define VIR_ALLOC_N(ptr, count) virAllocN(&(ptr), sizeof(*(ptr)), (count)) +#define VIR_ALLOC_N(ptr, count) __virAllocN(&(ptr), sizeof(*(ptr)), (count)) /** * VIR_REALLOC_N: @@ -67,7 +67,7 @@ * * Returns -1 on failure, 0 on success */ -#define VIR_REALLOC_N(ptr, count) virReallocN(&(ptr), sizeof(*(ptr)), (count)) +#define VIR_REALLOC_N(ptr, count) __virReallocN(&(ptr), sizeof(*(ptr)), (count)) /** * VIR_FREE: @@ -76,7 +76,7 @@ * Free the memory stored in 'ptr' and update to point * to NULL. */ -#define VIR_FREE(ptr) virFree(&(ptr)); +#define VIR_FREE(ptr) __virFree(&(ptr)) #if TEST_OOM -- |: Red Hat, Engineering, Boston -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

On Fri, May 30, 2008 at 03:37:06PM +0100, Daniel P. Berrange wrote:
THis patch switches over the remote daemon to use the memory allocation APIs. This required exporting the 4 apis in the .so. I discovered along the way that none of the remote RPC dispatcher impls checked for malloc failure, so I've had to add that in too.
Looks fine, this really cleans things up, especially reallocs
if (getsockname(sock->fd, (struct sockaddr *)(&sa), &salen) < 0) - return -1; + goto cleanup;
Hum, changes the semantic but it looks like it will avoid leaking file descriptors too.. [..]
+ +cleanup: + for (i = 0; i < nfds; ++i) + close(fds[0]); + return -1; [...] - if (ret->freeMems.freeMems_len == 0) + if (ret->freeMems.freeMems_len == 0) { + VIR_FREE(ret->freeMems.freeMems_val); return -1; + }
and memleaks ...
diff -r 06acc2c5c1fb src/memory.c --- a/src/memory.c Thu May 29 16:05:55 2008 -0400 +++ b/src/memory.c Fri May 30 10:36:42 2008 -0400 @@ -104,7 +104,7 @@ * * Returns -1 on failure to allocate, zero on success */ -int virAlloc(void *ptrptr, size_t size) +int __virAlloc(void *ptrptr, size_t size) { #if TEST_OOM if (virAllocTestFail()) { @@ -137,7 +137,7 @@ * * Returns -1 on failure to allocate, zero on success */ -int virAllocN(void *ptrptr, size_t size, size_t count) +int __virAllocN(void *ptrptr, size_t size, size_t count) { #if TEST_OOM if (virAllocTestFail()) { @@ -171,7 +171,7 @@ * * Returns -1 on failure to allocate, zero on success */ -int virReallocN(void *ptrptr, size_t size, size_t count) +int __virReallocN(void *ptrptr, size_t size, size_t count) { void *tmp; #if TEST_OOM @@ -203,7 +203,7 @@ * the 'ptrptr' variable. After release, 'ptrptr' will be * updated to point to NULL. */ -void virFree(void *ptrptr) +void __virFree(void *ptrptr) { free(*(void**)ptrptr); *(void**)ptrptr = NULL; diff -r 06acc2c5c1fb src/memory.h --- a/src/memory.h Thu May 29 16:05:55 2008 -0400 +++ b/src/memory.h Fri May 30 10:36:42 2008 -0400 @@ -26,10 +26,10 @@ #include "internal.h"
/* Don't call these directly - use the macros below */ -int virAlloc(void *ptrptr, size_t size) ATTRIBUTE_RETURN_CHECK; -int virAllocN(void *ptrptr, size_t size, size_t count) ATTRIBUTE_RETURN_CHECK; -int virReallocN(void *ptrptr, size_t size, size_t count) ATTRIBUTE_RETURN_CHECK; -void virFree(void *ptrptr); +int __virAlloc(void *ptrptr, size_t size) ATTRIBUTE_RETURN_CHECK; +int __virAllocN(void *ptrptr, size_t size, size_t count) ATTRIBUTE_RETURN_CHECK; +int __virReallocN(void *ptrptr, size_t size, size_t count) ATTRIBUTE_RETURN_CHECK; +void __virFree(void *ptrptr);
/** * VIR_ALLOC: @@ -41,7 +41,7 @@ * * Returns -1 on failure, 0 on success */ -#define VIR_ALLOC(ptr) virAlloc(&(ptr), sizeof(*(ptr))) +#define VIR_ALLOC(ptr) __virAlloc(&(ptr), sizeof(*(ptr)))
/** * VIR_ALLOC_N: @@ -54,7 +54,7 @@ * * Returns -1 on failure, 0 on success */ -#define VIR_ALLOC_N(ptr, count) virAllocN(&(ptr), sizeof(*(ptr)), (count)) +#define VIR_ALLOC_N(ptr, count) __virAllocN(&(ptr), sizeof(*(ptr)), (count))
/** * VIR_REALLOC_N: @@ -67,7 +67,7 @@ * * Returns -1 on failure, 0 on success */ -#define VIR_REALLOC_N(ptr, count) virReallocN(&(ptr), sizeof(*(ptr)), (count)) +#define VIR_REALLOC_N(ptr, count) __virReallocN(&(ptr), sizeof(*(ptr)), (count))
/** * VIR_FREE: @@ -76,7 +76,7 @@ * Free the memory stored in 'ptr' and update to point * to NULL. */ -#define VIR_FREE(ptr) virFree(&(ptr)); +#define VIR_FREE(ptr) __virFree(&(ptr))
ah, right we need to export those symbols now ... looks good to me, +1 Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/

On Fri, May 30, 2008 at 11:02:22AM -0400, Daniel Veillard wrote:
On Fri, May 30, 2008 at 03:37:06PM +0100, Daniel P. Berrange wrote:
THis patch switches over the remote daemon to use the memory allocation APIs. This required exporting the 4 apis in the .so. I discovered along the way that none of the remote RPC dispatcher impls checked for malloc failure, so I've had to add that in too.
Looks fine, this really cleans things up, especially reallocs
if (getsockname(sock->fd, (struct sockaddr *)(&sa), &salen) < 0) - return -1; + goto cleanup;
Hum, changes the semantic but it looks like it will avoid leaking file descriptors too..
Yes, that is correct - we were leaking FDs. It was a minor issue though because the daemon will shortly exit once control returns Regards, Daniel -- |: Red Hat, Engineering, Boston -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
participants (2)
-
Daniel P. Berrange
-
Daniel Veillard