[libvirt] [PATCH 3/3] esx: Add support for the controller element

[Matthias Bolte]

2010/6/23 Eric Blake <eblake at redhat.com>:
> On 06/22/2010 04:52 PM, Matthias Bolte wrote:
>> 2010/6/23 Eric Blake <eblake at redhat.com>:
>>> On 06/17/2010 03:15 PM, Matthias Bolte wrote:
>>>> Also don't abuse the disk driver name to specify the SCSI controller
>>>> model anymore:
>>>>
>>>>   <driver name='buslogic'/>
>>>>
>>>> Use the newly added model attribute of the controller element for this:
>>>>
>>>>   <controller type='scsi' index='0' model='buslogic'/>
>>>
>>> I don't see any change to docs/schemas/domain.rng for this new XML
>>> attribute.  Am I missing something, or how do you write an xml file that
>>> uses this attribute and still passes validation?
>>
>> The model attribute itself was added in a previous patch of this
>> series, including domain.rng extension. This patch adds usage for the
>> previously added attribute.
>
> Serves me right for only looking at 3/3, since 2/3 already had an ack ;)
>
> I see it now, and it looks okay.  Thanks for clearing up my question.
>
>>>>  docs/drvesx.html.in                              |   25 +-
>>>
>>> Is the xml addition ESX-specific, or should the new controller attribute
>>> also be documented in docs/formatdomain.html.in?  Is anything other than
>>> model='lsilogic' supported?
>>
>> Currently it's only used by ESX, but it should be documented in
>> docs/formatdomain.html.in. The problem here is that
>> docs/formatdomain.html.in currently completely lacks documentation for
>> the controller element, that was initially added for the QEMU driver a
>> while ago.
>
> formatdomain.html.in lacks a number of things, doesn't it ;)

Yes, we're adding new stuff faster than we're documenting it properly.

>> +++ b/tests/vmx2xmldata/vmx2xml-esx-in-the-wild-1.xml
>> @@ -13,10 +13,11 @@
>>    <on_crash>destroy</on_crash>
>>    <devices>
>>      <disk type='file' device='disk'>
>> -      <driver name='lsilogic'/>
>>        <source file='[datastore] directory/Fedora11.vmdk'/>
>>        <target dev='sda' bus='scsi'/>
>> +      <address type='drive' controller='0' bus='0' unit='0'/>
>>      </disk>
>> +    <controller type='scsi' index='0' model='lsilogic'/>
>>      <interface type='bridge'>
>>        <mac address='00:50:56:91:48:c7'/>
>>        <source bridge='VM Network'/>
>
> I noticed you converted most of your tests to the new scheme.  But
> shouldn't you leave at least one test on the old scheme (or better yet,
> create a new test whose sole purpose is to test the old scheme), to
> ensure you don't break backwards compatibility?

Well, the XML in the VMX-2-XML tests is the output generated by
libvirt, so it's correct that all those files use the new syntax. In
case of the XML-2-VMX tests the XML files are the input, and here all
tests still use the old way to define the SCSI controller model.

Actually, I should add XML-2-VMX tests that use the new syntax and I
should add some negative tests to make sure disk address verification
works correct. But that's stuff for an additional patch.

> At any rate:
>
> ACK to this patch, and the formatdomain.html.in cleanups can come as a
> separate patch later.
>

Thanks, pushed.

Matthias