[Libvirt-cim] [PATCH] Fix AppliedFilterList creation and deletion
Eduardo Lima (Etrunko)
eblima at linux.vnet.ibm.com
Thu Jan 26 14:03:07 UTC 2012
On 01/25/2012 04:24 PM, Chip Vincent wrote:
> From: Chip Vincent <cvincent at 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.
Nice, I noticed this could be also done for the NestedFilterList
instance creation.
>
> 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.
>
Is this necessary in other places as well?? A grep for
virDomainGetXMLDesc in the tree shows some more places where this
function is called.
> Signed-off-by: Chip Vincent <cvincent at us.ibm.com>
> ---
> libxkutil/device_parsing.c | 3 +-
> src/Virt_AppliedFilterList.c | 99 +++++++++++++++++++++++-------------------
> src/Virt_FilterList.c | 5 ++-
> 3 files changed, 60 insertions(+), 47 deletions(-)
>
> diff --git a/libxkutil/device_parsing.c b/libxkutil/device_parsing.c
> index a1e8d6c..238aa72 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;
>
Check if this is also necessary in other places where
virDomainGetXMLDesc is called, as stated in the comment above.
> diff --git a/src/Virt_AppliedFilterList.c b/src/Virt_AppliedFilterList.c
> index bc31c14..892a90c 100644
> --- a/src/Virt_AppliedFilterList.c
> +++ b/src/Virt_AppliedFilterList.c
> @@ -97,66 +97,59 @@ 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);
> - goto out;
> + CU_DEBUG("Failed to get XML from domain %s", dominfo->name);
> + return 1;
> }
>
> - if (virDomainUpdateDeviceFlags(dom, xml, flags) != 0) {
> - CU_DEBUG("Failed to dynamically update device");
> - goto out;
> + dom = virDomainDefineXML(conn, xml);
> + if (dom == NULL) {
> + CU_DEBUG("Failed to update domain %s", dominfo->name);
> + return 1;
> }
>
> - ret = 1;
> - out:
> - free(xml);
> + virDomainFree(dom);
>
Leaking xml.
> - 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, ret = 1;
> + struct virt_device *devices = dominfo->dev_net;
> + int count = dominfo->dev_net_ct;
> +
> + if (dev == NULL)
> + return ret;
>
> 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;
> + *dev = &devices[i];
> + ret = 0;
>
Just return 0 here instead as you are not doing anything else after the
loop. In this case you can also get rid of the variable ret.
> break;
> }
> }
>
> - cleanup_virt_devices(&devices, count);
> -
> return ret;
> }
>
> @@ -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);
So this could go to NestedFilterList as well?
> 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");
> + "Antedent.Name object does not exist");
Spell check.
> 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_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);
--
Eduardo de Barros Lima
Software Engineer, Open Virtualization
Linux Technology Center - IBM/Brazil
eblima at br.ibm.com
More information about the Libvirt-cim
mailing list