visual code review looks good not applied and tested the patch yet. -Sharad On Tue, 2012-03-06 at 14:55 -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, +};

#endif

Visual code inspection looks good. Need to apply and test the patch. -Sharad On Tue, 2012-03-06 at 14:55 -0300, Eduardo Lima (Etrunko) wrote:

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

For Shutdown, the RequestStateChange method returns immediately with 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 | 236 +++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 238 insertions(+), 7 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..778809d 100644 --- a/src/Virt_ComputerSystem.c +++ b/src/Virt_ComputerSystem.c @@ -30,23 +30,37 @@ #include <cmpift.h> #include <cmpimacs.h>

+#include <uuid.h> #include <libvirt/libvirt.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; + char *dom_name; + uint16_t dom_state; + uint16_t status; /* job status */ +}; + /* Set the "Name" property of an instance from a domain */ static int set_name_from_dom(virDomainPtr dom, CMPIInstance *instance) { @@ -1190,7 +1204,7 @@ static CMPIStatus __state_change(const char *name, else if (state == CIM_STATE_DISABLED) s = state_change_disable(dom, &info); else if (state == CIM_STATE_SHUTDOWN) - s = state_change_shutdown(dom, &info); + s.rc = CIM_SVPC_RETURN_JOB_STARTED; else if (state == CIM_STATE_PAUSED) s = state_change_pause(dom, &info); else if (state == CIM_STATE_REBOOT) @@ -1202,6 +1216,9 @@ static CMPIStatus __state_change(const char *name, 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 +1232,191 @@ 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_STARTING; + + 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 *) "Starting", 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); + + out: + free(type); + free(cn); + free(ns); + return s; +} + +static CMPI_THREAD_RETURN state_change_thread(void *data) +{ + CMPIStatus s; + CMPIInstance *inst = NULL; + state_change_job_t *job = (state_change_job_t *) data; + virConnectPtr conn = NULL; + virDomainPtr dom = NULL; + virDomainInfo info; + + if (job->dom_state != CIM_STATE_SHUTDOWN) { + CU_DEBUG("Unrecognized state '%d'", job->dom_state); + goto end; + } + + /* Set job state */ + CBAttachThread(_BROKER, job->context); + CU_DEBUG("State change job %s started", job->uuid); + job->status = CIM_JOB_STATE_RUNNING; + + inst = CBGetInstance(_BROKER, job->context, job->obj_path, NULL, &s); + if ((inst == NULL) || (s.rc != CMPI_RC_OK)) { + CU_DEBUG("Failed to get job instance (%i)", s.rc); + return NULL; + } + + CMSetProperty(inst, "JobState", + (CMPIValue *)&(job->status), CMPI_uint16); + CMSetProperty(inst, "Status", + (CMPIValue *) "Running", 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 out; + } + + dom = virDomainLookupByName(conn, job->dom_name); + if (dom == NULL) { + CU_DEBUG("Unable to get domain '%s'", job->dom_name); + goto out; + } + + if (virDomainGetInfo(dom, &info) != 0) { + CU_DEBUG("Unable to get domain info for '%s'", job->dom_name); + goto out; + } + + s = state_change_shutdown(dom, &info); + if (s.rc != CMPI_RC_OK) { + CU_DEBUG("Unable to trigger domain shutdown: '%s'", + CMGetCharPtr(s.msg)); + goto out; + } + + /* Wait for operation (shutdown/reboot) to complete */ + while (info.state != VIR_DOMAIN_SHUTOFF) { + usleep(100 * 1000); + virDomainGetInfo(dom, &info); + } + + CU_DEBUG("Job completed"); + + /* Set job state */ + if (job->status == CIM_JOB_STATE_COMPLETED) { + CMSetProperty(inst, "JobState", + (CMPIValue *)&(job->status), CMPI_uint16); + CMSetProperty(inst, "Status", + (CMPIValue *) "Completed", CMPI_chars); + } + + out: + virDomainFree(dom); + virConnectClose(conn); + + CBDetachThread(_BROKER, job->context); + free(job->dom_name); + free(job); + + end: + return NULL; +} + static CMPIStatus state_change(CMPIMethodMI *self, const CMPIContext *context, const CMPIResult *results, @@ -1244,7 +1446,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 +1459,27 @@ static CMPIStatus state_change(CMPIMethodMI *self,

s = __state_change(name, state, reference);

- if (s.rc == CMPI_RC_OK) + if (s.rc == CMPI_RC_OK) { rc = 0; + goto out; + } + + if (s.rc == CIM_SVPC_RETURN_JOB_STARTED) { + state_change_job_t *job = NULL; + s = create_state_change_job(reference, context, &job, state); + if (s.rc != CMPI_RC_OK) { + 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); + rc = s.rc; + }

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

Visual inspection looks good. Need to apply and test -Sharad On Tue, 2012-03-06 at 14:55 -0300, Eduardo Lima (Etrunko) wrote:

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

As it happens for Shutdown, the RequestStateChange method returns immediately with return code 0 (successful) even though the state change is still not completed.

Change the current job implementation to support reboot as well. For this we need the libvirt domain events APIs and event loop implementation, which are only available in libvirt version 0.9.0 or newer.

Signed-off-by: Eduardo Lima (Etrunko) <eblima@br.ibm.com> --- src/Virt_ComputerSystem.c | 90 ++++++++++++++++++++++++++++++++++++++------- 1 files changed, 76 insertions(+), 14 deletions(-)

diff --git a/src/Virt_ComputerSystem.c b/src/Virt_ComputerSystem.c index 778809d..04584f3 100644 --- a/src/Virt_ComputerSystem.c +++ b/src/Virt_ComputerSystem.c @@ -32,6 +32,7 @@

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

#include <libcmpiutil/libcmpiutil.h> #include <libcmpiutil/std_invokemethod.h> @@ -61,6 +62,8 @@ struct _state_change_job { 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) { @@ -1203,14 +1206,12 @@ 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.rc = CIM_SVPC_RETURN_JOB_STARTED; 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, @@ -1333,6 +1334,25 @@ static CMPIStatus create_state_change_job(const CMPIObjectPath *ref, 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; +} + +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; +} + static CMPI_THREAD_RETURN state_change_thread(void *data) { CMPIStatus s; @@ -1341,8 +1361,10 @@ static CMPI_THREAD_RETURN state_change_thread(void *data) virConnectPtr conn = NULL; virDomainPtr dom = NULL; virDomainInfo info; + int job_cb = -1;

- if (job->dom_state != CIM_STATE_SHUTDOWN) { + if (job->dom_state != CIM_STATE_SHUTDOWN && + job->dom_state != CIM_STATE_REBOOT) { CU_DEBUG("Unrecognized state '%d'", job->dom_state); goto end; } @@ -1382,17 +1404,56 @@ static CMPI_THREAD_RETURN state_change_thread(void *data) goto out; }

- s = state_change_shutdown(dom, &info); - if (s.rc != CMPI_RC_OK) { - CU_DEBUG("Unable to trigger domain shutdown: '%s'", - CMGetCharPtr(s.msg)); - goto out; + if (events_registered == false) { + events_registered = true; + virEventRegisterDefaultImpl(); + } + + 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 out; + } + + s = state_change_reboot(dom, &info); + + if (s.rc != CMPI_RC_OK) { + CU_DEBUG("Unable to trigger domain reboot: '%s'", + CMGetCharPtr(s.msg)); + goto out; + } + } 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 out; + } + + s = state_change_shutdown(dom, &info); + + if (s.rc != CMPI_RC_OK) { + CU_DEBUG("Unable to trigger domain shutdown: '%s'", + CMGetCharPtr(s.msg)); + goto out; + } }

/* Wait for operation (shutdown/reboot) to complete */ - while (info.state != VIR_DOMAIN_SHUTOFF) { - usleep(100 * 1000); - virDomainGetInfo(dom, &info); + 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"); + } }

CU_DEBUG("Job completed"); @@ -1405,6 +1466,8 @@ static CMPI_THREAD_RETURN state_change_thread(void *data) (CMPIValue *) "Completed", CMPI_chars); }

+ virConnectDomainEventDeregisterAny(conn, job_cb); + out: virDomainFree(dom); virConnectClose(conn); @@ -1412,7 +1475,6 @@ static CMPI_THREAD_RETURN state_change_thread(void *data) CBDetachThread(_BROKER, job->context); free(job->dom_name); free(job); - end: return NULL; }