Re: [Libvirt-cim] [PATCH] [RFC] ProcessorRASD, the class that won't go away
Kaitlin Rupert wrote:
Jay Gagnon wrote:
> # HG changeset patch
> # User Jay Gagnon <grendel(a)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