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

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