6:23 p.m.
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
6:23 p.m.
New subject: [PATCH 01/20] Add others member for saving unsupported tag and unknown device
From: Xu Wang <cngesaint(a)gmail.com>
Signed-off-by: Xu Wang <gesaint(a)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
6:23 p.m.
New subject: [PATCH 02/20] Add others and unknown_device clean up
From: Xu Wang <cngesaint(a)gmail.com>
Signed-off-by: Xu Wang <gesaint(a)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
6:23 p.m.
New subject: [PATCH 03/20] Add basic operations for reading data from xml node
From: Xu Wang <cngesaint(a)gmail.com>
Signed-off-by: Xu Wang <gesaint(a)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
6:23 p.m.
New subject: [PATCH 04/20] Fix xml parsing algorithm for parse_fs_device()
From: Xu Wang <cngesaint(a)gmail.com>
Signed-off-by: Xu Wang <gesaint(a)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
6:23 p.m.
New subject: [PATCH 05/20] Fix xml parsing algorithm for parse_block_device()
From: Xu Wang <cngesaint(a)gmail.com>
Signed-off-by: Xu Wang <gesaint(a)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
6:23 p.m.
New subject: [PATCH 06/20] Fix xml parsing algorithm for parse_vsi_device()
From: Xu Wang <cngesaint(a)gmail.com>
Signed-off-by: Xu Wang <gesaint(a)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
6:23 p.m.
New subject: [PATCH 07/20] Fix xml parsing algorithm for parse_net_device()
From: Xu Wang <cngesaint(a)gmail.com>
Signed-off-by: Xu Wang <gesaint(a)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
6:23 p.m.
New subject: [PATCH 08/20] Fix xml parsing algorithm for parse_vcpu_device()
From: Xu Wang <cngesaint(a)gmail.com>
Signed-off-by: Xu Wang <gesaint(a)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
6:23 p.m.
New subject: [PATCH 09/20] Fix xml parsing algorithm for parse_emu_device()
From: Xu Wang <cngesaint(a)gmail.com>
Signed-off-by: Xu Wang <gesaint(a)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
6:23 p.m.
New subject: [PATCH 10/20] Fix xml parsing algorithm for parse_mem_device()
From: Xu Wang <cngesaint(a)gmail.com>
Signed-off-by: Xu Wang <gesaint(a)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
6:24 p.m.
New subject: [PATCH 11/20] Fix xml parsing algorithm for parse_console_device()
From: Xu Wang <cngesaint(a)gmail.com>
Signed-off-by: Xu Wang <gesaint(a)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
6:24 p.m.
New subject: [PATCH 12/20] Fix xml parsing algorithm for parse_graphics_device()
From: Xu Wang <cngesaint(a)gmail.com>
Signed-off-by: Xu Wang <gesaint(a)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
6:24 p.m.
New subject: [PATCH 13/20] Fix xml parsing algorithm for parse_input_device()
From: Xu Wang <cngesaint(a)gmail.com>
Signed-off-by: Xu Wang <gesaint(a)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
6:24 p.m.
New subject: [PATCH 14/20] Add parse_unknown_device()
From: Xu Wang <cngesaint(a)gmail.com>
Signed-off-by: Xu Wang <gesaint(a)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
6:24 p.m.
New subject: [PATCH 15/20] Add parse_devices() for unknown type in get_dominfo_from_xml()
From: Xu Wang <cngesaint(a)gmail.com>
Signed-off-by: Xu Wang <gesaint(a)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
6:24 p.m.
New subject: [PATCH 16/20] Fix xml parsing algorithm in parse_domain()
From: Xu Wang <cngesaint(a)gmail.com>
Signed-off-by: Xu Wang <gesaint(a)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
6:24 p.m.
New subject: [PATCH 17/20] Fix xml parsing algorithm in parse_os()
From: Xu Wang <cngesaint(a)gmail.com>
Signed-off-by: Xu Wang <gesaint(a)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
6:24 p.m.
New subject: [PATCH 18/20] Fix xml parsing algorithm in parse_features()
From: Xu Wang <cngesaint(a)gmail.com>
Signed-off-by: Xu Wang <gesaint(a)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
6:24 p.m.
New subject: [PATCH 19/20] Add dup function for device copy
From: Xu Wang <cngesaint(a)gmail.com>
Signed-off-by: Xu Wang <gesaint(a)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
6:24 p.m.
New subject: [PATCH 20/20] Add type CIM_RES_TYPE_DELETED and modify type as it after resource_del
From: Xu Wang <cngesaint(a)gmail.com>
Signed-off-by: Xu Wang <gesaint(a)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
10:04 a.m.
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
12:41 p.m.
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
12:09 p.m.
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
1:49 p.m.
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
8:59 a.m.
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(-)
3:49 p.m.
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:
2:36 a.m.
于 2013/11/20 5:49, John Ferlan 写道:
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
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:
2:24 p.m.
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
7:27 a.m.
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
3:10 p.m.
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
>
>
9:15 p.m.
于 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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvirt-cim
10:18 a.m.
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! :-)
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
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(a)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
6:31 a.m.
于 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.
> 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
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(a)redhat.com
>> https://www.redhat.com/mailman/listinfo/libvirt-cim
>>
>
2:28 a.m.
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
8:05 p.m.
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.
7:05 a.m.
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
3978
days inactive
4006
days old
36 comments
5 participants
participants (5)
-
Boris Fiuczynski -
John Ferlan -
Viktor Mihajlovski -
Xu Wang -
Xu Wang