Re: [libvirt] [PATCH 3/3] esx: Add support for the controller element
On 06/22/2010 04:52 PM, Matthias Bolte wrote:
2010/6/23 Eric Blake <eblake(a)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 ;)
So, I'll have to document the controller element first in order to
document the model attribute in a central place. For now I adapted the
existing documentation in the ESX section to document the model
attribute.
Fair compromise.
I then resumed my patch review. A lot of it looks like mechanical
renaming, so it wasn't as big as the number of changed lines led me to
believe. For example:
+++ b/src/esx/esx_vmx.h
@@ -29,17 +29,25 @@
# include "esx_vi.h"
int
-esxVMX_SCSIDiskNameToControllerAndID(const char *name, int *controller, int *id);
+esxVMX_SCSIDiskNameToControllerAndUnit(const char *name, int *controller,
+ int *unit);
It may have been nicer to separate the renaming from the actual
functionality additions. Oh well; at this point I'm not going to insist
that you split it up, now that I've reviewed the whole thing as-is.
+++ 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?
At any rate:
ACK to this patch, and the formatdomain.html.in cleanups can come as a
separate patch later.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 619 bytes)