[PATCH 0/5] Persistent Device Address Support
This patch set is adding persistent device address support to libvirt-cim. Now it is possible to specify device bus addresses as supported by libvirt when defining or modifying domains. This applies on top of current master but I would recommend to first apply the little/big endian series posted last week. Reviewed-by: Daniel Hansel <daniel.hansel@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Viktor Mihajlovski (5): RASD/schema: Add properties for device address representation libxkutil: Support for device addresses RASD: Support for device address properties VSMS: Support for device addresses VSMS: Improve device cleanup libxkutil/device_parsing.c | 113 +++++++++++++++++++++++++++++ libxkutil/device_parsing.h | 11 +++ libxkutil/xmlgen.c | 28 +++++++ schema/ResourceAllocationSettingData.mof | 12 +++ src/Virt_RASD.c | 73 +++++++++++++++++++ src/Virt_VirtualSystemManagementService.c | 86 +++++++++++++++------- 6 files changed, 295 insertions(+), 28 deletions(-) -- 1.7.9.5
Certain device types are only identifiable uniquely via a device address from the guest's perspective. Adding or removing devices has the potential to change the address of other unrelated devices and impact the guest's operation. Therefore it is less desirable to rely on implicit device address allocation but rather to use persistent device addresses. Depending on the device's bus type (PCI, SCSI, USB, CCW ...) the address specification format can vary, see also the libvirt domain XML documentation. To account for the various formats device addresses are specified with two array properties in the respective RASD classes: AddressProperties and AddressValues. The former contains a list of address property names and the latter the values. E.g., a PCI address is specified by the properties domain, bus, slot and function. Therefore, for a PCI device RASD we could have: AddressProperties = ['type', 'domain','bus', 'slot', 'function'] AddressValues = ['pci', '0x0000', '0x00', '0x01', '0x2'] resulting in a libvirt address element: <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> Initially, we support only disk and network devices for KVM guests. Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> --- schema/ResourceAllocationSettingData.mof | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/schema/ResourceAllocationSettingData.mof b/schema/ResourceAllocationSettingData.mof index ebc4806..bf1fbb6 100644 --- a/schema/ResourceAllocationSettingData.mof +++ b/schema/ResourceAllocationSettingData.mof @@ -67,6 +67,12 @@ class KVM_DiskResourceAllocationSettingData : KVM_ResourceAllocationSettingData [Description ("if device is shareable")] boolean shareable; + + [Description ("Device address property names")] + string AddressProperties[]; + + [Description ("Device address property values")] + string AddressValues[]; }; [Description ("LXC virtual disk configuration"), @@ -167,6 +173,12 @@ class KVM_NetResourceAllocationSettingData : KVM_ResourceAllocationSettingData [Description ("Filter REF")] string FilterRef; + + [Description ("Device address property names")] + string AddressProperties[]; + + [Description ("Device address property values")] + string AddressValues[]; }; [Description ("LXC virtual network configuration"), -- 1.7.9.5
On 10/14/2013 11:29 AM, Viktor Mihajlovski wrote:
Certain device types are only identifiable uniquely via a device address from the guest's perspective. Adding or removing devices has the potential to change the address of other unrelated devices and impact the guest's operation. Therefore it is less desirable to rely on implicit device address allocation but rather to use persistent device addresses.
Depending on the device's bus type (PCI, SCSI, USB, CCW ...) the address specification format can vary, see also the libvirt domain XML documentation. To account for the various formats device addresses are specified with two array properties in the respective RASD classes: AddressProperties and AddressValues. The former contains a list of address property names and the latter the values.
E.g., a PCI address is specified by the properties domain, bus, slot and function. Therefore, for a PCI device RASD we could have:
AddressProperties = ['type', 'domain','bus', 'slot', 'function'] AddressValues = ['pci', '0x0000', '0x00', '0x01', '0x2']
resulting in a libvirt address element:
<address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/>
Initially, we support only disk and network devices for KVM guests.
Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> --- schema/ResourceAllocationSettingData.mof | 12 ++++++++++++ 1 file changed, 12 insertions(+)
ACK John
New data type for device addresses based on the generic "address" element of the libvirt domain XML. Contains XML parsing and generation code for device addresses. Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> --- libxkutil/device_parsing.c | 113 ++++++++++++++++++++++++++++++++++++++++++++ libxkutil/device_parsing.h | 11 +++++ libxkutil/xmlgen.c | 28 +++++++++++ 3 files changed, 152 insertions(+) diff --git a/libxkutil/device_parsing.c b/libxkutil/device_parsing.c index 076bec0..56e39c7 100644 --- a/libxkutil/device_parsing.c +++ b/libxkutil/device_parsing.c @@ -63,6 +63,22 @@ /* Device parse function */ typedef int (*dev_parse_func_t)(xmlNode *, struct virt_device **); +static void cleanup_device_address(struct device_address *addr) +{ + int i; + if (addr == NULL) + return; + + for (i = 0; i < addr->ct; i++) { + free(addr->key[i]); + free(addr->value[i]); + } + + free(addr->key); + free(addr->value); + addr->ct = 0; +} + static void cleanup_disk_device(struct disk_device *dev) { if (dev == NULL) @@ -77,6 +93,7 @@ static void cleanup_disk_device(struct disk_device *dev) free(dev->virtual_dev); free(dev->bus_type); free(dev->access_mode); + cleanup_device_address(&dev->address); } static void cleanup_vsi_device(struct vsi_device *dev) @@ -107,6 +124,7 @@ static void cleanup_net_device(struct net_device *dev) free(dev->filter_ref); free(dev->poolid); cleanup_vsi_device(&dev->vsi); + cleanup_device_address(&dev->address); } static void cleanup_emu_device(struct emu_device *dev) @@ -351,6 +369,67 @@ char *get_node_content(xmlNode *node) return buf; } +int add_device_address_property(struct device_address *devaddr, + const char *key, + const char *value) +{ + char *k = NULL; + char *v = NULL; + char **list = NULL; + + if (key != NULL && value != NULL) { + k = strdup(key); + v = strdup(value); + if (k == NULL || v == NULL) + goto err; + + list = realloc(devaddr->key, sizeof(char*) * (devaddr->ct+1)); + if (list == NULL) + goto err; + devaddr->key = list; + + list = realloc(devaddr->value, sizeof(char*) * (devaddr->ct+1)); + if (list == NULL) + goto err; + devaddr->value = list; + + devaddr->key[devaddr->ct] = k; + devaddr->value[devaddr->ct] = v; + devaddr->ct += 1; + return 1; + } + + err: + free(k); + free(v); + free(list); + return 0; +} + + +static int parse_device_address(xmlNode *anode, struct device_address *devaddr) +{ + xmlAttr *attr = NULL; + char *name = NULL; + char *value = NULL; + + for (attr = anode->properties; attr != NULL; attr = attr->next) { + name = (char*) attr->name; + value = get_attr_value(anode, name); + if (!add_device_address_property(devaddr, name, value)) + goto err; + free(value); + } + + return 1; + + err: + cleanup_device_address(devaddr); + free(value); + + return 0; +} + static int parse_fs_device(xmlNode *dnode, struct virt_device **vdevs) { struct virt_device *vdev = NULL; @@ -386,6 +465,8 @@ static int parse_fs_device(xmlNode *dnode, struct virt_device **vdevs) } } else if (XSTREQ(child->name, "driver")) { ddev->driver_type = get_attr_value(child, "type"); + } else if (XSTREQ(child->name, "address")) { + parse_device_address(child, &ddev->address); } } @@ -459,6 +540,8 @@ static int parse_block_device(xmlNode *dnode, struct virt_device **vdevs) ddev->readonly = true; } else if (XSTREQ(child->name, "shareable")) { ddev->shareable = true; + } else if (XSTREQ(child->name, "address")) { + parse_device_address(child, &ddev->address); } } @@ -598,6 +681,8 @@ static int parse_net_device(xmlNode *inode, struct virt_device **vdevs) ndev->filter_ref = get_attr_value(child, "filter"); } else if (XSTREQ(child->name, "virtualport")) { parse_vsi_device(child, ndev); + } else if (XSTREQ(child->name, "address")) { + parse_device_address(child, &ndev->address); #if LIBVIR_VERSION_NUMBER >= 9000 } else if (XSTREQ(child->name, "bandwidth")) { /* Network QoS bandwidth support */ @@ -1167,6 +1252,32 @@ static int parse_devices(const char *xml, struct virt_device **_list, int type) return count; } +static void duplicate_device_address(struct device_address *to, const struct device_address *from) +{ + int i; + + if (from == NULL || to == NULL || from->ct == 0) + return; + + to->ct = from->ct; + to->key = calloc(from->ct, sizeof(char*)); + to->value = calloc(from->ct, sizeof(char*)); + if (to->key == NULL || to->value == NULL) + goto err; + + for (i = 0; i < from->ct; i++) { + to->key[i] = strdup(from->key[i]); + to->value[i] = strdup(from->value[i]); + if (to->key[i] == NULL || to->value[i] == NULL) + goto err; + } + + return; + + err: + cleanup_device_address(to); +} + struct virt_device *virt_device_dup(struct virt_device *_dev) { struct virt_device *dev; @@ -1196,6 +1307,7 @@ struct virt_device *virt_device_dup(struct virt_device *_dev) DUP_FIELD(dev, _dev, dev.net.vsi.profile_id); dev->dev.net.reservation = _dev->dev.net.reservation; dev->dev.net.limit = _dev->dev.net.limit; + duplicate_device_address(&dev->dev.net.address, &_dev->dev.net.address); } else if (dev->type == CIM_RES_TYPE_DISK) { DUP_FIELD(dev, _dev, dev.disk.type); DUP_FIELD(dev, _dev, dev.disk.device); @@ -1209,6 +1321,7 @@ struct virt_device *virt_device_dup(struct virt_device *_dev) dev->dev.disk.disk_type = _dev->dev.disk.disk_type; dev->dev.disk.readonly = _dev->dev.disk.readonly; dev->dev.disk.shareable = _dev->dev.disk.shareable; + duplicate_device_address(&dev->dev.disk.address, &_dev->dev.disk.address); } else if (dev->type == CIM_RES_TYPE_MEM) { dev->dev.mem.size = _dev->dev.mem.size; dev->dev.mem.maxsize = _dev->dev.mem.maxsize; diff --git a/libxkutil/device_parsing.h b/libxkutil/device_parsing.h index 4beac5c..92427c1 100644 --- a/libxkutil/device_parsing.h +++ b/libxkutil/device_parsing.h @@ -33,6 +33,12 @@ #include "../src/svpc_types.h" +struct device_address { + uint32_t ct; + char **key; + char **value; +}; + struct vsi_device { char *vsi_type; char *manager_id; @@ -56,6 +62,7 @@ struct disk_device { char *bus_type; char *cache; char *access_mode; /* access modes for DISK_FS (filesystem) type */ + struct device_address address; }; struct net_device { @@ -70,6 +77,7 @@ struct net_device { uint64_t reservation; uint64_t limit; struct vsi_device vsi; + struct device_address address; }; struct mem_device { @@ -257,6 +265,9 @@ int get_devices(virDomainPtr dom, struct virt_device **list, int type, void cleanup_virt_device(struct virt_device *dev); void cleanup_virt_devices(struct virt_device **devs, int count); +int add_device_address_property(struct device_address *devaddr, + const char *key, const char *value); + char *get_node_content(xmlNode *node); char *get_attr_value(xmlNode *node, char *attrname); diff --git a/libxkutil/xmlgen.c b/libxkutil/xmlgen.c index 7e8801d..40e2905 100644 --- a/libxkutil/xmlgen.c +++ b/libxkutil/xmlgen.c @@ -178,6 +178,26 @@ static const char *console_xml(xmlNodePtr root, struct domain *dominfo) BAD_CAST cdev->target_type); } } + + return NULL; +} + +static char *device_address_xml(xmlNodePtr root, struct device_address *addr) +{ + int i; + xmlNodePtr address; + + if (addr == NULL || addr->ct == 0) + return NULL; + + address = xmlNewChild(root, NULL, BAD_CAST "address", NULL); + if (address == NULL) + return XML_ERROR; + + for (i = 0; i < addr->ct; i++) { + xmlNewProp(address, BAD_CAST addr->key[i] , BAD_CAST addr->value[i]); + } + return NULL; } @@ -225,6 +245,9 @@ static char *disk_block_xml(xmlNodePtr root, struct disk_device *dev) if (dev->shareable) xmlNewChild(disk, NULL, BAD_CAST "shareable", NULL); + if (dev->address.ct > 0) + return device_address_xml(disk, &dev->address); + return NULL; } @@ -279,6 +302,8 @@ static const char *disk_file_xml(xmlNodePtr root, struct disk_device *dev) if (dev->shareable) xmlNewChild(disk, NULL, BAD_CAST "shareable", NULL); + if (dev->address.ct > 0) + return device_address_xml(disk, &dev->address); return NULL; } @@ -520,6 +545,9 @@ static const char *net_xml(xmlNodePtr root, struct domain *dominfo) } #endif + if (dev->dev.net.address.ct > 0) + msg = device_address_xml(nic, &dev->dev.net.address); + if (STREQ(dev->dev.net.type, "network")) { msg = set_net_source(nic, net, "network"); } else if (STREQ(dev->dev.net.type, "bridge")) { -- 1.7.9.5
On 10/14/2013 11:29 AM, Viktor Mihajlovski wrote:
New data type for device addresses based on the generic "address" element of the libvirt domain XML. Contains XML parsing and generation code for device addresses.
Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> --- libxkutil/device_parsing.c | 113 ++++++++++++++++++++++++++++++++++++++++++++ libxkutil/device_parsing.h | 11 +++++ libxkutil/xmlgen.c | 28 +++++++++++ 3 files changed, 152 insertions(+)
diff --git a/libxkutil/device_parsing.c b/libxkutil/device_parsing.c index 076bec0..56e39c7 100644 --- a/libxkutil/device_parsing.c +++ b/libxkutil/device_parsing.c @@ -63,6 +63,22 @@ /* Device parse function */ typedef int (*dev_parse_func_t)(xmlNode *, struct virt_device **);
+static void cleanup_device_address(struct device_address *addr) +{ + int i; + if (addr == NULL) + return; + + for (i = 0; i < addr->ct; i++) { + free(addr->key[i]); + free(addr->value[i]); + } + + free(addr->key); + free(addr->value); + addr->ct = 0; +} + static void cleanup_disk_device(struct disk_device *dev) { if (dev == NULL) @@ -77,6 +93,7 @@ static void cleanup_disk_device(struct disk_device *dev) free(dev->virtual_dev); free(dev->bus_type); free(dev->access_mode); + cleanup_device_address(&dev->address); }
static void cleanup_vsi_device(struct vsi_device *dev) @@ -107,6 +124,7 @@ static void cleanup_net_device(struct net_device *dev) free(dev->filter_ref); free(dev->poolid); cleanup_vsi_device(&dev->vsi); + cleanup_device_address(&dev->address); }
static void cleanup_emu_device(struct emu_device *dev) @@ -351,6 +369,67 @@ char *get_node_content(xmlNode *node) return buf; }
+int add_device_address_property(struct device_address *devaddr, + const char *key, + const char *value) +{ + char *k = NULL; + char *v = NULL; + char **list = NULL; + + if (key != NULL && value != NULL) { + k = strdup(key); + v = strdup(value); + if (k == NULL || v == NULL) + goto err; + + list = realloc(devaddr->key, sizeof(char*) * (devaddr->ct+1)); + if (list == NULL) + goto err; + devaddr->key = list; + + list = realloc(devaddr->value, sizeof(char*) * (devaddr->ct+1)); + if (list == NULL)
There's a leak here since 'ct' isn't incremented until later, devaddr->key will have the 'list' from above and that'll either be lost or overwritten. I see two ways out... free(devaddr->key); devaddr->key = NULL; or let err: handle the free(list) below... list = devaddr->key; devaddr->key = NULL; Of course there's also list1 and list2... Let me know which you prefer, I can make the change before pushing... John
+ goto err; + devaddr->value = list; + + devaddr->key[devaddr->ct] = k; + devaddr->value[devaddr->ct] = v; + devaddr->ct += 1; + return 1; + } + + err: + free(k); + free(v); + free(list); + return 0; +} + + +static int parse_device_address(xmlNode *anode, struct device_address *devaddr) +{ + xmlAttr *attr = NULL; + char *name = NULL; + char *value = NULL; + + for (attr = anode->properties; attr != NULL; attr = attr->next) { + name = (char*) attr->name; + value = get_attr_value(anode, name); + if (!add_device_address_property(devaddr, name, value)) + goto err; + free(value); + } + + return 1; + + err: + cleanup_device_address(devaddr); + free(value); + + return 0; +} + static int parse_fs_device(xmlNode *dnode, struct virt_device **vdevs) { struct virt_device *vdev = NULL; @@ -386,6 +465,8 @@ static int parse_fs_device(xmlNode *dnode, struct virt_device **vdevs) } } else if (XSTREQ(child->name, "driver")) { ddev->driver_type = get_attr_value(child, "type"); + } else if (XSTREQ(child->name, "address")) { + parse_device_address(child, &ddev->address); } }
@@ -459,6 +540,8 @@ static int parse_block_device(xmlNode *dnode, struct virt_device **vdevs) ddev->readonly = true; } else if (XSTREQ(child->name, "shareable")) { ddev->shareable = true; + } else if (XSTREQ(child->name, "address")) { + parse_device_address(child, &ddev->address); } }
@@ -598,6 +681,8 @@ static int parse_net_device(xmlNode *inode, struct virt_device **vdevs) ndev->filter_ref = get_attr_value(child, "filter"); } else if (XSTREQ(child->name, "virtualport")) { parse_vsi_device(child, ndev); + } else if (XSTREQ(child->name, "address")) { + parse_device_address(child, &ndev->address); #if LIBVIR_VERSION_NUMBER >= 9000 } else if (XSTREQ(child->name, "bandwidth")) { /* Network QoS bandwidth support */ @@ -1167,6 +1252,32 @@ static int parse_devices(const char *xml, struct virt_device **_list, int type) return count; }
+static void duplicate_device_address(struct device_address *to, const struct device_address *from) +{ + int i; + + if (from == NULL || to == NULL || from->ct == 0) + return; + + to->ct = from->ct; + to->key = calloc(from->ct, sizeof(char*)); + to->value = calloc(from->ct, sizeof(char*)); + if (to->key == NULL || to->value == NULL) + goto err; + + for (i = 0; i < from->ct; i++) { + to->key[i] = strdup(from->key[i]); + to->value[i] = strdup(from->value[i]); + if (to->key[i] == NULL || to->value[i] == NULL) + goto err; + } + + return; + + err: + cleanup_device_address(to); +} + struct virt_device *virt_device_dup(struct virt_device *_dev) { struct virt_device *dev; @@ -1196,6 +1307,7 @@ struct virt_device *virt_device_dup(struct virt_device *_dev) DUP_FIELD(dev, _dev, dev.net.vsi.profile_id); dev->dev.net.reservation = _dev->dev.net.reservation; dev->dev.net.limit = _dev->dev.net.limit; + duplicate_device_address(&dev->dev.net.address, &_dev->dev.net.address); } else if (dev->type == CIM_RES_TYPE_DISK) { DUP_FIELD(dev, _dev, dev.disk.type); DUP_FIELD(dev, _dev, dev.disk.device); @@ -1209,6 +1321,7 @@ struct virt_device *virt_device_dup(struct virt_device *_dev) dev->dev.disk.disk_type = _dev->dev.disk.disk_type; dev->dev.disk.readonly = _dev->dev.disk.readonly; dev->dev.disk.shareable = _dev->dev.disk.shareable; + duplicate_device_address(&dev->dev.disk.address, &_dev->dev.disk.address); } else if (dev->type == CIM_RES_TYPE_MEM) { dev->dev.mem.size = _dev->dev.mem.size; dev->dev.mem.maxsize = _dev->dev.mem.maxsize; diff --git a/libxkutil/device_parsing.h b/libxkutil/device_parsing.h index 4beac5c..92427c1 100644 --- a/libxkutil/device_parsing.h +++ b/libxkutil/device_parsing.h @@ -33,6 +33,12 @@
#include "../src/svpc_types.h"
+struct device_address { + uint32_t ct; + char **key; + char **value; +}; + struct vsi_device { char *vsi_type; char *manager_id; @@ -56,6 +62,7 @@ struct disk_device { char *bus_type; char *cache; char *access_mode; /* access modes for DISK_FS (filesystem) type */ + struct device_address address; };
struct net_device { @@ -70,6 +77,7 @@ struct net_device { uint64_t reservation; uint64_t limit; struct vsi_device vsi; + struct device_address address; };
struct mem_device { @@ -257,6 +265,9 @@ int get_devices(virDomainPtr dom, struct virt_device **list, int type, void cleanup_virt_device(struct virt_device *dev); void cleanup_virt_devices(struct virt_device **devs, int count);
+int add_device_address_property(struct device_address *devaddr, + const char *key, const char *value); + char *get_node_content(xmlNode *node); char *get_attr_value(xmlNode *node, char *attrname);
diff --git a/libxkutil/xmlgen.c b/libxkutil/xmlgen.c index 7e8801d..40e2905 100644 --- a/libxkutil/xmlgen.c +++ b/libxkutil/xmlgen.c @@ -178,6 +178,26 @@ static const char *console_xml(xmlNodePtr root, struct domain *dominfo) BAD_CAST cdev->target_type); } } + + return NULL; +} + +static char *device_address_xml(xmlNodePtr root, struct device_address *addr) +{ + int i; + xmlNodePtr address; + + if (addr == NULL || addr->ct == 0) + return NULL; + + address = xmlNewChild(root, NULL, BAD_CAST "address", NULL); + if (address == NULL) + return XML_ERROR; + + for (i = 0; i < addr->ct; i++) { + xmlNewProp(address, BAD_CAST addr->key[i] , BAD_CAST addr->value[i]); + } + return NULL; }
@@ -225,6 +245,9 @@ static char *disk_block_xml(xmlNodePtr root, struct disk_device *dev) if (dev->shareable) xmlNewChild(disk, NULL, BAD_CAST "shareable", NULL);
+ if (dev->address.ct > 0) + return device_address_xml(disk, &dev->address); + return NULL; }
@@ -279,6 +302,8 @@ static const char *disk_file_xml(xmlNodePtr root, struct disk_device *dev) if (dev->shareable) xmlNewChild(disk, NULL, BAD_CAST "shareable", NULL);
+ if (dev->address.ct > 0) + return device_address_xml(disk, &dev->address);
return NULL; } @@ -520,6 +545,9 @@ static const char *net_xml(xmlNodePtr root, struct domain *dominfo) } #endif
+ if (dev->dev.net.address.ct > 0) + msg = device_address_xml(nic, &dev->dev.net.address); + if (STREQ(dev->dev.net.type, "network")) { msg = set_net_source(nic, net, "network"); } else if (STREQ(dev->dev.net.type, "bridge")) {
+int add_device_address_property(struct device_address *devaddr, + const char *key, + const char *value) +{ + char *k = NULL; + char *v = NULL; + char **list = NULL; + + if (key != NULL && value != NULL) { + k = strdup(key); + v = strdup(value); + if (k == NULL || v == NULL) + goto err; + + list = realloc(devaddr->key, sizeof(char*) * (devaddr->ct+1)); + if (list == NULL) + goto err; + devaddr->key = list; + + list = realloc(devaddr->value, sizeof(char*) * (devaddr->ct+1)); + if (list == NULL)
There's a leak here since 'ct' isn't incremented until later, devaddr->key will have the 'list' from above and that'll either be lost or overwritten. I have to admit that you're not the first reviewer I have confused with this approach ... so: If realloc of the 'value' list fails, the devaddr content remains valid and unchanged, exactly because we don't increment
On 11/12/2013 11:34 PM, John Ferlan wrote: [...] the counter. And the intent is to preserve the passed-in devaddr even if adding new key-value pairs fails. It's true that the 'key' list will have an excess element, which doesn't hurt as it doesn't need to be freed (it has not been set). Frankly, I don't have a better idea, unless not to use realloc but do malloc's followed by memcpy's instead.
I see two ways out... free(devaddr->key); devaddr->key = NULL;
or let err: handle the free(list) below...
list = devaddr->key; devaddr->key = NULL;
Of course there's also list1 and list2...
Let me know which you prefer, I can make the change before pushing...
John
+ goto err; + devaddr->value = list; + + devaddr->key[devaddr->ct] = k; + devaddr->value[devaddr->ct] = v; + devaddr->ct += 1; + return 1; + } + + err: + free(k); + free(v); + free(list); + return 0; +} + + +static int parse_device_address(xmlNode *anode, struct device_address *devaddr) +{ + xmlAttr *attr = NULL; + char *name = NULL; + char *value = NULL; + + for (attr = anode->properties; attr != NULL; attr = attr->next) { + name = (char*) attr->name; + value = get_attr_value(anode, name); + if (!add_device_address_property(devaddr, name, value)) + goto err; + free(value); + } + + return 1; + + err: + cleanup_device_address(devaddr); + free(value); + + return 0; +} + static int parse_fs_device(xmlNode *dnode, struct virt_device **vdevs) { struct virt_device *vdev = NULL; @@ -386,6 +465,8 @@ static int parse_fs_device(xmlNode *dnode, struct virt_device **vdevs) } } else if (XSTREQ(child->name, "driver")) { ddev->driver_type = get_attr_value(child, "type"); + } else if (XSTREQ(child->name, "address")) { + parse_device_address(child, &ddev->address); } }
@@ -459,6 +540,8 @@ static int parse_block_device(xmlNode *dnode, struct virt_device **vdevs) ddev->readonly = true; } else if (XSTREQ(child->name, "shareable")) { ddev->shareable = true; + } else if (XSTREQ(child->name, "address")) { + parse_device_address(child, &ddev->address); } }
@@ -598,6 +681,8 @@ static int parse_net_device(xmlNode *inode, struct virt_device **vdevs) ndev->filter_ref = get_attr_value(child, "filter"); } else if (XSTREQ(child->name, "virtualport")) { parse_vsi_device(child, ndev); + } else if (XSTREQ(child->name, "address")) { + parse_device_address(child, &ndev->address); #if LIBVIR_VERSION_NUMBER >= 9000 } else if (XSTREQ(child->name, "bandwidth")) { /* Network QoS bandwidth support */ @@ -1167,6 +1252,32 @@ static int parse_devices(const char *xml, struct virt_device **_list, int type) return count; }
+static void duplicate_device_address(struct device_address *to, const struct device_address *from) +{ + int i; + + if (from == NULL || to == NULL || from->ct == 0) + return; + + to->ct = from->ct; + to->key = calloc(from->ct, sizeof(char*)); + to->value = calloc(from->ct, sizeof(char*)); + if (to->key == NULL || to->value == NULL) + goto err; + + for (i = 0; i < from->ct; i++) { + to->key[i] = strdup(from->key[i]); + to->value[i] = strdup(from->value[i]); + if (to->key[i] == NULL || to->value[i] == NULL) + goto err; + } + + return; + + err: + cleanup_device_address(to); +} + struct virt_device *virt_device_dup(struct virt_device *_dev) { struct virt_device *dev; @@ -1196,6 +1307,7 @@ struct virt_device *virt_device_dup(struct virt_device *_dev) DUP_FIELD(dev, _dev, dev.net.vsi.profile_id); dev->dev.net.reservation = _dev->dev.net.reservation; dev->dev.net.limit = _dev->dev.net.limit; + duplicate_device_address(&dev->dev.net.address, &_dev->dev.net.address); } else if (dev->type == CIM_RES_TYPE_DISK) { DUP_FIELD(dev, _dev, dev.disk.type); DUP_FIELD(dev, _dev, dev.disk.device); @@ -1209,6 +1321,7 @@ struct virt_device *virt_device_dup(struct virt_device *_dev) dev->dev.disk.disk_type = _dev->dev.disk.disk_type; dev->dev.disk.readonly = _dev->dev.disk.readonly; dev->dev.disk.shareable = _dev->dev.disk.shareable; + duplicate_device_address(&dev->dev.disk.address, &_dev->dev.disk.address); } else if (dev->type == CIM_RES_TYPE_MEM) { dev->dev.mem.size = _dev->dev.mem.size; dev->dev.mem.maxsize = _dev->dev.mem.maxsize; diff --git a/libxkutil/device_parsing.h b/libxkutil/device_parsing.h index 4beac5c..92427c1 100644 --- a/libxkutil/device_parsing.h +++ b/libxkutil/device_parsing.h @@ -33,6 +33,12 @@
#include "../src/svpc_types.h"
+struct device_address { + uint32_t ct; + char **key; + char **value; +}; + struct vsi_device { char *vsi_type; char *manager_id; @@ -56,6 +62,7 @@ struct disk_device { char *bus_type; char *cache; char *access_mode; /* access modes for DISK_FS (filesystem) type */ + struct device_address address; };
struct net_device { @@ -70,6 +77,7 @@ struct net_device { uint64_t reservation; uint64_t limit; struct vsi_device vsi; + struct device_address address; };
struct mem_device { @@ -257,6 +265,9 @@ int get_devices(virDomainPtr dom, struct virt_device **list, int type, void cleanup_virt_device(struct virt_device *dev); void cleanup_virt_devices(struct virt_device **devs, int count);
+int add_device_address_property(struct device_address *devaddr, + const char *key, const char *value); + char *get_node_content(xmlNode *node); char *get_attr_value(xmlNode *node, char *attrname);
diff --git a/libxkutil/xmlgen.c b/libxkutil/xmlgen.c index 7e8801d..40e2905 100644 --- a/libxkutil/xmlgen.c +++ b/libxkutil/xmlgen.c @@ -178,6 +178,26 @@ static const char *console_xml(xmlNodePtr root, struct domain *dominfo) BAD_CAST cdev->target_type); } } + + return NULL; +} + +static char *device_address_xml(xmlNodePtr root, struct device_address *addr) +{ + int i; + xmlNodePtr address; + + if (addr == NULL || addr->ct == 0) + return NULL; + + address = xmlNewChild(root, NULL, BAD_CAST "address", NULL); + if (address == NULL) + return XML_ERROR; + + for (i = 0; i < addr->ct; i++) { + xmlNewProp(address, BAD_CAST addr->key[i] , BAD_CAST addr->value[i]); + } + return NULL; }
@@ -225,6 +245,9 @@ static char *disk_block_xml(xmlNodePtr root, struct disk_device *dev) if (dev->shareable) xmlNewChild(disk, NULL, BAD_CAST "shareable", NULL);
+ if (dev->address.ct > 0) + return device_address_xml(disk, &dev->address); + return NULL; }
@@ -279,6 +302,8 @@ static const char *disk_file_xml(xmlNodePtr root, struct disk_device *dev) if (dev->shareable) xmlNewChild(disk, NULL, BAD_CAST "shareable", NULL);
+ if (dev->address.ct > 0) + return device_address_xml(disk, &dev->address);
return NULL; } @@ -520,6 +545,9 @@ static const char *net_xml(xmlNodePtr root, struct domain *dominfo) } #endif
+ if (dev->dev.net.address.ct > 0) + msg = device_address_xml(nic, &dev->dev.net.address); + if (STREQ(dev->dev.net.type, "network")) { msg = set_net_source(nic, net, "network"); } else if (STREQ(dev->dev.net.type, "bridge")) {
_______________________________________________ Libvirt-cim mailing list Libvirt-cim@redhat.com https://www.redhat.com/mailman/listinfo/libvirt-cim
-- Mit freundlichen Grüßen/Kind Regards Viktor Mihajlovski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martina Köderitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On 11/13/2013 11:24 AM, Viktor Mihajlovski wrote:
+int add_device_address_property(struct device_address *devaddr, + const char *key, + const char *value) +{ + char *k = NULL; + char *v = NULL; + char **list = NULL; + + if (key != NULL && value != NULL) { + k = strdup(key); + v = strdup(value); + if (k == NULL || v == NULL) + goto err; + + list = realloc(devaddr->key, sizeof(char*) * (devaddr->ct+1)); + if (list == NULL) + goto err; + devaddr->key = list; + + list = realloc(devaddr->value, sizeof(char*) * (devaddr->ct+1)); + if (list == NULL)
There's a leak here since 'ct' isn't incremented until later, devaddr->key will have the 'list' from above and that'll either be lost or overwritten. I have to admit that you're not the first reviewer I have confused with this approach ... so: If realloc of the 'value' list fails, the devaddr content remains valid and unchanged, exactly because we don't increment
On 11/12/2013 11:34 PM, John Ferlan wrote: [...] the counter. And the intent is to preserve the passed-in devaddr even if adding new key-value pairs fails. It's true that the 'key' list will have an excess element, which doesn't hurt as it doesn't need to be freed (it has not been set). Frankly, I don't have a better idea, unless not to use realloc but do malloc's followed by memcpy's instead.
oh - right... The next time realloc comes along it could essentially realloc something of the same size which I supposed is a noop this is why I like the libvirt macros around memory (re)allocation and list manipulation. I never have to think about them... I'll push this series in a bit... Tks, John
On 10/14/2013 11:29 AM, Viktor Mihajlovski wrote:
New data type for device addresses based on the generic "address" element of the libvirt domain XML. Contains XML parsing and generation code for device addresses.
Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> --- libxkutil/device_parsing.c | 113 ++++++++++++++++++++++++++++++++++++++++++++ libxkutil/device_parsing.h | 11 +++++ libxkutil/xmlgen.c | 28 +++++++++++ 3 files changed, 152 insertions(+)
Although this is already pushed, while I was working through Xu's patch set I think I found a missed case. The parsing code handles "parse_fs_device", "parse_block_device", and "parse_net_device"; however, the generation code does "disk_block_xml", "disk_file_xml", and "net_xml" missing out on "disk_fs_xml". The "parse_block_device" handles both "source=file" and "source=dev", while "disk_xml" checks for DISK_PHY or DISK_FILE before parceling out to the corresponding subroutine. John
diff --git a/libxkutil/device_parsing.c b/libxkutil/device_parsing.c index 076bec0..56e39c7 100644 --- a/libxkutil/device_parsing.c +++ b/libxkutil/device_parsing.c @@ -63,6 +63,22 @@ /* Device parse function */ typedef int (*dev_parse_func_t)(xmlNode *, struct virt_device **);
+static void cleanup_device_address(struct device_address *addr) +{ + int i; + if (addr == NULL) + return; + + for (i = 0; i < addr->ct; i++) { + free(addr->key[i]); + free(addr->value[i]); + } + + free(addr->key); + free(addr->value); + addr->ct = 0; +} + static void cleanup_disk_device(struct disk_device *dev) { if (dev == NULL) @@ -77,6 +93,7 @@ static void cleanup_disk_device(struct disk_device *dev) free(dev->virtual_dev); free(dev->bus_type); free(dev->access_mode); + cleanup_device_address(&dev->address); }
static void cleanup_vsi_device(struct vsi_device *dev) @@ -107,6 +124,7 @@ static void cleanup_net_device(struct net_device *dev) free(dev->filter_ref); free(dev->poolid); cleanup_vsi_device(&dev->vsi); + cleanup_device_address(&dev->address); }
static void cleanup_emu_device(struct emu_device *dev) @@ -351,6 +369,67 @@ char *get_node_content(xmlNode *node) return buf; }
+int add_device_address_property(struct device_address *devaddr, + const char *key, + const char *value) +{ + char *k = NULL; + char *v = NULL; + char **list = NULL; + + if (key != NULL && value != NULL) { + k = strdup(key); + v = strdup(value); + if (k == NULL || v == NULL) + goto err; + + list = realloc(devaddr->key, sizeof(char*) * (devaddr->ct+1)); + if (list == NULL) + goto err; + devaddr->key = list; + + list = realloc(devaddr->value, sizeof(char*) * (devaddr->ct+1)); + if (list == NULL) + goto err; + devaddr->value = list; + + devaddr->key[devaddr->ct] = k; + devaddr->value[devaddr->ct] = v; + devaddr->ct += 1; + return 1; + } + + err: + free(k); + free(v); + free(list); + return 0; +} + + +static int parse_device_address(xmlNode *anode, struct device_address *devaddr) +{ + xmlAttr *attr = NULL; + char *name = NULL; + char *value = NULL; + + for (attr = anode->properties; attr != NULL; attr = attr->next) { + name = (char*) attr->name; + value = get_attr_value(anode, name); + if (!add_device_address_property(devaddr, name, value)) + goto err; + free(value); + } + + return 1; + + err: + cleanup_device_address(devaddr); + free(value); + + return 0; +} + static int parse_fs_device(xmlNode *dnode, struct virt_device **vdevs) { struct virt_device *vdev = NULL; @@ -386,6 +465,8 @@ static int parse_fs_device(xmlNode *dnode, struct virt_device **vdevs) } } else if (XSTREQ(child->name, "driver")) { ddev->driver_type = get_attr_value(child, "type"); + } else if (XSTREQ(child->name, "address")) { + parse_device_address(child, &ddev->address); } }
@@ -459,6 +540,8 @@ static int parse_block_device(xmlNode *dnode, struct virt_device **vdevs) ddev->readonly = true; } else if (XSTREQ(child->name, "shareable")) { ddev->shareable = true; + } else if (XSTREQ(child->name, "address")) { + parse_device_address(child, &ddev->address); } }
@@ -598,6 +681,8 @@ static int parse_net_device(xmlNode *inode, struct virt_device **vdevs) ndev->filter_ref = get_attr_value(child, "filter"); } else if (XSTREQ(child->name, "virtualport")) { parse_vsi_device(child, ndev); + } else if (XSTREQ(child->name, "address")) { + parse_device_address(child, &ndev->address); #if LIBVIR_VERSION_NUMBER >= 9000 } else if (XSTREQ(child->name, "bandwidth")) { /* Network QoS bandwidth support */ @@ -1167,6 +1252,32 @@ static int parse_devices(const char *xml, struct virt_device **_list, int type) return count; }
+static void duplicate_device_address(struct device_address *to, const struct device_address *from) +{ + int i; + + if (from == NULL || to == NULL || from->ct == 0) + return; + + to->ct = from->ct; + to->key = calloc(from->ct, sizeof(char*)); + to->value = calloc(from->ct, sizeof(char*)); + if (to->key == NULL || to->value == NULL) + goto err; + + for (i = 0; i < from->ct; i++) { + to->key[i] = strdup(from->key[i]); + to->value[i] = strdup(from->value[i]); + if (to->key[i] == NULL || to->value[i] == NULL) + goto err; + } + + return; + + err: + cleanup_device_address(to); +} + struct virt_device *virt_device_dup(struct virt_device *_dev) { struct virt_device *dev; @@ -1196,6 +1307,7 @@ struct virt_device *virt_device_dup(struct virt_device *_dev) DUP_FIELD(dev, _dev, dev.net.vsi.profile_id); dev->dev.net.reservation = _dev->dev.net.reservation; dev->dev.net.limit = _dev->dev.net.limit; + duplicate_device_address(&dev->dev.net.address, &_dev->dev.net.address); } else if (dev->type == CIM_RES_TYPE_DISK) { DUP_FIELD(dev, _dev, dev.disk.type); DUP_FIELD(dev, _dev, dev.disk.device); @@ -1209,6 +1321,7 @@ struct virt_device *virt_device_dup(struct virt_device *_dev) dev->dev.disk.disk_type = _dev->dev.disk.disk_type; dev->dev.disk.readonly = _dev->dev.disk.readonly; dev->dev.disk.shareable = _dev->dev.disk.shareable; + duplicate_device_address(&dev->dev.disk.address, &_dev->dev.disk.address); } else if (dev->type == CIM_RES_TYPE_MEM) { dev->dev.mem.size = _dev->dev.mem.size; dev->dev.mem.maxsize = _dev->dev.mem.maxsize; diff --git a/libxkutil/device_parsing.h b/libxkutil/device_parsing.h index 4beac5c..92427c1 100644 --- a/libxkutil/device_parsing.h +++ b/libxkutil/device_parsing.h @@ -33,6 +33,12 @@
#include "../src/svpc_types.h"
+struct device_address { + uint32_t ct; + char **key; + char **value; +}; + struct vsi_device { char *vsi_type; char *manager_id; @@ -56,6 +62,7 @@ struct disk_device { char *bus_type; char *cache; char *access_mode; /* access modes for DISK_FS (filesystem) type */ + struct device_address address; };
struct net_device { @@ -70,6 +77,7 @@ struct net_device { uint64_t reservation; uint64_t limit; struct vsi_device vsi; + struct device_address address; };
struct mem_device { @@ -257,6 +265,9 @@ int get_devices(virDomainPtr dom, struct virt_device **list, int type, void cleanup_virt_device(struct virt_device *dev); void cleanup_virt_devices(struct virt_device **devs, int count);
+int add_device_address_property(struct device_address *devaddr, + const char *key, const char *value); + char *get_node_content(xmlNode *node); char *get_attr_value(xmlNode *node, char *attrname);
diff --git a/libxkutil/xmlgen.c b/libxkutil/xmlgen.c index 7e8801d..40e2905 100644 --- a/libxkutil/xmlgen.c +++ b/libxkutil/xmlgen.c @@ -178,6 +178,26 @@ static const char *console_xml(xmlNodePtr root, struct domain *dominfo) BAD_CAST cdev->target_type); } } + + return NULL; +} + +static char *device_address_xml(xmlNodePtr root, struct device_address *addr) +{ + int i; + xmlNodePtr address; + + if (addr == NULL || addr->ct == 0) + return NULL; + + address = xmlNewChild(root, NULL, BAD_CAST "address", NULL); + if (address == NULL) + return XML_ERROR; + + for (i = 0; i < addr->ct; i++) { + xmlNewProp(address, BAD_CAST addr->key[i] , BAD_CAST addr->value[i]); + } + return NULL; }
@@ -225,6 +245,9 @@ static char *disk_block_xml(xmlNodePtr root, struct disk_device *dev) if (dev->shareable) xmlNewChild(disk, NULL, BAD_CAST "shareable", NULL);
+ if (dev->address.ct > 0) + return device_address_xml(disk, &dev->address); + return NULL; }
@@ -279,6 +302,8 @@ static const char *disk_file_xml(xmlNodePtr root, struct disk_device *dev) if (dev->shareable) xmlNewChild(disk, NULL, BAD_CAST "shareable", NULL);
+ if (dev->address.ct > 0) + return device_address_xml(disk, &dev->address);
return NULL; } @@ -520,6 +545,9 @@ static const char *net_xml(xmlNodePtr root, struct domain *dominfo) } #endif
+ if (dev->dev.net.address.ct > 0) + msg = device_address_xml(nic, &dev->dev.net.address); + if (STREQ(dev->dev.net.type, "network")) { msg = set_net_source(nic, net, "network"); } else if (STREQ(dev->dev.net.type, "bridge")) {
On 10/14/2013 11:29 AM, Viktor Mihajlovski wrote:
New data type for device addresses based on the generic "address" element of the libvirt domain XML. Contains XML parsing and generation code for device addresses.
Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> --- libxkutil/device_parsing.c | 113 ++++++++++++++++++++++++++++++++++++++++++++ libxkutil/device_parsing.h | 11 +++++ libxkutil/xmlgen.c | 28 +++++++++++ 3 files changed, 152 insertions(+)
Although this is already pushed, while I was working through Xu's patch set I think I found a missed case.
The parsing code handles "parse_fs_device", "parse_block_device", and "parse_net_device"; however, the generation code does "disk_block_xml", "disk_file_xml", and "net_xml" missing out on "disk_fs_xml".
On 11/19/2013 10:02 PM, John Ferlan wrote: true ... will send a patch fixing it.
The "parse_block_device" handles both "source=file" and "source=dev", while "disk_xml" checks for DISK_PHY or DISK_FILE before parceling out to the corresponding subroutine.
John
-- Mit freundlichen Grüßen/Kind Regards Viktor Mihajlovski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martina Köderitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
This change allows to enumerate KVM disk and network RASDs containing device addresses. A new function set_rasd_device_address fills the CIM instance properties from a device_address structure. Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> --- src/Virt_RASD.c | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 73 insertions(+) diff --git a/src/Virt_RASD.c b/src/Virt_RASD.c index e28d4e6..df1e921 100644 --- a/src/Virt_RASD.c +++ b/src/Virt_RASD.c @@ -289,6 +289,67 @@ static bool get_vol_size(const CMPIBroker *broker, } #endif +static CMPIStatus set_rasd_device_address(const CMPIBroker *broker, + const CMPIObjectPath *ref, + const struct device_address *addr, + CMPIInstance *inst) +{ + int i; + CMPIArray *arr_key; + CMPIArray *arr_value; + CMPIString *string; + CMPIStatus s = {CMPI_RC_OK, NULL}; + + arr_key = CMNewArray(broker, + addr->ct, + CMPI_string, + &s); + if (s.rc != CMPI_RC_OK) + goto out; + + arr_value = CMNewArray(broker, + addr->ct, + CMPI_string, + &s); + if (s.rc != CMPI_RC_OK) + goto out; + + for (i = 0; i < addr->ct; i++) { + string = CMNewString(broker, + addr->key[i], + &s); + if (s.rc != CMPI_RC_OK) + goto out; + + CMSetArrayElementAt(arr_key, + i, + (CMPIValue *)&string, + CMPI_string); + + string = CMNewString(broker, + addr->value[i], + &s); + if (s.rc != CMPI_RC_OK) + goto out; + + CMSetArrayElementAt(arr_value, + i, + (CMPIValue *)&string, + CMPI_string); + } + + CMSetProperty(inst, "AddressProperties", + (CMPIValue *)&arr_key, + CMPI_stringA); + + CMSetProperty(inst, "AddressValues", + (CMPIValue *)&arr_value, + CMPI_stringA); + + out: + return s; +} + static CMPIStatus set_disk_rasd_params(const CMPIBroker *broker, const CMPIObjectPath *ref, const struct virt_device *dev, @@ -427,6 +488,12 @@ static CMPIStatus set_disk_rasd_params(const CMPIBroker *broker, (CMPIValue *)&(dev->dev.disk.shareable), CMPI_boolean); + if (dev->dev.disk.address.ct > 0) + set_rasd_device_address(broker, + ref, + &dev->dev.disk.address, + inst); + virStoragePoolFree(pool); virStorageVolFree(vol); virConnectClose(conn); @@ -590,6 +657,12 @@ static CMPIStatus set_net_rasd_params(const CMPIBroker *broker, (CMPIValue *)dev->dev.net.poolid, CMPI_chars); + if (dev->dev.net.address.ct > 0) + set_rasd_device_address(broker, + ref, + &dev->dev.net.address, + inst); + #if LIBVIR_VERSION_NUMBER < 9000 out: #endif -- 1.7.9.5
On 10/14/2013 11:29 AM, Viktor Mihajlovski wrote:
This change allows to enumerate KVM disk and network RASDs containing device addresses. A new function set_rasd_device_address fills the CIM instance properties from a device_address structure.
Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> --- src/Virt_RASD.c | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 73 insertions(+)
ACK John
This allows to define KVM guests with persistent device addresses for disks and network interfaces. The new function rasd_to_device_address extracts the address properties from the disk or network RASD and fills the device_address sub-structure of the affected virt_device. Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> --- src/Virt_VirtualSystemManagementService.c | 51 ++++++++++++++++++++++++++++- 1 file changed, 50 insertions(+), 1 deletion(-) diff --git a/src/Virt_VirtualSystemManagementService.c b/src/Virt_VirtualSystemManagementService.c index 89221bc..a72d0ac 100644 --- a/src/Virt_VirtualSystemManagementService.c +++ b/src/Virt_VirtualSystemManagementService.c @@ -890,6 +890,50 @@ static const char *_net_rand_mac(const CMPIObjectPath *ref) return _mac; } +static const char *rasd_to_device_address(CMPIInstance *inst, + struct device_address *addr) +{ + CMPICount c1; + CMPICount c2; + CMPIArray *arr_keys; + CMPIArray *arr_values; + CMPIData data_key; + CMPIData data_value; + const char *str_key; + const char *str_value; + int i; + const char * msg = NULL; + + if (cu_get_array_prop(inst, "AddressProperties", &arr_keys) != CMPI_RC_OK || + cu_get_array_prop(inst, "AddressValues", &arr_values) != CMPI_RC_OK) + goto out; + + c1 = CMGetArrayCount(arr_keys, NULL); + c2 = CMGetArrayCount(arr_values, NULL); + + if (c1 != c2) { + msg = "AddressProperties not matching AddressValues"; + goto out; + } + + for (i = 0; i < c1; i++) { + data_key = CMGetArrayElementAt(arr_keys, i, NULL); + data_value = CMGetArrayElementAt(arr_values, i, NULL); + + if (!CMIsNullValue(data_key) && !CMIsNullValue(data_key)) { + str_key = CMGetCharPtr(data_key.value.string); + str_value = CMGetCharPtr(data_value.value.string); + if (!add_device_address_property(addr, str_key, str_value)) { + msg = "Could not set address properties in vdev"; + goto out; + } + } + } + + out: + return msg; +} + static const char *net_rasd_to_vdev(CMPIInstance *inst, struct virt_device *dev, const char *ns) @@ -1040,6 +1084,8 @@ static const char *net_rasd_to_vdev(CMPIInstance *inst, &dev->dev.net.limit) != CMPI_RC_OK) dev->dev.net.limit = 0; + msg = rasd_to_device_address(inst, &dev->dev.net.address); + out: free(network); return msg; @@ -1050,6 +1096,7 @@ static const char *disk_rasd_to_vdev(CMPIInstance *inst, char **p_error) { const char *val = NULL; + const char *msg = NULL; uint16_t type; bool read = false; int rc; @@ -1161,7 +1208,9 @@ static const char *disk_rasd_to_vdev(CMPIInstance *inst, free(dev->id); dev->id = strdup(dev->dev.disk.virtual_dev); - return NULL; + msg = rasd_to_device_address(inst, &dev->dev.disk.address); + + return msg; } static const char *lxc_disk_rasd_to_vdev(CMPIInstance *inst, -- 1.7.9.5
On 10/14/2013 11:29 AM, Viktor Mihajlovski wrote:
This allows to define KVM guests with persistent device addresses for disks and network interfaces. The new function rasd_to_device_address extracts the address properties from the disk or network RASD and fills the device_address sub-structure of the affected virt_device.
Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> --- src/Virt_VirtualSystemManagementService.c | 51 ++++++++++++++++++++++++++++- 1 file changed, 50 insertions(+), 1 deletion(-)
ACK John
Use cleanup_virt_device instead of single free's all over the place in net_rasd_to_vdev and disk_rasd_to_vdev. Further, make sure that the device type is always set independent of the implementation of the xxx_rasd_to_vdev functions. Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> --- src/Virt_VirtualSystemManagementService.c | 35 +++++++---------------------- 1 file changed, 8 insertions(+), 27 deletions(-) diff --git a/src/Virt_VirtualSystemManagementService.c b/src/Virt_VirtualSystemManagementService.c index a72d0ac..5c7238f 100644 --- a/src/Virt_VirtualSystemManagementService.c +++ b/src/Virt_VirtualSystemManagementService.c @@ -964,17 +964,15 @@ static const char *net_rasd_to_vdev(CMPIInstance *inst, } */ - free(dev->dev.net.mac); + cleanup_virt_device(dev); + dev->dev.net.mac = strdup(val); - free(dev->id); dev->id = strdup(dev->dev.net.mac); - free(dev->dev.net.type); if (cu_get_str_prop(inst, "NetworkType", &val) != CMPI_RC_OK) return "No Network Type specified"; - free(dev->dev.net.source); if (STREQC(val, BRIDGE_TYPE)) { dev->dev.net.type = strdup(BRIDGE_TYPE); if (cu_get_str_prop(inst, "NetworkName", &val) == CMPI_RC_OK) @@ -1011,44 +1009,37 @@ static const char *net_rasd_to_vdev(CMPIInstance *inst, else return "No Source Device specified"; - free(dev->dev.net.vsi.vsi_type); if (cu_get_str_prop(inst, "VSIType", &val) != CMPI_RC_OK) dev->dev.net.vsi.vsi_type = NULL; else dev->dev.net.vsi.vsi_type = strdup(val); - free(dev->dev.net.vsi.manager_id); if (cu_get_str_prop(inst, "VSIManagerID", &val) != CMPI_RC_OK) dev->dev.net.vsi.manager_id = NULL; else dev->dev.net.vsi.manager_id = strdup(val); - free(dev->dev.net.vsi.type_id); if (cu_get_str_prop(inst, "VSITypeID", &val) != CMPI_RC_OK) dev->dev.net.vsi.type_id = NULL; else dev->dev.net.vsi.type_id = strdup(val); - free(dev->dev.net.vsi.type_id_version); if (cu_get_str_prop(inst, "VSITypeIDVersion", &val) != CMPI_RC_OK) dev->dev.net.vsi.type_id_version = NULL; else dev->dev.net.vsi.type_id_version = strdup(val); - free(dev->dev.net.vsi.instance_id); if (cu_get_str_prop(inst, "VSIInstanceID", &val) != CMPI_RC_OK) dev->dev.net.vsi.instance_id = NULL; else dev->dev.net.vsi.instance_id = strdup(val); - free(dev->dev.net.vsi.filter_ref); if (cu_get_str_prop(inst, "FilterRef", &val) != CMPI_RC_OK) dev->dev.net.vsi.filter_ref = NULL; else dev->dev.net.vsi.filter_ref = strdup(val); - free(dev->dev.net.vsi.profile_id); if (cu_get_str_prop(inst, "ProfileID", &val) != CMPI_RC_OK) dev->dev.net.vsi.profile_id = NULL; else @@ -1057,20 +1048,16 @@ static const char *net_rasd_to_vdev(CMPIInstance *inst, } else return "Invalid Network Type specified"; - free(dev->dev.net.device); if (cu_get_str_prop(inst, "VirtualDevice", &val) != CMPI_RC_OK) dev->dev.net.device = NULL; else dev->dev.net.device = strdup(val); - free(dev->dev.net.net_mode); if (cu_get_str_prop(inst, "NetworkMode", &val) != CMPI_RC_OK) dev->dev.net.net_mode = NULL; else dev->dev.net.net_mode = strdup(val); - free(dev->dev.net.model); - if (cu_get_str_prop(inst, "ResourceSubType", &val) != CMPI_RC_OK) dev->dev.net.model = NULL; else @@ -1106,13 +1093,13 @@ static const char *disk_rasd_to_vdev(CMPIInstance *inst, if (cu_get_str_prop(inst, "VirtualDevice", &val) != CMPI_RC_OK) return "Missing `VirtualDevice' property"; - free(dev->dev.disk.virtual_dev); + cleanup_virt_device(dev); + dev->dev.disk.virtual_dev = strdup(val); if (cu_get_str_prop(inst, "Address", &val) != CMPI_RC_OK) val = "/dev/null"; - free(dev->dev.disk.source); dev->dev.disk.source = strdup(val); if (dev->dev.disk.source == NULL) { return "dev->dev.disk.source is null!"; @@ -1149,7 +1136,6 @@ static const char *disk_rasd_to_vdev(CMPIInstance *inst, } if (XSTREQ(dev->dev.disk.source, "/dev/null")) { dev->dev.disk.disk_type = DISK_FILE; - free(dev->dev.disk.source); dev->dev.disk.source = strdup(""); } @@ -1170,31 +1156,26 @@ static const char *disk_rasd_to_vdev(CMPIInstance *inst, else dev->dev.disk.readonly = read; - free(dev->dev.disk.bus_type); if (cu_get_str_prop(inst, "BusType", &val) != CMPI_RC_OK) dev->dev.disk.bus_type = NULL; else dev->dev.disk.bus_type = strdup(val); - free(dev->dev.disk.driver); if (cu_get_str_prop(inst, "DriverName", &val) != CMPI_RC_OK) dev->dev.disk.driver = NULL; else dev->dev.disk.driver = strdup(val); - free(dev->dev.disk.driver_type); if (cu_get_str_prop(inst, "DriverType", &val) != CMPI_RC_OK) dev->dev.disk.driver_type = NULL; else dev->dev.disk.driver_type = strdup(val); - free(dev->dev.disk.cache); if (cu_get_str_prop(inst, "DriverCache", &val) != CMPI_RC_OK) dev->dev.disk.cache = NULL; else dev->dev.disk.cache = strdup(val); - free(dev->dev.disk.access_mode); if (cu_get_str_prop(inst, "AccessMode", &val) != CMPI_RC_OK) dev->dev.disk.access_mode = NULL; else @@ -1205,7 +1186,6 @@ static const char *disk_rasd_to_vdev(CMPIInstance *inst, else dev->dev.disk.shareable = shareable; - free(dev->id); dev->id = strdup(dev->dev.disk.virtual_dev); msg = rasd_to_device_address(inst, &dev->dev.disk.address); @@ -1921,12 +1901,14 @@ static const char *rasd_to_vdev(CMPIInstance *inst, goto out; } - dev->type = (int)type; - if (domain->type == DOMAIN_LXC) msg = _container_rasd_to_vdev(inst, dev, type, ns); else msg = _sysvirt_rasd_to_vdev(inst, dev, type, ns, p_error); + + /* ensure device type is set */ + if (msg == NULL) + dev->type = type; out: if (msg && op) CU_DEBUG("rasd_to_vdev(%s): %s", CLASSNAME(op), msg); @@ -3112,7 +3094,6 @@ static CMPIStatus resource_add(struct domain *dominfo, dev = &list[*count]; - dev->type = type; msg = rasd_to_vdev(rasd, dominfo, dev, ns, &error_msg); if (msg != NULL) { cu_statusf(_BROKER, &s, -- 1.7.9.5
On 10/14/2013 11:29 AM, Viktor Mihajlovski wrote:
Use cleanup_virt_device instead of single free's all over the place in net_rasd_to_vdev and disk_rasd_to_vdev. Further, make sure that the device type is always set independent of the implementation of the xxx_rasd_to_vdev functions.
Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> --- src/Virt_VirtualSystemManagementService.c | 35 +++++++---------------------- 1 file changed, 8 insertions(+), 27 deletions(-)
ACK John
participants (2)
-
John Ferlan -
Viktor Mihajlovski