On 03/14/2014 10:20 AM, Boris Fiuczynski wrote:
Changes since last time -
1. Address review comments 2. I did not add CONTROLLER_INDEX_NOT_SET. Since that's a -1 value and the index is an unsigned value - it just didn't seem right. Furthermore, if libvirt doesn't find the 'index' value, it defaults to using 0 when parsing the XML - so I think following that model is better. I have yet to chase down all the libvirt paths to see what happens, but I think if someone adds a controller without defining an index and that index conflicts with something already there for that named/type of controller, then libvirt will reject the xml for the guest to start requiring the "user" to fix it. If I remember correctly libvirt has a mechanism to look for the next free index available. Defaulting the index when not specified to 0 would
On 03/14/2014 01:56 PM, John Ferlan wrote: prevent libvirt-cim users from exploiting the mechanism.
OK - I guess it's the -1 into the unsigned value that really caught my attention. Now that things are "merged" into one change it may be easier for my brain to comprehend where things are. I assume this would be the 'create' path where we'd have to allow that -1 value. The problem I have is how then is then the dev->id thing. I'm trying to get the "big picture" of how that will be generated then. I'll go back and look at the change in the context of your previous review. Once libvirt creates the guest, it will be assigned a number and thus the next read of the XML would have it.
3. I didn't yet do it, but I think the 'master' may need to be removed. The libvirt documented example is:
<devices> <controller type='usb' index='0' model='ich9-ehci1'> <address type='pci' domain='0' bus='0' slot='4' function='7'/> </controller> <controller type='usb' index='0' model='ich9-uhci1'> <master startport='0'/> <address type='pci' domain='0' bus='0' slot='4' function='0' multifunction='on'/> </controller> ... </devices>
This examples shows that the type is 'usb' and the 'index' is 0 for both which would violate our namespace rule. The 'model' is the same too. So unless we incorporate the address into the name, then there's a conflict. Yes, that is a correct observation and I agree with you that removing the master and restricting the support for master is a feasible way. If at some later point this special case really becomes a requirement than it would still be possible to extend the InstanceID for these cases, I guess.
I'll work on removing it then. Not all good intentions work out... John
I'll have to think about this one some more and of course take advice!
4. Just realized I forgot to switch the mof values for queues, ports, and vectors back to string types... I'll do that, but didn't want to lose my current cover letter.
Xu Wang (3): libxutil, xmlgen: Add Controller Support RASD: Schema and Provider Support for Controller RASDs VSMS: Support for domains with controller devices
libxkutil/device_parsing.c | 105 +++++++++++++++++++++- libxkutil/device_parsing.h | 15 ++++ libxkutil/xmlgen.c | 52 +++++++++++ schema/ResourceAllocationSettingData.mof | 41 +++++++++ schema/ResourceAllocationSettingData.registration | 1 + src/Virt_ElementSettingData.c | 1 + src/Virt_RASD.c | 81 +++++++++++++++-- src/Virt_SettingsDefineState.c | 1 + src/Virt_VSSDComponent.c | 1 + src/Virt_VirtualSystemManagementService.c | 76 ++++++++++++++++ src/svpc_types.h | 4 +- 11 files changed, 368 insertions(+), 10 deletions(-)