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