[PATCH v2] Fix AppliedFilterList creation and deletion
From: Chip Vincent <cvincent@us.ibm.com> Fixes many small issues with the current AppliedFilterList provider. 1) Fix Create to properly return a complete object path and fix Delete to properly parse that path. 2) Persist applied filer rules. Since it's not possible to dyanmically update a single device, I've changed the code to modify and re-define the VM to essentially add/remove ACL filter associations. I also updated the code to minimize domain/device parsing overhead. For some strange reason, our internal APIs sometimes take a virDomainPtr and other times a struct domain * forcing providers who work with domains *and* devices to parse everything twice. Until the internal APIs are cleaned up, I simply parse everything once and then fetch the device manually from the struct domain *. 3) Add VIR_DOMAIN_XML_INACTIVE to virDomainGetXML(). By default, libvirt only returns the XML of the running domain. We need to fetch the *stored* XML that will be used for the next boot so that all changes made while the VM is running are preserved. Changes from v1: - Fix leak and other comments - Fix all cases virDomainGetXML() - Fix NestedFilterList Create/Delete instance Signed-off-by: Chip Vincent <cvincent@us.ibm.com> --- libxkutil/device_parsing.c | 6 ++- src/Virt_AppliedFilterList.c | 97 +++++++++++++++++++---------------- src/Virt_ComputerSystem.c | 3 +- src/Virt_ComputerSystemIndication.c | 3 +- src/Virt_FilterList.c | 5 ++- src/Virt_NestedFilterList.c | 13 ++++- src/Virt_VSMigrationService.c | 3 +- 7 files changed, 78 insertions(+), 52 deletions(-) diff --git a/libxkutil/device_parsing.c b/libxkutil/device_parsing.c index a1e8d6c..ff86f2a 100644 --- a/libxkutil/device_parsing.c +++ b/libxkutil/device_parsing.c @@ -996,7 +996,8 @@ int get_devices(virDomainPtr dom, struct virt_device **list, int type) char *xml; int ret; - xml = virDomainGetXMLDesc(dom, VIR_DOMAIN_XML_SECURE); + xml = virDomainGetXMLDesc(dom, + VIR_DOMAIN_XML_INACTIVE | VIR_DOMAIN_XML_SECURE); if (xml == NULL) return 0; @@ -1241,7 +1242,8 @@ int get_dominfo(virDomainPtr dom, struct domain **dominfo) char *xml; int ret; int start; - xml = virDomainGetXMLDesc(dom, VIR_DOMAIN_XML_SECURE); + xml = virDomainGetXMLDesc(dom, + VIR_DOMAIN_XML_INACTIVE | VIR_DOMAIN_XML_SECURE); if (xml == NULL) return 0; diff --git a/src/Virt_AppliedFilterList.c b/src/Virt_AppliedFilterList.c index bc31c14..538adf4 100644 --- a/src/Virt_AppliedFilterList.c +++ b/src/Virt_AppliedFilterList.c @@ -97,67 +97,60 @@ static CMPIrc cu_get_ref_path(const CMPIObjectPath *reference, if ((s.rc != CMPI_RC_OK) || CMIsNullValue(value)) return CMPI_RC_ERR_NO_SUCH_PROPERTY; - /* how to parse and object path? */ + if ((value.type != CMPI_ref) || CMIsNullObject(value.value.ref)) + return CMPI_RC_ERR_TYPE_MISMATCH; + + *_reference = value.value.ref; return CMPI_RC_OK; } -/* TODO: Port to libxkutil/device_parsing.c */ -static int update_device(virDomainPtr dom, - struct virt_device *dev) +static int update_domain(virConnectPtr conn, + struct domain *dominfo) { -#if LIBVIR_VERSION_NUMBER > 8000 char *xml = NULL; - int flags = VIR_DOMAIN_DEVICE_MODIFY_CURRENT | - VIR_DOMAIN_DEVICE_MODIFY_CONFIG; - int ret = 0; + virDomainPtr dom = NULL; - xml = device_to_xml(dev); + xml = system_to_xml(dominfo); if (xml == NULL) { - CU_DEBUG("Failed to get XML for device '%s'", dev->id); + CU_DEBUG("Failed to get XML from domain %s", dominfo->name); goto out; } - if (virDomainUpdateDeviceFlags(dom, xml, flags) != 0) { - CU_DEBUG("Failed to dynamically update device"); + dom = virDomainDefineXML(conn, xml); + if (dom == NULL) { + CU_DEBUG("Failed to update domain %s", dominfo->name); goto out; } - ret = 1; out: free(xml); + virDomainFree(dom); - return ret; -#else return 0; -#endif } -/* TODO: Port to libxkutil/device_parsing.c */ -static int get_device_by_devid(virDomainPtr dom, +static int get_device_by_devid(struct domain *dominfo, const char *devid, - int type, struct virt_device **dev) { - int i, ret = 0; - struct virt_device *devices = NULL; - int count = get_devices(dom, &devices, type); + int i; + struct virt_device *devices = dominfo->dev_net; + int count = dominfo->dev_net_ct; + + if (dev == NULL) + return 0; for (i = 0; i < count; i++) { if (STREQC(devid, devices[i].id)) { CU_DEBUG("Found '%s'", devices[i].id); - *dev = virt_device_dup(&devices[i]); - if (*dev != NULL) - ret = 1; - - break; + *dev = &devices[i]; + return 0; } } - cleanup_virt_devices(&devices, count); - - return ret; + return 1; } /** @@ -425,6 +418,8 @@ static CMPIStatus CreateInstance( struct virt_device *device = NULL; virConnectPtr conn = NULL; virDomainPtr dom = NULL; + struct domain *dominfo = NULL; + CMPIObjectPath *_reference = NULL; conn = connect_by_classname(_BROKER, CLASSNAME(reference), &s); if (conn == NULL) @@ -487,8 +482,12 @@ static CMPIStatus CreateInstance( goto out; } - get_device_by_devid(dom, net_name, CIM_RES_TYPE_NET, &device); - if (device == NULL) { + if (get_dominfo(dom, &dominfo) == 0) { + CU_DEBUG("Failed to get dominfo"); + goto out; + } + + if (get_device_by_devid(dominfo, net_name, &device) != 0) { cu_statusf(_BROKER, &s, CMPI_RC_ERR_FAILED, "Dependent.Name object does not exist"); @@ -502,14 +501,19 @@ static CMPIStatus CreateInstance( device->dev.net.filter_ref = strdup(filter_name); - if (update_device(dom, device) == 0) { + if (update_domain(conn, dominfo) != 0) { cu_statusf(_BROKER, &s, CMPI_RC_ERR_FAILED, - "Failed to update device"); + "Failed to update domain"); goto out; } - CMReturnObjectPath(results, reference); + /* create new object path */ + _reference = CMClone(reference, NULL); + CMAddKey(_reference, "Antecedent", (CMPIValue *)&antecedent, CMPI_ref); + CMAddKey(_reference, "Dependent", (CMPIValue *)&dependent, CMPI_ref); + + CMReturnObjectPath(results, _reference); CU_DEBUG("CreateInstance complete"); out: @@ -542,6 +546,7 @@ static CMPIStatus DeleteInstance( struct virt_device *device = NULL; virConnectPtr conn = NULL; virDomainPtr dom = NULL; + struct domain *dominfo = NULL; conn = connect_by_classname(_BROKER, CLASSNAME(reference), &s); if (conn == NULL) @@ -557,7 +562,7 @@ static CMPIStatus DeleteInstance( goto out; } - if (cu_get_str_path(reference, "DeviceID", + if (cu_get_str_path(antecedent, "DeviceID", &device_name) != CMPI_RC_OK) { cu_statusf(_BROKER, &s, CMPI_RC_ERR_FAILED, @@ -573,7 +578,7 @@ static CMPIStatus DeleteInstance( goto out; } - if (cu_get_str_path(reference, "Name", + if (cu_get_str_path(dependent, "Name", &filter_name) != CMPI_RC_OK) { cu_statusf(_BROKER, &s, CMPI_RC_ERR_FAILED, @@ -585,7 +590,7 @@ static CMPIStatus DeleteInstance( if (filter == NULL) { cu_statusf(_BROKER, &s, CMPI_RC_ERR_FAILED, - "Antecedent.Name object does not exist"); + "Dependent.Name object does not exist"); goto out; } @@ -600,11 +605,15 @@ static CMPIStatus DeleteInstance( goto out; } - get_device_by_devid(dom, net_name, CIM_RES_TYPE_NET, &device); - if (device == NULL) { + if (get_dominfo(dom, &dominfo) == 0) { + CU_DEBUG("Failed to get dominfo"); + goto out; + } + + if (get_device_by_devid(dominfo, net_name, &device) != 0) { cu_statusf(_BROKER, &s, CMPI_RC_ERR_FAILED, - "Dependent.Name object does not exist"); + "Antecedent.Name object does not exist"); goto out; } @@ -613,14 +622,14 @@ static CMPIStatus DeleteInstance( device->dev.net.filter_ref = NULL; } - if (update_device(dom, device) == 0) { + if (update_domain(conn, dominfo) != 0) { cu_statusf(_BROKER, &s, CMPI_RC_ERR_FAILED, - "Failed to update device"); + "Failed to update domain"); goto out; } - CU_DEBUG("CreateInstance complete"); + CU_DEBUG("DeleteInstance complete"); out: free(domain_name); diff --git a/src/Virt_ComputerSystem.c b/src/Virt_ComputerSystem.c index 582253a..e6c7e55 100644 --- a/src/Virt_ComputerSystem.c +++ b/src/Virt_ComputerSystem.c @@ -926,7 +926,8 @@ static CMPIStatus domain_reset(virDomainPtr dom) return s; } - xml = virDomainGetXMLDesc(dom, VIR_DOMAIN_XML_SECURE); + xml = virDomainGetXMLDesc(dom, + VIR_DOMAIN_XML_INACTIVE |VIR_DOMAIN_XML_SECURE); if (xml == NULL) { CU_DEBUG("Unable to retrieve domain XML"); virt_set_status(_BROKER, &s, diff --git a/src/Virt_ComputerSystemIndication.c b/src/Virt_ComputerSystemIndication.c index eb1a71c..6ef2ddc 100644 --- a/src/Virt_ComputerSystemIndication.c +++ b/src/Virt_ComputerSystemIndication.c @@ -107,7 +107,8 @@ static int csi_dom_xml_set(csi_dom_xml_t *dom, virDomainPtr dom_ptr, CMPIStatus dom->name = strdup(name); /* xml */ - dom->xml = virDomainGetXMLDesc(dom_ptr, VIR_DOMAIN_XML_SECURE); + dom->xml = virDomainGetXMLDesc(dom_ptr, + VIR_DOMAIN_XML_INACTIVE | VIR_DOMAIN_XML_SECURE); if (dom->xml == NULL) { cu_statusf(_BROKER, s, CMPI_RC_ERR_FAILED, diff --git a/src/Virt_FilterList.c b/src/Virt_FilterList.c index 35d18a9..5b1b6e8 100644 --- a/src/Virt_FilterList.c +++ b/src/Virt_FilterList.c @@ -358,7 +358,10 @@ static CMPIStatus DeleteInstance( goto out; } - delete_filter(conn, filter); + if (delete_filter(conn, filter) != 0) { + CU_DEBUG("Failed to delete filter %s", filter->name); + goto out; + } out: cleanup_filters(&filter, 1); diff --git a/src/Virt_NestedFilterList.c b/src/Virt_NestedFilterList.c index 894cd7c..b72c582 100644 --- a/src/Virt_NestedFilterList.c +++ b/src/Virt_NestedFilterList.c @@ -98,7 +98,10 @@ static CMPIrc cu_get_ref_path(const CMPIObjectPath *reference, if ((s.rc != CMPI_RC_OK) || CMIsNullValue(value)) return CMPI_RC_ERR_NO_SUCH_PROPERTY; - /* how to parse and object path? */ + if ((value.type != CMPI_ref) || CMIsNullObject(value.value.ref)) + return CMPI_RC_ERR_TYPE_MISMATCH; + + *_reference = value.value.ref; return CMPI_RC_OK; } @@ -305,6 +308,7 @@ static CMPIStatus CreateInstance( const char *child_name = NULL; struct acl_filter *child_filter = NULL; virConnectPtr conn = NULL; + CMPIObjectPath *_reference = NULL; CU_DEBUG("Reference = %s", REF2STR(reference)); @@ -383,6 +387,11 @@ static CMPIStatus CreateInstance( goto out; } + /* create new object path */ + _reference = CMClone(reference, NULL); + CMAddKey(_reference, "Antecedent", (CMPIValue *)&antecedent, CMPI_ref); + CMAddKey(_reference, "Dependent", (CMPIValue *)&dependent, CMPI_ref); + CMReturnObjectPath(results, reference); CU_DEBUG("CreateInstance completed"); @@ -475,7 +484,7 @@ static CMPIStatus DeleteInstance( goto out; } - CU_DEBUG("CreateInstance completed"); + CU_DEBUG("DeleteInstance completed"); out: cleanup_filters(&parent_filter, 1); diff --git a/src/Virt_VSMigrationService.c b/src/Virt_VSMigrationService.c index d393787..76e3d25 100644 --- a/src/Virt_VSMigrationService.c +++ b/src/Virt_VSMigrationService.c @@ -1070,7 +1070,8 @@ static CMPIStatus prepare_migrate(virDomainPtr dom, { CMPIStatus s = {CMPI_RC_OK, NULL}; - *xml = virDomainGetXMLDesc(dom, VIR_DOMAIN_XML_SECURE); + *xml = virDomainGetXMLDesc(dom, + VIR_DOMAIN_XML_INACTIVE | VIR_DOMAIN_XML_SECURE); if (*xml == NULL) { virt_set_status(_BROKER, &s, -- 1.7.1
On 01/26/2012 12:58 PM, Chip Vincent wrote:
From: Chip Vincent <cvincent@us.ibm.com>
Fixes many small issues with the current AppliedFilterList provider.
1) Fix Create to properly return a complete object path and fix Delete to properly parse that path.
2) Persist applied filer rules. Since it's not possible to dyanmically update a single device, I've changed the code to modify and re-define the VM to essentially add/remove ACL filter associations.
I also updated the code to minimize domain/device parsing overhead. For some strange reason, our internal APIs sometimes take a virDomainPtr and other times a struct domain * forcing providers who work with domains *and* devices to parse everything twice. Until the internal APIs are cleaned up, I simply parse everything once and then fetch the device manually from the struct domain *.
3) Add VIR_DOMAIN_XML_INACTIVE to virDomainGetXML(). By default, libvirt only returns the XML of the running domain. We need to fetch the *stored* XML that will be used for the next boot so that all changes made while the VM is running are preserved.
Changes from v1: - Fix leak and other comments - Fix all cases virDomainGetXML() - Fix NestedFilterList Create/Delete instance
Almost there, see below: [snip]
diff --git a/src/Virt_NestedFilterList.c b/src/Virt_NestedFilterList.c index 894cd7c..b72c582 100644 --- a/src/Virt_NestedFilterList.c +++ b/src/Virt_NestedFilterList.c @@ -98,7 +98,10 @@ static CMPIrc cu_get_ref_path(const CMPIObjectPath *reference, if ((s.rc != CMPI_RC_OK) || CMIsNullValue(value)) return CMPI_RC_ERR_NO_SUCH_PROPERTY;
- /* how to parse and object path? */ + if ((value.type != CMPI_ref) || CMIsNullObject(value.value.ref)) + return CMPI_RC_ERR_TYPE_MISMATCH; + + *_reference = value.value.ref;
return CMPI_RC_OK; } @@ -305,6 +308,7 @@ static CMPIStatus CreateInstance( const char *child_name = NULL; struct acl_filter *child_filter = NULL; virConnectPtr conn = NULL; + CMPIObjectPath *_reference = NULL;
CU_DEBUG("Reference = %s", REF2STR(reference));
@@ -383,6 +387,11 @@ static CMPIStatus CreateInstance( goto out; }
+ /* create new object path */ + _reference = CMClone(reference, NULL); + CMAddKey(_reference, "Antecedent", (CMPIValue *)&antecedent, CMPI_ref); + CMAddKey(_reference, "Dependent", (CMPIValue *)&dependent, CMPI_ref); + CMReturnObjectPath(results, reference);
Should be returning _reference right? Another question, is it necessary to free the _reference variable somehow? Looking at Pegasus headers, I can see a CMRelease declaration near CMClone. -- Eduardo de Barros Lima Software Engineer, Open Virtualization Linux Technology Center - IBM/Brazil eblima@br.ibm.com
On 01/26/2012 11:08 AM, Eduardo Lima (Etrunko) wrote:
On 01/26/2012 12:58 PM, Chip Vincent wrote:
From: Chip Vincent<cvincent@us.ibm.com>
Fixes many small issues with the current AppliedFilterList provider.
1) Fix Create to properly return a complete object path and fix Delete to properly parse that path.
2) Persist applied filer rules. Since it's not possible to dyanmically update a single device, I've changed the code to modify and re-define the VM to essentially add/remove ACL filter associations.
I also updated the code to minimize domain/device parsing overhead. For some strange reason, our internal APIs sometimes take a virDomainPtr and other times a struct domain * forcing providers who work with domains *and* devices to parse everything twice. Until the internal APIs are cleaned up, I simply parse everything once and then fetch the device manually from the struct domain *.
3) Add VIR_DOMAIN_XML_INACTIVE to virDomainGetXML(). By default, libvirt only returns the XML of the running domain. We need to fetch the *stored* XML that will be used for the next boot so that all changes made while the VM is running are preserved.
Changes from v1: - Fix leak and other comments - Fix all cases virDomainGetXML() - Fix NestedFilterList Create/Delete instance Almost there, see below:
[snip]
diff --git a/src/Virt_NestedFilterList.c b/src/Virt_NestedFilterList.c index 894cd7c..b72c582 100644 --- a/src/Virt_NestedFilterList.c +++ b/src/Virt_NestedFilterList.c @@ -98,7 +98,10 @@ static CMPIrc cu_get_ref_path(const CMPIObjectPath *reference, if ((s.rc != CMPI_RC_OK) || CMIsNullValue(value)) return CMPI_RC_ERR_NO_SUCH_PROPERTY;
- /* how to parse and object path? */ + if ((value.type != CMPI_ref) || CMIsNullObject(value.value.ref)) + return CMPI_RC_ERR_TYPE_MISMATCH; + + *_reference = value.value.ref;
return CMPI_RC_OK; } @@ -305,6 +308,7 @@ static CMPIStatus CreateInstance( const char *child_name = NULL; struct acl_filter *child_filter = NULL; virConnectPtr conn = NULL; + CMPIObjectPath *_reference = NULL;
CU_DEBUG("Reference = %s", REF2STR(reference));
@@ -383,6 +387,11 @@ static CMPIStatus CreateInstance( goto out; }
+ /* create new object path */ + _reference = CMClone(reference, NULL); + CMAddKey(_reference, "Antecedent", (CMPIValue *)&antecedent, CMPI_ref); + CMAddKey(_reference, "Dependent", (CMPIValue *)&dependent, CMPI_ref); + CMReturnObjectPath(results, reference); Should be returning _reference right? Good catch! Will fix Another question, is it necessary to free the _reference variable somehow? Looking at Pegasus headers, I can see a CMRelease declaration near CMClone.
No. All CMPI objects are released once the request thread is completed. CMRelease() is exposed for symmetry and also for long running threads, like jobs or indication providers. -- Chip Vincent Open Virtualization IBM Linux Technology Center cvincent@linux.vnet.ibm.com
participants (2)
-
Chip Vincent -
Eduardo Lima (Etrunko)