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@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > ---
> > 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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list