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

[Daniel P. Berrange]

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 at 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 at 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 :|