[Libvirt-cim] [PATCH] [RFC] ProcessorRASD, the class that won't go away

[Kaitlin Rupert]

Jay Gagnon wrote:
> # HG changeset patch
> # User Jay Gagnon <grendel at linux.vnet.ibm.com>
> # Date 1208983463 14400
> # Node ID 741b757ad68c5c4b35c5c697acbc121ac88d1ecf
> # Parent  2806f8744946757036f9639d8c8fe9a95f689233
> [RFC] ProcessorRASD, the class that won't go away
> 
> Around and round we go on the merry-go-round of indecision, hopefully for our last ride.  The definitely probably absolutely tentative final plan is that the default representation of processors as virt_device structs will be one per domain, with a quantity.  Virt_Devices will just need to be told how to handle that, and all the RASD stuff will be much happier for it.  And then we will never speak of this again.
> 

=)  Don't forget the corresponding change in xml_parse.c


> +static char *get_vcpu_inst_id(const virDomainPtr dom,
> +                              int proc_num)
> +{
> +        int rc;
> +        char *id_num = NULL;
> +        char *dev_id = NULL;
> +
> +        rc = asprintf(&id_num, "%d", proc_num);
> +        if (rc == -1) {
> +                free(dev_id);
> +                dev_id = NULL;

dev_id should already be NULL, right?  The asprintf() doesn't attempt to 
modify it.

> +                goto out;
> +        }
> +        
> +        dev_id = get_fq_devid((char *)virDomainGetName(dom), id_num);
> +        free(id_num);
> +
> + out:
> +        return dev_id;
> +}                    
> +
> +static bool vcpu_instances(const CMPIBroker *broker,
> +                           const virDomainPtr dom,
> +                           const char *ns,
> +                           uint64_t proc_count,
> +                           struct inst_list *list)
> +{
> +        int i;
> +        char *dev_id;
> +        CMPIInstance *inst;
> +        virConnectPtr conn;
> +
> +        for (i = 0; i < proc_count; i++) {
> +                conn = virDomainGetConnect(dom);
> +                inst = get_typed_instance(broker,
> +                                          pfx_from_conn(conn),
> +                                          "Processor",
> +                                          ns);
> +                if (inst == NULL)
> +                        return false;
> +
> +                dev_id = get_vcpu_inst_id(dom, i);

If get_vcpu_inst_id() returns NULL, then the DeviceID gets set as NULL. 
  The DeviceID is a key, and while it doesn't have to be unique, we do 
use it in a number of places.

Would it make more sense to return an error here?  Returning an instance 
with a NULL DeviceID would make it difficult to call associations on it, 
I think.

> +                CMSetProperty(inst, "DeviceID",
> +                              (CMPIValue *)dev_id, CMPI_chars);
> +                free(dev_id);
> +                
> +                device_set_systemname(inst, dom);
> +                inst_list_add(list, inst);
> +        }
> +
> +        return true;
> +}
> +
> +static bool device_instances(const CMPIBroker *broker,
> +                             struct virt_device *devs,
> +                             int count,
> +                             const virDomainPtr dom,
> +                             const char *ns,
> +                             struct inst_list *list)
> +{
> +        int i;
> +        bool ret;
> +        uint64_t proc_count = 0;
> +        bool proc_found = false;
> +        CMPIInstance *instance = NULL;
> +
> +        for (i = 0; i < count; i++) {
> +                struct virt_device *dev = &devs[i];
> +
> +                if (dev->type == CIM_RES_TYPE_NET)
> +                        instance = net_instance(broker,
> +                                                &dev->dev.net,
> +                                                dom,
> +                                                ns);
> +                else if (dev->type == CIM_RES_TYPE_DISK)
> +                        instance = disk_instance(broker,
> +                                                 &dev->dev.disk,
> +                                                 dom,
> +                                                 ns);
> +                else if (dev->type == CIM_RES_TYPE_MEM)
> +                        instance = mem_instance(broker,
> +                                                &dev->dev.mem,
> +                                                dom,
> +                                                ns);
> +                else if (dev->type == CIM_RES_TYPE_PROC) {
> +                        proc_found = true;
> +                        proc_count = dev->dev.vcpu.quantity;
> +                        continue;
> +                } else
> +                        return false;
> +
> +                if (!instance)

I know this was already like this, but it should be:

if (instance == NULL)

> +                        return false;
> +                
> +                device_set_devid(instance, dev, dom);
> +                device_set_systemname(instance, dom);
> +                inst_list_add(list, instance);
> +        }
> +
> +        if (proc_count) {
> +                ret = vcpu_instances(broker,
> +                                     dom,
> +                                     ns,
> +                                     proc_count,
> +                                     list);
> +        }
> +
> +        return true;
>  }
> 
>  uint16_t res_type_from_device_classname(const char *classname)
> @@ -290,25 +344,24 @@ static CMPIStatus _get_devices(const CMP
>  {
>          CMPIStatus s = {CMPI_RC_OK, NULL};
>          int count;
> -        int i;
> +        bool rc;
>          struct virt_device *devs = NULL;
> 
>          count = get_devices(dom, &devs, type);
>          if (count <= 0)
>                  goto out;
> 
> -        for (i = 0; i < count; i++) {
> -                CMPIInstance *dev = NULL;
> -
> -                dev = device_instance(broker,
> -                                      &devs[i],
> -                                      dom,
> -                                      NAMESPACE(reference));
> -                if (dev)
> -                        inst_list_add(list, dev);
> -
> -                cleanup_virt_device(&devs[i]);
> -        }
> +        rc = device_instances(broker, 
> +                              devs, 
> +                              count,
> +                              dom, 
> +                              NAMESPACE(reference),
> +                              list);
> +
> +        if (!rc)
> +                CU_DEBUG("Problem");

Since this is just an RFC - I'm guessing this is a place holder?  Do you 
plan on returning an error here?

> +
> +        cleanup_virt_devices(&devs, count);
> 
>   out:
>          free(devs);
> @@ -501,10 +554,13 @@ CMPIStatus get_device_by_name(const CMPI
>                  goto err;
>          }
> 
> +#if 0
> +        /* TODO: Handle this one. */
>          instance = device_instance(broker, 
>                                     dev, 
>                                     dom, 
>                                     NAMESPACE(reference));
> +#endif
>          cleanup_virt_device(dev);

Usually I run the test site to test, but this causes the GetInstance() 
call to fail, which impacts a fair number of tests.  But I did some hand 
testing that looked good.


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