[Libvirt-cim] [PATCH V2 01/48] Add others member for saving unsupported tag and unknown device

[John Ferlan]

On 11/12/2013 10:27 PM, Xu Wang wrote:
> 
> 于 2013/11/13 8:59, John Ferlan 写道:
> Dear John,
> My idea is, using a data structure (a link list) to keep the elements
> left after all kinds of
> virtual devices fetched tags they needed from xml. And if I just mark a
> node of others with
> status instead of delete it, the whole structure of xml could be saved
> as well.
> The introduction of 'id' in 'others' structure is to identify several
> <tags> with the same
> name. The console device support new introduced and some tags in
> 'unknown_device'
> have to use 'id' to identify elements. It maybe a little coarse now so I
> need any suggestion
> from you to make it works better. If you have any better idea to solve
> the unsupported
> tags missing issue please share with me :-) Because so many patches
> about it merged
> and there will be more in the future.
> These 48 patches just the first part in my designing. After that I want
> to build the
> management about all data (even in 'others' member). And all of these
> updates are
> compatitable with older version libvirt-cim. So the upper layer need
> little change and
> keep the original feature but could make libvirt-cim becomes more powerful.
> XFAIL about hotplug disappeared after updated. About the warning you
> mentioned above
> I'll check and fix it in the new version.
> 
> Thanks,
> Xu Wang

I don't have any thoughts on better ideas - it's just not my focus.
There's a lot of changes being made in this series which I suppose in
hindsight would have been nice to have been broken up a bit more to make
it manageable to review, understand, and test.  I've started with the
first 3 patches as it seems all they do is create infrastructure and
"for the most part" no one should be calling them.

I started with patch 1...  got to patch 2 and had issues.  I ran patch 2
through coverity and it complained about the memset() in
cleanup_virt_device().

A recent patch changed that from "memset(&dev->dev, 0,
sizeof(dev->dev));" to "memset(dev, 0, sizeof(*dev));".  Coverity
complained about the path from "classify_resources()" for
CIM_RES_TYPE_DISK into the rasd_to_vdev() call, then into
_sysvirt_rasd_to_vdev(), disk_rasd_to_vdev(), and finally
cleanup_virt_device().

It seems the change to device_parsing.h, inclusion by
Virt_VirtualSystemManagementService.c, and usage of virt_device as a
static structure caused the "issue" as a "clean" build fixed things.
Although it took a while for me to figure it out.  I suppose that's more
a build/Makefile issue than anything else...

The second issue I've run into is that by only applying the first two
patches, I can get a test to cause cimprovagt to fail:

--------------------------------------------------------------------
VirtualSystemManagementService - 16_removeresource.py: FAIL
ERROR 	- (1, u'CIM_ERR_FAILED: Lost connection with cimprovagt
"libvirt-cim".')
InvokeMethod(RemoveResourceSettings): CIM_ERR_FAILED: Lost connection
with cimprovagt "libvirt-cim".
--------------------------------------------------------------------

This one's really frustrating as I'm really perplexed why a possible fix
could work. It seems if I don't call cleanup_others() from
cleanup_unknown_device(), then I don't get that core.  I have no idea
why though. Is it possibly related to the static usage of virt_device? I
see this even after a clean build.

Maybe this "cleanup_unknown_device()" and it's counter-part
"dev_unknown" should have been added after the others work. It seems
there are two things being done at once - creating an 'others' list to
store the read xml and creating a list of unknown devices.

You've done a lot of work and it seems promising as a way handle new
tags.  Of course I ask myself why is it so important to go through this
exercise.  Are the libvirt API's deficient?  I would think libvirt
should be the only thing handling XML format and the applications that
use libvirt would have API's to create, define, get, set, start, stop,
etc VM's.  Having to store the XML is "dangerous" insomuch as we've
already found it can change and libvirt-cim cannot keep up.

Anyway, as an exercise for me to learn and because I pushed a number of
changes which conflict with what you've already done... I will try to
help with the effort to compact each patch series into manageable parts.
FWIW: I pushed the other patches because they were posted first - I just
went in order.

John