[libvirt] [PATCH] Add missing OOM error checks, reports and cleanups
Matthias Bolte
matthias.bolte at googlemail.com
Mon Nov 9 11:07:40 UTC 2009
2009/11/9 Daniel Veillard <veillard at redhat.com>:
> 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
I think you misread this one. The original code assigns the result of
virXPathString() or strdup() to def->name. After that it checks
def->name for NULL and reports an no-name error even if the NULL was
returned by strdup(), indicating an OOM error.
I moved the no-name error report into the virXPathString() case and
added an OOM error in the strdup() case.
> [...]
>> 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 !
Well, DPB used this pattern in his "Convert virDomainObjListPtr to use
a hash of domain objects" patch, I just applied it here too :-)
> ACK except for the one in virNodeDeviceDefParseXML(), very good job
> you must have spent a lot of time, thanks a lot !
>
> Daniel
>
It took some hours, but the task was simple: search and fix.
Matthias
More information about the libvir-list
mailing list