δΊ 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
[email protected]
https://www.redhat.com/mailman/listinfo/libvirt-cim
_______________________________________________
Libvirt-cim mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/libvirt-cim
