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&gt;: >> 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 ;)

> +++ 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.