4:59 a.m.
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