From: Chip Vincent <cvincent(a)us.ibm.com&gt; 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 v3: - Fix NestedFilterList DeleteInstance - Fix remove_filter_ref() in acl_parsing.c (intend to refactor in future patch) Changes from v2: - Return the correct reference in NestedFilterList Changes from v1: - Fix leak and other comments - Fix all cases virDomainGetXML() - Fix NestedFilterList Create/Delete instance Signed-off-by: Chip Vincent <cvincent(a)us.ibm.com&gt; --- libxkutil/acl_parsing.c | 1 + 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 | 19 +++++-- src/Virt_VSMigrationService.c | 3 +- 8 files changed, 82 insertions(+), 55 deletions(-) diff --git a/libxkutil/acl_parsing.c b/libxkutil/acl_parsing.c index 5b6d7bb..9c4b4b2 100644 --- a/libxkutil/acl_parsing.c +++ b/libxkutil/acl_parsing.c @@ -652,6 +652,7 @@ int remove_filter_ref(struct acl_filter *filter, const char *name) /* TODO: called infrequently, but needs optimization */ old_refs = filter->refs; + filter->ref_ct = 0; for (i = 0; i < filter->ref_ct; i++) { if (STREQC(old_refs[i], name)) { 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..81c4408 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,7 +387,12 @@ static CMPIStatus CreateInstance( 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 completed"); out: @@ -423,7 +432,7 @@ static CMPIStatus DeleteInstance( goto out; } - if (cu_get_str_path(reference, "Name", &parent_name) != CMPI_RC_OK) { + if (cu_get_str_path(antecedent, "Name", &parent_name) != CMPI_RC_OK) { cu_statusf(_BROKER, &s, CMPI_RC_ERR_FAILED, "Unable to get Antecedent.Name property"); @@ -446,7 +455,7 @@ static CMPIStatus DeleteInstance( goto out; } - if (cu_get_str_path(reference, "Name", &child_name) != CMPI_RC_OK) { + if (cu_get_str_path(dependent, "Name", &child_name) != CMPI_RC_OK) { cu_statusf(_BROKER, &s, CMPI_RC_ERR_FAILED, "Unable to get Dependent.Name property"); @@ -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