[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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 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 redhat com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]