5:07 a.m.
2009/11/9 Daniel Veillard <veillard(a)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