Re: [libvirt] [PATCH 02/12] Add JSON serialization of virLockSpacePtr objects for process re-exec()

On 12.09.2012 18:28, Daniel P. Berrange wrote: > From: "Daniel P. Berrange" <berrange(a)redhat.com&gt; > > Add two new APIs virLockSpaceNewPostExecRestart and > virLockSpacePreExecRestart which allow a virLockSpacePtr > object to be created from a JSON object and saved to a > JSON object, for the purposes of re-exec'ing a process. > > As well as saving the state in JSON format, the second > method will disable the O_CLOEXEC flag so that the open > file descriptors are preserved across the process re-exec() > > Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com&gt; > --- > src/libvirt_private.syms | 2 + > src/util/virlockspace.c | 236 +++++++++++++++++++++++++++++++++++++++++++++++ > src/util/virlockspace.h | 4 + > 3 files changed, 242 insertions(+) > ACK but see my comments below

> + for (i = 0 ; i < n ; i++) { > + virJSONValuePtr child = virJSONValueArrayGet(resources, i); > + virLockSpaceResourcePtr res; > + const char *tmp; > + virJSONValuePtr owners; > + size_t j; > + int m; > + > + if (VIR_ALLOC(res) < 0) { > + virReportOOMError(); > + goto error; > + } > + res->fd = -1; This is useless ... > + > + if (!(tmp = virJSONValueObjectGetString(child, "name"))) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("Missing resource name in JSON document")); > + virLockSpaceResourceFree(res); > + goto error; > + } > + if (!(res->name = strdup(tmp))) { > + virReportOOMError(); > + virLockSpaceResourceFree(res); > + goto error; > + } > + > + if (!(tmp = virJSONValueObjectGetString(child, "path"))) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("Missing resource path in JSON document")); > + virLockSpaceResourceFree(res); > + goto error; > + } > + if (!(res->path = strdup(tmp))) { > + virReportOOMError(); > + virLockSpaceResourceFree(res); > + goto error; > + } > + if (virJSONValueObjectGetNumberInt(child, "fd", &res->fd) < 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("Missing resource fd in JSON document")); > + virLockSpaceResourceFree(res); > + goto error; > + } ... since we require 'fd' attribute anyway.

> + if (virSetInherit(res->fd, false) < 0) { > + virReportSystemError(errno, "%s", > + _("Cannot enable close-on-exec flag")); > + goto error; > + } > + if (virJSONValueObjectGetBoolean(child, "lockHeld", &res->lockHeld) < 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("Missing resource lockHeld in JSON document")); > + virLockSpaceResourceFree(res); > + goto error; > + } > + > + if (virJSONValueObjectGetNumberUint(child, "flags", &res->flags) < 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("Missing resource flags in JSON document")); > + virLockSpaceResourceFree(res); > + goto error; > + } > + > + if (!(owners = virJSONValueObjectGet(child, "owners"))) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("Missing resource owners in JSON document")); > + virLockSpaceResourceFree(res); > + goto error; > + } > + > + if ((m = virJSONValueArraySize(owners)) < 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("Malformed owners value in JSON document")); > + virLockSpaceResourceFree(res); > + goto error; > + } > + Since we already know the res->owners size, wouldn't it be better to allocate it here instead of ... > + for (j = 0 ; j < m ; j++) { > + unsigned long long int owner; > + virJSONValuePtr ownerval = virJSONValueArrayGet(owners, j); > + > + if (virJSONValueGetNumberUlong(ownerval, &owner) < 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("Malformed owner value in JSON document")); > + virLockSpaceResourceFree(res); > + goto error; > + } > + > + if (VIR_EXPAND_N(res->owners, res->nOwners, 1) < 0) { > + virReportOOMError(); > + virLockSpaceResourceFree(res); > + goto error; ... doing this?