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

[Jay Gagnon]

Kaitlin Rupert wrote:
> 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
Ugh.
>
>
>> +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.
That was my original assumption as well, but the asprintf() man page says:
"If memory allocation wasn’t possible, or some other error occurs, these 
functions will return -1, and the contents of strp is undefined."

That "undefined" bit made me all paranoid and I really hate segfaults so 
I figured better safe than sorry.

>
>>
>> +
>> + 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.
Yea, at some point in the stack call that needs to turn into a real 
error. Just not sure at the moment if there's a particular point that 
makes more sense or not.
>
>>
>> + if (!instance)
>
> I know this was already like this, but it should be:
>
> if (instance == NULL)
Okay I can fix that.
>
>> 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?
Yea, that was purely a "more important things to do" thing.
>
>> +
>> + 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.
>
Yea, sorry about that; this bit here puts things in a bit of a sad state 
of affairs.


-- 

-Jay