[libvirt] [PATCH] Add missing OOM error checks, reports and cleanups
Daniel Veillard
veillard at redhat.com
Mon Nov 9 10:15:38 UTC 2009
On Sun, Nov 08, 2009 at 10:40:42PM +0100, Matthias Bolte wrote:
> diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
> index c2c5a44..c5083cc 100644
> --- a/src/conf/node_device_conf.c
> +++ b/src/conf/node_device_conf.c
> @@ -1057,13 +1057,18 @@ virNodeDeviceDefParseXML(virConnectPtr conn, xmlXPathContextPtr ctxt, int create
> /* Extract device name */
> if (create == EXISTING_DEVICE) {
> def->name = virXPathString(conn, "string(./name[1])", ctxt);
> +
> + if (!def->name) {
> + virNodeDeviceReportError(conn, VIR_ERR_NO_NAME, NULL);
> + goto error;
> + }
> } else {
> def->name = strdup("new device");
> - }
>
> - if (!def->name) {
> - virNodeDeviceReportError(conn, VIR_ERR_NO_NAME, NULL);
> - goto error;
> + if (!def->name) {
> + virReportOOMError(conn);
> + goto error;
> + }
> }
>
> /* Extract device parent, if any */
I disagree with this one. The XPath string(./name[1]) can fail without
this being an allocation error, it mays just be that there is no name
element at the current node, and current behaviour looks better to me.
Moreover if virXPathString() returns NULL because of a string allocation
error it will already raise an OOMError
[...]
> diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
> index ee7a046..c866111 100644
> --- a/src/remote/remote_driver.c
> +++ b/src/remote/remote_driver.c
> @@ -870,12 +870,12 @@ doRemoteOpen (virConnectPtr conn,
> }
>
> if(VIR_ALLOC(priv->callbackList)<0) {
> - error(conn, VIR_ERR_INVALID_ARG, _("Error allocating callbacks list"));
> + virReportOOMError(conn);
> goto failed;
> }
>
> if(VIR_ALLOC(priv->domainEvents)<0) {
> - error(conn, VIR_ERR_INVALID_ARG, _("Error allocating domainEvents"));
> + virReportOOMError(conn);
> goto failed;
> }
>
> @@ -2751,9 +2751,18 @@ remoteListDefinedDomains (virConnectPtr conn, char **const names, int maxnames)
> * names and the list of pointers, so we have to strdup the
> * names here.
> */
> - for (i = 0; i < ret.names.names_len; ++i)
> + for (i = 0; i < ret.names.names_len; ++i) {
> names[i] = strdup (ret.names.names_val[i]);
>
> + if (names[i] == NULL) {
> + for (--i; i >= 0; --i)
> + VIR_FREE(names[i]);
> +
> + virReportOOMError(conn);
> + goto cleanup;
> + }
> + }
> +
> rv = ret.names.names_len;
>
> cleanup:
> @@ -3086,7 +3095,7 @@ remoteDomainSetSchedulerParameters (virDomainPtr domain,
> /* Serialise the scheduler parameters. */
> args.params.params_len = nparams;
> if (VIR_ALLOC_N(args.params.params_val, nparams) < 0) {
> - error (domain->conn, VIR_ERR_RPC, _("out of memory allocating array"));
> + virReportOOMError(domain->conn);
> goto done;
> }
>
> @@ -3432,9 +3441,18 @@ remoteListNetworks (virConnectPtr conn, char **const names, int maxnames)
> * names and the list of pointers, so we have to strdup the
> * names here.
> */
> - for (i = 0; i < ret.names.names_len; ++i)
> + for (i = 0; i < ret.names.names_len; ++i) {
> names[i] = strdup (ret.names.names_val[i]);
>
> + if (names[i] == NULL) {
> + for (--i; i >= 0; --i)
> + VIR_FREE(names[i]);
> +
> + virReportOOMError(conn);
> + goto cleanup;
> + }
> + }
> +
> rv = ret.names.names_len;
>
> cleanup:
> @@ -3505,9 +3523,18 @@ remoteListDefinedNetworks (virConnectPtr conn,
> * names and the list of pointers, so we have to strdup the
> * names here.
> */
> - for (i = 0; i < ret.names.names_len; ++i)
> + for (i = 0; i < ret.names.names_len; ++i) {
> names[i] = strdup (ret.names.names_val[i]);
>
> + if (names[i] == NULL) {
> + for (--i; i >= 0; --i)
> + VIR_FREE(names[i]);
> +
> + virReportOOMError(conn);
> + goto cleanup;
> + }
> + }
> +
> rv = ret.names.names_len;
>
> cleanup:
> @@ -3921,9 +3948,18 @@ remoteListInterfaces (virConnectPtr conn, char **const names, int maxnames)
> * names and the list of pointers, so we have to strdup the
> * names here.
> */
> - for (i = 0; i < ret.names.names_len; ++i)
> + for (i = 0; i < ret.names.names_len; ++i) {
> names[i] = strdup (ret.names.names_val[i]);
>
> + if (names[i] == NULL) {
> + for (--i; i >= 0; --i)
> + VIR_FREE(names[i]);
> +
> + virReportOOMError(conn);
> + goto cleanup;
> + }
> + }
> +
> rv = ret.names.names_len;
>
> cleanup:
> @@ -3993,9 +4029,18 @@ remoteListDefinedInterfaces (virConnectPtr conn, char **const names, int maxname
> * names and the list of pointers, so we have to strdup the
> * names here.
> */
> - for (i = 0; i < ret.names.names_len; ++i)
> + for (i = 0; i < ret.names.names_len; ++i) {
> names[i] = strdup (ret.names.names_val[i]);
>
> + if (names[i] == NULL) {
> + for (--i; i >= 0; --i)
> + VIR_FREE(names[i]);
> +
> + virReportOOMError(conn);
> + goto cleanup;
> + }
> + }
> +
> rv = ret.names.names_len;
>
> cleanup:
> @@ -4314,9 +4359,18 @@ remoteListStoragePools (virConnectPtr conn, char **const names, int maxnames)
> * names and the list of pointers, so we have to strdup the
> * names here.
> */
> - for (i = 0; i < ret.names.names_len; ++i)
> + for (i = 0; i < ret.names.names_len; ++i) {
> names[i] = strdup (ret.names.names_val[i]);
>
> + if (names[i] == NULL) {
> + for (--i; i >= 0; --i)
> + VIR_FREE(names[i]);
> +
> + virReportOOMError(conn);
> + goto cleanup;
> + }
> + }
> +
> rv = ret.names.names_len;
>
> cleanup:
> @@ -4383,9 +4437,18 @@ remoteListDefinedStoragePools (virConnectPtr conn,
> * names and the list of pointers, so we have to strdup the
> * names here.
> */
> - for (i = 0; i < ret.names.names_len; ++i)
> + for (i = 0; i < ret.names.names_len; ++i) {
> names[i] = strdup (ret.names.names_val[i]);
>
> + if (names[i] == NULL) {
> + for (--i; i >= 0; --i)
> + VIR_FREE(names[i]);
> +
> + virReportOOMError(conn);
> + goto cleanup;
> + }
> + }
> +
> rv = ret.names.names_len;
>
> cleanup:
> @@ -4890,9 +4953,18 @@ remoteStoragePoolListVolumes (virStoragePoolPtr pool, char **const names, int ma
> * names and the list of pointers, so we have to strdup the
> * names here.
> */
> - for (i = 0; i < ret.names.names_len; ++i)
> + for (i = 0; i < ret.names.names_len; ++i) {
> names[i] = strdup (ret.names.names_val[i]);
>
> + if (names[i] == NULL) {
> + for (--i; i >= 0; --i)
> + VIR_FREE(names[i]);
> +
> + virReportOOMError(pool->conn);
> + goto cleanup;
> + }
> + }
> +
> rv = ret.names.names_len;
>
> cleanup:
> @@ -5290,9 +5362,18 @@ static int remoteNodeListDevices(virConnectPtr conn,
> * names and the list of pointers, so we have to strdup the
> * names here.
> */
> - for (i = 0; i < ret.names.names_len; ++i)
> + for (i = 0; i < ret.names.names_len; ++i) {
> names[i] = strdup (ret.names.names_val[i]);
>
> + if (names[i] == NULL) {
> + for (--i; i >= 0; --i)
> + VIR_FREE(names[i]);
> +
> + virReportOOMError(conn);
> + goto cleanup;
> + }
> + }
> +
> rv = ret.names.names_len;
>
> cleanup:
> @@ -5443,9 +5524,18 @@ static int remoteNodeDeviceListCaps(virNodeDevicePtr dev,
> * names and the list of pointers, so we have to strdup the
> * names here.
> */
> - for (i = 0; i < ret.names.names_len; ++i)
> + for (i = 0; i < ret.names.names_len; ++i) {
> names[i] = strdup (ret.names.names_val[i]);
>
> + if (names[i] == NULL) {
> + for (--i; i >= 0; --i)
> + VIR_FREE(names[i]);
> +
> + virReportOOMError(dev->conn);
> + goto cleanup;
> + }
> + }
> +
> rv = ret.names.names_len;
>
> cleanup:
> @@ -6496,9 +6586,18 @@ remoteSecretListSecrets (virConnectPtr conn, char **uuids, int maxuuids)
> * names and the list of pointers, so we have to strdup the
> * names here.
> */
> - for (i = 0; i < ret.uuids.uuids_len; ++i)
> + for (i = 0; i < ret.uuids.uuids_len; ++i) {
> uuids[i] = strdup (ret.uuids.uuids_val[i]);
>
> + if (uuids[i] == NULL) {
> + for (--i; i >= 0; --i)
> + VIR_FREE(uuids[i]);
> +
> + virReportOOMError(conn);
> + goto cleanup;
> + }
> + }
> +
> rv = ret.uuids.uuids_len;
>
> cleanup:
> @@ -6707,8 +6806,10 @@ remoteStreamOpen(virStreamPtr st,
> struct private_data *priv = st->conn->privateData;
> struct private_stream_data *stpriv;
>
> - if (VIR_ALLOC(stpriv) < 0)
> + if (VIR_ALLOC(stpriv) < 0) {
> + virReportOOMError(st->conn);
> return NULL;
> + }
>
> /* Initialize call object used to receive replies */
> stpriv->proc_nr = proc_nr;
> @@ -7106,8 +7207,10 @@ prepareCall(virConnectPtr conn,
> struct remote_message_header hdr;
> struct remote_thread_call *rv;
>
> - if (VIR_ALLOC(rv) < 0)
> + if (VIR_ALLOC(rv) < 0) {
> + virReportOOMError(conn);
> return NULL;
> + }
>
> if (virCondInit(&rv->cond) < 0) {
> VIR_FREE(rv);
> diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c
> index 1d5b4f7..1eef468 100644
> --- a/src/secret/secret_driver.c
> +++ b/src/secret/secret_driver.c
> @@ -449,8 +449,10 @@ secretLoad(virConnectPtr conn, virSecretDriverStatePtr driver,
> if (secretLoadValidateUUID(conn, def, xml_basename) < 0)
> goto cleanup;
>
> - if (VIR_ALLOC(secret) < 0)
> + if (VIR_ALLOC(secret) < 0) {
> + virReportOOMError(conn);
> goto cleanup;
> + }
> secret->def = def;
> def = NULL;
>
> @@ -578,8 +580,10 @@ secretListSecrets(virConnectPtr conn, char **uuids, int maxuuids)
> char *uuidstr;
> if (i == maxuuids)
> break;
> - if (VIR_ALLOC_N(uuidstr, VIR_UUID_STRING_BUFLEN) < 0)
> + if (VIR_ALLOC_N(uuidstr, VIR_UUID_STRING_BUFLEN) < 0) {
> + virReportOOMError(conn);
> goto cleanup;
> + }
> virUUIDFormat(secret->def->uuid, uuidstr);
> uuids[i] = uuidstr;
> i++;
Okay, I was just a bit worried about virReportOOMError() in the
context of a remote driver entry point but apparently there are
previous uses in that context, so nice :-)
[...]
> @@ -2748,6 +2759,7 @@ cleanup:
>
> struct xenXMListIteratorContext {
> virConnectPtr conn;
> + int oom;
> int max;
> int count;
> char ** names;
> @@ -2757,13 +2769,18 @@ static void xenXMListIterator(void *payload ATTRIBUTE_UNUSED, const char *name,
> struct xenXMListIteratorContext *ctx = data;
> virDomainPtr dom = NULL;
>
> + if (ctx->oom)
> + return;
> +
> if (ctx->count == ctx->max)
> return;
>
> dom = xenDaemonLookupByName(ctx->conn, name);
> if (!dom) {
> - ctx->names[ctx->count] = strdup(name);
> - ctx->count++;
> + if (!(ctx->names[ctx->count] = strdup(name)))
> + ctx->oom = 1;
> + else
> + ctx->count++;
> } else {
> virDomainFree(dom);
> }
> @@ -2777,7 +2794,7 @@ static void xenXMListIterator(void *payload ATTRIBUTE_UNUSED, const char *name,
> int xenXMListDefinedDomains(virConnectPtr conn, char **const names, int maxnames) {
> xenUnifiedPrivatePtr priv;
> struct xenXMListIteratorContext ctx;
> - int ret = -1;
> + int i, ret = -1;
>
> if (!VIR_IS_CONNECT(conn)) {
> xenXMError(conn, VIR_ERR_INVALID_CONN, __FUNCTION__);
> @@ -2794,11 +2811,21 @@ int xenXMListDefinedDomains(virConnectPtr conn, char **const names, int maxnames
> maxnames = virHashSize(priv->configCache);
>
> ctx.conn = conn;
> + ctx.oom = 0;
> ctx.count = 0;
> ctx.max = maxnames;
> ctx.names = names;
>
> virHashForEach(priv->nameConfigMap, xenXMListIterator, &ctx);
> +
> + if (ctx.oom) {
> + for (i = 0; i < ctx.count; i++)
> + VIR_FREE(ctx.names[i]);
> +
> + virReportOOMError(conn);
> + goto cleanup;
> + }
> +
> ret = ctx.count;
>
> cleanup:
So you had to add a filed in the iterator structure to report OOMs
while running the iterator, nice work !
ACK except for the one in virNodeDeviceDefParseXML(), very good job
you must have spent a lot of time, thanks a lot !
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel at veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
More information about the libvir-list
mailing list