Re: [Libvirt-cim] [PATCH 00/20] REWORK/PARIAL: Changes to solve unsupported tag issue

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