[PATCH 0 of 3] #3 - Add configurable enum_devices() to Device provider and Adopt interface changes to SD & EAFP
This patch set implements the configurable enum_devices() interface to the Device provider. This slight interface change is adopted to the associations SystemDevice and ElementAllocatedFromPool. Both associations profit from the easy access to the device instances. Diff to patch set 1: - fixed style issues Diff to patch set 2: - moved adoption of new resource types CIM_RES_TYPE_foo into separate patch set
# HG changeset patch # User Heidi Eckhart <heidieck@linux.vnet.ibm.com> # Date 1204892341 -3600 # Node ID 3afbff37cc3946a1f584fff572717f75aa8facc2 # Parent d09896378f5dc5dfd70054a9c514682a23cd4fd3 Device: rename dom_device to enum_devices and make it configurable - update resource type to CIM_RES_TYPE_foo Signed-off-by: Heidi Eckhart <heidieck@linux.vnet.ibm.com> diff -r d09896378f5d -r 3afbff37cc39 src/Virt_Device.c --- a/src/Virt_Device.c Fri Mar 07 13:19:00 2008 +0100 +++ b/src/Virt_Device.c Fri Mar 07 13:19:01 2008 +0100 @@ -236,22 +236,22 @@ static CMPIInstance *device_instance(con { CMPIInstance *instance; - if (dev->type == VIRT_DEV_NET) + if (dev->type == CIM_RES_TYPE_NET) instance = net_instance(broker, &dev->dev.net, dom, ns); - else if (dev->type == VIRT_DEV_DISK) + else if (dev->type == CIM_RES_TYPE_DISK) instance = disk_instance(broker, &dev->dev.disk, dom, ns); - else if (dev->type == VIRT_DEV_MEM) + else if (dev->type == CIM_RES_TYPE_MEM) instance = mem_instance(broker, &dev->dev.mem, dom, ns); - else if (dev->type == VIRT_DEV_VCPU) + else if (dev->type == CIM_RES_TYPE_PROC) instance = vcpu_instance(broker, &dev->dev.vcpu, dom, @@ -271,23 +271,24 @@ uint16_t device_type_from_classname(cons uint16_t device_type_from_classname(const char *classname) { if (strstr(classname, "NetworkPort")) - return VIRT_DEV_NET; + return CIM_RES_TYPE_NET; else if (strstr(classname, "LogicalDisk")) - return VIRT_DEV_DISK; + return CIM_RES_TYPE_DISK; else if (strstr(classname, "Memory")) - return VIRT_DEV_MEM; + return CIM_RES_TYPE_MEM; else if (strstr(classname, "Processor")) - return VIRT_DEV_VCPU; + return CIM_RES_TYPE_PROC; else - return VIRT_DEV_UNKNOWN; -} - -int dom_devices(const CMPIBroker *broker, - virDomainPtr dom, - const char *ns, - int type, - struct inst_list *list) -{ + return CIM_RES_TYPE_UNKNOWN; +} + +static CMPIStatus _get_devices(const CMPIBroker *broker, + const CMPIObjectPath *reference, + const virDomainPtr dom, + const uint16_t type, + struct inst_list *list) +{ + CMPIStatus s = {CMPI_RC_OK, NULL}; int count; int i; struct virt_device *devs = NULL; @@ -299,7 +300,10 @@ int dom_devices(const CMPIBroker *broker for (i = 0; i < count; i++) { CMPIInstance *dev = NULL; - dev = device_instance(broker, &devs[i], dom, ns); + dev = device_instance(broker, + &devs[i], + dom, + NAMESPACE(reference)); if (dev) inst_list_add(list, dev); @@ -308,57 +312,106 @@ int dom_devices(const CMPIBroker *broker out: free(devs); - - return 1; -} - -static int dom_list_devices(virConnectPtr conn, - const CMPIObjectPath *ref, - struct inst_list *list) -{ - virDomainPtr *doms; - int ndom; + return s; +} + +static CMPIStatus _enum_devices(const CMPIBroker *broker, + const CMPIObjectPath *reference, + const virDomainPtr dom, + const uint16_t type, + struct inst_list *list) +{ + CMPIStatus s; + + if (type == CIM_RES_TYPE_ALL) { + s = _get_devices(broker, + reference, + dom, + CIM_RES_TYPE_PROC, + list); + s = _get_devices(broker, + reference, + dom, + CIM_RES_TYPE_MEM, + list); + s = _get_devices(broker, + reference, + dom, + CIM_RES_TYPE_NET, + list); + s = _get_devices(broker, + reference, + dom, + CIM_RES_TYPE_DISK, + list); + } + else + s = _get_devices(broker, + reference, + dom, + type, + list); + + return s; +} + +CMPIStatus enum_devices(const CMPIBroker *broker, + const CMPIObjectPath *reference, + const char *domain, + const uint16_t type, + struct inst_list *list) +{ + CMPIStatus s = {CMPI_RC_OK, NULL}; + virConnectPtr conn = NULL; + virDomainPtr *doms = NULL; + int count = 1; int i; - int type; - - type = device_type_from_classname(CLASSNAME(ref)); - - ndom = get_domain_list(conn, &doms); - if (ndom == 0) - return 1; - else if (ndom < 0) - return 0; - - for (i = 0; i < ndom; i++) { - dom_devices(_BROKER, doms[i], NAMESPACE(ref), type, list); - } - - return 1; -} - -static CMPIStatus enum_devices(const CMPIObjectPath *reference, - const CMPIResult *results, - int names_only) -{ - CMPIStatus s; - virConnectPtr conn; + + conn = connect_by_classname(broker, CLASSNAME(reference), &s); + if (conn == NULL) + goto out; + + if (domain) { + doms = calloc(1, sizeof(virDomainPtr)); + doms[0] = virDomainLookupByName(conn, domain); + } + else + count = get_domain_list(conn, &doms); + + for (i = 0; i < count; i++) { + s = _enum_devices(broker, + reference, + doms[i], + type, + list); + + virDomainFree(doms[i]); + } + + out: + virConnectClose(conn); + free(doms); + + return s; +} + +static CMPIStatus return_enum_devices(const CMPIObjectPath *reference, + const CMPIResult *results, + int names_only) +{ + CMPIStatus s = {CMPI_RC_OK, NULL}; struct inst_list list; if (!provider_is_responsible(_BROKER, reference, &s)) - return s; + goto out; inst_list_init(&list); - conn = connect_by_classname(_BROKER, CLASSNAME(reference), &s); - if (conn == NULL) - return s; - - if (!dom_list_devices(conn, reference, &list)) { - cu_statusf(_BROKER, &s, - CMPI_RC_ERR_FAILED, - "Failed to list domains"); - return s; - } + s = enum_devices(_BROKER, reference, NULL, + device_type_from_classname(CLASSNAME(reference)), + &list); + if (s.rc != CMPI_RC_OK) + goto out; if (list.cur == 0) goto out; @@ -371,11 +424,6 @@ static CMPIStatus enum_devices(const CMP inst_list_free(&list); out: - cu_statusf(_BROKER, &s, - CMPI_RC_OK, - ""); - virConnectClose(conn); - return s; } @@ -524,7 +572,7 @@ static CMPIStatus EnumInstanceNames(CMPI const CMPIResult *results, const CMPIObjectPath *reference) { - return enum_devices(reference, results, 1); + return return_enum_devices(reference, results, true); } static CMPIStatus EnumInstances(CMPIInstanceMI *self, @@ -533,7 +581,7 @@ static CMPIStatus EnumInstances(CMPIInst const CMPIObjectPath *reference, const char **properties) { - return enum_devices(reference, results, 0); + return return_enum_devices(reference, results, false); } static CMPIStatus GetInstance(CMPIInstanceMI *self, diff -r d09896378f5d -r 3afbff37cc39 src/Virt_Device.h --- a/src/Virt_Device.h Fri Mar 07 13:19:00 2008 +0100 +++ b/src/Virt_Device.h Fri Mar 07 13:19:01 2008 +0100 @@ -27,19 +27,18 @@ * Return a list of devices for a given domain * * @param broker A pointer to the CIM broker - * @param dom The domain in question - * @param ref The namespace - * @param list A pointer to an array of CMPIInstance objects (should - * be NULL initially) - * @param cur The number of items in the list (0 initially) - * @param max The size of the list (0 initially) - * @returns Nonzero on success + * @param reference Defines the libvirt connection to use + * @param domain The domain id (NULL means for all domains) + * @param type The device type or CIM_RES_TYPE_ALL to get + * all devices + * @param list A pointer to an array of CMPIInstance objects + * (should be NULL initially) */ -int dom_devices(const CMPIBroker *broker, - virDomainPtr dom, - const char *ns, - int type, - struct inst_list *list); +CMPIStatus enum_devices(const CMPIBroker *broker, + const CMPIObjectPath *reference, + const char *domain, + const uint16_t type, + struct inst_list *list); /** * Returns the device instance defined by the reference
+static CMPIStatus _enum_devices(const CMPIBroker *broker, + const CMPIObjectPath *reference, + const virDomainPtr dom, + const uint16_t type, + struct inst_list *list) +{ + CMPIStatus s; + + if (type == CIM_RES_TYPE_ALL) { + s = _get_devices(broker, + reference, + dom, + CIM_RES_TYPE_PROC, + list); + s = _get_devices(broker, + reference, + dom, + CIM_RES_TYPE_MEM, + list); + s = _get_devices(broker, + reference, + dom, + CIM_RES_TYPE_NET, + list); + s = _get_devices(broker, + reference, + dom, + CIM_RES_TYPE_DISK, + list); + }
Would it be better to have a list of all the resource types, and then call _get_devices() in a loop? This is probably fine as is, and I don't suspect we'll add more resource types in the future, so it's probably not worth worrying about. Thoughts?
+ +static CMPIStatus return_enum_devices(const CMPIObjectPath *reference, + const CMPIResult *results, + int names_only) +{ + CMPIStatus s = {CMPI_RC_OK, NULL}; struct inst_list list;
if (!provider_is_responsible(_BROKER, reference, &s)) - return s; + goto out;
inst_list_init(&list);
- conn = connect_by_classname(_BROKER, CLASSNAME(reference), &s); - if (conn == NULL) - return s; - - if (!dom_list_devices(conn, reference, &list)) { - cu_statusf(_BROKER, &s, - CMPI_RC_ERR_FAILED, - "Failed to list domains"); - return s; - } + s = enum_devices(_BROKER, reference, NULL, + device_type_from_classname(CLASSNAME(reference)), + &list); + if (s.rc != CMPI_RC_OK) + goto out;
if (list.cur == 0) goto out;
Should inst_list_free(&list); go after the out:? Even if list.cur == 0, we still probably want to set list->list = NULL to be sure. -- Kaitlin Rupert IBM Linux Technology Center kaitlin@linux.vnet.ibm.com
Would it be better to have a list of all the resource types, and then call _get_devices() in a loop? This is probably fine as is, and I don't suspect we'll add more resource types in the future, so it's probably not worth worrying about.
Thoughts? Good idea. I will update svpc_types with an array for all resource types and use this one to loop over here. I will send this out as a separate
Kaitlin Rupert wrote: patch set.
Should inst_list_free(&list); go after the out:?
Even if list.cur == 0, we still probably want to set list->list = NULL to be sure.
I had a look into libcmpiutil and its possible to remove this check (list.cur==0) as cu_return_instance_names() and cu_return_instances() take care of this case. Thanks for making me aware of this. -- Regards Heidi Eckhart Software Engineer IBM Linux Technology Center - Open Hypervisor
HE> + s = enum_devices(_BROKER, reference, NULL, HE> + device_type_from_classname(CLASSNAME(reference)), HE> + &list); This should be one argument per line. -- Dan Smith IBM Linux Technology Center Open Hypervisor Team email: danms@us.ibm.com
# HG changeset patch # User Heidi Eckhart <heidieck@linux.vnet.ibm.com> # Date 1204892341 -3600 # Node ID 366b317afbf382f85ed75b86b6df9eae30dd6c94 # Parent 3afbff37cc3946a1f584fff572717f75aa8facc2 SD: adopt Device interface changes - update resource type to CIM_RES_TYPE_foo Signed-off-by: Heidi Eckhart <heidieck@linux.vnet.ibm.com> diff -r 3afbff37cc39 -r 366b317afbf3 src/Virt_SystemDevice.c --- a/src/Virt_SystemDevice.c Fri Mar 07 13:19:01 2008 +0100 +++ b/src/Virt_SystemDevice.c Fri Mar 07 13:19:01 2008 +0100 @@ -48,55 +48,6 @@ const static CMPIBroker *_BROKER; -#define DEV_TYPE_COUNT 4 -const static int device_types[DEV_TYPE_COUNT] = - {VIRT_DEV_NET, - VIRT_DEV_DISK, - VIRT_DEV_MEM, - VIRT_DEV_VCPU, - }; - -static int get_dom_devices(const char *name, - struct inst_list *list, - int type, - const char *host_cn, - const char *ns) -{ - virConnectPtr conn = NULL; - virDomainPtr dom = NULL; - CMPIStatus s; - int ret = 0; - - conn = connect_by_classname(_BROKER, host_cn, &s); - if (conn == NULL) - goto out; - - dom = virDomainLookupByName(conn, name); - if (dom == NULL) - goto out; - - ret = dom_devices(_BROKER, dom, ns, type, list); - - virDomainFree(dom); - out: - virConnectClose(conn); - - return ret; -} - -static int get_all_devices(const char *name, - struct inst_list *list, - const char *host_cn, - const char *ns) -{ - int i; - - for (i = 0; i < DEV_TYPE_COUNT; i++) - get_dom_devices(name, list, device_types[i], host_cn, ns); - - return i; -} - static CMPIStatus sys_to_dev(const CMPIObjectPath *ref, struct std_assoc_info *info, struct inst_list *list) @@ -104,7 +55,6 @@ static CMPIStatus sys_to_dev(const CMPIO const char *host = NULL; CMPIStatus s = {CMPI_RC_OK, NULL}; CMPIInstance *inst = NULL; - int ret; if (!match_hypervisor_prefix(ref, info)) return s; @@ -120,20 +70,7 @@ static CMPIStatus sys_to_dev(const CMPIO goto out; } - ret = get_all_devices(host, - list, - CLASSNAME(ref), - NAMESPACE(ref)); - - if (ret >= 0) { - cu_statusf(_BROKER, &s, - CMPI_RC_OK, - ""); - } else { - cu_statusf(_BROKER, &s, - CMPI_RC_ERR_FAILED, - "Unable to get devices"); - } + s = enum_devices(_BROKER, ref, host, CIM_RES_TYPE_ALL, list); out: return s;
# HG changeset patch # User Heidi Eckhart <heidieck@linux.vnet.ibm.com> # Date 1204892342 -3600 # Node ID 50f47a071fb1d4668a5eb1b3e0c649f87ce8f376 # Parent 366b317afbf382f85ed75b86b6df9eae30dd6c94 EAFP: adopt Device interface changes Signed-off-by: Heidi Eckhart <heidieck@linux.vnet.ibm.com> diff -r 366b317afbf3 -r 50f47a071fb1 src/Virt_ElementAllocatedFromPool.c --- a/src/Virt_ElementAllocatedFromPool.c Fri Mar 07 13:19:01 2008 +0100 +++ b/src/Virt_ElementAllocatedFromPool.c Fri Mar 07 13:19:02 2008 +0100 @@ -77,7 +77,7 @@ static CMPIStatus vdev_to_pool(const CMP goto out; type = class_to_type(ref); - if (type == 0) { + if (type == CIM_RES_TYPE_UNKNOWN) { cu_statusf(_BROKER, &s, CMPI_RC_ERR_FAILED, "Unknown device type"); @@ -138,47 +138,6 @@ static int filter_by_pool(struct inst_li return dest->cur; } -static int devs_from_pool(uint16_t type, - const CMPIObjectPath *ref, - const char *poolid, - struct inst_list *list) -{ - CMPIStatus s; - virConnectPtr conn = NULL; - virDomainPtr *doms = NULL; - int count; - int i; - const char *ns = NAMESPACE(ref); - const char *cn = CLASSNAME(ref); - - conn = connect_by_classname(_BROKER, cn, &s); - if (conn == NULL) - return 0; - - count = get_domain_list(conn, &doms); - - for (i = 0; i < count; i++) { - const char *name; - struct inst_list tmp; - - inst_list_init(&tmp); - - name = virDomainGetName(doms[i]); - - dom_devices(_BROKER, doms[i], ns, type, &tmp); - - filter_by_pool(list, &tmp, type, poolid); - - inst_list_free(&tmp); - virDomainFree(doms[i]); - } - - free(doms); - virConnectClose(conn); - - return count; -} - static CMPIStatus pool_to_vdev(const CMPIObjectPath *ref, struct std_assoc_info *info, struct inst_list *list) @@ -186,7 +145,8 @@ static CMPIStatus pool_to_vdev(const CMP const char *poolid; CMPIStatus s = {CMPI_RC_OK, NULL}; uint16_t type; - CMPIInstance *inst; + CMPIInstance *inst = NULL; + struct inst_list tmp; if (!match_hypervisor_prefix(ref, info)) return s; @@ -210,7 +170,15 @@ static CMPIStatus pool_to_vdev(const CMP goto out; } - devs_from_pool(type, ref, poolid, list); + inst_list_init(&tmp); + + s = enum_devices(_BROKER, ref, NULL, type, &tmp); + if (s.rc != CMPI_RC_OK) + goto out; + + filter_by_pool(list, &tmp, type, poolid); + + inst_list_free(&tmp); out: return s;
HE> # HG changeset patch HE> # User Heidi Eckhart <heidieck@linux.vnet.ibm.com> HE> # Date 1204892342 -3600 HE> # Node ID 50f47a071fb1d4668a5eb1b3e0c649f87ce8f376 HE> # Parent 366b317afbf382f85ed75b86b6df9eae30dd6c94 HE> EAFP: adopt Device interface changes This patch is throwing a reject for me: src/Virt_ElementAllocatedFromPool.c Hunk #1 succeeded at 73 with fuzz 1 (offset -4 lines). Hunk #2 FAILED at 134. Hunk #4 succeeded at 168 with fuzz 2 (offset -2 lines). 1 out of 4 hunks FAILED -- saving rejects to file src/Virt_ElementAllocatedFromPool.c.rej The actual rejected hunk is: *************** *** 134,180 **** return dest->cur; } - static int devs_from_pool(uint16_t type, - const CMPIObjectPath *ref, - const char *poolid, - struct inst_list *list) - { - CMPIStatus s; - virConnectPtr conn = NULL; - virDomainPtr *doms = NULL; - int count; - int i; - const char *ns = NAMESPACE(ref); - const char *cn = CLASSNAME(ref); - - conn = connect_by_classname(_BROKER, cn, &s); - if (conn == NULL) - return 0; - - count = get_domain_list(conn, &doms); - - for (i = 0; i < count; i++) { - const char *name; - struct inst_list tmp; - - inst_list_init(&tmp); - - name = virDomainGetName(doms[i]); - - dom_devices(_BROKER, doms[i], ns, type, &tmp); - - filter_by_pool(list, &tmp, type, poolid); - - inst_list_free(&tmp); - virDomainFree(doms[i]); - } - - free(doms); - virConnectClose(conn); - - return count; - } - static CMPIStatus pool_to_vdev(const CMPIObjectPath *ref, struct std_assoc_info *info, struct inst_list *list) --- 134,139 ---- return dest->cur; } static CMPIStatus pool_to_vdev(const CMPIObjectPath *ref, struct std_assoc_info *info, struct inst_list *list) I applied this after this set: Update resource types to CIM_RES_TYPE_foo and Adopt changes to affected providers Did I mess up the order? Thanks! -- Dan Smith IBM Linux Technology Center Open Hypervisor Team email: danms@us.ibm.com
I applied this after this set:
Update resource types to CIM_RES_TYPE_foo and Adopt changes to affected providers
Did I mess up the order? Thanks!
This applied alright for me. Here's the order I applied in: [PATCH 0 of 7] Update resource types to CIM_RES_TYPE_foo and Adopt changes to affected providers [PATCH 0 of 6] #3 - Adopt get_() common look and feel to DevicePool and Adopted interface changes to assocs [PATCH 0 of 3] #3 - Add configurable enum_devices() to Device provider and Adopt interface changes to SD & EAFP Sounds like the same order though. -- Kaitlin Rupert IBM Linux Technology Center kaitlin@linux.vnet.ibm.com
KR> This applied alright for me. Here's the order I applied in: KR> [PATCH 0 of 7] Update resource types to CIM_RES_TYPE_foo and Adopt KR> changes to affected providers KR> [PATCH 0 of 6] #3 - Adopt get_() common look and feel to DevicePool KR> and Adopted interface changes to assocs KR> [PATCH 0 of 3] #3 - Add configurable enum_devices() to Device provider KR> and Adopt interface changes to SD & EAFP Okay, I was missing the 6-set in the middle, as it showed up much later in my mailer. Thanks! -- Dan Smith IBM Linux Technology Center Open Hypervisor Team email: danms@us.ibm.com
participants (3)
-
Dan Smith -
Heidi Eckhart -
Kaitlin Rupert