3:44 p.m.
# 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.
Signed-off-by: Jay Gagnon <grendel(a)linux.vnet.ibm.com>
diff -r 2806f8744946 -r 741b757ad68c libxkutil/device_parsing.c
--- a/libxkutil/device_parsing.c Wed Apr 16 17:50:49 2008 -0700
+++ b/libxkutil/device_parsing.c Wed Apr 23 16:44:23 2008 -0400
@@ -337,7 +337,6 @@ static int parse_vcpu_device(xmlNode *no
struct virt_device *list = NULL;
char *count_str;
int count;
- int i;
count_str = get_node_content(node);
if (count_str == NULL)
@@ -347,24 +346,15 @@ static int parse_vcpu_device(xmlNode *no
free(count_str);
- list = calloc(count, sizeof(*list));
+ list = calloc(1, sizeof(*list));
if (list == NULL)
goto err;
-
- for (i = 0; i < count; i++) {
- struct virt_device *vdev = &list[i];
- struct vcpu_device *cdev = &vdev->dev.vcpu;
-
- cdev->number = i;
-
- vdev->type = CIM_RES_TYPE_PROC;
- if (asprintf(&vdev->id, "%i", i) == -1)
- vdev->id = NULL;
- }
+
+ list->dev.vcpu.quantity = count;
*vdevs = list;
- return count;
+ return 1;
err:
free(list);
@@ -620,7 +610,7 @@ struct virt_device *virt_device_dup(stru
dev->dev.mem.size = _dev->dev.mem.size;
dev->dev.mem.maxsize = _dev->dev.mem.maxsize;
} else if (dev->type == CIM_RES_TYPE_PROC) {
- dev->dev.vcpu.number = _dev->dev.vcpu.number;
+ dev->dev.vcpu.quantity = _dev->dev.vcpu.quantity;
} else if (dev->type == CIM_RES_TYPE_EMU) {
DUP_FIELD(dev, _dev, dev.emu.path);
} else if (dev->type == CIM_RES_TYPE_GRAPHICS) {
@@ -672,6 +662,32 @@ static int _get_mem_device(const char *x
return 1;
}
+static int _get_proc_device(const char *xml, struct virt_device **list)
+{
+ struct virt_device *proc_devs = NULL;
+ struct virt_device *proc_dev = NULL;
+ int ret;
+
+ ret = parse_devices(xml, &proc_devs, CIM_RES_TYPE_PROC);
+ if (ret <= 0)
+ return ret;
+
+ proc_dev = malloc(sizeof(*proc_dev));
+ if (proc_dev == NULL)
+ return 0;
+
+ memset(proc_dev, 0, sizeof(*proc_dev));
+
+ proc_dev->type = CIM_RES_TYPE_PROC;
+ proc_dev->id = strdup("proc");
+ proc_dev->dev.vcpu.quantity = proc_devs[0].dev.vcpu.quantity;
+ *list = proc_dev;
+
+ cleanup_virt_devices(&proc_devs, ret);
+
+ return 1;
+};
+
int get_devices(virDomainPtr dom, struct virt_device **list, int type)
{
char *xml;
@@ -683,6 +699,8 @@ int get_devices(virDomainPtr dom, struct
if (type == CIM_RES_TYPE_MEM)
ret = _get_mem_device(xml, list);
+ else if (type == CIM_RES_TYPE_PROC)
+ ret = _get_proc_device(xml, list);
else
ret = parse_devices(xml, list, type);
diff -r 2806f8744946 -r 741b757ad68c libxkutil/device_parsing.h
--- a/libxkutil/device_parsing.h Wed Apr 16 17:50:49 2008 -0700
+++ b/libxkutil/device_parsing.h Wed Apr 23 16:44:23 2008 -0400
@@ -50,7 +50,7 @@ struct mem_device {
};
struct vcpu_device {
- uint64_t number;
+ uint64_t quantity;
};
struct emu_device {
diff -r 2806f8744946 -r 741b757ad68c src/Virt_Device.c
--- a/src/Virt_Device.c Wed Apr 16 17:50:49 2008 -0700
+++ b/src/Virt_Device.c Wed Apr 23 16:44:23 2008 -0400
@@ -174,23 +174,6 @@ static CMPIInstance *mem_instance(const
return inst;
}
-static CMPIInstance *vcpu_instance(const CMPIBroker *broker,
- struct vcpu_device *dev,
- const virDomainPtr dom,
- const char *ns)
-{
- CMPIInstance *inst;
- virConnectPtr conn;
-
- conn = virDomainGetConnect(dom);
- inst = get_typed_instance(broker,
- pfx_from_conn(conn),
- "Processor",
- ns);
-
- return inst;
-}
-
static int device_set_devid(CMPIInstance *instance,
struct virt_device *dev,
const virDomainPtr dom)
@@ -229,43 +212,114 @@ static int device_set_systemname(CMPIIns
return 1;
}
-static CMPIInstance *device_instance(const CMPIBroker *broker,
- struct virt_device *dev,
- const virDomainPtr dom,
- const char *ns)
-{
- CMPIInstance *instance;
-
- 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)
- instance = vcpu_instance(broker,
- &dev->dev.vcpu,
- dom,
- ns);
- else
- return NULL;
-
- if (!instance)
- return NULL;
-
- device_set_devid(instance, dev, dom);
- device_set_systemname(instance, dom);
-
- return instance;
+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;
+ 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);
+ 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)
+ 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");
+
+ 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);
*_inst = instance;
diff -r 2806f8744946 -r 741b757ad68c src/Virt_RASD.c
--- a/src/Virt_RASD.c Wed Apr 16 17:50:49 2008 -0700
+++ b/src/Virt_RASD.c Wed Apr 23 16:44:23 2008 -0400
@@ -173,10 +173,8 @@ static CMPIInstance *rasd_from_vdev(cons
CMSetProperty(inst, "Limit",
(CMPIValue *)&dev->dev.mem.maxsize, CMPI_uint64);
} else if (dev->type == CIM_RES_TYPE_PROC) {
- /* This would be the place to set the virtualquantity. */
- uint64_t quantity = dev->dev.vcpu.number + 1;
CMSetProperty(inst, "VirtualQuantity",
- (CMPIValue *)&quantity, CMPI_uint64);
+ (CMPIValue *)&dev->dev.vcpu.quantity, CMPI_uint64);
}
/* FIXME: Put the HostResource in place */
10:02 a.m.
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
+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(a)linux.vnet.ibm.com
10:41 a.m.
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
11:53 a.m.
JG> That was my original assumption as well, but the asprintf() man
JG> page says: "If memory allocation wasn’t possible, or some other
JG> error occurs, these functions will return -1, and the contents of
JG> strp is undefined."
JG> That "undefined" bit made me all paranoid and I really hate
JG> segfaults so I figured better safe than sorry.
...but you're not passing dev_id to the asprintf()... :)
--
Dan Smith
IBM Linux Technology Center
Open Hypervisor Team
email: danms(a)us.ibm.com
1:39 p.m.
Dan Smith wrote:
JG> That was my original assumption as well, but the asprintf()
man
JG> page says: "If memory allocation wasn’t possible, or some other
JG> error occurs, these functions will return -1, and the contents of
JG> strp is undefined."
JG> That "undefined" bit made me all paranoid and I really hate
JG> segfaults so I figured better safe than sorry.
...but you're not passing dev_id to the asprintf()... :)
Sigh...
--
-Jay
10:57 a.m.
JG> +static bool device_instances(const CMPIBroker *broker,
JG> + struct virt_device *devs,
JG> + int count,
JG> + const virDomainPtr dom,
JG> + const char *ns,
JG> + struct inst_list *list)
JG> +{
JG> + int i;
JG> + bool ret;
JG> + uint64_t proc_count = 0;
JG> + bool proc_found = false;
JG> + CMPIInstance *instance = NULL;
JG> +
JG> + for (i = 0; i < count; i++) {
JG> + struct virt_device *dev = &devs[i];
JG> +
JG> + if (dev->type == CIM_RES_TYPE_NET)
JG> + instance = net_instance(broker,
JG> + &dev->dev.net,
JG> + dom,
JG> + ns);
JG> + else if (dev->type == CIM_RES_TYPE_DISK)
JG> + instance = disk_instance(broker,
JG> + &dev->dev.disk,
JG> + dom,
JG> + ns);
JG> + else if (dev->type == CIM_RES_TYPE_MEM)
JG> + instance = mem_instance(broker,
JG> + &dev->dev.mem,
JG> + dom,
JG> + ns);
JG> + else if (dev->type == CIM_RES_TYPE_PROC) {
JG> + proc_found = true;
JG> + proc_count = dev->dev.vcpu.quantity;
JG> + continue;
JG> + } else
JG> + return false;
I don't think you need proc_found.
JG> +
JG> + if (!instance)
JG> + return false;
JG> +
JG> + device_set_devid(instance, dev, dom);
JG> + device_set_systemname(instance, dom);
JG> + inst_list_add(list, instance);
JG> + }
JG> +
JG> + if (proc_count) {
This should be:
if (proc_count > 0) {
JG> + ret = vcpu_instances(broker,
JG> + dom,
JG> + ns,
JG> + proc_count,
JG> + list);
JG> + }
JG> +
JG> + return true;
JG> }
Also, a quick comment in here about why the processor case is so
strange would be good. We know why because we've been over it too
many times, but for future generations, this might be a bit hard to
figure out :)
--
Dan Smith
IBM Linux Technology Center
Open Hypervisor Team
email: danms(a)us.ibm.com
8:12 a.m.
Dan Smith wrote:
I don't think you need proc_found.
You're right. That was silly of me.
JG> +
JG> + if (!instance)
JG> + return false;
JG> +
JG> + device_set_devid(instance, dev, dom);
JG> + device_set_systemname(instance, dom);
JG> + inst_list_add(list, instance);
JG> + }
JG> +
JG> + if (proc_count) {
This should be:
if (proc_count > 0) {
Fair enough.
JG> + ret = vcpu_instances(broker,
JG> + dom,
JG> + ns,
JG> + proc_count,
JG> + list);
JG> + }
JG> +
JG> + return true;
JG> }
Also, a quick comment in here about why the processor case is so
strange would be good. We know why because we've been over it too
many times, but for future generations, this might be a bit hard to
figure out :)
Can do. I'll try my best to not make it sound too pathetic. :)
--
-Jay
6082
days inactive
6087
days old
6 comments
3 participants
participants (3)
-
Dan Smith -
Jay Gagnon -
Kaitlin Rupert