[PATCH 00/20] REWORK/PARIAL: Changes to solve unsupported tag issue
This is a *partial rework* of Xu Wang's patches sent last month: https://www.redhat.com/archives/libvirt-cim/2013-October/msg00081.html Although not the complete set of changes - it's a good stopping point insomuch as it handles the "others" parsing. If this looks good, I can push it, then work through the changes to write the xml. I have run all the changes through cimtest - even with the patches on the list from Viktor. No new issues are found. Changes to the original patches 1. I rebased to top of tree as of today (11/14/13) 2. I reworked each of the patches to only add the 'others' to the particular *_device structure. This was done to be able to show the progression and to ensure I didn't forget something 2a. This found that there needed to be a cleanup_vcpu_device() and cleanup_mem_device() since both added "others" to their structure but were never cleaned up during cleanup_virt_device() 2b. I did not see a need for "others" in "vnc_device" and "sdl_device" 2c. I added a cleanup_others() call in _get_dominfo() 3. I added fetch_device_address_from_others() to replace parse_device_address(). This bridges the "gap" bewteen the parsing of the <address> tag that was recently added and the 'others' parsing code which didn't handle it. All I did was traverse the others list looking for parent name and id match for TYPE_PROP entries. Essentially anything inside of <address...> us then copied in to name/value structure managed by devaddr. Once the last caller of parse_device_address() was removed, that function went away 4. I changed the type for unknown from CIM_RES_TYPE_UNKNOWN to CIM_RES_TYPE_UNKDEV. Turns out the former is used in other contexts and if/when cleanup_virt_device() was called from one of those contexts the result was a core in cimprovagt. This was seen in the cimtest for VirtualSystemManagementService 16_removeresource.py. 4. Various "bug fixes" based on a coverity run and what I saw in code review from Boris. - Original code had a "ddev->disk_type == DISK_PHY;" - There was a "if (node->name == NULL)" check in parse_graphics_device after "else if (STREQC(gdev->type, "pty")) {" which coverity flagged as unnecessary since earlier code assumed node->name != NULL - In parse_os() there was a "STREQC(dominfo->os_info.pv.type, "linux")" which needed a "dominfo->os_info.pv.type &&" prior to it since the value could have been NULL according to a check earlier in the routine - I added the "free(dev->name);" to cleanup_unknown_device() - Checks for others->{name|parent_name|value} were removed. Since the structures are calloc()'d and free(NULL) does nothing, this is OK. Xu Wang (20): Add others member for saving unsupported tag and unknown device Add others and unknown_device clean up Add basic operations for reading data from xml node Fix xml parsing algorithm for parse_fs_device() Fix xml parsing algorithm for parse_block_device() Fix xml parsing algorithm for parse_vsi_device() Fix xml parsing algorithm for parse_net_device() Fix xml parsing algorithm for parse_vcpu_device() Fix xml parsing algorithm for parse_emu_device() Fix xml parsing algorithm for parse_mem_device() Fix xml parsing algorithm for parse_console_device() Fix xml parsing algorithm for parse_graphics_device() Fix xml parsing algorithm for parse_input_device() Add parse_unknown_device() Add parse_devices() for unknown type in get_dominfo_from_xml() Fix xml parsing algorithm in parse_domain() Fix xml parsing algorithm in parse_os() Fix xml parsing algorithm in parse_features() Add dup function for device copy Add type CIM_RES_TYPE_DELETED and modify type as it after resource_del libxkutil/device_parsing.c | 2055 ++++++++++++++++++++++++----- libxkutil/device_parsing.h | 58 + src/Virt_VirtualSystemManagementService.c | 2 +- src/svpc_types.h | 2 + 4 files changed, 1774 insertions(+), 343 deletions(-) -- 1.8.3.1
From: Xu Wang <cngesaint@gmail.com> Signed-off-by: Xu Wang <gesaint@linux.vnet.ibm.com> --- libxkutil/device_parsing.h | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/libxkutil/device_parsing.h b/libxkutil/device_parsing.h index 92427c1..603f29a 100644 --- a/libxkutil/device_parsing.h +++ b/libxkutil/device_parsing.h @@ -39,6 +39,27 @@ struct device_address { char **value; }; +/* The structure for saving unknown tag in the xml */ +enum others_type { + TYPE_PROP, + TYPE_NODE +}; + +struct others { + /* To identify the different tags with same name */ + int id; + char *name; + int parent_id; + char *parent_name; + enum others_type type; + char *value; + struct others *next; + enum { + ACTIVE, + INACTIVE + } status; +}; + struct vsi_device { char *vsi_type; char *manager_id; -- 1.8.3.1
From: Xu Wang <cngesaint@gmail.com> Signed-off-by: Xu Wang <gesaint@linux.vnet.ibm.com> --- libxkutil/device_parsing.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/libxkutil/device_parsing.c b/libxkutil/device_parsing.c index 56e39c7..6cdcfa9 100644 --- a/libxkutil/device_parsing.c +++ b/libxkutil/device_parsing.c @@ -79,6 +79,28 @@ static void cleanup_device_address(struct device_address *addr) addr->ct = 0; } +static void cleanup_node_of_others(struct others *others) +{ + if (others == NULL) + return; + + free(others->name); + free(others->parent_name); + free(others->value); + free(others); +} + +static void cleanup_others(struct others *others) +{ + struct others *head = others; + + while (others) { + head = others->next; + cleanup_node_of_others(others); + others = head; + } +} + static void cleanup_disk_device(struct disk_device *dev) { if (dev == NULL) -- 1.8.3.1
From: Xu Wang <cngesaint@gmail.com> Signed-off-by: Xu Wang <gesaint@linux.vnet.ibm.com> --- libxkutil/device_parsing.c | 297 +++++++++++++++++++++++++++++++++++++++++++++ libxkutil/device_parsing.h | 17 +++ 2 files changed, 314 insertions(+) diff --git a/libxkutil/device_parsing.c b/libxkutil/device_parsing.c index 6cdcfa9..610712f 100644 --- a/libxkutil/device_parsing.c +++ b/libxkutil/device_parsing.c @@ -452,6 +452,303 @@ static int parse_device_address(xmlNode *anode, struct device_address *devaddr) return 0; } +/* + * This function is just used for debugging. If you found something wrong + * please call this function and check the result of output in the logs. + */ +void print_others(struct others *head) +{ + CU_DEBUG("**************************************************"); + while (head) { + CU_DEBUG("---------------------------"); + CU_DEBUG("- others id: %d", head->id); + CU_DEBUG("- others name: %s", head->name); + CU_DEBUG("- others value: %s", head->value); + CU_DEBUG("- others type: %d", head->type); + CU_DEBUG("- others parent_id: %d", head->parent_id); + CU_DEBUG("- others parent_name: %s", head->parent_name); + if (head->status == ACTIVE) { + CU_DEBUG("- others status: ACTIVE"); + } else { + CU_DEBUG("- others status: INACTIVE"); + } + CU_DEBUG("---------------------------"); + head = head->next; + } + CU_DEBUG("**************************************************"); +} + +/* + * Add one node into head link list. All items are set as parameters. + * + * @head: The link list head pointer to be added. + * @node: Where the value of node come from. + * @name: Name of this link list node (maybe a node or an attribute in xml). + * @type: TYPE_NODE: Stands for the value from a xml node. + * TYPE_PROP: Means that this value is an attribute of a node. + * @parent_id: The id of parent node. + * @parent_name: Name of parent node. + */ +static struct others *add_others(struct others *head, + xmlNode *node, + const xmlChar *name, + enum others_type type, + int parent_id, + const xmlChar *parent_name) +{ + struct others *new = NULL; + struct others *end = head; + + new = calloc(1, sizeof(*new)); + if (new == NULL) { + CU_DEBUG("calloc space failed."); + return NULL; + } + + new->id = 0; + new->name = strdup((char *)name); + new->parent_id = parent_id; + if (parent_name) { + new->parent_name = strdup((char *)parent_name); + } + new->type = type; + if (type == TYPE_PROP) { + new->value = get_attr_value(node, (char *)name); + } else if (type == TYPE_NODE) { + new->value = get_node_content(node); + } + new->next = NULL; + + if (head == NULL) { + head = new; + } else { + /* Seek to the end of link list and calculate conflicts */ + while (end) { + if (STREQ(end->name, (char *)name) && + end->type == type) { + /* id conflict, +1 */ + new->id++; + } + if (end->next) { + end = end->next; + } else { + break; + } + } + end->next = new; + } + new->status = ACTIVE; + + return head; +} + +bool compare_param_int(int a, int b) +{ + if (a == -1 || b == -1) { + return true; + } else if (a == b) { + return true; + } else { + return false; + } +} + +bool compare_param_str(const char *a, const char *b) +{ + if (a == NULL || b == NULL) { + return true; + } else if (STREQ(a, b)) { + return true; + } else { + return false; + } +} + +/* + * This function is the main operation for members of virtual device structure. + * The whole data of xml is saved in the others link list. By calling this + * function each variable of virtual device could get the value it wanted. + * After value was fetched, the node in the others link list to be INACTIVE + * status. + * + * @head: The link list saved all data needed. + * @id: Id of name, to identify different nodes but with same name. + * @name: Name of node the caller want to get value. + * @type: TYPE_NODE: Caller want to get a value of node. + * TYPE_PROP: Caller want to get a value of attribute. + * @parent_id: The id of parent node caller want. + * @parent_name: The name of parent node caller want. + * + * If caller doesn't want to point @id, @parent_id or @parent_name, + * @id could be set as -1, @parent_id and @parent_name should be + * set as NULL. + */ +char *fetch_from_others(struct others **head, + int id, + char *name, + enum others_type type, + int parent_id, + char *parent_name) +{ + struct others *tmp = *head; + char *value = NULL; + + while (tmp) { + if (compare_param_int(tmp->id, id) && + STREQ(tmp->name, name) && + compare_param_int(tmp->parent_id, parent_id) && + compare_param_str(tmp->parent_name, parent_name) && + tmp->type == type && + tmp->status == ACTIVE) { + value = strdup(tmp->value); + tmp->status = INACTIVE; + return value; + } + tmp = tmp->next; + } + + return NULL; +} + +/* + * This function seeks in the others link list and return the result of + * seeking. If success the node seeked will to be INACTIVE status. + * + * @head: The link list head pointer to be seeked. + * @id: Id of node to be seeked. + * @name: Name of node to be seeked. + * @type: TYPE_NODE: The node to be seeked is a node of xml. + * TYPE_PROP: The node to be seeked is a attribute of a node. + * @parent_id: Id of parent node which to be seeked. + * @parent_name: Name of parent node which to be seeked. + * + * If the caller doesn't point @id, @parent_id or @parent_name, they should + * be set as -1, -1 or NULL. Other parameters is mandatory. + */ +static bool seek_in_others(struct others **head, + int id, + char *name, + enum others_type type, + int parent_id, + char *parent_name) +{ + struct others *tmp = *head; + + while (tmp) { + if (compare_param_int(tmp->id, id) && + STREQ(tmp->name, name) && + compare_param_int(tmp->parent_id, parent_id) && + compare_param_str(tmp->parent_name, parent_name) && + tmp->type == type && + tmp->status == ACTIVE) { + tmp->status = INACTIVE; + return true; + } + tmp = tmp->next; + } + + return false; +} + +struct others *combine_others(struct others *head1, + struct others *head2) +{ + struct others *tail1 = head1; + + if (tail1 == NULL) { + return head2; + } + + while (tail1->next) { + tail1 = tail1->next; + } + + tail1->next = head2; + return head1; +} + +static struct others *seek_to_tail(struct others *others) +{ + if (others == NULL) { + return NULL; + } + + while (others->next) { + others = others->next; + } + + return others; +} + +/* + * Parse all data from xml and build a link list to save it. + * + * @head: Where data from xml to save. + * @node: The root node of xml to be parsed. + * @parent_id: Parent node id of root node, usually zero. + * @parent_name: Parent node name of root node. + */ +static struct others *parse_data_to_others(struct others *head, + xmlNode *node, + int parent_id, + const xmlChar *parent_name) +{ + xmlNode *child = NULL; + xmlAttrPtr attrPtr = NULL; + struct others *tail = NULL; + int tail_id = 0; + + /* If name of node is "text", all operations will skip */ + if (XSTREQ(node->name, "text")) { + return head; + } + + head = add_others(head, + node, + node->name, + TYPE_NODE, + parent_id, + parent_name); + + if (head == NULL) { + goto err; + } + + /* tail is the node we added above */ + tail = seek_to_tail(head); + if (tail == NULL) { + tail_id = 0; + } else { + tail_id = tail->id; + } + + /* Get properties of node */ + attrPtr = node->properties; + while (attrPtr) { + head = add_others(head, + node, + attrPtr->name, + TYPE_PROP, + tail_id, + node->name); + if (head == NULL) { + goto err; + } + + attrPtr = attrPtr->next; + } + + for (child = node->children; child != NULL; child = child->next) { + /* Recursion to restore child's properties or child if have */ + head = parse_data_to_others(head, child, tail_id, node->name); + } + + return head; +err: + CU_DEBUG("add_others failed."); + return NULL; +} + static int parse_fs_device(xmlNode *dnode, struct virt_device **vdevs) { struct virt_device *vdev = NULL; diff --git a/libxkutil/device_parsing.h b/libxkutil/device_parsing.h index 603f29a..92d8638 100644 --- a/libxkutil/device_parsing.h +++ b/libxkutil/device_parsing.h @@ -299,6 +299,23 @@ int attach_device(virDomainPtr dom, struct virt_device *dev); int detach_device(virDomainPtr dom, struct virt_device *dev); int change_device(virDomainPtr dom, struct virt_device *dev); +char *fetch_from_others(struct others **head, + int id, + char *name, + enum others_type type, + int parent_id, + char *parent_name); + +bool compare_parent(const char *a, const char *b); + +void print_others(struct others *head); + +struct others *combine_others(struct others *head1, struct others *head2); + +bool compare_param_int(int a, int b); + +bool compare_param_str(const char *a, const char *b); + #define XSTREQ(x, y) (STREQ((char *)x, y)) #define STRPROP(d, p, n) (d->p = get_node_content(n)) -- 1.8.3.1
From: Xu Wang <cngesaint@gmail.com> Signed-off-by: Xu Wang <gesaint@linux.vnet.ibm.com> --- libxkutil/device_parsing.c | 156 ++++++++++++++++++++++++++++++++++++++------- libxkutil/device_parsing.h | 1 + 2 files changed, 134 insertions(+), 23 deletions(-) diff --git a/libxkutil/device_parsing.c b/libxkutil/device_parsing.c index 610712f..20b35e1 100644 --- a/libxkutil/device_parsing.c +++ b/libxkutil/device_parsing.c @@ -116,6 +116,7 @@ static void cleanup_disk_device(struct disk_device *dev) free(dev->bus_type); free(dev->access_mode); cleanup_device_address(&dev->address); + cleanup_others(dev->others); } static void cleanup_vsi_device(struct vsi_device *dev) @@ -453,6 +454,45 @@ static int parse_device_address(xmlNode *anode, struct device_address *devaddr) } /* + * Device addresses can have many forms based on the 'type' field, so rather + * than try to create a function that could parse each particular option, we + * will just fetch all the name/value pairs with "address" as the parent name + * and store them in a devaddr array to be parsed elsewhere. + * + * This function will also assume that the 'id' and 'parentid' values are -1 + * at least for now + */ +static int fetch_device_address_from_others(struct others **head, + struct device_address *devaddr) +{ + struct others *tmp = *head; + + while (tmp) { + if (compare_param_int(tmp->id, -1) && + compare_param_int(tmp->parent_id, -1) && + compare_param_str(tmp->parent_name, "address") && + tmp->type == TYPE_PROP && + tmp->status == ACTIVE) { + + if (!add_device_address_property(devaddr, tmp->name, + tmp->value)) + goto err; + + /* Since we're now managing it... */ + tmp->status = INACTIVE; + } + tmp = tmp->next; + } + + return 1; + + err: + cleanup_device_address(devaddr); + + return 0; +} + +/* * This function is just used for debugging. If you found something wrong * please call this function and check the result of output in the logs. */ @@ -753,7 +793,8 @@ static int parse_fs_device(xmlNode *dnode, struct virt_device **vdevs) { struct virt_device *vdev = NULL; struct disk_device *ddev = NULL; - xmlNode *child = NULL; + + CU_DEBUG("Enter parse_fs_device()."); vdev = calloc(1, sizeof(*vdev)); if (vdev == NULL) @@ -761,37 +802,106 @@ static int parse_fs_device(xmlNode *dnode, struct virt_device **vdevs) ddev = (&vdev->dev.disk); - ddev->type = get_attr_value(dnode, "type"); + ddev->others = parse_data_to_others(ddev->others, + dnode, + 0, + BAD_CAST "devices"); + if (ddev->others == NULL) { + CU_DEBUG("parse xml failed."); + goto err; + } + + /* fetch out <filesystem> tag from others. It will be removed + * after others management finished. */ + fetch_from_others(&ddev->others, + -1, + "filesystem", + TYPE_NODE, + -1, + "devices"); + + ddev->type = fetch_from_others(&ddev->others, + -1, + "type", + TYPE_PROP, + -1, + (char *)dnode->name); if (ddev->type == NULL) { CU_DEBUG("No type"); goto err; } - ddev->access_mode = get_attr_value(dnode, "accessmode"); + ddev->access_mode = fetch_from_others(&ddev->others, + -1, + "accessmode", + TYPE_PROP, + -1, + (char *)dnode->name); + + if (seek_in_others(&ddev->others, + -1, + "source", + TYPE_NODE, + -1, + (char *)dnode->name)) { + ddev->source = fetch_from_others(&ddev->others, + -1, + "dir", + TYPE_PROP, + -1, + "source"); + + + if (ddev->source == NULL) { + CU_DEBUG("no source dir"); + goto err; + } + } - for (child = dnode->children; child != NULL; child = child->next) { - if (XSTREQ(child->name, "source")) { - ddev->source = get_attr_value(child, "dir"); - if (ddev->source == NULL) { - CU_DEBUG("No source dir"); - goto err; - } - } else if (XSTREQ(child->name, "target")) { - ddev->virtual_dev = get_attr_value(child, "dir"); - if (ddev->virtual_dev == NULL) { - CU_DEBUG("No target dir"); - goto err; - } - } 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); + if (seek_in_others(&ddev->others, + -1, + "target", + TYPE_NODE, + -1, + (char *)dnode->name)) { + ddev->virtual_dev = fetch_from_others(&ddev->others, + -1, + "dir", + TYPE_PROP, + -1, + "target"); + + if (ddev->virtual_dev == NULL) { + CU_DEBUG("no target dir"); + goto err; } } - if ((ddev->source == NULL) || (ddev->virtual_dev == NULL)) { - CU_DEBUG("S: %s D: %s", ddev->source, ddev->virtual_dev); - goto err; + if (seek_in_others(&ddev->others, + -1, + "driver", + TYPE_NODE, + -1, + (char *)dnode->name)) { + ddev->driver_type = fetch_from_others(&ddev->others, + -1, + "type", + TYPE_PROP, + -1, + "driver"); + } + + if (seek_in_others(&ddev->others, + -1, + "address", + TYPE_NODE, + -1, + (char *)dnode->name)) { + if (!fetch_device_address_from_others(&ddev->others, + &ddev->address)) { + CU_DEBUG("error fetching device address"); + goto err; + } } ddev->disk_type = DISK_FS; diff --git a/libxkutil/device_parsing.h b/libxkutil/device_parsing.h index 92d8638..6e8fe25 100644 --- a/libxkutil/device_parsing.h +++ b/libxkutil/device_parsing.h @@ -84,6 +84,7 @@ struct disk_device { char *cache; char *access_mode; /* access modes for DISK_FS (filesystem) type */ struct device_address address; + struct others *others; }; struct net_device { -- 1.8.3.1
From: Xu Wang <cngesaint@gmail.com> Signed-off-by: Xu Wang <gesaint@linux.vnet.ibm.com> --- libxkutil/device_parsing.c | 180 +++++++++++++++++++++++++++++++++++++-------- 1 file changed, 149 insertions(+), 31 deletions(-) diff --git a/libxkutil/device_parsing.c b/libxkutil/device_parsing.c index 20b35e1..dac2ea1 100644 --- a/libxkutil/device_parsing.c +++ b/libxkutil/device_parsing.c @@ -925,7 +925,8 @@ static int parse_block_device(xmlNode *dnode, struct virt_device **vdevs) { struct virt_device *vdev = NULL; struct disk_device *ddev = NULL; - xmlNode * child = NULL; + + CU_DEBUG("Enter parse_block_device()."); vdev = calloc(1, sizeof(*vdev)); if (vdev == NULL) @@ -933,47 +934,164 @@ static int parse_block_device(xmlNode *dnode, struct virt_device **vdevs) ddev = &(vdev->dev.disk); - ddev->type = get_attr_value(dnode, "type"); - if (ddev->type == NULL) + ddev->others = parse_data_to_others(ddev->others, + dnode, + 0, + BAD_CAST "devices"); + if (ddev->others == NULL) { + CU_DEBUG("xml file parse failed."); goto err; + } + + /* fetch out <disk> tag from others. It will be removed + * after others management finished. */ + fetch_from_others(&ddev->others, + -1, + "disk", + TYPE_NODE, + -1, + "devices"); - ddev->device = get_attr_value(dnode, "device"); - if (ddev->device == NULL) + ddev->type = fetch_from_others(&ddev->others, + -1, + "type", + TYPE_PROP, + -1, + (char *)dnode->name); + if (ddev->type == NULL) { + CU_DEBUG("no type"); goto err; + } - for (child = dnode->children; child != NULL; child = child->next) { - if (XSTREQ(child->name, "driver")) { - ddev->driver = get_attr_value(child, "name"); - if (ddev->driver == NULL) + ddev->device = fetch_from_others(&ddev->others, + -1, + "device", + TYPE_PROP, + -1, + (char *)dnode->name); + if (ddev->device == NULL) { + CU_DEBUG("no device"); + goto err; + } + + if (seek_in_others(&ddev->others, + -1, + "driver", + TYPE_NODE, + -1, + (char *)dnode->name)) { + ddev->driver = fetch_from_others(&ddev->others, + -1, + "name", + TYPE_PROP, + -1, + "driver"); + + if (ddev->driver == NULL) { + CU_DEBUG("no driver name"); + goto err; + } + + ddev->driver_type = fetch_from_others(&ddev->others, + -1, + "type", + TYPE_PROP, + -1, + "driver"); + + ddev->cache = fetch_from_others(&ddev->others, + -1, + "cache", + TYPE_PROP, + -1, + "driver"); + } + + if (seek_in_others(&ddev->others, + -1, + "source", + TYPE_NODE, + -1, + (char *)dnode->name)) { + ddev->source = fetch_from_others(&ddev->others, + -1, + "file", + TYPE_PROP, + -1, + "source"); + if (ddev->source == NULL) { + ddev->source = fetch_from_others(&ddev->others, + -1, + "dev", + TYPE_PROP, + -1, + "source"); + + if (ddev->source == NULL) { + CU_DEBUG("no source file/dev"); goto err; - ddev->driver_type = get_attr_value(child, "type"); - ddev->cache = get_attr_value(child, "cache"); - } else if (XSTREQ(child->name, "source")) { - ddev->source = get_attr_value(child, "file"); - if (ddev->source) { - ddev->disk_type = DISK_FILE; - continue; - } - ddev->source = get_attr_value(child, "dev"); - if (ddev->source) { + } else { ddev->disk_type = DISK_PHY; - continue; } + } else { + ddev->disk_type = DISK_FILE; + } + } + + if (seek_in_others(&ddev->others, + -1, + "target", + TYPE_NODE, + -1, + (char *)dnode->name)) { + ddev->virtual_dev = fetch_from_others(&ddev->others, + -1, + "dev", + TYPE_PROP, + -1, + "target"); + + if (ddev->virtual_dev == NULL) { + CU_DEBUG("no target dev"); goto err; - } else if (XSTREQ(child->name, "target")) { - ddev->virtual_dev = get_attr_value(child, "dev"); - if (ddev->virtual_dev == NULL) - goto err; - ddev->bus_type = get_attr_value(child, "bus"); - } else if (XSTREQ(child->name, "readonly")) { - ddev->readonly = true; - } else if (XSTREQ(child->name, "shareable")) { - ddev->shareable = true; - } else if (XSTREQ(child->name, "address")) { - parse_device_address(child, &ddev->address); } + + ddev->bus_type = fetch_from_others(&ddev->others, + -1, + "bus", + TYPE_PROP, + -1, + "target"); } + ddev->readonly = seek_in_others(&ddev->others, + -1, + "readonly", + TYPE_NODE, + -1, + (char *)dnode->name); + + ddev->shareable = seek_in_others(&ddev->others, + -1, + "shareable", + TYPE_NODE, + -1, + (char *)dnode->name); + + if (seek_in_others(&ddev->others, + -1, + "address", + TYPE_NODE, + -1, + (char *)dnode->name)) { + if (!fetch_device_address_from_others(&ddev->others, + &ddev->address)) { + CU_DEBUG("error fetching device address"); + goto err; + } + } + + /* handle the situation that a cdrom device have no disk in it, no ISO file */ if ((XSTREQ(ddev->device, "cdrom")) && (ddev->source == NULL)) { ddev->source = strdup(""); -- 1.8.3.1
From: Xu Wang <cngesaint@gmail.com> Signed-off-by: Xu Wang <gesaint@linux.vnet.ibm.com> --- libxkutil/device_parsing.c | 89 ++++++++++++++++++++++++++++++++++------------ 1 file changed, 66 insertions(+), 23 deletions(-) diff --git a/libxkutil/device_parsing.c b/libxkutil/device_parsing.c index dac2ea1..940b105 100644 --- a/libxkutil/device_parsing.c +++ b/libxkutil/device_parsing.c @@ -1129,40 +1129,83 @@ static int parse_disk_device(xmlNode *dnode, struct virt_device **vdevs) } } -static int parse_vsi_device(xmlNode *dnode, struct net_device *vdevs) +static int parse_vsi_device(struct others **others, struct net_device *vdevs) { struct vsi_device *vsi_dev = NULL; - xmlNode * child = NULL; + + CU_DEBUG("Enter parse_vsi_device()."); vsi_dev = calloc(1, sizeof(*vsi_dev)); - if (vsi_dev == NULL) + if (vsi_dev == NULL) { + CU_DEBUG("calloc failed."); goto err; + } - vsi_dev->vsi_type = get_attr_value(dnode, "type"); - if (vsi_dev->vsi_type == NULL) + vsi_dev->vsi_type = fetch_from_others(others, + -1, + "type", + TYPE_PROP, + -1, + "virtualport"); + if (vsi_dev->vsi_type == NULL) { + CU_DEBUG("no vsi type"); goto err; + } - for (child = dnode->children; child != NULL; child = child->next) { - if (XSTREQ(child->name, "parameters")) { - vsi_dev->manager_id = get_attr_value(child, - "managerid"); - if (vsi_dev->manager_id == NULL) - goto err; + if (seek_in_others(others, + -1, + "parameters", + TYPE_NODE, + -1, + "virtualport")) { + vsi_dev->manager_id = fetch_from_others(others, + -1, + "managerid", + TYPE_PROP, + -1, + "parameters"); + + if (vsi_dev->manager_id == NULL) { + CU_DEBUG("no managerid"); + goto err; + } - vsi_dev->type_id = get_attr_value(child, "typeid"); - if (vsi_dev->type_id == NULL) - goto err; + vsi_dev->type_id = fetch_from_others(others, + -1, + "typeid", + TYPE_PROP, + -1, + "parameters"); + if (vsi_dev->type_id == NULL) { + CU_DEBUG("no typeid"); + goto err; + } - vsi_dev->type_id_version = - get_attr_value(child, "typeidversion"); - if (vsi_dev->type_id_version == NULL) - goto err; + vsi_dev->type_id_version = fetch_from_others(others, + -1, + "typeidversion", + TYPE_PROP, + -1, + "parameters"); - vsi_dev->instance_id = get_attr_value(child, - "instanceid"); - vsi_dev->profile_id = get_attr_value(child, - "profileid"); - } + if (vsi_dev->type_id_version == NULL) { + CU_DEBUG("no typeidversion"); + goto err; + } + + vsi_dev->instance_id = fetch_from_others(others, + -1, + "instanceid", + TYPE_PROP, + -1, + "parameters"); + + vsi_dev->profile_id = fetch_from_others(others, + -1, + "profileid", + TYPE_PROP, + -1, + "parameters"); } memcpy(&(vdevs->vsi), vsi_dev, sizeof(*vsi_dev)); -- 1.8.3.1
From: Xu Wang <cngesaint@gmail.com> Signed-off-by: Xu Wang <gesaint@linux.vnet.ibm.com> --- libxkutil/device_parsing.c | 301 +++++++++++++++++++++++++++++++-------------- libxkutil/device_parsing.h | 1 + 2 files changed, 211 insertions(+), 91 deletions(-) diff --git a/libxkutil/device_parsing.c b/libxkutil/device_parsing.c index 940b105..8b39cda 100644 --- a/libxkutil/device_parsing.c +++ b/libxkutil/device_parsing.c @@ -148,6 +148,7 @@ static void cleanup_net_device(struct net_device *dev) free(dev->poolid); cleanup_vsi_device(&dev->vsi); cleanup_device_address(&dev->address); + cleanup_others(dev->others); } static void cleanup_emu_device(struct emu_device *dev) @@ -429,30 +430,6 @@ int add_device_address_property(struct device_address *devaddr, 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; -} - /* * Device addresses can have many forms based on the 'type' field, so rather * than try to create a function that could parse each particular option, we @@ -1222,92 +1199,234 @@ static int parse_net_device(xmlNode *inode, struct virt_device **vdevs) { struct virt_device *vdev = NULL; struct net_device *ndev = NULL; - xmlNode *child = NULL; + + CU_DEBUG("Enter parse_net_device()."); vdev = calloc(1, sizeof(*vdev)); - if (vdev == NULL) + if (vdev == NULL) { + CU_DEBUG("calloc failed."); goto err; + } ndev = &(vdev->dev.net); - ndev->type = get_attr_value(inode, "type"); - if (ndev->type == NULL) + ndev->others = parse_data_to_others(ndev->others, + inode, + 0, + BAD_CAST "devices"); + if (ndev->others == NULL) { + CU_DEBUG("parse xml data to others failed."); goto err; + } + + /* fetch out <interface> tag from others. It will be removed + * after others management finished. */ + fetch_from_others(&ndev->others, + -1, + "interface", + TYPE_NODE, + -1, + "devices"); + + ndev->type = fetch_from_others(&ndev->others, + -1, + "type", + TYPE_PROP, + -1, + (char *)inode->name); + + if (ndev->type == NULL) { + CU_DEBUG("no type"); + goto err; + } + + if (seek_in_others(&ndev->others, + -1, + "mac", + TYPE_NODE, + -1, + (char *)inode->name)) { + ndev->mac = fetch_from_others(&ndev->others, + -1, + "address", + TYPE_PROP, + -1, + "mac"); + + if (ndev->mac == NULL) { + CU_DEBUG("no mac address"); + goto err; + } + } + + if (seek_in_others(&ndev->others, + -1, + "source", + TYPE_NODE, + -1, + (char *)inode->name)) { + ndev->source = fetch_from_others(&ndev->others, + -1, + "bridge", + TYPE_PROP, + -1, + "source"); + + if (ndev->source == NULL) { + ndev->source = fetch_from_others(&ndev->others, + -1, + "network", + TYPE_PROP, + -1, + "source"); - for (child = inode->children; child != NULL; child = child->next) { - if (XSTREQ(child->name, "mac")) { - ndev->mac = get_attr_value(child, "address"); - if (ndev->mac == NULL) - goto err; - } else if (XSTREQ(child->name, "source")) { - ndev->source = get_attr_value(child, "bridge"); - if (ndev->source != NULL) - continue; - ndev->source = get_attr_value(child, "network"); if (ndev->source != NULL) { - int ret = asprintf(&ndev->poolid, + int ret = asprintf(&ndev->poolid, "NetworkPool/%s", ndev->source); + if (ret == -1) { - CU_DEBUG("Failed to get network" - " poolid"); + CU_DEBUG("Failed to get network poolid"); + goto err; } - continue; + } else { + ndev->source = fetch_from_others(&ndev->others, + -1, + "dev", + TYPE_PROP, + -1, + "source"); + + ndev->net_mode = fetch_from_others(&ndev->others, + -1, + "mode", + TYPE_PROP, + -1, + "source"); + + if ((ndev->source == NULL) || (ndev->net_mode == NULL)) { + CU_DEBUG("source %s, mode %s", ndev->source, ndev->net_mode); + goto err; + } + } - ndev->source = get_attr_value(child, "dev"); - ndev->net_mode = get_attr_value(child, "mode"); - if ((ndev->source != NULL) && (ndev->net_mode != NULL)) - continue; + } + } + + if (seek_in_others(&ndev->others, + -1, + "target", + TYPE_NODE, + -1, + (char *)inode->name)) { + ndev->device = fetch_from_others(&ndev->others, + -1, + "dev", + TYPE_PROP, + -1, + "target"); + + if (ndev->device == NULL) { + CU_DEBUG("no dev in target."); goto err; - } else if (XSTREQ(child->name, "target")) { - ndev->device = get_attr_value(child, "dev"); - if (ndev->device == NULL) - goto err; - } else if (XSTREQ(child->name, "model")) { - ndev->model = get_attr_value(child, "type"); - if (ndev->model == NULL) - goto err; - } else if (XSTREQ(child->name, "filterref")) { - 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 (seek_in_others(&ndev->others, + -1, + "model", + TYPE_NODE, + -1, + (char *)inode->name)) { + ndev->model = fetch_from_others(&ndev->others, + -1, + "type", + TYPE_PROP, + -1, + "model"); + if (ndev->model == NULL) { + CU_DEBUG("no model type."); + goto err; + } + } + + if (seek_in_others(&ndev->others, + -1, + "filterref", + TYPE_NODE, + -1, + (char *)inode->name)) { + ndev->filter_ref = fetch_from_others(&ndev->others, + -1, + "filter", + TYPE_PROP, + -1, + "filterref"); + } + + if (seek_in_others(&ndev->others, -1, "virtualport", + TYPE_NODE, -1, (char *)inode->name)) { + parse_vsi_device(&ndev->others, ndev); + } + + if (seek_in_others(&ndev->others, + -1, + "address", + TYPE_NODE, + -1, + (char *)inode->name)) { + if (!fetch_device_address_from_others(&ndev->others, + &ndev->address)) { + CU_DEBUG("error fetching device address"); + goto err; + } + } + #if LIBVIR_VERSION_NUMBER >= 9000 - } else if (XSTREQ(child->name, "bandwidth")) { - /* Network QoS bandwidth support */ - xmlNode *grandchild = NULL; - for (grandchild = child->children; - grandchild != NULL; - grandchild = grandchild->next) { - if (XSTREQ(grandchild->name, "inbound")) { - /* Only expose inbound bandwidth */ - char *val; - - val = get_attr_value(grandchild, - "average"); - if (val != NULL) { - sscanf(val, "%" PRIu64, - &ndev->reservation); - free(val); - } else - ndev->reservation = 0; - - val = get_attr_value(grandchild, - "peak"); - if (val != NULL) { - sscanf(val, "%" PRIu64, - &ndev->limit); - free(val); - } else - ndev->limit = 0; - break; - } - } + /* Network QoS bandwidth support */ + /* Only expose inbound bandwidth */ + char *val; + if (seek_in_others(&ndev->others, + -1, + "bandwidth", + TYPE_NODE, + -1, + (char *)inode->name) && + seek_in_others(&ndev->others, + -1, + "inbound", + TYPE_NODE, + -1, + "bandwidth")) { + val = fetch_from_others(&ndev->others, + -1, + "average", + TYPE_PROP, + -1, + "inbound"); + + if (val != NULL) { + sscanf(val, "%" PRIu64, &ndev->reservation); + free(val); + } else { + ndev->reservation = 0; } -#endif + val = fetch_from_others(&ndev->others, + -1, + "peak", + TYPE_PROP, + -1, + "inbound"); + + if (val != NULL) { + sscanf(val, "%" PRIu64, &ndev->limit); + free(val); + } else { + ndev->limit = 0; + } } +#endif if (ndev->source == NULL) CU_DEBUG("No network source defined, leaving blank\n"); diff --git a/libxkutil/device_parsing.h b/libxkutil/device_parsing.h index 6e8fe25..4d26fc8 100644 --- a/libxkutil/device_parsing.h +++ b/libxkutil/device_parsing.h @@ -100,6 +100,7 @@ struct net_device { uint64_t limit; struct vsi_device vsi; struct device_address address; + struct others *others; }; struct mem_device { -- 1.8.3.1
From: Xu Wang <cngesaint@gmail.com> Signed-off-by: Xu Wang <gesaint@linux.vnet.ibm.com> --- libxkutil/device_parsing.c | 35 ++++++++++++++++++++++++++++++----- libxkutil/device_parsing.h | 1 + 2 files changed, 31 insertions(+), 5 deletions(-) diff --git a/libxkutil/device_parsing.c b/libxkutil/device_parsing.c index 8b39cda..8082b37 100644 --- a/libxkutil/device_parsing.c +++ b/libxkutil/device_parsing.c @@ -151,6 +151,14 @@ static void cleanup_net_device(struct net_device *dev) cleanup_others(dev->others); } +static void cleanup_vcpu_device(struct vcpu_device *dev) +{ + if (dev == NULL) + return; + + cleanup_others(dev->others); +} + static void cleanup_emu_device(struct emu_device *dev) { if (dev == NULL) @@ -339,6 +347,8 @@ void cleanup_virt_device(struct virt_device *dev) cleanup_disk_device(&dev->dev.disk); else if (dev->type == CIM_RES_TYPE_NET) cleanup_net_device(&dev->dev.net); + else if (dev->type == CIM_RES_TYPE_PROC) + cleanup_vcpu_device(&dev->dev.vcpu); else if (dev->type == CIM_RES_TYPE_EMU) cleanup_emu_device(&dev->dev.emu); else if (dev->type == CIM_RES_TYPE_GRAPHICS) @@ -1450,7 +1460,26 @@ static int parse_vcpu_device(xmlNode *node, struct virt_device **vdevs) char *count_str; int count; - count_str = get_node_content(node); + CU_DEBUG("Enter parse_vcpu_device()."); + + list = calloc(1, sizeof(*list)); + if (list == NULL) { + CU_DEBUG("calloc failed."); + goto err; + } + + list->dev.vcpu.others = parse_data_to_others(list->dev.vcpu.others, + node, + 0, + BAD_CAST "domain"); + + count_str = fetch_from_others(&list->dev.vcpu.others, + -1, + "vcpu", + TYPE_NODE, + -1, + "domain"); + if (count_str == NULL) count = 1; /* Default to 1 VCPU if non specified */ else if (sscanf(count_str, "%i", &count) != 1) @@ -1458,10 +1487,6 @@ static int parse_vcpu_device(xmlNode *node, struct virt_device **vdevs) free(count_str); - list = calloc(1, sizeof(*list)); - if (list == NULL) - goto err; - list->dev.vcpu.quantity = count; list->type = CIM_RES_TYPE_PROC; diff --git a/libxkutil/device_parsing.h b/libxkutil/device_parsing.h index 4d26fc8..c9e2b29 100644 --- a/libxkutil/device_parsing.h +++ b/libxkutil/device_parsing.h @@ -115,6 +115,7 @@ struct vcpu_device { uint64_t quantity; uint32_t weight; uint64_t limit; + struct others *others; }; struct emu_device { -- 1.8.3.1
From: Xu Wang <cngesaint@gmail.com> Signed-off-by: Xu Wang <gesaint@linux.vnet.ibm.com> --- libxkutil/device_parsing.c | 24 +++++++++++++++++++++--- libxkutil/device_parsing.h | 1 + 2 files changed, 22 insertions(+), 3 deletions(-) diff --git a/libxkutil/device_parsing.c b/libxkutil/device_parsing.c index 8082b37..5fdf28e 100644 --- a/libxkutil/device_parsing.c +++ b/libxkutil/device_parsing.c @@ -165,6 +165,7 @@ static void cleanup_emu_device(struct emu_device *dev) return; free(dev->path); + cleanup_others(dev->others); } static void cleanup_vnc_device(struct graphics_device *dev) @@ -1506,15 +1507,32 @@ static int parse_emu_device(xmlNode *node, struct virt_device **vdevs) struct virt_device *vdev = NULL; struct emu_device *edev = NULL; + CU_DEBUG("Enter parse_emu_device()."); + vdev = calloc(1, sizeof(*vdev)); - if (vdev == NULL) + if (vdev == NULL) { + CU_DEBUG("calloc failed."); goto err; + } edev = &(vdev->dev.emu); - edev->path = get_node_content(node); - if (edev->path == NULL) + edev->others = parse_data_to_others(edev->others, + node, + 0, + BAD_CAST "devices"); + + edev->path = fetch_from_others(&edev->others, + -1, + (char *)node->name, + TYPE_NODE, + -1, + "devices"); + + if (edev->path == NULL) { + CU_DEBUG("no path"); goto err; + } vdev->type = CIM_RES_TYPE_EMU; diff --git a/libxkutil/device_parsing.h b/libxkutil/device_parsing.h index c9e2b29..c5517e3 100644 --- a/libxkutil/device_parsing.h +++ b/libxkutil/device_parsing.h @@ -120,6 +120,7 @@ struct vcpu_device { struct emu_device { char *path; + struct others *others; }; struct vnc_device { -- 1.8.3.1
From: Xu Wang <cngesaint@gmail.com> Signed-off-by: Xu Wang <gesaint@linux.vnet.ibm.com> --- libxkutil/device_parsing.c | 52 +++++++++++++++++++++++++++++++++++++++++----- libxkutil/device_parsing.h | 1 + 2 files changed, 48 insertions(+), 5 deletions(-) diff --git a/libxkutil/device_parsing.c b/libxkutil/device_parsing.c index 5fdf28e..0a74ff3 100644 --- a/libxkutil/device_parsing.c +++ b/libxkutil/device_parsing.c @@ -159,6 +159,14 @@ static void cleanup_vcpu_device(struct vcpu_device *dev) cleanup_others(dev->others); } +static void cleanup_mem_device(struct mem_device *dev) +{ + if (dev == NULL) + return; + + cleanup_others(dev->others); +} + static void cleanup_emu_device(struct emu_device *dev) { if (dev == NULL) @@ -350,6 +358,8 @@ void cleanup_virt_device(struct virt_device *dev) cleanup_net_device(&dev->dev.net); else if (dev->type == CIM_RES_TYPE_PROC) cleanup_vcpu_device(&dev->dev.vcpu); + else if (dev->type == CIM_RES_TYPE_MEM) + cleanup_mem_device(&dev->dev.mem); else if (dev->type == CIM_RES_TYPE_EMU) cleanup_emu_device(&dev->dev.emu); else if (dev->type == CIM_RES_TYPE_GRAPHICS) @@ -1553,20 +1563,50 @@ static int parse_mem_device(xmlNode *node, struct virt_device **vdevs) char *content = NULL; char *tmpval = NULL; int ret = 0; + struct others *new_others = NULL; + + CU_DEBUG("Enter parse_mem_device()."); vdev = calloc(1, sizeof(*vdev)); - if (vdev == NULL) + if (vdev == NULL) { + CU_DEBUG("calloc failed."); goto err; + } mdev = &(vdev->dev.mem); - content = get_node_content(node); + new_others = parse_data_to_others(new_others, + node, + 0, + BAD_CAST "domain"); + mdev->others = combine_others(mdev->others, new_others); + + if (XSTREQ(node->name, "currentMemory")) { + content = fetch_from_others(&mdev->others, + -1, + "currentMemory", + TYPE_NODE, + -1, + "domain"); - if (XSTREQ(node->name, "currentMemory")) sscanf(content, "%" PRIu64, &mdev->size); - else if (XSTREQ(node->name, "memory")) { + } else if (XSTREQ(node->name, "memory")) { + content = fetch_from_others(&mdev->others, + -1, + "memory", + TYPE_NODE, + -1, + "domain"); + sscanf(content, "%" PRIu64, &mdev->maxsize); - tmpval = get_attr_value(node, "dumpCore"); + + tmpval = fetch_from_others(&mdev->others, + -1, + "dumpCore", + TYPE_PROP, + -1, + "memory"); + if (tmpval && XSTREQ(tmpval, "on")) { mdev->dumpCore = MEM_DUMP_CORE_ON; } else if (tmpval && XSTREQ(content, "off")) { @@ -1574,6 +1614,8 @@ static int parse_mem_device(xmlNode *node, struct virt_device **vdevs) } else { mdev->dumpCore = MEM_DUMP_CORE_NOT_SET; } + } else { + /* do nothing */ } *vdevs = vdev; diff --git a/libxkutil/device_parsing.h b/libxkutil/device_parsing.h index c5517e3..0855166 100644 --- a/libxkutil/device_parsing.h +++ b/libxkutil/device_parsing.h @@ -109,6 +109,7 @@ struct mem_device { enum { MEM_DUMP_CORE_NOT_SET, MEM_DUMP_CORE_ON, MEM_DUMP_CORE_OFF } dumpCore; + struct others *others; }; struct vcpu_device { -- 1.8.3.1
From: Xu Wang <cngesaint@gmail.com> Signed-off-by: Xu Wang <gesaint@linux.vnet.ibm.com> --- libxkutil/device_parsing.c | 273 ++++++++++++++++++++++++++++++++++----------- libxkutil/device_parsing.h | 1 + 2 files changed, 207 insertions(+), 67 deletions(-) diff --git a/libxkutil/device_parsing.c b/libxkutil/device_parsing.c index 0a74ff3..8edd2e3 100644 --- a/libxkutil/device_parsing.c +++ b/libxkutil/device_parsing.c @@ -287,6 +287,7 @@ static void cleanup_console_device(struct console_device *dev) dev->source_type = 0; free(dev->target_type); memset(&dev->source_dev, 0, sizeof(dev->source_dev)); + cleanup_others(dev->others); }; static void console_device_dup(struct console_device *t, @@ -1648,18 +1649,37 @@ static int parse_console_device(xmlNode *node, struct virt_device **vdevs) char *source_type_str = NULL; char *target_port_ID = NULL; char *udp_source_mode = NULL; + int id; - xmlNode *child = NULL; + CU_DEBUG("Enter parse_console_device()."); vdev = calloc(1, sizeof(*vdev)); - if (vdev == NULL) + if (vdev == NULL) { + CU_DEBUG("calloc failed."); goto err; + } cdev = &(vdev->dev.console); - source_type_str = get_attr_value(node, "type"); - if (source_type_str == NULL) + cdev->others = parse_data_to_others(cdev->others, + node, + 0, + BAD_CAST "devices"); + if (cdev->others == NULL) { + CU_DEBUG("parse data to others failed."); + goto err; + } + + source_type_str = fetch_from_others(&cdev->others, + -1, + "type", + TYPE_PROP, + -1, + (char *)node->name); + if (source_type_str == NULL) { + CU_DEBUG("no type"); goto err; + } CU_DEBUG("console device type = %s", source_type_str ? : "NULL"); cdev->source_type = chardev_source_type_StrToID(source_type_str); @@ -1668,85 +1688,204 @@ static int parse_console_device(xmlNode *node, struct virt_device **vdevs) CU_DEBUG("console device type ID = %d", cdev->source_type); - for (child = node->children; child != NULL; child = child->next) { - if (XSTREQ(child->name, "target")) { - cdev->target_type = get_attr_value(child, "type"); - CU_DEBUG("Console device target type = '%s'", - cdev->target_type ? : "NULL"); - target_port_ID = get_attr_value(child, "port"); - if (target_port_ID == NULL) - goto err; - } + if (seek_in_others(&cdev->others, + -1, + "target", + TYPE_NODE, + -1, + (char *)node->name)) { + cdev->target_type = fetch_from_others(&cdev->others, + -1, + "type", + TYPE_PROP, + -1, + "target"); + CU_DEBUG("Console device target type = '%s'", + cdev->target_type ? : "NULL"); - if (XSTREQ(child->name, "source")) { - switch (cdev->source_type) - { - case CIM_CHARDEV_SOURCE_TYPE_PTY: - cdev->source_dev.pty.path = - get_attr_value(child, "path"); - break; - case CIM_CHARDEV_SOURCE_TYPE_DEV: - cdev->source_dev.dev.path = - get_attr_value(child, "path"); - break; - case CIM_CHARDEV_SOURCE_TYPE_FILE: - cdev->source_dev.file.path = - get_attr_value(child, "path"); - break; - case CIM_CHARDEV_SOURCE_TYPE_PIPE: - cdev->source_dev.pipe.path = - get_attr_value(child, "path"); - break; - case CIM_CHARDEV_SOURCE_TYPE_UNIXSOCK: - cdev->source_dev.unixsock.mode = - get_attr_value(child, "mode"); - cdev->source_dev.unixsock.path = - get_attr_value(child, "path"); - break; - case CIM_CHARDEV_SOURCE_TYPE_UDP: - udp_source_mode = get_attr_value(child, "mode"); + target_port_ID = fetch_from_others(&cdev->others, + -1, + "port", + TYPE_PROP, + -1, + "target"); + if (target_port_ID == NULL) + goto err; + } + + if (seek_in_others(&cdev->others, + 0, + "source", + TYPE_NODE, + -1, + (char *)node->name)) { + switch (cdev->source_type) + { + case CIM_CHARDEV_SOURCE_TYPE_PTY: + cdev->source_dev.pty.path = + fetch_from_others(&cdev->others, + -1, + "path", + TYPE_PROP, + -1, + "source"); + break; + case CIM_CHARDEV_SOURCE_TYPE_DEV: + cdev->source_dev.dev.path = + fetch_from_others(&cdev->others, + -1, + "path", + TYPE_PROP, + -1, + "source"); + break; + case CIM_CHARDEV_SOURCE_TYPE_FILE: + cdev->source_dev.file.path = + fetch_from_others(&cdev->others, + -1, + "path", + TYPE_PROP, + -1, + "source"); + break; + case CIM_CHARDEV_SOURCE_TYPE_PIPE: + cdev->source_dev.pipe.path = + fetch_from_others(&cdev->others, + -1, + "path", + TYPE_PROP, + -1, + "source"); + break; + case CIM_CHARDEV_SOURCE_TYPE_UNIXSOCK: + cdev->source_dev.unixsock.mode = + fetch_from_others(&cdev->others, + -1, + "mode", + TYPE_PROP, + -1, + "source"); + cdev->source_dev.unixsock.path = + fetch_from_others(&cdev->others, + -1, + "path", + TYPE_PROP, + -1, + "source"); + break; + case CIM_CHARDEV_SOURCE_TYPE_UDP: + /* Something different from other cases. It has two + * <source> tags with mode="bind" and "connect" value. + * Hence here id MUST NOT be ignored. + */ + + /* Just fetch out another <source> tag but we need to do + * nothing with it. + */ + fetch_from_others(&cdev->others, + 1, + "source", + TYPE_NODE, + -1, + "console"); + + for (id = 0; id < 2; id++) { + udp_source_mode = + fetch_from_others(&cdev->others, + -1, + "mode", + TYPE_PROP, + id, + "source"); if (udp_source_mode == NULL) goto err; + if (STREQC(udp_source_mode, "bind")) { cdev->source_dev.udp.bind_host = - get_attr_value(child, "host"); + fetch_from_others(&cdev->others, + -1, + "host", + TYPE_PROP, + id, + "source"); cdev->source_dev.udp.bind_service = - get_attr_value(child, "service"); + fetch_from_others(&cdev->others, + -1, + "service", + TYPE_PROP, + id, + "source"); } else if (STREQC(udp_source_mode, "connect")) { cdev->source_dev.udp.connect_host = - get_attr_value(child, "host"); + fetch_from_others(&cdev->others, + -1, + "host", + TYPE_PROP, + id, + "source"); cdev->source_dev.udp.connect_service = - get_attr_value(child, "service"); + fetch_from_others(&cdev->others, + -1, + "service", + TYPE_PROP, + id, + "source"); } else { CU_DEBUG("unknown udp mode: %s", udp_source_mode ? : "NULL"); goto err; } - break; - case CIM_CHARDEV_SOURCE_TYPE_TCP: - cdev->source_dev.tcp.mode = - get_attr_value(child, "mode"); - cdev->source_dev.tcp.host = - get_attr_value(child, "host"); - cdev->source_dev.tcp.service = - get_attr_value(child, "service"); - break; - - default: - /* Nothing to do for : - CIM_CHARDEV_SOURCE_TYPE_STDIO - CIM_CHARDEV_SOURCE_TYPE_NULL - CIM_CHARDEV_SOURCE_TYPE_VC - CIM_CHARDEV_SOURCE_TYPE_SPICEVMC - */ - break; } + break; + case CIM_CHARDEV_SOURCE_TYPE_TCP: + cdev->source_dev.tcp.mode = + fetch_from_others(&cdev->others, + -1, + "mode", + TYPE_PROP, + -1, + "source"); + cdev->source_dev.tcp.host = + fetch_from_others(&cdev->others, + -1, + "host", + TYPE_PROP, + -1, + "source"); + cdev->source_dev.tcp.service = + fetch_from_others(&cdev->others, + -1, + "service", + TYPE_PROP, + -1, + "source"); + break; + + default: + /* Nothing to do for : + CIM_CHARDEV_SOURCE_TYPE_STDIO + CIM_CHARDEV_SOURCE_TYPE_NULL + CIM_CHARDEV_SOURCE_TYPE_VC + CIM_CHARDEV_SOURCE_TYPE_SPICEVMC + */ + break; } - if ((cdev->source_type == CIM_CHARDEV_SOURCE_TYPE_TCP) - && XSTREQ(child->name, "protocol")) { - cdev->source_dev.tcp.protocol = - get_attr_value(child, "type"); - } + } + + if ((cdev->source_type == CIM_CHARDEV_SOURCE_TYPE_TCP) + && seek_in_others(&cdev->others, + -1, + "protocol", + TYPE_NODE, + -1, + (char *)node->name)) { + cdev->source_dev.tcp.protocol = + fetch_from_others(&cdev->others, + -1, + "type", + TYPE_PROP, + -1, + "protocol"); } vdev->type = CIM_RES_TYPE_CONSOLE; diff --git a/libxkutil/device_parsing.h b/libxkutil/device_parsing.h index 0855166..24b7c1d 100644 --- a/libxkutil/device_parsing.h +++ b/libxkutil/device_parsing.h @@ -180,6 +180,7 @@ struct console_device { struct udp_device udp; } source_dev; char *target_type; + struct others *others; }; struct input_device { -- 1.8.3.1
From: Xu Wang <cngesaint@gmail.com> Signed-off-by: Xu Wang <gesaint@linux.vnet.ibm.com> --- libxkutil/device_parsing.c | 155 +++++++++++++++++++++++++++++++++++---------- libxkutil/device_parsing.h | 1 + 2 files changed, 121 insertions(+), 35 deletions(-) diff --git a/libxkutil/device_parsing.c b/libxkutil/device_parsing.c index 8edd2e3..c77f3e5 100644 --- a/libxkutil/device_parsing.c +++ b/libxkutil/device_parsing.c @@ -202,6 +202,7 @@ static void cleanup_graphics_device(struct graphics_device *dev) cleanup_vnc_device(dev); free(dev->type); + cleanup_others(dev->others); } static void cleanup_path_device(struct path_device *dev) @@ -1631,17 +1632,6 @@ static int parse_mem_device(xmlNode *node, struct virt_device **vdevs) return ret; } -static char *get_attr_value_default(xmlNode *node, char *attrname, - const char *default_value) -{ - char *ret = get_attr_value(node, attrname); - - if (ret == NULL && default_value != NULL) - ret = strdup(default_value); - - return ret; -} - static int parse_console_device(xmlNode *node, struct virt_device **vdevs) { struct virt_device *vdev = NULL; @@ -1916,28 +1906,91 @@ static int parse_graphics_device(xmlNode *node, struct virt_device **vdevs) { struct virt_device *vdev = NULL; struct graphics_device *gdev = NULL; - xmlNode *child = NULL; int ret; + CU_DEBUG("Enter parse_graphics_device()."); + vdev = calloc(1, sizeof(*vdev)); - if (vdev == NULL) + if (vdev == NULL) { + CU_DEBUG("calloc failed."); goto err; + } gdev = &(vdev->dev.graphics); - gdev->type = get_attr_value(node, "type"); - if (gdev->type == NULL) + gdev->others = parse_data_to_others(gdev->others, + node, + 0, + BAD_CAST "devices"); + if (gdev->others == NULL) { + CU_DEBUG("parse data to others failed."); goto err; + } + + /* fetch out <graphics> tag from others. It will be removed + * after others management finished. */ + fetch_from_others(&gdev->others, + -1, + "graphics", + TYPE_NODE, + -1, + "devices"); + fetch_from_others(&gdev->others, + -1, + "serial", + TYPE_NODE, + -1, + "devices"); + + gdev->type = fetch_from_others(&gdev->others, + -1, + "type", + TYPE_PROP, + -1, + (char *)node->name); + if (gdev->type == NULL) { + CU_DEBUG("no type"); + goto err; + } CU_DEBUG("graphics device type = %s", gdev->type); if (STREQC(gdev->type, "vnc")) { - gdev->dev.vnc.port = get_attr_value_default(node, "port", - "-1"); - gdev->dev.vnc.host = get_attr_value_default(node, "listen", - "127.0.0.1"); - gdev->dev.vnc.keymap = get_attr_value(node, "keymap"); - gdev->dev.vnc.passwd = get_attr_value(node, "passwd"); + gdev->dev.vnc.port = fetch_from_others(&gdev->others, + -1, + "port", + TYPE_PROP, + -1, + (char *)node->name); + + if (gdev->dev.vnc.port == NULL) { + gdev->dev.vnc.port = strdup("-1"); + } + + gdev->dev.vnc.host = fetch_from_others(&gdev->others, + -1, + "listen", + TYPE_PROP, + -1, + (char *)node->name); + + if (gdev->dev.vnc.host == NULL) { + gdev->dev.vnc.host = strdup("127.0.0.1"); + } + + gdev->dev.vnc.keymap = fetch_from_others(&gdev->others, + -1, + "keymap", + TYPE_PROP, + -1, + (char *)node->name); + + gdev->dev.vnc.passwd = fetch_from_others(&gdev->others, + -1, + "passwd", + TYPE_PROP, + -1, + (char *)node->name); if (gdev->dev.vnc.port == NULL || gdev->dev.vnc.host == NULL) { CU_DEBUG("Error vnc port '%p' host '%p'", @@ -1946,27 +1999,59 @@ static int parse_graphics_device(xmlNode *node, struct virt_device **vdevs) } } else if (STREQC(gdev->type, "sdl")) { - gdev->dev.sdl.display = get_attr_value(node, "display"); - gdev->dev.sdl.xauth = get_attr_value(node, "xauth"); - gdev->dev.sdl.fullscreen = get_attr_value(node, "fullscreen"); + gdev->dev.sdl.display = fetch_from_others(&gdev->others, + -1, + "display", + TYPE_PROP, + -1, + (char *)node->name); + + gdev->dev.sdl.xauth = fetch_from_others(&gdev->others, + -1, + "xauth", + TYPE_PROP, + -1, + (char *)node->name); + + gdev->dev.sdl.fullscreen = fetch_from_others(&gdev->others, + -1, + "fullscreen", + TYPE_PROP, + -1, + (char *)node->name); } else if (STREQC(gdev->type, "pty")) { - if (node->name == NULL) - goto err; - /* Change type to serial, console, etc. It will be converted * back in xmlgen.c */ free(gdev->type); gdev->type = strdup((char *)node->name); - for (child = node->children; child != NULL; - child = child->next) { - if (XSTREQ(child->name, "source")) - gdev->dev.vnc.host = get_attr_value(child, "path"); - else if (XSTREQ(child->name, "target")) { - gdev->dev.vnc.port = - get_attr_value(child, "port"); - } + if (seek_in_others(&gdev->others, + -1, + "source", + TYPE_NODE, + -1, + (char *)node->name)) { + gdev->dev.vnc.host = fetch_from_others(&gdev->others, + -1, + "path", + TYPE_PROP, + -1, + "source"); + } + + if (seek_in_others(&gdev->others, + -1, + "target", + TYPE_NODE, + -1, + (char *)node->name)) { + gdev->dev.vnc.port = fetch_from_others(&gdev->others, + -1, + "port", + TYPE_PROP, + -1, + "target"); } } else { diff --git a/libxkutil/device_parsing.h b/libxkutil/device_parsing.h index 24b7c1d..b29e58c 100644 --- a/libxkutil/device_parsing.h +++ b/libxkutil/device_parsing.h @@ -143,6 +143,7 @@ struct graphics_device { struct vnc_device vnc; struct sdl_device sdl; } dev; + struct others *others; }; struct path_device { -- 1.8.3.1
From: Xu Wang <cngesaint@gmail.com> Signed-off-by: Xu Wang <gesaint@linux.vnet.ibm.com> --- libxkutil/device_parsing.c | 41 ++++++++++++++++++++++++++++++++++++++--- libxkutil/device_parsing.h | 1 + 2 files changed, 39 insertions(+), 3 deletions(-) diff --git a/libxkutil/device_parsing.c b/libxkutil/device_parsing.c index c77f3e5..5a012bc 100644 --- a/libxkutil/device_parsing.c +++ b/libxkutil/device_parsing.c @@ -347,6 +347,7 @@ static void cleanup_input_device(struct input_device *dev) free(dev->type); free(dev->bus); + cleanup_others(dev->others); } void cleanup_virt_device(struct virt_device *dev) @@ -2087,14 +2088,48 @@ static int parse_input_device(xmlNode *node, struct virt_device **vdevs) struct input_device *idev = NULL; int ret; + CU_DEBUG("Enter parse_input_device()."); + vdev = calloc(1, sizeof(*vdev)); - if (vdev == NULL) + if (vdev == NULL) { + CU_DEBUG("calloc failed."); goto err; + } idev = &(vdev->dev.input); - idev->type = get_attr_value(node, "type"); - idev->bus = get_attr_value(node, "bus"); + idev->others = parse_data_to_others(idev->others, + node, + 0, + BAD_CAST "devices"); + + if (idev->others == NULL) { + CU_DEBUG("parse data to others failed."); + goto err; + } + + /* fetch out <input> tag from others. It will be removed + * after others management finished. */ + fetch_from_others(&idev->others, + -1, + "input", + TYPE_NODE, + -1, + "devices"); + + idev->type = fetch_from_others(&idev->others, + -1, + "type", + TYPE_PROP, + -1, + (char *)node->name); + + idev->bus = fetch_from_others(&idev->others, + -1, + "bus", + TYPE_PROP, + -1, + (char *)node->name); if ((idev->type == NULL) || (idev->bus == NULL)) goto err; diff --git a/libxkutil/device_parsing.h b/libxkutil/device_parsing.h index b29e58c..275d91f 100644 --- a/libxkutil/device_parsing.h +++ b/libxkutil/device_parsing.h @@ -187,6 +187,7 @@ struct console_device { struct input_device { char *type; char *bus; + struct others *others; }; struct virt_device { -- 1.8.3.1
From: Xu Wang <cngesaint@gmail.com> Signed-off-by: Xu Wang <gesaint@linux.vnet.ibm.com> --- libxkutil/device_parsing.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++ libxkutil/device_parsing.h | 7 +++++ src/svpc_types.h | 1 + 3 files changed, 83 insertions(+) diff --git a/libxkutil/device_parsing.c b/libxkutil/device_parsing.c index 5a012bc..a6a6cfe 100644 --- a/libxkutil/device_parsing.c +++ b/libxkutil/device_parsing.c @@ -101,6 +101,15 @@ static void cleanup_others(struct others *others) } } +static void cleanup_unknown_device(struct unknown_device *dev) +{ + if (dev == NULL) + return; + + free(dev->name); + cleanup_others(dev->others); +} + static void cleanup_disk_device(struct disk_device *dev) { if (dev == NULL) @@ -371,6 +380,8 @@ void cleanup_virt_device(struct virt_device *dev) cleanup_input_device(&dev->dev.input); else if (dev->type == CIM_RES_TYPE_CONSOLE) cleanup_console_device(&dev->dev.console); + else if (dev->type == CIM_RES_TYPE_UNKDEV) + cleanup_unknown_device(&dev->dev.unknown); free(dev->id); @@ -2152,6 +2163,70 @@ static int parse_input_device(xmlNode *node, struct virt_device **vdevs) return 0; } +static int parse_unknown_device(xmlNode *node, struct virt_device **vdevs) +{ + struct virt_device *vdev = NULL; + struct unknown_device *udev = NULL; + xmlNode *child = NULL; + + CU_DEBUG("Enter parse_unknown_device()."); + + vdev = calloc(1, sizeof(*vdev)); + if (vdev == NULL) { + CU_DEBUG("calloc failed."); + goto err; + } + + udev = &(vdev->dev.unknown); + + for (child = node->children; child != NULL; child = child->next) { + /* Skip all items parsed in other parse_*() functions. + * Everything here is just to be compatible with old versions. + * Here may need some improvement in the future. + */ + if (XSTREQ(child->name, "disk") || + XSTREQ(child->name, "filesystem") || + XSTREQ(child->name, "interface") || + XSTREQ(child->name, "emulator") || + XSTREQ(child->name, "graphics") || + XSTREQ(child->name, "console") || + XSTREQ(child->name, "serial") || + XSTREQ(child->name, "input")) { + /* Just skip them and do nothing */ + } else { + udev->others = parse_data_to_others(udev->others, + child, + 0, + BAD_CAST "devices"); + } + } + + if (udev->others == NULL) { + CU_DEBUG("no others."); + /* Should this really be an error? or success that + * we didn't find something unknown?!! + */ + goto err; + } + + vdev->type = CIM_RES_TYPE_UNKDEV; + + udev->name = strdup("unknown"); + if (udev->name == NULL) { + CU_DEBUG("Failed to strdup unknown name"); + goto err; + } + + *vdevs = vdev; + + return 1; +err: + cleanup_unknown_device(udev); + free(vdev); + + return 0; +} + static bool resize_devlist(struct virt_device **list, int newsize) { struct virt_device *_list; diff --git a/libxkutil/device_parsing.h b/libxkutil/device_parsing.h index 275d91f..a4e60ee 100644 --- a/libxkutil/device_parsing.h +++ b/libxkutil/device_parsing.h @@ -60,6 +60,12 @@ struct others { } status; }; +/* The structure for saving unknown device */ +struct unknown_device { + char *name; + struct others *others; +}; + struct vsi_device { char *vsi_type; char *manager_id; @@ -201,6 +207,7 @@ struct virt_device { struct graphics_device graphics; struct console_device console; struct input_device input; + struct unknown_device unknown; } dev; char *id; }; diff --git a/src/svpc_types.h b/src/svpc_types.h index 404e428..4928742 100644 --- a/src/svpc_types.h +++ b/src/svpc_types.h @@ -33,6 +33,7 @@ #define CIM_RES_TYPE_GRAPHICS 24 #define CIM_RES_TYPE_INPUT 13 #define CIM_RES_TYPE_UNKNOWN 1000 +#define CIM_RES_TYPE_UNKDEV 1001 #define CIM_RES_TYPE_IMAGE 32768 #define CIM_RES_TYPE_CONSOLE 32769 #define CIM_RES_TYPE_EMU 32770 -- 1.8.3.1
From: Xu Wang <cngesaint@gmail.com> Signed-off-by: Xu Wang <gesaint@linux.vnet.ibm.com> --- libxkutil/device_parsing.c | 10 ++++++++++ libxkutil/device_parsing.h | 3 +++ 2 files changed, 13 insertions(+) diff --git a/libxkutil/device_parsing.c b/libxkutil/device_parsing.c index a6a6cfe..99f6eda 100644 --- a/libxkutil/device_parsing.c +++ b/libxkutil/device_parsing.c @@ -49,6 +49,7 @@ #define GRAPHICS_XPATH (xmlChar *)"/domain/devices/graphics | "\ "/domain/devices/console" #define INPUT_XPATH (xmlChar *)"/domain/devices/input" +#define UNKNOWN_XPATH (xmlChar *)"/domain/devices" #define DEFAULT_BRIDGE "xenbr0" #define DEFAULT_NETWORK "default" @@ -2350,6 +2351,11 @@ static int parse_devices(const char *xml, struct virt_device **_list, int type) func = &parse_input_device; break; + case CIM_RES_TYPE_UNKNOWN: + xpathstr = UNKNOWN_XPATH; + func = &parse_unknown_device; + break; + default: CU_DEBUG("Unrecognized device type. Returning."); goto err1; @@ -2846,6 +2852,9 @@ int get_dominfo_from_xml(const char *xml, struct domain **dominfo) (*dominfo)->dev_vcpu_ct = parse_devices(xml, &(*dominfo)->dev_vcpu, CIM_RES_TYPE_PROC); + (*dominfo)->dev_unknown_ct = parse_devices(xml, + &(*dominfo)->dev_unknown, + CIM_RES_TYPE_UNKNOWN); return ret; @@ -2934,6 +2943,7 @@ void cleanup_dominfo(struct domain **dominfo) cleanup_virt_devices(&dom->dev_graphics, dom->dev_graphics_ct); cleanup_virt_devices(&dom->dev_input, dom->dev_input_ct); cleanup_virt_devices(&dom->dev_console, dom->dev_console_ct); + cleanup_virt_devices(&dom->dev_unknown, dom->dev_unknown_ct); free(dom); diff --git a/libxkutil/device_parsing.h b/libxkutil/device_parsing.h index a4e60ee..fa8aa17 100644 --- a/libxkutil/device_parsing.h +++ b/libxkutil/device_parsing.h @@ -283,6 +283,9 @@ struct domain { struct virt_device *dev_vcpu; int dev_vcpu_ct; + + struct virt_device *dev_unknown; + int dev_unknown_ct; }; struct virt_device *virt_device_dup(struct virt_device *dev); -- 1.8.3.1
From: Xu Wang <cngesaint@gmail.com> Signed-off-by: Xu Wang <gesaint@linux.vnet.ibm.com> --- libxkutil/device_parsing.c | 164 +++++++++++++++++++++++++++++++++++++-------- libxkutil/device_parsing.h | 2 + 2 files changed, 139 insertions(+), 27 deletions(-) diff --git a/libxkutil/device_parsing.c b/libxkutil/device_parsing.c index 99f6eda..b244d8a 100644 --- a/libxkutil/device_parsing.c +++ b/libxkutil/device_parsing.c @@ -2736,10 +2736,8 @@ static int parse_features(struct domain *dominfo, xmlNode *features) return 1; } -static void set_action(uint16_t *val, xmlNode *child) +static void set_action(uint16_t *val, const char *action) { - char *action = (char *)xmlNodeGetContent(child); - if (action == NULL) *val = CIM_VSSD_RECOVERY_NONE; else if (STREQ(action, "destroy")) @@ -2750,38 +2748,148 @@ static void set_action(uint16_t *val, xmlNode *child) *val = CIM_VSSD_RECOVERY_RESTART; else *val = CIM_VSSD_RECOVERY_NONE; - - xmlFree(action); } static int parse_domain(xmlNodeSet *nsv, struct domain *dominfo) { xmlNode **nodes = nsv->nodeTab; xmlNode *child; + xmlAttrPtr xmlAttr = NULL; + char *action = NULL; + + CU_DEBUG("Enter parse_domain()"); + + /* parsing attributions of domain into others */ + xmlAttr = nodes[0]->properties; + while(xmlAttr) { + dominfo->others = add_others(dominfo->others, + nodes[0], + xmlAttr->name, + TYPE_PROP, + 0, + nodes[0]->name); + xmlAttr = xmlAttr->next; + } + + dominfo->typestr = fetch_from_others(&dominfo->others, + -1, + "type", + TYPE_PROP, + -1, + (char *)nodes[0]->name); + + /* parse every item in the field of domain and skip some will be parsed + * in other functions. The white list contains: + * <memory> (parsed in parse_mem_device()) + * <currentMemory> (parsed in parse_mem_device()) + * <vcpu> (parsed in parse_vcpu_device()) + * <devices> (parsed in other parse_*_device()) + */ + for (child = nodes[0]->children; child != NULL; child = child->next) { + if (XSTREQ(child->name, "memory") || + XSTREQ(child->name, "currentMemory") || + XSTREQ(child->name, "vcpu") || + XSTREQ(child->name, "devices")) { + continue; + } else { + dominfo->others = parse_data_to_others(dominfo->others, + child, + 0, + nodes[0]->name); + } + } - dominfo->typestr = get_attr_value(nodes[0], "type"); + dominfo->name = fetch_from_others(&dominfo->others, + -1, + "name", + TYPE_NODE, + -1, + (char *)nodes[0]->name); - for (child = nodes[0]->children; child != NULL; child = child->next) { - if (XSTREQ(child->name, "name")) - STRPROP(dominfo, name, child); - else if (XSTREQ(child->name, "uuid")) - STRPROP(dominfo, uuid, child); - else if (XSTREQ(child->name, "bootloader")) - STRPROP(dominfo, bootloader, child); - else if (XSTREQ(child->name, "bootloader_args")) - STRPROP(dominfo, bootloader_args, child); - else if (XSTREQ(child->name, "os")) - parse_os(dominfo, child); - else if (XSTREQ(child->name, "on_poweroff")) - set_action(&dominfo->on_poweroff, child); - else if (XSTREQ(child->name, "on_reboot")) - set_action(&dominfo->on_reboot, child); - else if (XSTREQ(child->name, "on_crash")) - set_action(&dominfo->on_crash, child); - else if (XSTREQ(child->name, "clock")) - dominfo->clock = get_attr_value(child, "offset"); - else if (XSTREQ(child->name, "features")) - parse_features(dominfo, child); + dominfo->uuid = fetch_from_others(&dominfo->others, + -1, + "uuid", + TYPE_NODE, + -1, + (char *)nodes[0]->name); + + dominfo->bootloader = fetch_from_others(&dominfo->others, + -1, + "bootloader", + TYPE_NODE, + -1, + (char *)nodes[0]->name); + + dominfo->bootloader_args = fetch_from_others(&dominfo->others, + -1, + "bootloader_args", + TYPE_NODE, + -1, + (char *)nodes[0]->name); + + if (seek_in_others(&dominfo->others, + -1, + "os", + TYPE_NODE, + -1, + (char *)nodes[0]->name)) { + /* parse_os(); */ + } + + action = fetch_from_others(&dominfo->others, + -1, + "on_poweroff", + TYPE_NODE, + -1, + (char *)nodes[0]->name); + if (action) { + set_action(&dominfo->on_poweroff, action); + free(action); + } + + action = fetch_from_others(&dominfo->others, + -1, + "on_reboot", + TYPE_NODE, + -1, + (char *)nodes[0]->name); + if (action) { + set_action(&dominfo->on_reboot, action); + free(action); + } + + action = fetch_from_others(&dominfo->others, + -1, + "on_crash", + TYPE_NODE, + -1, + (char *)nodes[0]->name); + if (action) { + set_action(&dominfo->on_crash, action); + free(action); + } + + if (seek_in_others(&dominfo->others, + -1, + "clock", + TYPE_NODE, + -1, + (char *)nodes[0]->name)) { + dominfo->clock = fetch_from_others(&dominfo->others, + -1, + "offset", + TYPE_PROP, + -1, + "clock"); + } + + if (seek_in_others(&dominfo->others, + -1, + "features", + TYPE_NODE, + -1, + (char *)nodes[0]->name)) { + /* parse_features(); */ } return 1; @@ -2945,6 +3053,8 @@ void cleanup_dominfo(struct domain **dominfo) cleanup_virt_devices(&dom->dev_console, dom->dev_console_ct); cleanup_virt_devices(&dom->dev_unknown, dom->dev_unknown_ct); + cleanup_others(dom->others); + free(dom); *dominfo = NULL; diff --git a/libxkutil/device_parsing.h b/libxkutil/device_parsing.h index fa8aa17..35011ae 100644 --- a/libxkutil/device_parsing.h +++ b/libxkutil/device_parsing.h @@ -286,6 +286,8 @@ struct domain { struct virt_device *dev_unknown; int dev_unknown_ct; + + struct others *others; }; struct virt_device *virt_device_dup(struct virt_device *dev); -- 1.8.3.1
From: Xu Wang <cngesaint@gmail.com> Signed-off-by: Xu Wang <gesaint@linux.vnet.ibm.com> --- libxkutil/device_parsing.c | 120 ++++++++++++++++++++++++++++++++------------- 1 file changed, 87 insertions(+), 33 deletions(-) diff --git a/libxkutil/device_parsing.c b/libxkutil/device_parsing.c index b244d8a..56c9d3d 100644 --- a/libxkutil/device_parsing.c +++ b/libxkutil/device_parsing.c @@ -2617,9 +2617,8 @@ static void cleanup_bootlist(char **blist, unsigned blist_ct) free(blist); } -static int parse_os(struct domain *dominfo, xmlNode *os) +static int parse_os(struct domain *dominfo) { - xmlNode *child; char **blist = NULL; unsigned bl_size = 0; char *arch = NULL; @@ -2631,39 +2630,93 @@ static int parse_os(struct domain *dominfo, xmlNode *os) char *boot = NULL; char *init = NULL; - for (child = os->children; child != NULL; child = child->next) { - if (XSTREQ(child->name, "type")) { - STRPROP(dominfo, os_info.pv.type, child); - arch = get_attr_value(child, "arch"); - machine = get_attr_value(child, "machine"); - } else if (XSTREQ(child->name, "kernel")) - kernel = get_node_content(child); - else if (XSTREQ(child->name, "initrd")) - initrd = get_node_content(child); - else if (XSTREQ(child->name, "cmdline")) - cmdline = get_node_content(child); - else if (XSTREQ(child->name, "loader")) - loader = get_node_content(child); - else if (XSTREQ(child->name, "boot")) { - char **tmp_list = NULL; - - tmp_list = (char **)realloc(blist, - (bl_size + 1) * - sizeof(char *)); - if (tmp_list == NULL) { - // Nothing you can do. Just go on. - CU_DEBUG("Could not alloc space for " - "boot device"); - continue; - } - blist = tmp_list; + CU_DEBUG("Enter parse_os()"); + + dominfo->os_info.pv.type = fetch_from_others(&dominfo->others, + 0, + "type", + TYPE_NODE, + 0, + "os"); + if (dominfo->os_info.pv.type) { + arch = fetch_from_others(&dominfo->others, + -1, + "arch", + TYPE_PROP, + -1, + "type"); + + machine = fetch_from_others(&dominfo->others, + -1, + "machine", + TYPE_PROP, + -1, + "type"); + } + + dominfo->os_info.pv.kernel = fetch_from_others(&dominfo->others, + -1, + "kernel", + TYPE_NODE, + -1, + "os"); + + dominfo->os_info.pv.initrd = fetch_from_others(&dominfo->others, + -1, + "initrd", + TYPE_NODE, + -1, + "os"); + + dominfo->os_info.pv.cmdline = fetch_from_others(&dominfo->others, + -1, + "cmdline", + TYPE_NODE, + -1, + "os"); + + dominfo->os_info.fv.loader = fetch_from_others(&dominfo->others, + -1, + "loader", + TYPE_NODE, + -1, + "os"); + + if (seek_in_others(&dominfo->others, + -1, + "boot", + TYPE_NODE, + -1, + "os")) { + char **tmp_list = NULL; + + tmp_list = (char **)realloc(blist, + (bl_size + 1) * sizeof(char *)); + + if (tmp_list == NULL) { + /* Nothing you can do. Just go on. */ + CU_DEBUG("Could not alloc space for " + "boot device"); + } else { + blist = tmp_list; - blist[bl_size] = get_attr_value(child, "dev"); + blist[bl_size] = fetch_from_others(&dominfo->others, + -1, + "dev", + TYPE_PROP, + -1, + "boot"); bl_size++; - } else if (XSTREQ(child->name, "init")) - init = get_node_content(child); + } } + dominfo->os_info.lxc.init = fetch_from_others(&dominfo->others, + -1, + "init", + TYPE_NODE, + -1, + "os"); + if ((STREQC(dominfo->os_info.fv.type, "hvm")) && (STREQC(dominfo->typestr, "xen"))) dominfo->type = DOMAIN_XENFV; @@ -2673,7 +2726,8 @@ static int parse_os(struct domain *dominfo, xmlNode *os) dominfo->type = DOMAIN_QEMU; else if (STREQC(dominfo->typestr, "lxc")) dominfo->type = DOMAIN_LXC; - else if (STREQC(dominfo->os_info.pv.type, "linux")) + else if (dominfo->os_info.pv.type && + STREQC(dominfo->os_info.pv.type, "linux")) dominfo->type = DOMAIN_XENPV; else dominfo->type = -1; @@ -2833,7 +2887,7 @@ static int parse_domain(xmlNodeSet *nsv, struct domain *dominfo) TYPE_NODE, -1, (char *)nodes[0]->name)) { - /* parse_os(); */ + parse_os(dominfo); } action = fetch_from_others(&dominfo->others, -- 1.8.3.1
From: Xu Wang <cngesaint@gmail.com> Signed-off-by: Xu Wang <gesaint@linux.vnet.ibm.com> --- libxkutil/device_parsing.c | 38 ++++++++++++++++++++++++++++---------- 1 file changed, 28 insertions(+), 10 deletions(-) diff --git a/libxkutil/device_parsing.c b/libxkutil/device_parsing.c index 56c9d3d..43174c1 100644 --- a/libxkutil/device_parsing.c +++ b/libxkutil/device_parsing.c @@ -2774,17 +2774,35 @@ static int parse_os(struct domain *dominfo) return 1; } -static int parse_features(struct domain *dominfo, xmlNode *features) +static int parse_features(struct domain *dominfo) { - xmlNode *child; + CU_DEBUG("Enter parse_features()"); - for (child = features->children; child != NULL; child = child->next) { - if (XSTREQ(child->name, "acpi")) - dominfo->acpi = true; - else if (XSTREQ(child->name, "apic")) - dominfo->apic = true; - else if (XSTREQ(child->name, "pae")) - dominfo->pae = true; + if (seek_in_others(&dominfo->others, + -1, + "acpi", + TYPE_NODE, + -1, + "features")) { + dominfo->acpi = true; + } + + if (seek_in_others(&dominfo->others, + -1, + "apic", + TYPE_NODE, + -1, + "features")) { + dominfo->apic = true; + } + + if (seek_in_others(&dominfo->others, + -1, + "pae", + TYPE_NODE, + -1, + "features")) { + dominfo->pae = true; } return 1; @@ -2943,7 +2961,7 @@ static int parse_domain(xmlNodeSet *nsv, struct domain *dominfo) TYPE_NODE, -1, (char *)nodes[0]->name)) { - /* parse_features(); */ + parse_features(dominfo); } return 1; -- 1.8.3.1
From: Xu Wang <cngesaint@gmail.com> Signed-off-by: Xu Wang <gesaint@linux.vnet.ibm.com> --- libxkutil/device_parsing.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/libxkutil/device_parsing.c b/libxkutil/device_parsing.c index 43174c1..a0ef407 100644 --- a/libxkutil/device_parsing.c +++ b/libxkutil/device_parsing.c @@ -2412,6 +2412,49 @@ static void duplicate_device_address(struct device_address *to, const struct dev cleanup_device_address(to); } +static struct others *dup_others(struct others *others) +{ + struct others *new_node = NULL; + struct others *head = NULL; + + if (others == NULL) { + return NULL; + } + + while (others) { + new_node = calloc(1, sizeof(*new_node)); + if (new_node == NULL) { + CU_DEBUG("calloc failed."); + goto err; + } + + new_node->name = NULL; + new_node->value = NULL; + new_node->parent_name = NULL; + new_node->next = NULL; + + if (head == NULL) { + head = new_node; + } else { + new_node->next = head; + head = new_node; + } + + new_node->id = others->id; + DUP_FIELD(new_node, others, name); + DUP_FIELD(new_node, others, value); + new_node->parent_id = others->parent_id; + DUP_FIELD(new_node, others, parent_name); + new_node->type = others->type; + others = others->next; + } + + return head; +err: + cleanup_others(head); + return NULL; +} + struct virt_device *virt_device_dup(struct virt_device *_dev) { struct virt_device *dev; @@ -2442,6 +2485,7 @@ struct virt_device *virt_device_dup(struct virt_device *_dev) 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); + dev->dev.net.others = dup_others(_dev->dev.net.others); } else if (dev->type == CIM_RES_TYPE_DISK) { DUP_FIELD(dev, _dev, dev.disk.type); DUP_FIELD(dev, _dev, dev.disk.device); @@ -2456,25 +2500,32 @@ struct virt_device *virt_device_dup(struct virt_device *_dev) 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); + dev->dev.disk.others = dup_others(_dev->dev.disk.others); } else if (dev->type == CIM_RES_TYPE_MEM) { dev->dev.mem.size = _dev->dev.mem.size; dev->dev.mem.maxsize = _dev->dev.mem.maxsize; + dev->dev.mem.others = dup_others(_dev->dev.mem.others); } else if (dev->type == CIM_RES_TYPE_PROC) { dev->dev.vcpu.quantity = _dev->dev.vcpu.quantity; + dev->dev.vcpu.others = dup_others(_dev->dev.vcpu.others); } else if (dev->type == CIM_RES_TYPE_EMU) { DUP_FIELD(dev, _dev, dev.emu.path); + dev->dev.emu.others = dup_others(_dev->dev.emu.others); } else if (dev->type == CIM_RES_TYPE_GRAPHICS) { DUP_FIELD(dev, _dev, dev.graphics.type); DUP_FIELD(dev, _dev, dev.graphics.dev.vnc.host); DUP_FIELD(dev, _dev, dev.graphics.dev.vnc.port); DUP_FIELD(dev, _dev, dev.graphics.dev.vnc.keymap); DUP_FIELD(dev, _dev, dev.graphics.dev.vnc.passwd); + dev->dev.graphics.others = dup_others(_dev->dev.graphics.others); } else if (dev->type == CIM_RES_TYPE_INPUT) { DUP_FIELD(dev, _dev, dev.input.type); DUP_FIELD(dev, _dev, dev.input.bus); + dev->dev.input.others = dup_others(_dev->dev.input.others); } else if (dev->type == CIM_RES_TYPE_CONSOLE) { console_device_dup(&dev->dev.console, &_dev->dev.console); + dev->dev.console.others = dup_others(_dev->dev.console.others); } return dev; } -- 1.8.3.1
From: Xu Wang <cngesaint@gmail.com> Signed-off-by: Xu Wang <gesaint@linux.vnet.ibm.com> --- src/Virt_VirtualSystemManagementService.c | 2 +- src/svpc_types.h | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Virt_VirtualSystemManagementService.c b/src/Virt_VirtualSystemManagementService.c index 5c7238f..e1f7640 100644 --- a/src/Virt_VirtualSystemManagementService.c +++ b/src/Virt_VirtualSystemManagementService.c @@ -3032,7 +3032,7 @@ static CMPIStatus resource_del(struct domain *dominfo, CLASSNAME(op)); } - dev->type = CIM_RES_TYPE_UNKNOWN; + dev->type = CIM_RES_TYPE_DELETED; break; } diff --git a/src/svpc_types.h b/src/svpc_types.h index 4928742..e70c16c 100644 --- a/src/svpc_types.h +++ b/src/svpc_types.h @@ -32,6 +32,7 @@ #define CIM_RES_TYPE_DISK 17 #define CIM_RES_TYPE_GRAPHICS 24 #define CIM_RES_TYPE_INPUT 13 +#define CIM_RES_TYPE_DELETED 999 #define CIM_RES_TYPE_UNKNOWN 1000 #define CIM_RES_TYPE_UNKDEV 1001 #define CIM_RES_TYPE_IMAGE 32768 -- 1.8.3.1
On 11/15/2013 01:23 AM, John Ferlan wrote:
This is a *partial rework* of Xu Wang's patches sent last month:
https://www.redhat.com/archives/libvirt-cim/2013-October/msg00081.html
Although not the complete set of changes - it's a good stopping point insomuch as it handles the "others" parsing. If this looks good, I can push it, then work through the changes to write the xml.
I have run all the changes through cimtest - even with the patches on the list from Viktor. No new issues are found.
wow, hats off to that :-) However I need to report an issue running on s390, cimprovagt core dumps, so I need to investigate further and ask to please hold off until I figure out the reason. Thanks! -- 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/15/2013 11:04 AM, Viktor Mihajlovski wrote:
On 11/15/2013 01:23 AM, John Ferlan wrote:
This is a *partial rework* of Xu Wang's patches sent last month:
https://www.redhat.com/archives/libvirt-cim/2013-October/msg00081.html
Although not the complete set of changes - it's a good stopping point insomuch as it handles the "others" parsing. If this looks good, I can push it, then work through the changes to write the xml.
I have run all the changes through cimtest - even with the patches on the list from Viktor. No new issues are found.
wow, hats off to that :-)
thanks - it was a way to review too as the series was just so large...
However I need to report an issue running on s390, cimprovagt core dumps, so I need to investigate further and ask to please hold off until I figure out the reason. Thanks!
I found investigating cimprovagt to be very painful, hence the reason why I redid 1-15 a bit. I'd suggest trying to apply 1-3 first - make sure they work. Then 4-13 to make sure they work. Then go slower on 14-20. I found that 14/15 were the most problematic... 16-18 were mechanical. 19 seemed to be harmless; however, who knows. The issue with 14/15 was that CIM_RES_TYPE_UNKNOWN already existed and was in use for other things - that's why I added UNKDEV, although I could have messed the numbering scheme up. I know there are specific rules about what numbers can be used, but I'm not sure of the details... John
On 11/15/2013 07:41 PM, John Ferlan wrote:
However I need to report an issue running on s390, cimprovagt core dumps, so I need to investigate further and ask to please hold off until I figure out the reason. Thanks!
I found investigating cimprovagt to be very painful, hence the reason why I redid 1-15 a bit. I'd suggest trying to apply 1-3 first - make sure they work. Then 4-13 to make sure they work. Then go slower on 14-20. I found that 14/15 were the most problematic... 16-18 were mechanical. 19 seemed to be harmless; however, who knows.
well, it turns out that I was hitting a low memory condition, maybe a side effect of patches, redoing the tests with more memory worked well, which it should anyway because no "other" libvirt XML is written back so far. So, no worries, especially as the patches are under discussion anyway. -- 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/20/2013 01:09 PM, Viktor Mihajlovski wrote:
On 11/15/2013 07:41 PM, John Ferlan wrote:
However I need to report an issue running on s390, cimprovagt core dumps, so I need to investigate further and ask to please hold off until I figure out the reason. Thanks!
I found investigating cimprovagt to be very painful, hence the reason why I redid 1-15 a bit. I'd suggest trying to apply 1-3 first - make sure they work. Then 4-13 to make sure they work. Then go slower on 14-20. I found that 14/15 were the most problematic... 16-18 were mechanical. 19 seemed to be harmless; however, who knows.
well, it turns out that I was hitting a low memory condition, maybe a side effect of patches, redoing the tests with more memory worked well, which it should anyway because no "other" libvirt XML is written back so far. So, no worries, especially as the patches are under discussion anyway.
That brings up an interesting test - how much memory is used by the current algorithm and how much gets used by the new algorithm... Not that there's a leak, I just think more memory is held onto than necessary. If each device has its own parse_data tree, then there's a lot of unnecessary duplication and even worse unused fields. Unfortunately when the data is then fetched - we don't add another pile of memory which isn't freed. At least this new code does a better job at failing on memory allocation compared to many other paths in the code... John
John and Xu Wang, here are a few general observations from side: 1) I agree that it makes sense to preserve the unknown xml "entities" even so it can create complex scenarios and even new kinds of errors if unknown entities depend on known entities which get modified making them unusable for the unknown entities. This error would probably be the reversal of what is currently the problem when unknown entities simply disappear. 2) The implementation of putting hierarchical data (xml elements and properties) into a single "flat" list and storing the hierarchical dependencies with strings worries me. Wouldn't it make things easier to create the hierarchy in the others structure (it also would remove the need for the ids I guess!). The others structure is stored on the internal data object that represents a defined known xml entity. This defined known xml entity can itself be the parent for unknown properties (xml properites) and/or the parent for unknown sub entities (xml elements) where all these entities at the end of the "data parsing" would be flagged ACTIVE and the unknown ones INACTIVE. /* This could actually be just other_node */ struct others { struct other_node *known_node; /* this is the root representing the defined known xml element */ }; struct other_node { char *name; char *value; struct other_prop *properties; /* list of the nodes properties */ struct other_node *children; /* list with the direct sets of children data */ enum { ACTIVE, INACTIVE } status; }; struct other_prop { char *name; char *value; struct other_prop *properties; enum { ACTIVE, INACTIVE } status; }; Probably the above data structures could be streamlined even more when one starts to use these structures writing the code. The structures are very close to the xml and recursive methods could be used for writing the xml out. References to the other_node and other_prop structures could be made available for writing and reading data from them via methods. e.g. writing back the console (just a snippet from xmlgen.c) - console = xmlNewChild(root, NULL, BAD_CAST "console", NULL); - if (console == NULL) + other_node *console = NULL; + console = add_node_to_others(cdev->others->known_node, + "console", + NULL, /* text value of the xml element */ + "devices"); + + if (console == NULL) { + CU_DEBUG("Add tag <console> failed."); return XML_ERROR; + } - xmlNewProp(console, BAD_CAST "type", - BAD_CAST - chardev_source_type_IDToStr(cdev->source_type)); + other_prop *cprop = NULL; + cprop = add_prop_to_others(console, + "type", + chardev_source_type_IDToStr( + cdev->source_type)); As you can see it is much closer to the known pluming as it used to be directly with xml and also hides the list internals (name & ids) used for referencing. I know that this would cause a rather large rework but I think that the usability would be much enhanced for everyone writing/fixing the provider code and overall would improve code stability in the long run. Please use this as an idea for improvement. Also note: The above is just email written code for reference and not thought to be bug free. :-) Mit freundlichen Grüßen/Kind regards Boris Fiuczynski 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/15/2013 01:23 AM, John Ferlan wrote:
This is a *partial rework* of Xu Wang's patches sent last month:
https://www.redhat.com/archives/libvirt-cim/2013-October/msg00081.html
Although not the complete set of changes - it's a good stopping point insomuch as it handles the "others" parsing. If this looks good, I can push it, then work through the changes to write the xml.
I have run all the changes through cimtest - even with the patches on the list from Viktor. No new issues are found.
Changes to the original patches
1. I rebased to top of tree as of today (11/14/13)
2. I reworked each of the patches to only add the 'others' to the particular *_device structure. This was done to be able to show the progression and to ensure I didn't forget something
2a. This found that there needed to be a cleanup_vcpu_device() and cleanup_mem_device() since both added "others" to their structure but were never cleaned up during cleanup_virt_device() 2b. I did not see a need for "others" in "vnc_device" and "sdl_device" 2c. I added a cleanup_others() call in _get_dominfo()
3. I added fetch_device_address_from_others() to replace parse_device_address(). This bridges the "gap" bewteen the parsing of the <address> tag that was recently added and the 'others' parsing code which didn't handle it. All I did was traverse the others list looking for parent name and id match for TYPE_PROP entries. Essentially anything inside of <address...> us then copied in to name/value structure managed by devaddr. Once the last caller of parse_device_address() was removed, that function went away
4. I changed the type for unknown from CIM_RES_TYPE_UNKNOWN to CIM_RES_TYPE_UNKDEV. Turns out the former is used in other contexts and if/when cleanup_virt_device() was called from one of those contexts the result was a core in cimprovagt. This was seen in the cimtest for VirtualSystemManagementService 16_removeresource.py.
4. Various "bug fixes" based on a coverity run and what I saw in code review from Boris. - Original code had a "ddev->disk_type == DISK_PHY;" - There was a "if (node->name == NULL)" check in parse_graphics_device after "else if (STREQC(gdev->type, "pty")) {" which coverity flagged as unnecessary since earlier code assumed node->name != NULL - In parse_os() there was a "STREQC(dominfo->os_info.pv.type, "linux")" which needed a "dominfo->os_info.pv.type &&" prior to it since the value could have been NULL according to a check earlier in the routine - I added the "free(dev->name);" to cleanup_unknown_device() - Checks for others->{name|parent_name|value} were removed. Since the structures are calloc()'d and free(NULL) does nothing, this is OK.
Xu Wang (20): Add others member for saving unsupported tag and unknown device Add others and unknown_device clean up Add basic operations for reading data from xml node Fix xml parsing algorithm for parse_fs_device() Fix xml parsing algorithm for parse_block_device() Fix xml parsing algorithm for parse_vsi_device() Fix xml parsing algorithm for parse_net_device() Fix xml parsing algorithm for parse_vcpu_device() Fix xml parsing algorithm for parse_emu_device() Fix xml parsing algorithm for parse_mem_device() Fix xml parsing algorithm for parse_console_device() Fix xml parsing algorithm for parse_graphics_device() Fix xml parsing algorithm for parse_input_device() Add parse_unknown_device() Add parse_devices() for unknown type in get_dominfo_from_xml() Fix xml parsing algorithm in parse_domain() Fix xml parsing algorithm in parse_os() Fix xml parsing algorithm in parse_features() Add dup function for device copy Add type CIM_RES_TYPE_DELETED and modify type as it after resource_del
libxkutil/device_parsing.c | 2055 ++++++++++++++++++++++++----- libxkutil/device_parsing.h | 58 + src/Virt_VirtualSystemManagementService.c | 2 +- src/svpc_types.h | 2 + 4 files changed, 1774 insertions(+), 343 deletions(-)
On 11/18/2013 09:59 AM, Boris Fiuczynski wrote:
John and Xu Wang, here are a few general observations from side:
First off - I tend to find myself agreeing with Boris here. I think the concept is important and necessary; however, I'm not convinced the implementation goes far enough.
1) I agree that it makes sense to preserve the unknown xml "entities" even so it can create complex scenarios and even new kinds of errors if unknown entities depend on known entities which get modified making them unusable for the unknown entities. This error would probably be the reversal of what is currently the problem when unknown entities simply disappear.
Is there a more concrete example of "new kinds of errors if unknown entities depend on known entities which get modified making them unusable for the unknown entities" that can be given? Just for clarity. I've read that line a few times and I'm still not sure :-)
2) The implementation of putting hierarchical data (xml elements and properties) into a single "flat" list and storing the hierarchical dependencies with strings worries me. Wouldn't it make things easier to create the hierarchy in the others structure (it also would remove the need for the ids I guess!).
The primary reason why I went through the trouble of adjusting the commit was to learn a bit more about the code, but I also was trying to figure out a way to split up the submit into more manageable chunks... While going through the changes I found myself wondering what purpose the parent values were serving and why we kept passing -1 for ID values. When I went to integrate the "device address" changes into this new model I found myself beginning to think that perhaps the model used there to grab the property key/value pairs from the node generically was more suited for what you were trying to do. Since you're trying to protect against is someone changing/extending the XML "node" to add more "properties" which are then not restored when libvirt-cim goes to write out the XML, then why not take the next step and just handle the whole hierarchy?
The others structure is stored on the internal data object that represents a defined known xml entity. This defined known xml entity can itself be the parent for unknown properties (xml properites) and/or the parent for unknown sub entities (xml elements) where all these entities at the end of the "data parsing" would be flagged ACTIVE and the unknown ones INACTIVE.
/* This could actually be just other_node */ struct others { struct other_node *known_node; /* this is the root representing the defined known xml element */ };
struct other_node { char *name; char *value; struct other_prop *properties; /* list of the nodes properties */ struct other_node *children; /* list with the direct sets of children data */ enum { ACTIVE, INACTIVE } status; };
struct other_prop { char *name; char *value; struct other_prop *properties; enum { ACTIVE, INACTIVE } status; };
Probably the above data structures could be streamlined even more when one starts to use these structures writing the code. The structures are very close to the xml and recursive methods could be used for writing the xml out.
See the "device_address" structure and subsequent processing by parse_device_address() and add_device_address_property() for more opaque view on how to process the xml. If you take the concept and apply it to say "<domain>" - I think you'll be able to create one structure for dominfo that contains one instance of the read XML. Then using the active/inactive tagging you should be able to tell what's been read/used by the libvirt-cim code. I'd even go as far to say why not add a "CHANGED" status, but I'm not as clear on the lifecycle with respect to how the code the code that ends up writing things out works. My one concern with a single instance would be what happens if it changes without libvirt-cim's knowledge? Not sure its possible or supported. I think the way the code is written now there's perhaps more than one traversal of the tree, although without seeing in "in action" I cannot be sure.
References to the other_node and other_prop structures could be made available for writing and reading data from them via methods. e.g. writing back the console (just a snippet from xmlgen.c)
- console = xmlNewChild(root, NULL, BAD_CAST "console", NULL); - if (console == NULL) + other_node *console = NULL; + console = add_node_to_others(cdev->others->known_node, + "console", + NULL, /* text value of the xml element */ + "devices"); + + if (console == NULL) { + CU_DEBUG("Add tag <console> failed."); return XML_ERROR; + }
- xmlNewProp(console, BAD_CAST "type", - BAD_CAST - chardev_source_type_IDToStr(cdev->source_type)); + other_prop *cprop = NULL; + cprop = add_prop_to_others(console, + "type", + chardev_source_type_IDToStr( + cdev->source_type));
As you can see it is much closer to the known pluming as it used to be directly with xml and also hides the list internals (name & ids) used for referencing.
I know that this would cause a rather large rework but I think that the usability would be much enhanced for everyone writing/fixing the provider code and overall would improve code stability in the long run. Please use this as an idea for improvement.
I think as part of the rewrite creating macros to replace commonly cut-n-pasted code is a must. Currently there's numerous calls : + ddev->others = parse_data_to_others(ddev->others, + dnode, + 0, + BAD_CAST "devices"); + if (ddev->others == NULL) { + CU_DEBUG("parse xml failed."); + goto err; + } + or + if (seek_in_others(&ddev->others, + -1, + "source", + TYPE_NODE, + -1, + (char *)dnode->name)) { + ddev->source = fetch_from_others(&ddev->others, + -1, + "dir", + TYPE_PROP, + -1, + "source"); + + + if (ddev->source == NULL) { + CU_DEBUG("no source dir"); + goto err; + } + } There's macros that hide and do the error processing: GET_BASE_NODE(ddev, dnode, "devices", err); or ddev->source = GET_NODE_NODE(ddev, dnode, "source", err); if (ddev->source) { ddev->dir = GET_NODE_PROP(ddev->source, "dir", err); ddev->startupPolicy = GET_NODE_PROP(ddev, "startupPolicy", err); GET_NODE_NODE(ddev, ddev->source, "address", err); if (ddev->address) { } } The various macros would handle the CU_DEBUG (and any other error) processing. Similarly there would be PUT_BASE_NODE, PUT_NODE_NODE, PUT_NODE_PROP. The macros aren't completely thought out, but I would hope you see their value...
Also note: The above is just email written code for reference and not thought to be bug free. :-)
Hah - bug free code... If it were all bug free we'd be out of a job :-) I think we need to come up with an agreement on the architecture first (currently patches 1-3 and then what was the original 21/48). Those should be submitted first without any other patches and without necessarily being called except perhaps through xml_parse_test which should be used to prove that the "new" parsing and generation does no worse than the old (a 5th patch perhaps). Once those patches are in, then submit the patches that read/write domain level data... Then submit the patches that read/write domain/mem data... then domain/vcpu, etc. Also, rather than separate the xml parsing & generating, keep it together for each type. Doing things in smaller bunches will be easier to review and less prone to "other" changes causing issues. As an aside, I'm personally not a fan of the name 'others', but I don't have a better suggestion :-) I do ask that you make the testing component of this a priority. Being able to use xml_parse_test to validate is key. You may also want to add code to it in order to print out elements that are "inactive" - e.g. all the unknown stuff. Having visibility into what is not processed would be a good thing. John
Mit freundlichen Grüßen/Kind regards Boris Fiuczynski
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/15/2013 01:23 AM, John Ferlan wrote:
On 11/18/2013 09:59 AM, Boris Fiuczynski wrote:
John and Xu Wang, here are a few general observations from side: First off - I tend to find myself agreeing with Boris here. I think the concept is important and necessary; however, I'm not convinced the implementation goes far enough.
1) I agree that it makes sense to preserve the unknown xml "entities" even so it can create complex scenarios and even new kinds of errors if unknown entities depend on known entities which get modified making them unusable for the unknown entities. This error would probably be the reversal of what is currently the problem when unknown entities simply disappear. Is there a more concrete example of "new kinds of errors if unknown entities depend on known entities which get modified making them unusable for the unknown entities" that can be given? Just for clarity. I've read that line a few times and I'm still not sure :-)
2) The implementation of putting hierarchical data (xml elements and properties) into a single "flat" list and storing the hierarchical dependencies with strings worries me. Wouldn't it make things easier to create the hierarchy in the others structure (it also would remove the need for the ids I guess!). The primary reason why I went through the trouble of adjusting the commit was to learn a bit more about the code, but I also was trying to figure out a way to split up the submit into more manageable chunks... While going through the changes I found myself wondering what purpose the parent values were serving and why we kept passing -1 for ID values.
When I went to integrate the "device address" changes into this new model I found myself beginning to think that perhaps the model used there to grab the property key/value pairs from the node generically was more suited for what you were trying to do.
Since you're trying to protect against is someone changing/extending the XML "node" to add more "properties" which are then not restored when libvirt-cim goes to write out the XML, then why not take the next step and just handle the whole hierarchy?
The others structure is stored on the internal data object that represents a defined known xml entity. This defined known xml entity can itself be the parent for unknown properties (xml properites) and/or the parent for unknown sub entities (xml elements) where all these entities at the end of the "data parsing" would be flagged ACTIVE and the unknown ones INACTIVE.
/* This could actually be just other_node */ struct others { struct other_node *known_node; /* this is the root representing the defined known xml element */ };
struct other_node { char *name; char *value; struct other_prop *properties; /* list of the nodes properties */ struct other_node *children; /* list with the direct sets of children data */ enum { ACTIVE, INACTIVE } status; };
struct other_prop { char *name; char *value; struct other_prop *properties; enum { ACTIVE, INACTIVE } status; };
Probably the above data structures could be streamlined even more when one starts to use these structures writing the code. The structures are very close to the xml and recursive methods could be used for writing the xml out. See the "device_address" structure and subsequent processing by parse_device_address() and add_device_address_property() for more opaque view on how to process the xml.
If you take the concept and apply it to say "<domain>" - I think you'll be able to create one structure for dominfo that contains one instance of the read XML. Then using the active/inactive tagging you should be able to tell what's been read/used by the libvirt-cim code. I'd even go as far to say why not add a "CHANGED" status, but I'm not as clear on the lifecycle with respect to how the code the code that ends up writing things out works. My one concern with a single instance would be what happens if it changes without libvirt-cim's knowledge? Not sure its possible or supported.
I think the way the code is written now there's perhaps more than one traversal of the tree, although without seeing in "in action" I cannot be sure. Dear John and Boris, I am considering pros/cons of suggestion from Boris. If the parsing
于 2013/11/20 5:49, John Ferlan 写道: process likes this, 1. Traversal the whole xml and save all data into data structure Boris describe above; 2. Calling ***_parse() functions in device_parsing.c fetch data from the data structure generated in step 1. 3. All kinds of resource operations. (Here I want to enhance the power of libvirt-cim after these patches merged. Some new name and value pairs could be passed from API and Management could be done. The work next step maybe more complex and I started to think about it) 4. Restore xml after operation, ***_xml() restore all data in the virt_devices fetched from data structure 5. Based on data structure restored, new xml re-generated. I think code could be more tiny and maintained more easily. Do you have any suggestion or think the original one is better? I'll start to update all patches once we have a better plan:-) Other points, 1. Macros is a good idea. It could make code more clear. 2. I am not fans of 'others', too. It's just a 'historical issue' from my early design. Thanks, Xu Wang
References to the other_node and other_prop structures could be made available for writing and reading data from them via methods. e.g. writing back the console (just a snippet from xmlgen.c)
- console = xmlNewChild(root, NULL, BAD_CAST "console", NULL); - if (console == NULL) + other_node *console = NULL; + console = add_node_to_others(cdev->others->known_node, + "console", + NULL, /* text value of the xml element */ + "devices"); + + if (console == NULL) { + CU_DEBUG("Add tag <console> failed."); return XML_ERROR; + }
- xmlNewProp(console, BAD_CAST "type", - BAD_CAST - chardev_source_type_IDToStr(cdev->source_type)); + other_prop *cprop = NULL; + cprop = add_prop_to_others(console, + "type", + chardev_source_type_IDToStr( + cdev->source_type));
As you can see it is much closer to the known pluming as it used to be directly with xml and also hides the list internals (name & ids) used for referencing.
I know that this would cause a rather large rework but I think that the usability would be much enhanced for everyone writing/fixing the provider code and overall would improve code stability in the long run. Please use this as an idea for improvement. I think as part of the rewrite creating macros to replace commonly cut-n-pasted code is a must. Currently there's numerous calls :
+ ddev->others = parse_data_to_others(ddev->others, + dnode, + 0, + BAD_CAST "devices"); + if (ddev->others == NULL) { + CU_DEBUG("parse xml failed."); + goto err; + } +
or
+ if (seek_in_others(&ddev->others, + -1, + "source", + TYPE_NODE, + -1, + (char *)dnode->name)) { + ddev->source = fetch_from_others(&ddev->others, + -1, + "dir", + TYPE_PROP, + -1, + "source"); + + + if (ddev->source == NULL) { + CU_DEBUG("no source dir"); + goto err; + } + }
There's macros that hide and do the error processing:
GET_BASE_NODE(ddev, dnode, "devices", err);
or
ddev->source = GET_NODE_NODE(ddev, dnode, "source", err); if (ddev->source) { ddev->dir = GET_NODE_PROP(ddev->source, "dir", err); ddev->startupPolicy = GET_NODE_PROP(ddev, "startupPolicy", err); GET_NODE_NODE(ddev, ddev->source, "address", err); if (ddev->address) { } }
The various macros would handle the CU_DEBUG (and any other error) processing. Similarly there would be PUT_BASE_NODE, PUT_NODE_NODE, PUT_NODE_PROP.
The macros aren't completely thought out, but I would hope you see their value...
Also note: The above is just email written code for reference and not thought to be bug free. :-) Hah - bug free code... If it were all bug free we'd be out of a job :-)
I think we need to come up with an agreement on the architecture first (currently patches 1-3 and then what was the original 21/48).
Those should be submitted first without any other patches and without necessarily being called except perhaps through xml_parse_test which should be used to prove that the "new" parsing and generation does no worse than the old (a 5th patch perhaps).
Once those patches are in, then submit the patches that read/write domain level data... Then submit the patches that read/write domain/mem data... then domain/vcpu, etc. Also, rather than separate the xml parsing & generating, keep it together for each type.
Doing things in smaller bunches will be easier to review and less prone to "other" changes causing issues.
As an aside, I'm personally not a fan of the name 'others', but I don't have a better suggestion :-)
I do ask that you make the testing component of this a priority. Being able to use xml_parse_test to validate is key. You may also want to add code to it in order to print out elements that are "inactive" - e.g. all the unknown stuff. Having visibility into what is not processed would be a good thing.
John
Mit freundlichen Grüßen/Kind regards Boris Fiuczynski
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/15/2013 01:23 AM, John Ferlan wrote:
On 11/20/2013 03:36 AM, Xu Wang wrote:
Dear John and Boris, I am considering pros/cons of suggestion from Boris. If the parsing process likes this, 1. Traversal the whole xml and save all data into data structure Boris describe above;
I hope the issue Viktor ran into with running out of memory shows that a duplicitous design is problematic. If you have to - rather than strdup() value, set INACTIVE, and return (unchecked) strdup()'d memory - it would have been just as easy to return the memory already allocated, set tmp->value = NULL, and set INACTIVE. That way you know it's "managed" by something else. The free(NULL) is a no-op anyway... Be sure to use/change the "xml_parse_test" in order to validate. In fact that print function you created could be called by xml_parse_test - at least temporarily. At first everything would be unknown. As you update parse functions, you should be able to prove that you've gone from unknown to known... Eventually your known should be the same output as what one could get 'today' if they ran the test (I forget the exact switches, but you'll get to know them).
2. Calling ***_parse() functions in device_parsing.c fetch data from the data structure generated in step 1.
I think 2 & 4/5 need to be intermingled. That is when you update 'parse_fs_device', also update 'disk_fs_xml'. I'd change 'parse_block_device', 'disk_block_xml', and 'disk_file_xml' all at once. Change all the network functions together. Etc.
3. All kinds of resource operations. (Here I want to enhance the power of libvirt-cim after these patches merged. Some new name and value pairs could be passed from API and Management could be done. The work next step maybe more complex and I started to think about it)
I'm not exactly sure what this means, but it seems like this would be after step 5. Although it cannot be ignored or forgotten when devising the mechanism to parse/generate XML The change validity concern when there's unknown
4. Restore xml after operation, ***_xml() restore all data in the virt_devices fetched from data structure 5. Based on data structure restored, new xml re-generated.
What's the difference between 4 & 5?
I think code could be more tiny and maintained more easily. Do you have any suggestion or think the original one is better? I'll start to update all patches once we have a better plan:-)
Be conservative - I know it's hard to not make all the changes at once, but let's get the infrastructure right first. Updating the various _parse() and _xml() modules is a rote operation, but splatting out 50 patches at a time is tough to review/test... John
Other points, 1. Macros is a good idea. It could make code more clear. 2. I am not fans of 'others', too. It's just a 'historical issue' from my early design.
It's the "unmanaged" or "unrealized" list... John
Thanks, Xu Wang
John and Xu, On 11/19/2013 10:49 PM, John Ferlan wrote:
On 11/18/2013 09:59 AM, Boris Fiuczynski wrote:
John and Xu Wang, here are a few general observations from side:
First off - I tend to find myself agreeing with Boris here. I think the concept is important and necessary; however, I'm not convinced the implementation goes far enough.
1) I agree that it makes sense to preserve the unknown xml "entities" even so it can create complex scenarios and even new kinds of errors if unknown entities depend on known entities which get modified making them unusable for the unknown entities. This error would probably be the reversal of what is currently the problem when unknown entities simply disappear.
Is there a more concrete example of "new kinds of errors if unknown entities depend on known entities which get modified making them unusable for the unknown entities" that can be given? Just for clarity. I've read that line a few times and I'm still not sure :-) OK, let's take a look at device type disk. Since 1.0.2 the property sgio was added. Let's assume this is the unknown entity. sgio is only valid for property device "lun". If one now changes the property device to "disk" than the unknown entity sgio would cause an error when specified.
2) The implementation of putting hierarchical data (xml elements and properties) into a single "flat" list and storing the hierarchical dependencies with strings worries me. Wouldn't it make things easier to create the hierarchy in the others structure (it also would remove the need for the ids I guess!).
The primary reason why I went through the trouble of adjusting the commit was to learn a bit more about the code, but I also was trying to figure out a way to split up the submit into more manageable chunks... While going through the changes I found myself wondering what purpose the parent values were serving and why we kept passing -1 for ID values.
When I went to integrate the "device address" changes into this new model I found myself beginning to think that perhaps the model used there to grab the property key/value pairs from the node generically was more suited for what you were trying to do.
Since you're trying to protect against is someone changing/extending the XML "node" to add more "properties" which are then not restored when libvirt-cim goes to write out the XML, then why not take the next step and just handle the whole hierarchy?
The others structure is stored on the internal data object that represents a defined known xml entity. This defined known xml entity can itself be the parent for unknown properties (xml properites) and/or the parent for unknown sub entities (xml elements) where all these entities at the end of the "data parsing" would be flagged ACTIVE and the unknown ones INACTIVE.
/* This could actually be just other_node */ struct others { struct other_node *known_node; /* this is the root representing the defined known xml element */ };
struct other_node { char *name; char *value; struct other_prop *properties; /* list of the nodes properties */ struct other_node *children; /* list with the direct sets of children data */ enum { ACTIVE, INACTIVE } status; };
struct other_prop { char *name; char *value; struct other_prop *properties; enum { ACTIVE, INACTIVE } status; };
Probably the above data structures could be streamlined even more when one starts to use these structures writing the code. The structures are very close to the xml and recursive methods could be used for writing the xml out.
See the "device_address" structure and subsequent processing by parse_device_address() and add_device_address_property() for more opaque view on how to process the xml.
If you take the concept and apply it to say "<domain>" - I think you'll be able to create one structure for dominfo that contains one instance of the read XML. Then using the active/inactive tagging you should be able to tell what's been read/used by the libvirt-cim code. I'd even go as far to say why not add a "CHANGED" status, but I'm not as clear on the lifecycle with respect to how the code the code that ends up writing things out works. My one concern with a single instance would be what happens if it changes without libvirt-cim's knowledge? Not sure its possible or supported.
The idea to use one tree is fine with me. It would actually mean that the XML data structure would end up in a XML alike libvirt-cim data structure. This structure would be easier to convert back into the required xml document. If I understand you, John, correctly you also like to maintain the existing internal libvirt-cim data structures as they are today. If we don't maintain these data structures this would cause code changes at all rasd read and write methods. Not nice and more headaches. In addition I would like to suggest that for easier access into the new tree structure a reference to the corresponding tree node element should be stored as "other" (maybe rename it to node_ref) in the existing libvirt-cim data structure. A question comes to my mind about the usage of one tree: Does it create a problem for the helper methods (seek..., fetch..., ...) on instances that are not leafs (e.g. domain). I guess it would now be needed to know the hierarchy depth at which these methods stop. That is something Xu is the best to answer.
I think the way the code is written now there's perhaps more than one traversal of the tree, although without seeing in "in action" I cannot be sure.
References to the other_node and other_prop structures could be made available for writing and reading data from them via methods. e.g. writing back the console (just a snippet from xmlgen.c)
- console = xmlNewChild(root, NULL, BAD_CAST "console", NULL); - if (console == NULL) + other_node *console = NULL; + console = add_node_to_others(cdev->others->known_node, + "console", + NULL, /* text value of the xml element */ + "devices"); + + if (console == NULL) { + CU_DEBUG("Add tag <console> failed."); return XML_ERROR; + }
- xmlNewProp(console, BAD_CAST "type", - BAD_CAST - chardev_source_type_IDToStr(cdev->source_type)); + other_prop *cprop = NULL; + cprop = add_prop_to_others(console, + "type", + chardev_source_type_IDToStr( + cdev->source_type));
As you can see it is much closer to the known pluming as it used to be directly with xml and also hides the list internals (name & ids) used for referencing.
I know that this would cause a rather large rework but I think that the usability would be much enhanced for everyone writing/fixing the provider code and overall would improve code stability in the long run. Please use this as an idea for improvement.
I think as part of the rewrite creating macros to replace commonly cut-n-pasted code is a must. Currently there's numerous calls :
+ ddev->others = parse_data_to_others(ddev->others, + dnode, + 0, + BAD_CAST "devices"); + if (ddev->others == NULL) { + CU_DEBUG("parse xml failed."); + goto err; + } +
or
+ if (seek_in_others(&ddev->others, + -1, + "source", + TYPE_NODE, + -1, + (char *)dnode->name)) { + ddev->source = fetch_from_others(&ddev->others, + -1, + "dir", + TYPE_PROP, + -1, + "source"); + + + if (ddev->source == NULL) { + CU_DEBUG("no source dir"); + goto err; + } + }
There's macros that hide and do the error processing:
GET_BASE_NODE(ddev, dnode, "devices", err);
or
ddev->source = GET_NODE_NODE(ddev, dnode, "source", err); if (ddev->source) { ddev->dir = GET_NODE_PROP(ddev->source, "dir", err); ddev->startupPolicy = GET_NODE_PROP(ddev, "startupPolicy", err); GET_NODE_NODE(ddev, ddev->source, "address", err); if (ddev->address) { } }
The various macros would handle the CU_DEBUG (and any other error) processing. Similarly there would be PUT_BASE_NODE, PUT_NODE_NODE, PUT_NODE_PROP.
The macros aren't completely thought out, but I would hope you see their value...
I think it is a good idea but suggest to wait and see how complex the helper methods end up to be. Instead of a macro it would maybe also make more sense regarding performance to write a single method e.g. for seek&fetch.
Also note: The above is just email written code for reference and not thought to be bug free. :-)
Hah - bug free code... If it were all bug free we'd be out of a job :-)
I think we need to come up with an agreement on the architecture first (currently patches 1-3 and then what was the original 21/48).
Those should be submitted first without any other patches and without necessarily being called except perhaps through xml_parse_test which should be used to prove that the "new" parsing and generation does no worse than the old (a 5th patch perhaps).
Good point! The xml_parse_test is a good validation tool for the first part.
Once those patches are in, then submit the patches that read/write domain level data... Then submit the patches that read/write domain/mem data... then domain/vcpu, etc. Also, rather than separate the xml parsing & generating, keep it together for each type.
This seems like a valid and doable approach. Do you agree as well, Xu?
Doing things in smaller bunches will be easier to review and less prone to "other" changes causing issues.
As an aside, I'm personally not a fan of the name 'others', but I don't have a better suggestion :-)
I do ask that you make the testing component of this a priority. Being able to use xml_parse_test to validate is key. You may also want to add code to it in order to print out elements that are "inactive" - e.g. all the unknown stuff. Having visibility into what is not processed would be a good thing.
A smooth transition is key. Testing and comparison of old and new results seems crucial.
John
-- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski 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/20/2013 08:27 AM, Boris Fiuczynski wrote:
John and Xu,
On 11/19/2013 10:49 PM, John Ferlan wrote:
On 11/18/2013 09:59 AM, Boris Fiuczynski wrote:
John and Xu Wang, here are a few general observations from side:
First off - I tend to find myself agreeing with Boris here. I think the concept is important and necessary; however, I'm not convinced the implementation goes far enough.
1) I agree that it makes sense to preserve the unknown xml "entities" even so it can create complex scenarios and even new kinds of errors if unknown entities depend on known entities which get modified making them unusable for the unknown entities. This error would probably be the reversal of what is currently the problem when unknown entities simply disappear.
Is there a more concrete example of "new kinds of errors if unknown entities depend on known entities which get modified making them unusable for the unknown entities" that can be given? Just for clarity. I've read that line a few times and I'm still not sure :-) OK, let's take a look at device type disk. Since 1.0.2 the property sgio was added. Let's assume this is the unknown entity. sgio is only valid for property device "lun". If one now changes the property device to "disk" than the unknown entity sgio would cause an error when specified.
Ah - I see. Not only do you have to manage the properties you have to know how to use them as well and all their rules. I forgot about that. I came from HP/HPVM and yes, this brings back all sorts of memories... Seems like in this case, when/if the property was changed from "lun" to "disk" - code would have to search that 'others' list for the "sgio" property and know how to handle adjusting it. That'll get tricky...
2) The implementation of putting hierarchical data (xml elements and properties) into a single "flat" list and storing the hierarchical dependencies with strings worries me. Wouldn't it make things easier to create the hierarchy in the others structure (it also would remove the need for the ids I guess!).
The primary reason why I went through the trouble of adjusting the commit was to learn a bit more about the code, but I also was trying to figure out a way to split up the submit into more manageable chunks... While going through the changes I found myself wondering what purpose the parent values were serving and why we kept passing -1 for ID values.
When I went to integrate the "device address" changes into this new model I found myself beginning to think that perhaps the model used there to grab the property key/value pairs from the node generically was more suited for what you were trying to do.
Since you're trying to protect against is someone changing/extending the XML "node" to add more "properties" which are then not restored when libvirt-cim goes to write out the XML, then why not take the next step and just handle the whole hierarchy?
The others structure is stored on the internal data object that represents a defined known xml entity. This defined known xml entity can itself be the parent for unknown properties (xml properites) and/or the parent for unknown sub entities (xml elements) where all these entities at the end of the "data parsing" would be flagged ACTIVE and the unknown ones INACTIVE.
/* This could actually be just other_node */ struct others { struct other_node *known_node; /* this is the root representing the defined known xml element */ };
struct other_node { char *name; char *value; struct other_prop *properties; /* list of the nodes properties */ struct other_node *children; /* list with the direct sets of children data */ enum { ACTIVE, INACTIVE } status; };
struct other_prop { char *name; char *value; struct other_prop *properties; enum { ACTIVE, INACTIVE } status; };
Probably the above data structures could be streamlined even more when one starts to use these structures writing the code. The structures are very close to the xml and recursive methods could be used for writing the xml out.
See the "device_address" structure and subsequent processing by parse_device_address() and add_device_address_property() for more opaque view on how to process the xml.
If you take the concept and apply it to say "<domain>" - I think you'll be able to create one structure for dominfo that contains one instance of the read XML. Then using the active/inactive tagging you should be able to tell what's been read/used by the libvirt-cim code. I'd even go as far to say why not add a "CHANGED" status, but I'm not as clear on the lifecycle with respect to how the code the code that ends up writing things out works. My one concern with a single instance would be what happens if it changes without libvirt-cim's knowledge? Not sure its possible or supported.
The idea to use one tree is fine with me. It would actually mean that the XML data structure would end up in a XML alike libvirt-cim data structure. This structure would be easier to convert back into the required xml document. If I understand you, John, correctly you also like to maintain the existing internal libvirt-cim data structures as they are today. If we don't maintain these data structures this would cause code changes at all rasd read and write methods. Not nice and more headaches.
I hadn't given much thought to adjusting the internal structures at all. I agree keeping the change/churn to a minimum is a goal. As alluded to in my other response as we fill these data structures and grab memory/data from the other or unmanaged list - we probably shouldn't strdup() the value. Instead just take it, clear the value field, and mark the entry as managed. I haven't fully thought out the ramifications yet though... When we go to update/generate the output the opposite happens - we know the value moved from "others" to some "*_device" field - now we have to grab whatever is current, do some sort of validation... Thus our status field is UNMANAGED, MANAGED, CHANGED... When we come across CHANGED we may have to do a couple of handstands based on whether other properties rely on this particular property changing (as from above). Either that or when it changes we find/manage the property on the others list and deal with it...
In addition I would like to suggest that for easier access into the new tree structure a reference to the corresponding tree node element should be stored as "other" (maybe rename it to node_ref) in the existing libvirt-cim data structure.
That works too (rather than borrowing and returning).
A question comes to my mind about the usage of one tree: Does it create a problem for the helper methods (seek..., fetch..., ...) on instances that are not leafs (e.g. domain). I guess it would now be needed to know the hierarchy depth at which these methods stop. That is something Xu is the best to answer.
Right! and a bit of prototyping... Validate the algorithm/thoughts work with 'domain' and perhaps 'file'/'block'...
I think the way the code is written now there's perhaps more than one traversal of the tree, although without seeing in "in action" I cannot be sure.
References to the other_node and other_prop structures could be made available for writing and reading data from them via methods. e.g. writing back the console (just a snippet from xmlgen.c)
- console = xmlNewChild(root, NULL, BAD_CAST "console", NULL); - if (console == NULL) + other_node *console = NULL; + console = add_node_to_others(cdev->others->known_node, + "console", + NULL, /* text value of the xml element */ + "devices"); + + if (console == NULL) { + CU_DEBUG("Add tag <console> failed."); return XML_ERROR; + }
- xmlNewProp(console, BAD_CAST "type", - BAD_CAST - chardev_source_type_IDToStr(cdev->source_type)); + other_prop *cprop = NULL; + cprop = add_prop_to_others(console, + "type", + chardev_source_type_IDToStr( + cdev->source_type));
As you can see it is much closer to the known pluming as it used to be directly with xml and also hides the list internals (name & ids) used for referencing.
I know that this would cause a rather large rework but I think that the usability would be much enhanced for everyone writing/fixing the provider code and overall would improve code stability in the long run. Please use this as an idea for improvement.
I think as part of the rewrite creating macros to replace commonly cut-n-pasted code is a must. Currently there's numerous calls :
+ ddev->others = parse_data_to_others(ddev->others, + dnode, + 0, + BAD_CAST "devices"); + if (ddev->others == NULL) { + CU_DEBUG("parse xml failed."); + goto err; + } +
or
+ if (seek_in_others(&ddev->others, + -1, + "source", + TYPE_NODE, + -1, + (char *)dnode->name)) { + ddev->source = fetch_from_others(&ddev->others, + -1, + "dir", + TYPE_PROP, + -1, + "source"); + + + if (ddev->source == NULL) { + CU_DEBUG("no source dir"); + goto err; + } + }
There's macros that hide and do the error processing:
GET_BASE_NODE(ddev, dnode, "devices", err);
or
ddev->source = GET_NODE_NODE(ddev, dnode, "source", err); if (ddev->source) { ddev->dir = GET_NODE_PROP(ddev->source, "dir", err); ddev->startupPolicy = GET_NODE_PROP(ddev, "startupPolicy", err); GET_NODE_NODE(ddev, ddev->source, "address", err); if (ddev->address) { } }
The various macros would handle the CU_DEBUG (and any other error) processing. Similarly there would be PUT_BASE_NODE, PUT_NODE_NODE, PUT_NODE_PROP.
The macros aren't completely thought out, but I would hope you see their value...
I think it is a good idea but suggest to wait and see how complex the helper methods end up to be. Instead of a macro it would maybe also make more sense regarding performance to write a single method e.g. for seek&fetch.
Anything that reduces cut-n-paste of the same 10-20 lines is better. I'm a big code reuse proponent. I chose macros because it's easier to have them jump to failure points rather than checking routine status. John
Also note: The above is just email written code for reference and not thought to be bug free. :-)
Hah - bug free code... If it were all bug free we'd be out of a job :-)
I think we need to come up with an agreement on the architecture first (currently patches 1-3 and then what was the original 21/48).
Those should be submitted first without any other patches and without necessarily being called except perhaps through xml_parse_test which should be used to prove that the "new" parsing and generation does no worse than the old (a 5th patch perhaps).
Good point! The xml_parse_test is a good validation tool for the first part.
Once those patches are in, then submit the patches that read/write domain level data... Then submit the patches that read/write domain/mem data... then domain/vcpu, etc. Also, rather than separate the xml parsing & generating, keep it together for each type.
This seems like a valid and doable approach. Do you agree as well, Xu?
Doing things in smaller bunches will be easier to review and less prone to "other" changes causing issues.
As an aside, I'm personally not a fan of the name 'others', but I don't have a better suggestion :-)
I do ask that you make the testing component of this a priority. Being able to use xml_parse_test to validate is key. You may also want to add code to it in order to print out elements that are "inactive" - e.g. all the unknown stuff. Having visibility into what is not processed would be a good thing.
A smooth transition is key. Testing and comparison of old and new results seems crucial.
John
于 2013/11/21 5:10, John Ferlan 写道:
On 11/20/2013 08:27 AM, Boris Fiuczynski wrote:
John and Xu,
On 11/19/2013 10:49 PM, John Ferlan wrote:
On 11/18/2013 09:59 AM, Boris Fiuczynski wrote:
John and Xu Wang, here are a few general observations from side: First off - I tend to find myself agreeing with Boris here. I think the concept is important and necessary; however, I'm not convinced the implementation goes far enough.
1) I agree that it makes sense to preserve the unknown xml "entities" even so it can create complex scenarios and even new kinds of errors if unknown entities depend on known entities which get modified making them unusable for the unknown entities. This error would probably be the reversal of what is currently the problem when unknown entities simply disappear. Is there a more concrete example of "new kinds of errors if unknown entities depend on known entities which get modified making them unusable for the unknown entities" that can be given? Just for clarity. I've read that line a few times and I'm still not sure :-) OK, let's take a look at device type disk. Since 1.0.2 the property sgio was added. Let's assume this is the unknown entity. sgio is only valid for property device "lun". If one now changes the property device to "disk" than the unknown entity sgio would cause an error when specified.
Ah - I see. Not only do you have to manage the properties you have to know how to use them as well and all their rules. I forgot about that. I came from HP/HPVM and yes, this brings back all sorts of memories...
Seems like in this case, when/if the property was changed from "lun" to "disk" - code would have to search that 'others' list for the "sgio" property and know how to handle adjusting it. That'll get tricky... In fact there is lots of hacking code like this in libvirt-cim.
My suggestion is, if some node was changed (such as "lun" to "disk"), its unknown sub-entities should be marked as INACTIVE to avoid that issue. That is, we couldn't keep the validity of unknown entities, so maybe let libvirt-cim just do like now is a better choice. If you think sometimes the idea above still could miss something useful, some hack code need to be added to handle issue like this.
2) The implementation of putting hierarchical data (xml elements and properties) into a single "flat" list and storing the hierarchical dependencies with strings worries me. Wouldn't it make things easier to create the hierarchy in the others structure (it also would remove the need for the ids I guess!). The primary reason why I went through the trouble of adjusting the commit was to learn a bit more about the code, but I also was trying to figure out a way to split up the submit into more manageable chunks... While going through the changes I found myself wondering what purpose the parent values were serving and why we kept passing -1 for ID values.
When I went to integrate the "device address" changes into this new model I found myself beginning to think that perhaps the model used there to grab the property key/value pairs from the node generically was more suited for what you were trying to do.
Since you're trying to protect against is someone changing/extending the XML "node" to add more "properties" which are then not restored when libvirt-cim goes to write out the XML, then why not take the next step and just handle the whole hierarchy?
The others structure is stored on the internal data object that represents a defined known xml entity. This defined known xml entity can itself be the parent for unknown properties (xml properites) and/or the parent for unknown sub entities (xml elements) where all these entities at the end of the "data parsing" would be flagged ACTIVE and the unknown ones INACTIVE.
/* This could actually be just other_node */ struct others { struct other_node *known_node; /* this is the root representing the defined known xml element */ };
struct other_node { char *name; char *value; struct other_prop *properties; /* list of the nodes properties */ struct other_node *children; /* list with the direct sets of children data */ enum { ACTIVE, INACTIVE } status; };
struct other_prop { char *name; char *value; struct other_prop *properties; enum { ACTIVE, INACTIVE } status; };
Probably the above data structures could be streamlined even more when one starts to use these structures writing the code. The structures are very close to the xml and recursive methods could be used for writing the xml out. See the "device_address" structure and subsequent processing by parse_device_address() and add_device_address_property() for more opaque view on how to process the xml.
If you take the concept and apply it to say "<domain>" - I think you'll be able to create one structure for dominfo that contains one instance of the read XML. Then using the active/inactive tagging you should be able to tell what's been read/used by the libvirt-cim code. I'd even go as far to say why not add a "CHANGED" status, but I'm not as clear on the lifecycle with respect to how the code the code that ends up writing things out works. My one concern with a single instance would be what happens if it changes without libvirt-cim's knowledge? Not sure its possible or supported.
The idea to use one tree is fine with me. It would actually mean that the XML data structure would end up in a XML alike libvirt-cim data structure. This structure would be easier to convert back into the required xml document. If I understand you, John, correctly you also like to maintain the existing internal libvirt-cim data structures as they are today. If we don't maintain these data structures this would cause code changes at all rasd read and write methods. Not nice and more headaches. I hadn't given much thought to adjusting the internal structures at all. I agree keeping the change/churn to a minimum is a goal.
As alluded to in my other response as we fill these data structures and grab memory/data from the other or unmanaged list - we probably shouldn't strdup() the value. Instead just take it, clear the value field, and mark the entry as managed. I haven't fully thought out the ramifications yet though...
When we go to update/generate the output the opposite happens - we know the value moved from "others" to some "*_device" field - now we have to grab whatever is current, do some sort of validation...
Thus our status field is UNMANAGED, MANAGED, CHANGED... When we come across CHANGED we may have to do a couple of handstands based on whether other properties rely on this particular property changing (as from above). Either that or when it changes we find/manage the property on the others list and deal with it...
In addition I would like to suggest that for easier access into the new tree structure a reference to the corresponding tree node element should be stored as "other" (maybe rename it to node_ref) in the existing libvirt-cim data structure. That works too (rather than borrowing and returning). That's a good idea. I think using 'strdup()' less could save memory space and running time.
A question comes to my mind about the usage of one tree: Does it create a problem for the helper methods (seek..., fetch..., ...) on instances that are not leafs (e.g. domain). I guess it would now be needed to know the hierarchy depth at which these methods stop. That is something Xu is the best to answer.
Right! and a bit of prototyping... Validate the algorithm/thoughts work with 'domain' and perhaps 'file'/'block'...
I think the way the code is written now there's perhaps more than one traversal of the tree, although without seeing in "in action" I cannot be sure.
References to the other_node and other_prop structures could be made available for writing and reading data from them via methods. e.g. writing back the console (just a snippet from xmlgen.c)
- console = xmlNewChild(root, NULL, BAD_CAST "console", NULL); - if (console == NULL) + other_node *console = NULL; + console = add_node_to_others(cdev->others->known_node, + "console", + NULL, /* text value of the xml element */ + "devices"); + + if (console == NULL) { + CU_DEBUG("Add tag <console> failed."); return XML_ERROR; + }
- xmlNewProp(console, BAD_CAST "type", - BAD_CAST - chardev_source_type_IDToStr(cdev->source_type)); + other_prop *cprop = NULL; + cprop = add_prop_to_others(console, + "type", + chardev_source_type_IDToStr( + cdev->source_type));
As you can see it is much closer to the known pluming as it used to be directly with xml and also hides the list internals (name & ids) used for referencing.
I know that this would cause a rather large rework but I think that the usability would be much enhanced for everyone writing/fixing the provider code and overall would improve code stability in the long run. Please use this as an idea for improvement. I think as part of the rewrite creating macros to replace commonly cut-n-pasted code is a must. Currently there's numerous calls :
+ ddev->others = parse_data_to_others(ddev->others, + dnode, + 0, + BAD_CAST "devices"); + if (ddev->others == NULL) { + CU_DEBUG("parse xml failed."); + goto err; + } +
or
+ if (seek_in_others(&ddev->others, + -1, + "source", + TYPE_NODE, + -1, + (char *)dnode->name)) { + ddev->source = fetch_from_others(&ddev->others, + -1, + "dir", + TYPE_PROP, + -1, + "source"); + + + if (ddev->source == NULL) { + CU_DEBUG("no source dir"); + goto err; + } + }
There's macros that hide and do the error processing:
GET_BASE_NODE(ddev, dnode, "devices", err);
or
ddev->source = GET_NODE_NODE(ddev, dnode, "source", err); if (ddev->source) { ddev->dir = GET_NODE_PROP(ddev->source, "dir", err); ddev->startupPolicy = GET_NODE_PROP(ddev, "startupPolicy", err); GET_NODE_NODE(ddev, ddev->source, "address", err); if (ddev->address) { } }
The various macros would handle the CU_DEBUG (and any other error) processing. Similarly there would be PUT_BASE_NODE, PUT_NODE_NODE, PUT_NODE_PROP.
The macros aren't completely thought out, but I would hope you see their value... I think it is a good idea but suggest to wait and see how complex the helper methods end up to be. Instead of a macro it would maybe also make more sense regarding performance to write a single method e.g. for seek&fetch. Anything that reduces cut-n-paste of the same 10-20 lines is better. I'm a big code reuse proponent. I chose macros because it's easier to have them jump to failure points rather than checking routine status.
John
Also note: The above is just email written code for reference and not thought to be bug free. :-) Hah - bug free code... If it were all bug free we'd be out of a job :-)
I think we need to come up with an agreement on the architecture first (currently patches 1-3 and then what was the original 21/48).
Those should be submitted first without any other patches and without necessarily being called except perhaps through xml_parse_test which should be used to prove that the "new" parsing and generation does no worse than the old (a 5th patch perhaps).
Good point! The xml_parse_test is a good validation tool for the first part.
Once those patches are in, then submit the patches that read/write domain level data... Then submit the patches that read/write domain/mem data... then domain/vcpu, etc. Also, rather than separate the xml parsing & generating, keep it together for each type. This seems like a valid and doable approach. Do you agree as well, Xu? I can't agree with you more:-) Doing things in smaller bunches will be easier to review and less prone to "other" changes causing issues.
As an aside, I'm personally not a fan of the name 'others', but I don't have a better suggestion :-)
I do ask that you make the testing component of this a priority. Being able to use xml_parse_test to validate is key. You may also want to add code to it in order to print out elements that are "inactive" - e.g. all the unknown stuff. Having visibility into what is not processed would be a good thing. A smooth transition is key. Testing and comparison of old and new results seems crucial. John
And the whole process like this?
1. Traverse content of xml -> build references -> save into 'others' (just name it temporarily) 2. Fetch/handle data into virt_device structure 3. Resource operations (just like original, some change may need to be done such as status mark) 4. Restore the data in the virt_devices back to the xml 5. Regenerate the xml based on the references (status mainly) 6. The following steps (just like original) Some maros (functions) like this maybe needed, BUILD_REFERENCES, GET_NODE, GET_PROP, UPDATE_STATUS, ...... Thanks, Xu Wang
_______________________________________________ Libvirt-cim mailing list Libvirt-cim@redhat.com https://www.redhat.com/mailman/listinfo/libvirt-cim
On 11/21/2013 04:15 AM, Xu Wang wrote:
于 2013/11/21 5:10, John Ferlan 写道:
On 11/20/2013 08:27 AM, Boris Fiuczynski wrote:
John and Xu,
On 11/19/2013 10:49 PM, John Ferlan wrote:
On 11/18/2013 09:59 AM, Boris Fiuczynski wrote:
John and Xu Wang, here are a few general observations from side: First off - I tend to find myself agreeing with Boris here. I think the concept is important and necessary; however, I'm not convinced the implementation goes far enough.
1) I agree that it makes sense to preserve the unknown xml "entities" even so it can create complex scenarios and even new kinds of errors if unknown entities depend on known entities which get modified making them unusable for the unknown entities. This error would probably be the reversal of what is currently the problem when unknown entities simply disappear. Is there a more concrete example of "new kinds of errors if unknown entities depend on known entities which get modified making them unusable for the unknown entities" that can be given? Just for clarity. I've read that line a few times and I'm still not sure :-) OK, let's take a look at device type disk. Since 1.0.2 the property sgio was added. Let's assume this is the unknown entity. sgio is only valid for property device "lun". If one now changes the property device to "disk" than the unknown entity sgio would cause an error when specified.
Ah - I see. Not only do you have to manage the properties you have to know how to use them as well and all their rules. I forgot about that. I came from HP/HPVM and yes, this brings back all sorts of memories...
Seems like in this case, when/if the property was changed from "lun" to "disk" - code would have to search that 'others' list for the "sgio" property and know how to handle adjusting it. That'll get tricky...
In fact there is lots of hacking code like this in libvirt-cim.
My suggestion is, if some node was changed (such as "lun" to "disk"), its unknown sub-entities should be marked as INACTIVE to avoid that issue. That is, we couldn't keep the validity of unknown entities, so maybe let libvirt-cim just do like now is a better choice.
If you think sometimes the idea above still could miss something useful, some hack code need to be added to handle issue like this. The problem here is actually that the dependency is new and the old code does not know about it. The old libvirt-cim pattern was getting around
You cannot search in others for something you do not know about... unless you can code magic! :-) this trouble by just maintaining what is known and neglect the unknown. Xu is trying to prevent the neglecting of tags and properties with his patch. With this in mind I would find it strange to break this new pattern now by starting to remove/invalidate unknown tag/properties. The trouble with the new pattern is that the error causing tags/properties are outside of the libvirt-cim management scope and cannot be adjust via libvirt-cim to allow modifications of the domain in this case. I think that the errors that are caused in this situations are the reversal of errors caused by the current pattern of "neglect the unknown". I do not know if it would be a good idea to mix these patterns up. Looking at the mixed case from the users perspective I currently suppose that most users would proably call it a bug if tags would mostly be maintained and sometimes not. Even if the mixed case would be used: which known tags/properties change would trigger the invalidation/deletion of what unknown sub tags/properties and to which tree depth? What happens when relationships across device instances arise? We really need to be cautious of what we are doing here. Just an idea (while talking about users): Should the user perhaps be made capable to choose the "mode" of libvirt-cim regarding the handling of unknown tags & properties? If we would do that the freedom of how the new mode can behave increases. I know that this would mean a rather large change but I did not want to let this opportunity pass without mentioning it. I am also aware that this would double the testing efforts and likely increase the coding efforts for the future.
...
In addition I would like to suggest that for easier access into the new tree structure a reference to the corresponding tree node element should be stored as "other" (maybe rename it to node_ref) in the existing libvirt-cim data structure. That works too (rather than borrowing and returning). That's a good idea. I think using 'strdup()' less could save memory space and running time. It is likely to save also some write back operations in step 4./5. outline by Xu below.
A question comes to my mind about the usage of one tree: Does it create a problem for the helper methods (seek..., fetch..., ...) on instances that are not leafs (e.g. domain). I guess it would now be needed to know the hierarchy depth at which these methods stop. That is something Xu is the best to answer.
Right! and a bit of prototyping... Validate the algorithm/thoughts work with 'domain' and perhaps 'file'/'block'...
...
I think we need to come up with an agreement on the architecture first (currently patches 1-3 and then what was the original 21/48).
Those should be submitted first without any other patches and without necessarily being called except perhaps through xml_parse_test which should be used to prove that the "new" parsing and generation does no worse than the old (a 5th patch perhaps). Good point! The xml_parse_test is a good validation tool for the first part. Once those patches are in, then submit the patches that read/write domain level data... Then submit the patches that read/write domain/mem data... then domain/vcpu, etc. Also, rather than separate the xml parsing & generating, keep it together for each type. This seems like a valid and doable approach. Do you agree as well, Xu? I can't agree with you more:-) Looks like we all agree on this. :-)
And the whole process like this?
1. Traverse content of xml -> build references -> save into 'others' (just name it temporarily) 2. Fetch/handle data into virt_device structure 3. Resource operations (just like original, some change may need to be done such as status mark) 4. Restore the data in the virt_devices back to the xml
"back to the xml"? Shouldn't that be "back into the 'others' structure"?
5. Regenerate the xml based on the references (status mainly) 6. The following steps (just like original)
Some maros (functions) like this maybe needed, BUILD_REFERENCES, GET_NODE, GET_PROP, UPDATE_STATUS, ......
Thanks, Xu Wang
_______________________________________________ Libvirt-cim mailing list Libvirt-cim@redhat.com https://www.redhat.com/mailman/listinfo/libvirt-cim
-- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski 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
于 2013/11/23 0:18, Boris Fiuczynski 写道:
On 11/21/2013 04:15 AM, Xu Wang wrote:
于 2013/11/21 5:10, John Ferlan 写道:
On 11/20/2013 08:27 AM, Boris Fiuczynski wrote:
John and Xu,
On 11/19/2013 10:49 PM, John Ferlan wrote:
John and Xu Wang, here are a few general observations from side: First off - I tend to find myself agreeing with Boris here. I
On 11/18/2013 09:59 AM, Boris Fiuczynski wrote: think the concept is important and necessary; however, I'm not convinced the implementation goes far enough.
1) I agree that it makes sense to preserve the unknown xml "entities" even so it can create complex scenarios and even new kinds of errors if unknown entities depend on known entities which get modified making them unusable for the unknown entities. This error would probably be the reversal of what is currently the problem when unknown entities simply disappear. Is there a more concrete example of "new kinds of errors if unknown entities depend on known entities which get modified making them unusable for the unknown entities" that can be given? Just for clarity. I've read that line a few times and I'm still not sure :-) OK, let's take a look at device type disk. Since 1.0.2 the property sgio was added. Let's assume this is the unknown entity. sgio is only valid for property device "lun". If one now changes the property device to "disk" than the unknown entity sgio would cause an error when specified.
Ah - I see. Not only do you have to manage the properties you have to know how to use them as well and all their rules. I forgot about that. I came from HP/HPVM and yes, this brings back all sorts of memories...
Seems like in this case, when/if the property was changed from "lun" to "disk" - code would have to search that 'others' list for the "sgio" property and know how to handle adjusting it. That'll get tricky...
In fact there is lots of hacking code like this in libvirt-cim.
My suggestion is, if some node was changed (such as "lun" to "disk"), its unknown sub-entities should be marked as INACTIVE to avoid that issue. That is, we couldn't keep the validity of unknown entities, so maybe let libvirt-cim just do like now is a better choice.
If you think sometimes the idea above still could miss something useful, some hack code need to be added to handle issue like this. The problem here is actually that the dependency is new and the old code does not know about it. The old libvirt-cim pattern was getting around this trouble by just maintaining what is known and neglect the unknown. Xu is trying to prevent the neglecting of tags and properties with his
You cannot search in others for something you do not know about... unless you can code magic! :-) If needed that's possible. We just coding just like...xpath. Before that we should make sure if it's necessary. patch. With this in mind I would find it strange to break this new pattern now by starting to remove/invalidate unknown tag/properties. The trouble with the new pattern is that the error causing tags/properties are outside of the libvirt-cim management scope and cannot be adjust via libvirt-cim to allow modifications of the domain in this case.
I think that the errors that are caused in this situations are the reversal of errors caused by the current pattern of "neglect the unknown". I do not know if it would be a good idea to mix these patterns up. Looking at the mixed case from the users perspective I currently suppose that most users would proably call it a bug if tags would mostly be maintained and sometimes not. Even if the mixed case would be used: which known tags/properties change would trigger the invalidation/deletion of what unknown sub tags/properties and to which tree depth? What happens when relationships across device instances arise?
We really need to be cautious of what we are doing here.
Just an idea (while talking about users): Should the user perhaps be made capable to choose the "mode" of libvirt-cim regarding the handling of unknown tags & properties? If we would do that the freedom of how the new mode can behave increases. I know that this would mean a rather large change but I did not want to let this opportunity pass without mentioning it. I am also aware that this would double the testing efforts and likely increase the coding efforts for the future. Yeah,that's a good choice to decide behavior by option. We can keep a 'compatible mode' to those users who 'like old things'. On the other hand it's no doubt that will add the scale of code and maintaining work. But...what should be like with the new mode? Just keep the original content or add some hacking code to solve conflicts like this?
...
In addition I would like to suggest that for easier access into the new tree structure a reference to the corresponding tree node element should be stored as "other" (maybe rename it to node_ref) in the existing libvirt-cim data structure. That works too (rather than borrowing and returning). That's a good idea. I think using 'strdup()' less could save memory space and running time. It is likely to save also some write back operations in step 4./5. outline by Xu below.
A question comes to my mind about the usage of one tree: Does it create a problem for the helper methods (seek..., fetch..., ...) on instances that are not leafs (e.g. domain). I guess it would now be needed to know the hierarchy depth at which these methods stop. That is something Xu is the best to answer.
Right! and a bit of prototyping... Validate the algorithm/thoughts work with 'domain' and perhaps 'file'/'block'...
...
I think we need to come up with an agreement on the architecture first (currently patches 1-3 and then what was the original 21/48).
Those should be submitted first without any other patches and without necessarily being called except perhaps through xml_parse_test which should be used to prove that the "new" parsing and generation does no worse than the old (a 5th patch perhaps). Good point! The xml_parse_test is a good validation tool for the first part. Once those patches are in, then submit the patches that read/write domain level data... Then submit the patches that read/write domain/mem data... then domain/vcpu, etc. Also, rather than separate the xml parsing & generating, keep it together for each type. This seems like a valid and doable approach. Do you agree as well, Xu? I can't agree with you more:-) Looks like we all agree on this. :-)
And the whole process like this?
1. Traverse content of xml -> build references -> save into 'others' (just name it temporarily) 2. Fetch/handle data into virt_device structure 3. Resource operations (just like original, some change may need to be done such as status mark) 4. Restore the data in the virt_devices back to the xml
"back to the xml"? Shouldn't that be "back into the 'others' structure"? You're right. Maybe my original understanding isn't accurate. I repeat it. Firstly libvirt-cim read data from xml and save them into 'others' structure. Then, some data in the virt_devices just use pointer point to the data in 'others' intead of using strdup(), isn't it?
Thanks, Xu Wang
5. Regenerate the xml based on the references (status mainly) 6. The following steps (just like original)
Some maros (functions) like this maybe needed, BUILD_REFERENCES, GET_NODE, GET_PROP, UPDATE_STATUS, ......
Thanks, Xu Wang
_______________________________________________ Libvirt-cim mailing list Libvirt-cim@redhat.com https://www.redhat.com/mailman/listinfo/libvirt-cim
John and Xu, in the last view days I have done some thinking about the idea from Xu to work with the unknown tags and properties. Starting off by looking at how one would get into the situation of losing additional tags and properties I ended up with actually only one feasible explanation: A libvirt-cim user wants to use a libvirt feature not implemented in libvirt-cim. With that in mind I revisited the new proposed pattern again: Before I was thinking that the errors caused by new (unknown) tags and properties are just kind of the reversal of losing the new (unknown) tags and properties. I now come to another conclusion: Now I think that the errors are far worse than I thought, because as long as libvirt-cim is reducing everything to the known management scope the libvirt-cim user has it in his control to create and manage a new error free and working libvirt domain. With the new proposed pattern that is no longer true since if errors are caused by new (unknown) tags and properties that are outside of the known management scope of libvirt-cim the users have NO LONGER the capability to fix these problems from within libvirt-cim! What would be the result? I would guess that bugs would be opened against libvirt-cim reporting that it creates erroneous libvirt domain definitions. I would really not like to see libvirt-cim fixes in support for unsupported libvirt features which are caused by the new pattern of how unknown tag and properties are maintained. If currently bugs are opened that libvirt-cim is following the pattern of reducing a domain definition to its known management scope the answer is very easy: The used libvirt feature that is removed from the domain definition is not supported by libvirt-cim and libvirt-cim only maintains what it is supporting in the domain definition. The question should actually be how the missing libvirt feature can be implemented in libvirt-cim. This unsurprisingly correlates exactly with the second paragraph above ("A libvirt-cim user wants to..."). In summary: I no longer agree to the idea of replacing the old pattern with the new proposed pattern of how to handle unknown tags and properties. Sorry that it took me a while to realize that. I hope that my above explanations can be followed of why I changed my mind. On 11/26/2013 01:31 PM, Xu Wang wrote:
于 2013/11/23 0:18, Boris Fiuczynski 写道:
On 11/21/2013 04:15 AM, Xu Wang wrote:
于 2013/11/21 5:10, John Ferlan 写道:
On 11/20/2013 08:27 AM, Boris Fiuczynski wrote:
John and Xu,
On 11/19/2013 10:49 PM, John Ferlan wrote:
On 11/18/2013 09:59 AM, Boris Fiuczynski wrote: > John and Xu Wang, > here are a few general observations from side: First off - I tend to find myself agreeing with Boris here. I think the concept is important and necessary; however, I'm not convinced the implementation goes far enough.
> 1) I agree that it makes sense to preserve the unknown xml > "entities" > even so it can create complex scenarios and even new kinds of > errors if > unknown entities depend on known entities which get modified making > them > unusable for the unknown entities. This error would probably be the > reversal of what is currently the problem when unknown entities > simply > disappear. Is there a more concrete example of "new kinds of errors if unknown entities depend on known entities which get modified making them unusable for the unknown entities" that can be given? Just for clarity. I've read that line a few times and I'm still not sure :-) OK, let's take a look at device type disk. Since 1.0.2 the property sgio was added. Let's assume this is the unknown entity. sgio is only valid for property device "lun". If one now changes the property device to "disk" than the unknown entity sgio would cause an error when specified.
Ah - I see. Not only do you have to manage the properties you have to know how to use them as well and all their rules. I forgot about that. I came from HP/HPVM and yes, this brings back all sorts of memories...
Seems like in this case, when/if the property was changed from "lun" to "disk" - code would have to search that 'others' list for the "sgio" property and know how to handle adjusting it. That'll get tricky...
You cannot search in others for something you do not know about... unless you can code magic! :-) If needed that's possible. We just coding just like...xpath. Before that we should make sure if it's necessary.
-- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski 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
Dear Boris and John, Firstly thank you very much for your suggestion. Every comment from you make me gain my knowledge on libvirt-cim. But I think I should explain what the goal we want to reach. The reason I got this mission is most of bugs reported recently is users lost some of tags during resource operation. Even more serious is the lost tag had nothing to do with the device they updated. For example, if one attribute in <disk> device changed, the "model" attribute in the <cpu> lost after updated! Isn't it a serious bug? What should we do after a bug reported? One patch generated. And another bug, another patch...But our users may fall into reporting bugs like this, again and again. After thought about the whole process carefully I think we may lost my original intention. That is just adding some struture to keep some unsupported tags without missing during updating even OTHER devices. Hence, with regard to the device updating issue we ever focused on, an option could be offered to the code may use it instead of user. Such as <lun> and <disk> Boris mentioned. That situation is impossible occur now because we haven't added this type into libvirt-cim so it belongs to UNKNOWN_TYPE now. That option or some other hack code could be added into this patch (maybe made in the future). So my design is just designed a set of basic operations and use them intead of changed the old pattern. All xml reading and generating logic just like before. As we discuss more the design becomes more complex. So I think I should describe my new version design now, 1. Boris's data structure is so good that it looks very tiny and suitable. That's a good choice to keep data structure and unsupported tags now. 2. Basic operations based on macros is a good choice. 3. We shouldn't extend the function of data structure but just mark it with status UNMANAGED, MANAGED and DELETED (UPDATED is not necessary because all data just could be updated in the virt_device members now). 3. As to resource updating, we just could change the variables in the libvirt-cim API so some resource updating issue would not occur. 4. I am glad to be responsible for all works about it. At last, I sincerely hope get any suggestion from you to solve that issue (my customers and boss almost take me to shreds because that), even an absolutely new one :-) Thanks, Xu Wang 于 2013/12/2 16:28, Boris Fiuczynski 写道:
John and Xu, in the last view days I have done some thinking about the idea from Xu to work with the unknown tags and properties.
Starting off by looking at how one would get into the situation of losing additional tags and properties I ended up with actually only one feasible explanation: A libvirt-cim user wants to use a libvirt feature not implemented in libvirt-cim.
With that in mind I revisited the new proposed pattern again: Before I was thinking that the errors caused by new (unknown) tags and properties are just kind of the reversal of losing the new (unknown) tags and properties. I now come to another conclusion: Now I think that the errors are far worse than I thought, because as long as libvirt-cim is reducing everything to the known management scope the libvirt-cim user has it in his control to create and manage a new error free and working libvirt domain. With the new proposed pattern that is no longer true since if errors are caused by new (unknown) tags and properties that are outside of the known management scope of libvirt-cim the users have NO LONGER the capability to fix these problems from within libvirt-cim! What would be the result? I would guess that bugs would be opened against libvirt-cim reporting that it creates erroneous libvirt domain definitions. I would really not like to see libvirt-cim fixes in support for unsupported libvirt features which are caused by the new pattern of how unknown tag and properties are maintained. If currently bugs are opened that libvirt-cim is following the pattern of reducing a domain definition to its known management scope the answer is very easy: The used libvirt feature that is removed from the domain definition is not supported by libvirt-cim and libvirt-cim only maintains what it is supporting in the domain definition. The question should actually be how the missing libvirt feature can be implemented in libvirt-cim. This unsurprisingly correlates exactly with the second paragraph above ("A libvirt-cim user wants to...").
In summary: I no longer agree to the idea of replacing the old pattern with the new proposed pattern of how to handle unknown tags and properties.
Sorry that it took me a while to realize that. I hope that my above explanations can be followed of why I changed my mind.
On 11/26/2013 01:31 PM, Xu Wang wrote:
于 2013/11/23 0:18, Boris Fiuczynski 写道:
On 11/21/2013 04:15 AM, Xu Wang wrote:
于 2013/11/21 5:10, John Ferlan 写道:
On 11/20/2013 08:27 AM, Boris Fiuczynski wrote:
John and Xu,
On 11/19/2013 10:49 PM, John Ferlan wrote: > On 11/18/2013 09:59 AM, Boris Fiuczynski wrote: >> John and Xu Wang, >> here are a few general observations from side: > First off - I tend to find myself agreeing with Boris here. I > think the > concept is important and necessary; however, I'm not convinced the > implementation goes far enough. > >> 1) I agree that it makes sense to preserve the unknown xml >> "entities" >> even so it can create complex scenarios and even new kinds of >> errors if >> unknown entities depend on known entities which get modified >> making >> them >> unusable for the unknown entities. This error would probably be >> the >> reversal of what is currently the problem when unknown entities >> simply >> disappear. > Is there a more concrete example of "new kinds of errors if unknown > entities depend on known entities which get modified making them > unusable for the unknown entities" that can be given? Just for > clarity. > I've read that line a few times and I'm still not sure :-) OK, let's take a look at device type disk. Since 1.0.2 the property sgio was added. Let's assume this is the unknown entity. sgio is only valid for property device "lun". If one now changes the property device to "disk" than the unknown entity sgio would cause an error when specified.
Ah - I see. Not only do you have to manage the properties you have to know how to use them as well and all their rules. I forgot about that. I came from HP/HPVM and yes, this brings back all sorts of memories...
Seems like in this case, when/if the property was changed from "lun" to "disk" - code would have to search that 'others' list for the "sgio" property and know how to handle adjusting it. That'll get tricky...
You cannot search in others for something you do not know about... unless you can code magic! :-) If needed that's possible. We just coding just like...xpath. Before that we should make sure if it's necessary.
On 12/03/2013 03:05 AM, Xu Wang wrote:
> Dear Boris and John,
> Firstly thank you very much for your suggestion. Every comment from
> you make me gain my knowledge on libvirt-cim. But I think I should
> explain what the goal we want to reach.
> The reason I got this mission is most of bugs reported recently is
> users lost some of tags during resource operation. Even more serious is
> the lost tag had nothing to do with the device they updated. For
> example, if one attribute in <disk> device changed, the "model"
> attribute in the <cpu> lost after updated! Isn't it a serious bug?
Actually this is not a bug but a behavior of libvirt-cim that it reduces
a guest definition to the scope of its management domain. This is done
so that the user can observe and set all the settings in the management
domain of libvirt-cim and nothing is preventing him from storing this by
libvirt-cim changed guest definition.
To make my point: How did the model attribute get into the cpu tag? Not
via libvirt-cim! Why did someone use other means to do this modification
and than uses libvirt-cim again? Why not implement this libvirt feature
so that is available in libvirt-cim?
I think that the solution should be to implement the missing feature in
libvirt-cim so that the model attribute and whatever else might be
missing to support it can be observed and set with libvirt-cim.
> What
> should we do after a bug reported? One patch generated. And another bug,
> another patch...But our users may fall into reporting bugs like this,
> again and again.
Isn't that business as usual? If libvirt-cim does not support a libvirt
feature is that a bug or a feature request? I would say it is a feature
request.
But if one uses other means to set this libvirt feature in the guest
definition and is using libvirt-cim again on this guest definition he
should be made aware that libvirt-cim manages within its own management
scope and settings will get lost that are outside of this management
scope to maintain the manageability from within libvirt-cim.
> After thought about the whole process carefully I think we may lost
> my original intention. That is just adding some struture to keep some
> unsupported tags without missing during updating even OTHER devices.
> Hence, with regard to the device updating issue we ever focused on, an
> option could be offered to the code may use it instead of user. Such as
> <lun> and <disk> Boris mentioned. That situation is impossible occur now
> because we haven't added this type into libvirt-cim so it belongs to
> UNKNOWN_TYPE now. That option or some other hack code could be added
> into this patch (maybe made in the future).
Actually all we need to look at is the use of the method
ModifyResourceSettings on KVM_VirtualSystemManagementService. Correct?
Looking at how the new pattern behaves:
The modified RASD will delete the others structure of internal data
representation (meaning all unknown tags and attributes of the
corresponding guest xml instance are deleted).
You are correct that the example I gave is not a problem... with the way
you handle modified instances by deleting all unknown elements and guest
definitionerties.
A few questions are coming to my mind:
1) Is it acceptable that e.g. the unsupported per device boot setting is
eliminated by an update of e.g. a disk resource? Is that the next bug
someone will open?
2) What happens if two or more device instances have dependencies on
each other by settings that libvirt-cim does not handle but libvirt does?
3) If someone by other means chose to use per device boot elements in
the guest definition and now wants to modify this guest definition
setting the boot order with libvirt-cim it will no longer work since the
OS boot element and the per-device boot elements are mutually exclusive
and libvirt is enforcing this! It would leave the user stuck using
libvirt-cim.
> So my design is just designed a set of basic operations and use them
> intead of changed the old pattern. All xml reading and generating logic
> just like before.
>
> As we discuss more the design becomes more complex. So I think I
> should describe my new version design now,
> 1. Boris's data structure is so good that it looks very tiny and
> suitable. That's a good choice to keep data structure and unsupported
> tags now.
> 2. Basic operations based on macros is a good choice.
> 3. We shouldn't extend the function of data structure but just mark
> it with status UNMANAGED, MANAGED and DELETED (UPDATED is not necessary
> because all data just could be updated in the virt_device members now).
> 3. As to resource updating, we just could change the variables in the
> libvirt-cim API so some resource updating issue would not occur.
> 4. I am glad to be responsible for all works about it.
>
> At last, I sincerely hope get any suggestion from you to solve that
> issue (my customers and boss almost take me to shreds because that),
> even an absolutely new one :-)
>
> Thanks,
> Xu Wang
>
>
> 于 2013/12/2 16:28, Boris Fiuczynski 写道:
>> John and Xu,
>> in the last view days I have done some thinking about the idea from Xu
>> to work with the unknown tags and guest definitionerties.
>>
>> Starting off by looking at how one would get into the situation of
>> losing additional tags and guest definitionerties I ended up with actually only
>> one feasible explanation: A libvirt-cim user wants to use a libvirt
>> feature not implemented in libvirt-cim.
>>
>> With that in mind I revisited the new guest definitionosed pattern again:
>> Before I was thinking that the errors caused by new (unknown) tags and
>> guest definitionerties are just kind of the reversal of losing the new (unknown)
>> tags and guest definitionerties.
>> I now come to another conclusion: Now I think that the errors are far
>> worse than I thought, because as long as libvirt-cim is reducing
>> everything to the known management scope the libvirt-cim user has it
>> in his control to create and manage a new error free and working
>> libvirt domain.
>> With the new guest definitionosed pattern that is no longer true since if errors
>> are caused by new (unknown) tags and guest definitionerties that are outside of
>> the known management scope of libvirt-cim the users have NO LONGER the
>> capability to fix these problems from within libvirt-cim!
>> What would be the result? I would guess that bugs would be opened
>> against libvirt-cim reporting that it creates erroneous libvirt domain
>> definitions.
>> I would really not like to see libvirt-cim fixes in support for
>> unsupported libvirt features which are caused by the new pattern of
>> how unknown tag and guest definitionerties are maintained.
>> If currently bugs are opened that libvirt-cim is following the pattern
>> of reducing a domain definition to its known management scope the
>> answer is very easy: The used libvirt feature that is removed from the
>> domain definition is not supported by libvirt-cim and libvirt-cim only
>> maintains what it is supporting in the domain definition.
>> The question should actually be how the missing libvirt feature can be
>> implemented in libvirt-cim. This unsurprisingly correlates exactly
>> with the second paragraph above ("A libvirt-cim user wants to...").
>>
>> In summary: I no longer agree to the idea of replacing the old pattern
>> with the new guest definitionosed pattern of how to handle unknown tags and
>> guest definitionerties.
>>
>> Sorry that it took me a while to realize that. I hope that my above
>> explanations can be followed of why I changed my mind.
>>
>>
>> On 11/26/2013 01:31 PM, Xu Wang wrote:
>>>
>>> 于 2013/11/23 0:18, Boris Fiuczynski 写道:
>>>> On 11/21/2013 04:15 AM, Xu Wang wrote:
>>>>>
>>>>> 于 2013/11/21 5:10, John Ferlan 写道:
>>>>>> On 11/20/2013 08:27 AM, Boris Fiuczynski wrote:
>>>>>>> John and Xu,
>>>>>>>
>>>>>>> On 11/19/2013 10:49 PM, John Ferlan wrote:
>>>>>>>> On 11/18/2013 09:59 AM, Boris Fiuczynski wrote:
>>>>>>>>> John and Xu Wang,
>>>>>>>>> here are a few general observations from side:
>>>>>>>> First off - I tend to find myself agreeing with Boris here. I
>>>>>>>> think the
>>>>>>>> concept is important and necessary; however, I'm not convinced the
>>>>>>>> implementation goes far enough.
>>>>>>>>
>>>>>>>>> 1) I agree that it makes sense to preserve the unknown xml
>>>>>>>>> "entities"
>>>>>>>>> even so it can create complex scenarios and even new kinds of
>>>>>>>>> errors if
>>>>>>>>> unknown entities depend on known entities which get modified
>>>>>>>>> making
>>>>>>>>> them
>>>>>>>>> unusable for the unknown entities. This error would probably be
>>>>>>>>> the
>>>>>>>>> reversal of what is currently the problem when unknown entities
>>>>>>>>> simply
>>>>>>>>> disappear.
>>>>>>>> Is there a more concrete example of "new kinds of errors if unknown
>>>>>>>> entities depend on known entities which get modified making them
>>>>>>>> unusable for the unknown entities" that can be given? Just for
>>>>>>>> clarity.
>>>>>>>> I've read that line a few times and I'm still not sure :-)
>>>>>>> OK, let's take a look at device type disk.
>>>>>>> Since 1.0.2 the guest definitionerty sgio was added. Let's assume this is the
>>>>>>> unknown entity. sgio is only valid for guest definitionerty device "lun".
>>>>>>> If one now changes the guest definitionerty device to "disk" than the unknown
>>>>>>> entity
>>>>>>> sgio would cause an error when specified.
>>>>>>>
>>>>>> Ah - I see. Not only do you have to manage the guest definitionerties you
>>>>>> have to
>>>>>> know how to use them as well and all their rules. I forgot about
>>>>>> that. I
>>>>>> came from HP/HPVM and yes, this brings back all sorts of memories...
>>>>>>
>>>>>> Seems like in this case, when/if the guest definitionerty was changed from
>>>>>> "lun" to
>>>>>> "disk" - code would have to search that 'others' list for the "sgio"
>>>>>> guest definitionerty and know how to handle adjusting it. That'll get tricky...
>>>> You cannot search in others for something you do not know about...
>>>> unless you can code magic! :-)
>>> If needed that's possible. We just coding just like...xpath. Before that
>>> we should make sure if it's necessary.
>>
>>
>
--
Mit freundlichen Grüßen/Kind regards
Boris Fiuczynski
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
participants (5)
-
Boris Fiuczynski -
John Ferlan -
Viktor Mihajlovski -
Xu Wang -
Xu Wang