[Libvirt-cim] [REPOST MERGED PATCH 0/3] Repost controller series merged

[John Ferlan]

On 03/14/2014 10:20 AM, Boris Fiuczynski wrote:
> On 03/14/2014 01:56 PM, John Ferlan 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 
> 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(-)
>>
> 
>