[Libvirt-cim] [PATCH 4 of 6] Support for resource indication was added to Virt_VirtualSystemManagementService

[Kaitlin Rupert]

> +#define CREATED "ResourceAllocationSettingDataCreatedIndication"
> +#define DELETED "ResourceAllocationSettingDataDeletedIndication"
> +#define MODIFIED "ResourceAllocationSettingDataModifiedIndication"

These are generic names - can you make them more specific to RASD 
indications?

> +        else if (rc == IM_RC_NOT_SUPPORTED)
> +                virt_set_status(_BROKER, &status,
> +                                CMPI_RC_ERR_NOT_FOUND,
> +                                conn,
> +                                "Unable to raise resource indication");

IM_RC_NOT_SUPPORTED is set because a connection to libvirt cannot be 
made.  Can you change the error message here to reflect this?

>          else if (rc == IM_RC_FAILED)
>                  virt_set_status(_BROKER, &status,
>                                  CMPI_RC_ERR_NOT_FOUND,

> @@ -2116,6 +2238,54 @@
>          if (xml != NULL) {
>                  CU_DEBUG("New XML:\n%s", xml);
>                  connect_and_create(xml, ref, &s);
> +
> +                if (func == &resource_add) {
> +                        indication = strdup(CREATED);
> +                }
> +                else if (func == &resource_del) {
> +                        indication = strdup(DELETED);
> +                }
> +                else {
> +                        indication = strdup(MODIFIED);
> +
> +                        s = enum_rasds(_BROKER, 
> +                                       ref, 
> +                                       dominfo->name, 
> +                                       type, 
> +                                       props, 
> +                                       &list);
> +                        if (s.rc != CMPI_RC_OK) {
> +                                CU_DEBUG("Failed to enumerate rasd");
> +                                goto out;
> +                        }
> +
> +                        for(i=0; i < list.cur; i++) {

Space needed between i and =, also between = and 0.

> +                                prev_inst = list.list[i];
> +                                ret = cu_get_str_prop(prev_inst, 
> +                                                      "InstanceID", 
> +                                                      &inst_id);
> +
> +                                if (ret != CMPI_RC_OK) 
> +                                        continue;
> +
> +                                if (STREQ(inst_id, 
> +                                          get_fq_devid(dominfo->name, 
> +                                                       (char *)devid)))
> +                                        break;
> +                        }

Can you break this out into a separate function? There's lots of 
indention here, an this else statement makes the function quite long.

> +
> +                }
> +
> +                inst_list_init(&list);
> +                if (inst_list_add(&list, rasd) == 0) {
> +                        CU_DEBUG("Unable to add RASD instance to the list\n");
> +                        goto out;
> +                }
> +                raise_rasd_indication(context, 
> +                                      indication, 
> +                                      prev_inst, 
> +                                      ref, 
> +                                      &list);
>          } else {
>                  cu_statusf(_BROKER, &s,
>                             CMPI_RC_ERR_FAILED,
> @@ -2125,6 +2295,8 @@
>   out:
>          cleanup_dominfo(&dominfo);
>          free(xml);
> +        free(indication);

Why not make indication a const char *?  Then you wouldn't need to free it.


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