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

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