On Wed, Jun 27, 2012 at 03:58:01PM -0300, Eduardo Lima (Etrunko) wrote:

From: "Eduardo Lima (Etrunko)" <eblima@br.ibm.com>

Further job implementations may reuse these values.

Signed-off-by: Eduardo Lima (Etrunko) <eblima@br.ibm.com> --- src/Virt_VSMigrationService.c | 14 +++++--------- src/svpc_types.h | 13 +++++++++++++ 2 files changed, 18 insertions(+), 9 deletions(-)

diff --git a/src/Virt_VSMigrationService.c b/src/Virt_VSMigrationService.c index 76e3d25..a6b5fc0 100644 --- a/src/Virt_VSMigrationService.c +++ b/src/Virt_VSMigrationService.c @@ -52,10 +52,6 @@

#include "config.h"

-#define CIM_JOBSTATE_STARTING 3 -#define CIM_JOBSTATE_RUNNING 4 -#define CIM_JOBSTATE_COMPLETE 7 - #define MIGRATE_SHUTDOWN_TIMEOUT 120

#define METHOD_RETURN(r, v) do { \ @@ -1289,19 +1285,19 @@ static CMPI_THREAD_RETURN migration_thread(struct migration_job *job) CBAttachThread(_BROKER, job->context);

CU_DEBUG("Migration Job %s started", job->uuid); - migrate_job_set_state(job, CIM_JOBSTATE_RUNNING, 0, "Running"); + migrate_job_set_state(job, CIM_JOB_STATE_RUNNING, 0, "Running");

s = migrate_vs(job);

CU_DEBUG("Migration Job %s finished: %i", job->uuid, s.rc); if (s.rc != CMPI_RC_OK) migrate_job_set_state(job, - CIM_JOBSTATE_COMPLETE, + CIM_JOB_STATE_COMPLETED, s.rc, CMGetCharPtr(s.msg)); else migrate_job_set_state(job, - CIM_JOBSTATE_COMPLETE, + CIM_JOB_STATE_COMPLETED, 0, "Completed");

@@ -1361,7 +1357,7 @@ static CMPIInstance *_migrate_job_new_instance(const char *cn, }

inst = CMNewInstance(_BROKER, op, &s); - if ((s.rc != CMPI_RC_OK) || (CMIsNullObject(op))) { + if ((s.rc != CMPI_RC_OK) || (CMIsNullObject(inst))) { CU_DEBUG("Failed to create instance from ref: %s", CMGetCharPtr(CMObjectPathToString(op, NULL))); return NULL; @@ -1378,7 +1374,7 @@ static CMPIStatus migrate_create_job_instance(const CMPIContext *context, CMPIInstance *jobinst; CMPIDateTime *start; CMPIBoolean autodelete = true; - uint16_t state = CIM_JOBSTATE_STARTING; + uint16_t state = CIM_JOB_STATE_STARTING; char *type = NULL;

start = CMNewDateTime(_BROKER, &s); diff --git a/src/svpc_types.h b/src/svpc_types.h index 90bb608..338a7ef 100644 --- a/src/svpc_types.h +++ b/src/svpc_types.h @@ -167,5 +167,18 @@ enum CIM_op_status { CIM_OP_STATUS_POWER_MODE = 18, };

+enum CIM_job_state { + CIM_JOB_STATE_NEW = 2, + CIM_JOB_STATE_STARTING, + CIM_JOB_STATE_RUNNING, + CIM_JOB_STATE_SUSPENDED, + CIM_JOB_STATE_SHUTTING_DOWN, + CIM_JOB_STATE_COMPLETED, + CIM_JOB_STATE_TERMINATED, + CIM_JOB_STATE_KILLED, + CIM_JOB_STATE_EXCEPTION, + CIM_JOB_STATE_SERVICE, + CIM_JOB_STATE_QUERY_PENDING, +};

Okay, that looks fairly safe as the values for the enums are preserved. i would just suggest to explicitely set the values instead of relying on C for propagating the right value: + CIM_JOB_STATE_STARTING = 3, + CIM_JOB_STATE_RUNNING = 4, + CIM_JOB_STATE_SUSPENDED = 5, + CIM_JOB_STATE_SHUTTING_DOWN = 6, + CIM_JOB_STATE_COMPLETED = 7, + CIM_JOB_STATE_TERMINATED = 8, + CIM_JOB_STATE_KILLED = 9, + CIM_JOB_STATE_EXCEPTION = 10, + CIM_JOB_STATE_SERVICE = 11, + CIM_JOB_STATE_QUERY_PENDING = 12, ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/

On Wed, Jun 27, 2012 at 03:58:02PM -0300, Eduardo Lima (Etrunko) wrote:

From: "Eduardo Lima (Etrunko)" <eblima@br.ibm.com>

For Reboot and Shutdown, th RequestStateChange method returns immediately with

typo th -> the

return code 0 (successful) even though the state change is still not completed.

According to the DMTF specification DSP1052 (Computer System Profile) the RequestStateChange() method should return 0x1000 and a corresponding job reference in the return parameters which can be polled for completion.

Signed-off-by: Eduardo Lima (Etrunko) <eblima@br.ibm.com> --- schema/ComputerSystem.mof | 9 ++ src/Virt_ComputerSystem.c | 326 +++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 325 insertions(+), 10 deletions(-)

diff --git a/schema/ComputerSystem.mof b/schema/ComputerSystem.mof index 10cb8c4..886c085 100644 --- a/schema/ComputerSystem.mof +++ b/schema/ComputerSystem.mof @@ -1,5 +1,14 @@ // Copyright IBM Corp. 2007

+class Xen_ComputerSystemStateChangeJob : CIM_ConcreteJob { +}; + +class KVM_ComputerSystemStateChangeJob : CIM_ConcreteJob { +}; + +class LXC_ComputerSystemStateChangeJob : CIM_ConcreteJob { +}; + [Description ( "A class derived from CIM_ComputerSystem to represent " "the Xen virtual machines/domains running on the system."), diff --git a/src/Virt_ComputerSystem.c b/src/Virt_ComputerSystem.c index e6c7e55..9546da2 100644 --- a/src/Virt_ComputerSystem.c +++ b/src/Virt_ComputerSystem.c @@ -30,23 +30,40 @@ #include <cmpift.h> #include <cmpimacs.h>

+#include <uuid.h> #include <libvirt/libvirt.h> +#include <libvirt/virterror.h>

-#include "cs_util.h" #include <libcmpiutil/libcmpiutil.h> -#include "misc_util.h" -#include "infostore.h" -#include "device_parsing.h" #include <libcmpiutil/std_invokemethod.h> #include <libcmpiutil/std_instance.h> #include <libcmpiutil/std_indication.h>

+#include "cs_util.h" +#include "misc_util.h" +#include "infostore.h" +#include "device_parsing.h" +#include "svpc_types.h" + #include "Virt_ComputerSystem.h" #include "Virt_HostSystem.h" #include "Virt_VirtualSystemSnapshotService.h"

const static CMPIBroker *_BROKER;

+typedef struct _state_change_job state_change_job_t; +struct _state_change_job { + char uuid[VIR_UUID_STRING_BUFLEN]; + CMPIContext *context; + CMPIObjectPath *obj_path; + CMPIInstance *inst; + char *dom_name; + uint16_t dom_state; + uint16_t status; /* job status */ +}; + +static bool events_registered = false; + /* Set the "Name" property of an instance from a domain */ static int set_name_from_dom(virDomainPtr dom, CMPIInstance *instance) { @@ -1189,19 +1206,20 @@ static CMPIStatus __state_change(const char *name, s = state_change_enable(dom, &info); else if (state == CIM_STATE_DISABLED) s = state_change_disable(dom, &info); - else if (state == CIM_STATE_SHUTDOWN) - s = state_change_shutdown(dom, &info); else if (state == CIM_STATE_PAUSED) s = state_change_pause(dom, &info); - else if (state == CIM_STATE_REBOOT) - s = state_change_reboot(dom, &info); else if (state == CIM_STATE_RESET) s = state_change_reset(dom, &info); + else if (state == CIM_STATE_SHUTDOWN || state == CIM_STATE_REBOOT) + s.rc = CIM_SVPC_RETURN_JOB_STARTED; else cu_statusf(_BROKER, &s, CMPI_RC_ERR_NOT_SUPPORTED, "State not supported");

+ if (s.rc != CMPI_RC_OK && s.rc != CIM_SVPC_RETURN_JOB_STARTED) + goto out; + infostore = infostore_open(dom); if (infostore != NULL) { infostore_set_u64(infostore, "reqstate", (uint64_t)state); @@ -1215,6 +1233,265 @@ static CMPIStatus __state_change(const char *name, return s; }

+static CMPIStatus create_state_change_job(const CMPIObjectPath *ref, + const CMPIContext *context, + state_change_job_t **job, + uint16_t state) +{ + CMPIStatus s = {CMPI_RC_OK, NULL}; + CMPIInstance *job_inst; + CMPIDateTime *start; + CMPIBoolean autodelete = true; + CMPIObjectPath *obj_path; + uuid_t uuid; + char *type = NULL, *cn = NULL, *ns = NULL; + + start = CMNewDateTime(_BROKER, &s); + if ((s.rc != CMPI_RC_OK) || CMIsNullObject(start)) { + cu_statusf(_BROKER, &s, + CMPI_RC_ERR_FAILED, + "Failed to get job start time"); + goto out; + } + + cn = strdup(CLASSNAME(ref)); + type = get_typed_class(cn, "ComputerSystemStateChangeJob"); + + obj_path = CMNewObjectPath(_BROKER, ns, type, &s); + if ((s.rc != CMPI_RC_OK) || (CMIsNullObject(obj_path))) { + cu_statusf(_BROKER, &s, + CMPI_RC_ERR_FAILED, + "Failed to get new object path"); + goto out; + } + + job_inst = CMNewInstance(_BROKER, obj_path, &s); + if ((s.rc != CMPI_RC_OK) || (CMIsNullObject(job_inst))) { + cu_statusf(_BROKER, &s, + CMPI_RC_ERR_FAILED, + "Failed to get new instance object"); + goto out; + } + + /* Alloc job struct */ + *job = calloc(1, sizeof(**job)); + if (*job == NULL) { + cu_statusf(_BROKER, &s, + CMPI_RC_ERR_FAILED, + "Failed to allocate memory for job structure"); + goto out; + } + + (*job)->dom_state = state; + (*job)->status = CIM_JOB_STATE_NEW; + + uuid_generate(uuid); + uuid_unparse(uuid, (*job)->uuid); + + /* Set Properties */ + CMSetProperty(job_inst, "InstanceID", + (CMPIValue *) (*job)->uuid, CMPI_chars); + CMSetProperty(job_inst, "Name", + (CMPIValue *) "ComputerSystemStateChange", CMPI_chars); + CMSetProperty(job_inst, "StartTime", + (CMPIValue *) &start, CMPI_dateTime); + CMSetProperty(job_inst, "JobState", + (CMPIValue *) &((*job)->status), CMPI_uint16); + CMSetProperty(job_inst, "Status", + (CMPIValue *) "New", CMPI_chars); + CMSetProperty(job_inst, "DeleteOnCompletion", + (CMPIValue *)&autodelete, CMPI_boolean); + + obj_path = CMGetObjectPath(job_inst, &s); + if ((obj_path == NULL) || (s.rc != CMPI_RC_OK)) { + cu_statusf(_BROKER, &s, + CMPI_RC_ERR_FAILED, + "Failed to get path for ComputerSystemStateChangeJob instance"); + goto out; + } + + CMSetNameSpace(obj_path, ns); + + CU_DEBUG("Creating ComputerSystemStateChangeJob instance: %s", + CMGetCharPtr(CMObjectPathToString(obj_path, NULL))); + + obj_path = CBCreateInstance(_BROKER, context, obj_path, job_inst, &s); + if ((s.rc != CMPI_RC_OK) || (CMIsNullObject(obj_path))) { + CU_DEBUG("Failed to create ComputerSystemStateChangeJob instance: %i", s.rc); + goto out; + } + + ns = strdup(NAMESPACE(ref)); + CMSetNameSpace(obj_path, ns); + + (*job)->obj_path = obj_path; + (*job)->context = CBPrepareAttachThread(_BROKER, context); + if ((*job)->context == NULL) { + CU_DEBUG("Failed to create thread context"); + cu_statusf(_BROKER, &s, CMPI_RC_ERR_FAILED, + "Unable to create thread context"); + goto out; + } + + (*job)->inst = CBGetInstance(_BROKER, (*job)->context, (*job)->obj_path, NULL, &s); + if (((*job)->inst == NULL) || (s.rc != CMPI_RC_OK)) { + CU_DEBUG("Failed to get job instance (%i)", s.rc); + goto out; + } + + out: + free(type); + free(cn); + free(ns);

Hum, we rely here on "free(NULL)' to do the right thing, but that's okay for all recent implementations :-)

+ return s; +} + +static void state_change_reboot_cb(virConnectPtr conn, + virDomainPtr dom, + void *data) +{ + state_change_job_t *job = (state_change_job_t *) data; + job->status = CIM_JOB_STATE_COMPLETED; + CU_DEBUG("state_change_reboot_cb(): Reboot complete"); +} + +static void state_change_shutdown_cb(virConnectPtr conn, + virDomainPtr dom, + int event, + int detail, + void *data) +{ + state_change_job_t *job = (state_change_job_t *) data; + if (event == VIR_DOMAIN_EVENT_SHUTDOWN) { + job->status = CIM_JOB_STATE_COMPLETED; + CU_DEBUG("state_change_shutdown_cb(): Shutdown complete"); + } +} + +static CMPI_THREAD_RETURN state_change_thread(void *data) +{ + CMPIStatus s; + state_change_job_t *job = (state_change_job_t *) data; + virConnectPtr conn = NULL; + virDomainPtr dom = NULL; + virDomainInfo info; + int job_cb = -1; + + /* Set job state */ + CBAttachThread(_BROKER, job->context); + CU_DEBUG("State change job %s started", job->uuid); + + job->status = CIM_JOB_STATE_STARTING; + CMSetProperty(job->inst, "JobState", + (CMPIValue *) &(job->status), CMPI_uint16); + CMSetProperty(job->inst, "Status", + (CMPIValue *) "Starting", CMPI_chars); + + /* Connect to domain event callback */ + conn = connect_by_classname(_BROKER, CLASSNAME(job->obj_path), &s); + if (conn == NULL) { + CU_DEBUG("Unable to connect to '%s' hypervisor", + CLASSNAME(job->obj_path)); + goto err; + } + + dom = virDomainLookupByName(conn, job->dom_name); + if (dom == NULL) { + CU_DEBUG("Unable to get domain '%s'", job->dom_name); + goto err; + } + + if (virDomainGetInfo(dom, &info) != 0) { + CU_DEBUG("Unable to get domain info for '%s'", job->dom_name); + goto err; + } + + if (job->dom_state == CIM_STATE_REBOOT) { + job_cb = virConnectDomainEventRegisterAny(conn, NULL, + VIR_DOMAIN_EVENT_ID_REBOOT, + VIR_DOMAIN_EVENT_CALLBACK(state_change_reboot_cb), + job, NULL); + + if (job_cb == -1) { + CU_DEBUG("Unable to connect domain reboot callback"); + goto err; + } + + s = state_change_reboot(dom, &info); + + if (s.rc != CMPI_RC_OK) { + CU_DEBUG("Unable to trigger domain reboot: '%s'", + CMGetCharPtr(s.msg)); + goto err; + } + } else if (job->dom_state == CIM_STATE_SHUTDOWN) { + job_cb = virConnectDomainEventRegisterAny(conn, NULL, + VIR_DOMAIN_EVENT_ID_LIFECYCLE, + VIR_DOMAIN_EVENT_CALLBACK(state_change_shutdown_cb), + job, NULL); + + if (job_cb == -1) { + CU_DEBUG("Unable to connect domain shutdown callback"); + goto err; + } + + s = state_change_shutdown(dom, &info); + + if (s.rc != CMPI_RC_OK) { + CU_DEBUG("Unable to trigger domain shutdown: '%s'", + CMGetCharPtr(s.msg)); + goto err; + } + } else { + CU_DEBUG("Unrecognized state '%d'", job->dom_state); + goto err; + } + + job->status = CIM_JOB_STATE_RUNNING; + CMSetProperty(job->inst, "JobState", + (CMPIValue *) &(job->status), CMPI_uint16); + CMSetProperty(job->inst, "Status", + (CMPIValue *) "Running", CMPI_chars); + + CU_DEBUG("Entering mainloop implementation"); + /* Wait for operation (shutdown/reboot) to complete */ + while (job->status == CIM_JOB_STATE_RUNNING) { + if (virEventRunDefaultImpl() < 0) { + virErrorPtr err = virGetLastError(); + CU_DEBUG("Failed to run event loop: %s\n", + err && err->message ? err->message : "Unknown error"); + goto err; + } + } + + CU_DEBUG("Job completed"); + CMSetProperty(job->inst, "JobState", + (CMPIValue *) &(job->status), CMPI_uint16); + CMSetProperty(job->inst, "Status", + (CMPIValue *) "Completed", CMPI_chars); + + goto out; + + err: + job->status = CIM_JOB_STATE_EXCEPTION; + CMSetProperty(job->inst, "JobState", + (CMPIValue *) &(job->status), CMPI_uint16); + CMSetProperty(job->inst, "Status", + (CMPIValue *) "Error", CMPI_chars); + + out: + if (job_cb != -1) + virConnectDomainEventDeregisterAny(conn, job_cb); + + virDomainFree(dom); + virConnectClose(conn); + + CBDetachThread(_BROKER, job->context); + free(job->dom_name); + free(job); + return NULL; +} + static CMPIStatus state_change(CMPIMethodMI *self, const CMPIContext *context, const CMPIResult *results, @@ -1244,7 +1521,8 @@ static CMPIStatus state_change(CMPIMethodMI *self, goto out; }

- /* Retain original instance of the guest to use for the PreviousInstance attribute when generating an indication. */ + /* Retain original instance of the guest to use for the PreviousInstance + attribute when generating an indication. */ s = get_domain_by_name(_BROKER, reference, name, &prev_inst); if (s.rc != CMPI_RC_OK || prev_inst == NULL) { cu_statusf(_BROKER, &s, @@ -1256,8 +1534,36 @@ static CMPIStatus state_change(CMPIMethodMI *self,

s = __state_change(name, state, reference);

+ rc = s.rc; + if (s.rc == CMPI_RC_OK) - rc = 0; + goto out; + + if (s.rc == CIM_SVPC_RETURN_JOB_STARTED) { + state_change_job_t *job = NULL; + + if (events_registered == false) { + events_registered = true; + if (virEventRegisterDefaultImpl() != 0) { + CU_DEBUG("virEventRegisterDefaultImpl() failed"); + goto out; + } + } + + s = create_state_change_job(reference, context, &job, state); + if (s.rc != CMPI_RC_OK) { + rc = s.rc; + free(job); + goto out; + } + + job->dom_name = strdup(name); + + _BROKER->xft->newThread(state_change_thread, job, 0); + + CMAddArg(argsout, "Job", (CMPIValue *)&(job->obj_path), + CMPI_ref); + }

out: CMReturnData(results, &rc, CMPI_uint32);

Okay, fairly complex but that's DMTF style :-) What i don't see is how/where the object associated to the job are freed is there some generic mechanism handling all of this, or are we missing some kind of routine which would be called asynchronously to free the data associated to the job ? I don't know, if you're sure that everything is deallocated then fine, ACK :-) Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/