[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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


> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 58a9520..6c94584 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1328,6 +1328,8 @@ virLockSpaceDeleteResource;
>  virLockSpaceFree;
>  virLockSpaceGetDirectory;
>  virLockSpaceNew;
> +virLockSpaceNewPostExecRestart;
> +virLockSpacePreExecRestart;
>  virLockSpaceReleaseResource;
>  virLockSpaceReleaseResourcesForOwner;
>  
> diff --git a/src/util/virlockspace.c b/src/util/virlockspace.c
> index 611592a..786513f 100644
> --- a/src/util/virlockspace.c
> +++ b/src/util/virlockspace.c
> @@ -295,6 +295,242 @@ error:
>  }
>  
>  
> +
> +virLockSpacePtr virLockSpaceNewPostExecRestart(virJSONValuePtr object)
> +{
> +    virLockSpacePtr lockspace;
> +    virJSONValuePtr resources;
> +    int n;
> +    size_t i;
> +
> +    VIR_DEBUG("object=%p", object);
> +
> +    if (VIR_ALLOC(lockspace) < 0)
> +        return NULL;
> +
> +    if (virMutexInit(&lockspace->lock) < 0) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("Unable to initialize lockspace mutex"));
> +        VIR_FREE(lockspace);
> +        return NULL;
> +    }
> +
> +    if (!(lockspace->resources = virHashCreate(10,
> +                                               virLockSpaceResourceDataFree)))

s/10/VIR_LOCK_SPACE_RESOURCE_HASH_TABLE_SIZE/ or the name you invent

> +        goto error;
> +
> +    if (virJSONValueObjectHasKey(object, "directory")) {
> +        const char *dir = virJSONValueObjectGetString(object, "directory");
> +        if (!(lockspace->dir = strdup(dir))) {
> +            virReportOOMError();
> +            goto error;
> +        }
> +    }
> +
> +    if (!(resources = virJSONValueObjectGet(object, "resources"))) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("Missing resources value in JSON document"));
> +        goto error;
> +    }
> +
> +    if ((n = virJSONValueArraySize(resources)) < 0) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("Malformed resources value in JSON document"));
> +        goto error;
> +    }
> +
> +    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?

> +            }
> +
> +            res->owners[res->nOwners-1] = (pid_t)owner;
> +        }
> +
> +        if (virHashAddEntry(lockspace->resources, res->name, res) < 0) {
> +            virLockSpaceResourceFree(res);
> +            goto error;
> +        }
> +    }
> +
> +    return lockspace;
> +
> +error:
> +    virLockSpaceFree(lockspace);
> +    return NULL;
> +}
> +
> +
> +virJSONValuePtr virLockSpacePreExecRestart(virLockSpacePtr lockspace)
> +{
> +    virJSONValuePtr object = virJSONValueNewObject();
> +    virJSONValuePtr resources;
> +    virHashKeyValuePairPtr pairs = NULL, tmp;
> +
> +    if (!object)
> +        return NULL;
> +
> +    virMutexLock(&lockspace->lock);
> +
> +    if (lockspace->dir &&
> +        virJSONValueObjectAppendString(object, "directory", lockspace->dir) < 0)
> +        goto error;
> +
> +    if (!(resources = virJSONValueNewArray()))
> +        goto error;
> +
> +    if (virJSONValueObjectAppend(object, "resources", resources) < 0) {
> +        virJSONValueFree(resources);
> +        goto error;
> +    }
> +
> +    tmp = pairs = virHashGetItems(lockspace->resources, NULL);
> +    while (tmp && tmp->value) {
> +        virLockSpaceResourcePtr res = (virLockSpaceResourcePtr)tmp->value;
> +        virJSONValuePtr child = virJSONValueNewObject();
> +        virJSONValuePtr owners = NULL;
> +        size_t i;
> +
> +        if (virJSONValueArrayAppend(resources, child) < 0) {
> +            virJSONValueFree(child);
> +            goto error;
> +        }
> +
> +        if (virJSONValueObjectAppendString(child, "name", res->name) < 0 ||
> +            virJSONValueObjectAppendString(child, "path", res->path) < 0 ||
> +            virJSONValueObjectAppendNumberInt(child, "fd", res->fd) < 0 ||
> +            virJSONValueObjectAppendBoolean(child, "lockHeld", res->lockHeld) < 0 ||
> +            virJSONValueObjectAppendNumberUint(child, "flags", res->flags) < 0)
> +            goto error;
> +
> +        if (virSetInherit(res->fd, true) < 0) {
> +            virReportSystemError(errno, "%s",
> +                                 _("Cannot disable close-on-exec flag"));
> +            goto error;
> +        }
> +
> +        if (!(owners = virJSONValueNewArray()))
> +            goto error;
> +
> +        if (virJSONValueObjectAppend(child, "owners", owners) < 0) {
> +            virJSONValueFree(owners);
> +            goto error;
> +        }
> +
> +        for (i = 0 ; i < res->nOwners ; i++) {
> +            virJSONValuePtr owner = virJSONValueNewNumberUlong(res->owners[i]);
> +            if (!owner)
> +                goto error;
> +
> +            if (virJSONValueArrayAppend(owners, owner) < 0) {
> +                virJSONValueFree(owner);
> +                goto error;
> +            }
> +        }
> +
> +        tmp++;
> +    }
> +    VIR_FREE(pairs);
> +
> +    virMutexUnlock(&lockspace->lock);
> +    return object;
> +
> +  error:
> +    VIR_FREE(pairs);
> +    virJSONValueFree(object);
> +    virMutexUnlock(&lockspace->lock);
> +    return NULL;
> +}
> +
> +
>  void virLockSpaceFree(virLockSpacePtr lockspace)
>  {
>      if (!lockspace)
> diff --git a/src/util/virlockspace.h b/src/util/virlockspace.h
> index bd8f91c..9c5128b 100644
> --- a/src/util/virlockspace.h
> +++ b/src/util/virlockspace.h
> @@ -23,11 +23,15 @@
>  # define __VIR_LOCK_SPACE_H__
>  
>  # include "internal.h"
> +# include "json.h"
>  
>  typedef struct _virLockSpace virLockSpace;
>  typedef virLockSpace *virLockSpacePtr;
>  
>  virLockSpacePtr virLockSpaceNew(const char *directory);
> +virLockSpacePtr virLockSpaceNewPostExecRestart(virJSONValuePtr object);
> +
> +virJSONValuePtr virLockSpacePreExecRestart(virLockSpacePtr lockspace);
>  
>  void virLockSpaceFree(virLockSpacePtr lockspace);
>  
> 


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]