[Libvirt-cim] [PATCH 1 of 3] Add VirtualSystemSnapshotService

[Heidi Eckhart]

Dan Smith wrote:
> +++ b/src/Virt_VirtualSystemSnapshotService.c	Tue Feb 26 11:19:56 2008 -0800
> @@ -0,0 +1,521 @@
> +/*
> + * Copyright IBM Corp. 2007
>   
2008 ;)

> +static void do_snapshot(struct snap_context *ctx,
> +                        virConnectPtr conn,
> +                        virDomainPtr dom)
> +{
> +  ...
> +        if (ctx->restore) {
> +                CU_DEBUG("Starting restore from %s", ctx->save_path);
> +
> +                ret = virDomainRestore(conn, ctx->save_path);
> +                if (ret == -1) {
> +                        CU_DEBUG("Restore failed");
> +                        snap_job_set_status(ctx,
> +                                            CIM_JOBSTATE_COMPLETE,
> +                                            "Snapshot Failed (restore)");
> +                        return;
> +                }
> +
> +                CU_DEBUG("Restore completed");
> +                snap_job_set_status(ctx,
> +                                    CIM_JOBSTATE_RUNNING,
> +                                    "Restore finished");
>   
Please can you explain why you set the status here and overwrite it 
right afterwards ? I think the CIM_JOBSTATE is already COMPLETE here.
> +        }
> +
> +        CU_DEBUG("Snapshot (%s/%s) completed",
> +                 ctx->save ? "Save" : "None",
> +                 ctx->restore ? "Restore" : "None");
> +
> +        snap_job_set_status(ctx,
> +                            CIM_JOBSTATE_COMPLETE,
> +                            "Snapshot complete");
> +
> +        return;
> +}
> +
> +static CMPI_THREAD_RETURN snapshot_thread(struct snap_context *ctx)
> +{...
> +        conn = connect_by_classname(_BROKER, ctx->ref_cn, &s);
> +        if (conn == NULL) {
> +                CU_DEBUG("Failed to connect with classname `%s'", ctx->ref_cn);
> +                snap_job_set_status(ctx,
> +                                    CIM_JOBSTATE_COMPLETE,
> +                                    "Unable to connect to hypervisor");
> +                goto out;
> +        }
> +
> +        dom = virDomainLookupByName(conn, ctx->domain);
> +        if (dom == NULL) {
> +                CU_DEBUG("No such domain `%s'", ctx->domain);
> +                snap_job_set_status(ctx,
> +                                    CIM_JOBSTATE_COMPLETE,
> +                                    "No such domain");
> +                goto out;
> +        }
> +
> +        do_snapshot(ctx, conn, dom);
> +
> + out:
>   
FYI - I encountered some problems with another provider, where I tried 
virDomainFree() with a not initialized dom. This could also happen here 
with the goto out from conn. Maybe virDomainFree() can not handle NULL ?
> +        virDomainFree(dom);
> +        virConnectClose(conn);
> +
> +        snap_job_free(ctx);
> +
> +        return NULL;
> +}
> +
> +static CMPIStatus create_job(const CMPIContext *context,
> +                             const CMPIObjectPath *ref,
> +                             struct snap_context *ctx,
> +                             CMPIObjectPath **job)
> +{
> +        CMPIObjectPath *op;
> +        CMPIInstance *inst;
> +        CMPIStatus s;
> +
> +        op = CMNewObjectPath(_BROKER,
> +                             NAMESPACE(ref),
> +                             "CIM_ConcreteJob", /*FIXME*/
> +                             &s);
> +        if ((s.rc != CMPI_RC_OK) || (op == NULL)) {
> +                CU_DEBUG("Failed to create job path");
> +                goto out;
> +        }
> +
> +        CMSetNameSpace(op, NAMESPACE(ref));
>   
This step is not necessary as NewObjectPath already sets the namespace.
> +
> +        inst = CMNewInstance(_BROKER, op, &s);
> +        if ((s.rc != CMPI_RC_OK) || (inst == NULL)) {
> +                CU_DEBUG("Failed to create job instance");
> +                goto out;
> +        }
> +
> +        CMSetProperty(inst, "InstanceID",
> +                      (CMPIValue *)ctx->uuid, CMPI_chars);
> +
> +        CMSetProperty(inst, "Name",
> +                      (CMPIValue *)"Snapshot", CMPI_chars);
> +
> +        CMSetProperty(inst, "Status",
> +                      (CMPIValue *)"Queued", CMPI_chars);
> +
> +        op = CMGetObjectPath(inst, &s);
> +        if ((op == NULL) || (s.rc != CMPI_RC_OK)) {
> +                CU_DEBUG("Failed to get path of job instance");
> +                goto out;
> +        }
> +
> +        CMSetNameSpace(op, NAMESPACE(ref));
>   
This should also be not necessary. But as you debug the ref right 
afterwards ... did you encounter some problems here ?
> +
> +        CU_DEBUG("ref was %s", CMGetCharPtr(CMObjectPathToString(op, NULL)));
> +
> +        *job = CBCreateInstance(_BROKER, context, op, inst, &s);
> +        if ((*job == NULL) || (s.rc != CMPI_RC_OK)) {
> +                CU_DEBUG("Failed to create job");
> +                goto out;
> +        }
> +
> +        ctx->ref_ns = strdup(NAMESPACE(ref));
> +        ctx->ref_cn = strdup(CLASSNAME(ref));
> +
> +        ctx->context = CBPrepareAttachThread(_BROKER, context);
> +
> +        _BROKER->xft->newThread((void *)snapshot_thread, ctx, 0);
> +
> +        cu_statusf(_BROKER, &s,
> +                   CMPI_RC_OK,
> +                   "");
>   
Its more my personal view, but as you "goto out" each time the status is 
something else than CMPI_RC_OK, it should still be CMPI_RC_OK at this 
point, if you initialize it right at the beginning of the function with
CMPIStatus s = {CMPI_RC_OK, NULL}; .
> + out:
> +        return s;
> +}
> +
> +char *vsss_get_save_path(const char *domname)
> +{
> +        int ret;
> +        char *path = NULL;
>   
You could use a CMPIString and return its char* to the caller. This 
avoids missing frees.
> +
> +        ret = asprintf(&path,
> +                       "/var/lib/libvirt/%s.save", domname);
> +        if (ret == -1)
> +                return NULL;
> +
> +        return path;
> +}
> + ...
> +
> +
> +static CMPIStatus create_snapshot(CMPIMethodMI *self,
> +                                  const CMPIContext *context,
> +                                  const CMPIResult *results,
> +                                  const CMPIObjectPath *reference,
> +                                  const CMPIArgs *argsin,
> +                                  CMPIArgs *argsout)
> +{
> +        CMPIStatus s = {CMPI_RC_OK, NULL};
> +        CMPIObjectPath *system;
> +        CMPIInstance *sd;
> +        uint16_t type;
> +        uint32_t retcode = 0;
> +        const char *name;
> +
> +        if (cu_get_u16_arg(argsin, "SnapshotType", &type) != CMPI_RC_OK) {
> +                cu_statusf(_BROKER, &s,
> +                           CMPI_RC_ERR_INVALID_PARAMETER,
> +                           "Missing SnapshotType");
> +                goto out;
> +        }
> + ...
> +
> +        s = start_snapshot_job(reference, context, name, type);
> +
> +        CMReturnData(results, (CMPIValue *)&retcode, CMPI_uint32);
> + out:
> +        CU_DEBUG("Returning: %i", s.rc);
>   
You also need to do a CMReturnData for a failed request.
> +        return s;
> +}
> +
> +static CMPIStatus destroy_snapshot(CMPIMethodMI *self,
> +                                   const CMPIContext *context,
> +                                   const CMPIResult *results,
> +                                   const CMPIObjectPath *reference,
> +                                   const CMPIArgs *argsin,
> +                                   CMPIArgs *argsout)
> +{
> +        CMPIStatus s = {CMPI_RC_OK, NULL};
> +        CMPIObjectPath *snap;
> +        char *name = NULL;
> +        char *path = NULL;
> + ...
> + out:
> +        free(path);
> +        free(name);
You need to return the uint32 status of this method. The same for 
apply_snapshot().
> +        return s;
> +}
> + ...
> +
> +STDIM_MethodMIStub(, Virt_VirtualSystemSnapshotService,
> +                   _BROKER, libvirt_cim_init(), handlers);
>   
I miss the instance provider interface. Do you plan to add this ?


-- 
Regards

Heidi Eckhart
Software Engineer
IBM Linux Technology Center - Open Hypervisor