[Libvirt-cim] [PATCH 00/20] REWORK/PARIAL: Changes to solve unsupported tag issue
Boris Fiuczynski
fiuczy at linux.vnet.ibm.com
Mon Nov 18 14:59:36 UTC 2013
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(-)
>
More information about the Libvirt-cim
mailing list