Re: [Libvirt-cim] [PATCH 1 of 3] Added support to convert data in the internal format to the format used by the BootDevices property

3 Jun 2009

...
diff -r aa8e071730d2 -r abc90cae6c08 src/Virt_VSSD.c

--- a/src/Virt_VSSD.c	Wed May 20 10:41:46 2009 -0700
+++ b/src/Virt_VSSD.c	Mon Jun 01 18:19:38 2009 -0300
@@ -38,20 +38,79 @@
const static CMPIBroker *_BROKER;
-static void _set_fv_prop(struct domain *dominfo,
-                         CMPIInstance *inst)
+static CMPIStatus _set_fv_prop(struct domain *dominfo,
+                               CMPIInstance *inst)
 {
         bool fv = true;
+        CMPIArray *array;
+        CMPICount bl_ct;
+        CMPIStatus s = {CMPI_RC_OK, NULL};
if (dominfo->type == DOMAIN_XENFV)
                 CMSetProperty(inst, "IsFullVirt",
                               (CMPIValue *)&fv, CMPI_boolean);
-        if (dominfo->os_info.fv.boot != NULL)
-                CMSetProperty(inst,
-                              "BootDevice",
-                              (CMPIValue *)dominfo->os_info.fv.boot,
-                              CMPI_chars);
+        bl_ct = dominfo->os_info.fv.bootlist_ct;
+        if (bl_ct > 0) {
If you want to avoid placing everything within an if, you could do the 
following instead:

if (bl_ct <= 0)
         return s;

That will save you on some indention.
...
+                CMPICount i;
+                CMPIStatus s;
CMPIStatus is already defined at the beginning of the function.
...
+
+                CU_DEBUG("bootlist_ct = %d", bl_ct);
+
+                array = CMNewArray(_BROKER,
You'll need to modify _set_fv_prop() so that is takes a CMPIBroker as an 
argument.  This means instance_from_dom() will also need to take a broker.

The reason for this is is because of the following call chain:

  get_vssd_by_name() --> _get_vssd() --> instance_from_dom() --> 
_set_fv_prop()

get_vssd_by_name() is not a static function; it is called by association 
providers, and those association providers need to be able to pass in 
their own CMPIBroker.
...
+                                   bl_ct,
+                                   CMPI_string,
+                                   &s);
+
+                if (s.rc != CMPI_RC_OK) {
+                        CU_DEBUG("Error creating BootDevice list");
Call cu_statusf() here - the will give you a more specific error message.
...
+                        goto error;
Instead of error, we generally use out instead of error.
...
+                }
+
+                for (i = 0; i < bl_ct; i++) {
+                        CMPIString *cm_str;
+
+                        CU_DEBUG("BootList[%u]=%s",
+                                 i,
+                                 dominfo->os_info.fv.bootlist[i]);
+
+                        cm_str = CMNewString(_BROKER,
+                                             (const char *)dominfo->os_info.
+                                             fv.bootlist[i],
+                                             &s);
+                        if (s.rc != CMPI_RC_OK) {
+                                CU_DEBUG("Error creating CMPIString");
+                                goto error;
Call cu_statusf() here.
...
+                        }
+
+                        s = CMSetArrayElementAt(array,
+                                                i,
+                                                (CMPIValue *)&cm_str,
+                                                CMPI_string);
+                        if (s.rc != CMPI_RC_OK) {
+                                CU_DEBUG("Error setting BootDevice array "
+                                         "element");
+                                goto error;
Call cu_statusf() here.
...
+                        }
+                }
+
+                s = CMSetProperty(inst,
+                                  "BootDevices",
+                                  (CMPIValue *)&array,
+                                  CMPI_stringA);
+
+                if (s.rc != CMPI_RC_OK) {
+                        CU_DEBUG("Error setting BootDevices property");
+                        goto error;
Call cu_statusf() here.
...
+                }
+        }
+        return s;
If you remove the cu_statusf() call below, then you won't need this 
return here.
...
+
+ error:
Instead of using error, use out here.
...
+        cu_statusf(_BROKER, &s, 
+                   CMPI_RC_ERR_FAILED, 
+                   "Unable to set BootDevices property");
Remove this call, and call cu_statusf() whenever a failure occurs - that 
will give you more specific failure messages.
...
+        return s;
 }
static void _set_pv_prop(struct domain *dominfo,
@@ -104,6 +163,7 @@
         char *pfx = NULL;
         char *vsid = NULL;
         int ret = 1;
+        CMPIStatus s = {CMPI_RC_OK, NULL};
         CMPIObjectPath *op;
         struct domain *dominfo = NULL;
@@ -158,7 +218,12 @@
if ((dominfo->type == DOMAIN_XENFV) ||
             (dominfo->type == DOMAIN_KVM) || (dominfo->type == DOMAIN_QEMU))
-                _set_fv_prop(dominfo, inst);
+                s = _set_fv_prop(dominfo, inst);
+                if (s.rc != CMPI_RC_OK) {
+                        ret = 0;
+                        goto out;
There's several points of failure in _set_fv_prop(), so I would change 
instance_from_dom() so that is returns a CMPIStatus instead of a int. 
And in _get_vssd(), you can just call something like:

s = instance_from_dom();

That way, the error code and error message will already be set.


-- 
Kaitlin Rupert
IBM Linux Technology Center
kaitlin@linux.vnet.ibm.com

    