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(-)