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

[Kaitlin Rupert]

> 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 at linux.vnet.ibm.com