On Fri, Oct 05, 2012 at 01:25:51PM +0200, Michal Privoznik wrote:
On 12.09.2012 18:28, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
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@redhat.com> --- 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.
It is not useless actually. The virLockSpaceResourceFree() method will do VIR_FORCE_CLOSE(res->fd); So if we were to hit the error paths before we read the 'fd' attribute, we'll end up doing VIR_FORCE_CLOSE(0) which is stdin. We can't rely on init-to-zero for file descriptor fields in structs - you must always initialize them to -1 explicitly.
+ 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?
Yes, we can do that. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|