[libvirt] [PATCH v2 3/3] storage_conf: Resolve libvirtd crash matching scsi_host

[Ján Tomko]

On 10/29/2014 03:56 PM, John Ferlan wrote:
> 
> 
> On 10/29/2014 09:33 AM, Ján Tomko wrote:
>> On 10/29/2014 01:49 PM, John Ferlan wrote:
> 
> <...snip...>
>>> I think I'm missing your point. The finding of duplicate sources for
>>> storage pools and disallowing them to be defined (or created) has been
>>> around since commit id '5a1f27287".
>>>
>>> I'm not sure what you meant by your first sentence - perhaps an example
>>> would help me especially in the accepted, but invalid condition.
>>
>> Perhaps 'invalid' wasn't the best choice - the definition itself is valid, but
>> it doesn't refer to an existing device so the pool cannot be started up.
>>
>> It's possible to define a pool using parentaddr (even on a host with no SCSI,
>> as the address is only used on pool startup, not definition):
>>     <adapter type='scsi_host'>
>>       <parentaddr unique_id='1'>
>>         <address domain='0x0000' bus='0x00' slot='0x1f' function='0x0'/>
>>       </parentaddr>
>>     </adapter>
>>
>> After that, defining a pool with scsi_host source fails. Both via name:
>> <adapter type='scsi_host' name='scsi_host13'/>
>> and via parenaddr:
>>     <adapter type='scsi_host'>
>>       <parentaddr unique_id='1'>
>>         <address domain='0x0000' bus='0x00' slot='0x10' function='0x0'/>
>>       </parentaddr>
>>     </adapter>
>> because of the first, already accepted definition:
>> error: Failed to define pool from scsi2.xml
>> error: XML error: Failed to find scsi_host using PCI '0000:00:1f.0' and
>> unique_id='1'
>>
>>
> 
> OK - so the issue you are bringing up resolves around an "incorrect"
> definition of a pool that will fail on startup and why would restrict
> someone from creating a second, but perhaps correct definition rather
> than perhaps fixing their first definition.
> 
> As an interesting corollary how is this any different than the following
> example using a DIR pool with a bad target :
> 
> # cat x1.xml
> <pool type='dir'>
>   <name>x1</name>
>   <source>
>   </source>
>   <target>
>     <path>/avr/lib/libvirt/images</path>
>     <permissions>
>       <mode>0755</mode>
>       <owner>-1</owner>
>       <group>-1</group>
>     </permissions>
>   </target>
> </pool>
> 
> # cat x2.xml
> <pool type='dir'>
>   <name>x2</name>
>   <source>
>   </source>
>   <target>
>     <path>/var/lib/libvirt/images</path>
>     <permissions>
>       <mode>0755</mode>
>       <owner>-1</owner>
>       <group>-1</group>
>     </permissions>
>   </target>
> </pool>
> 
> # virsh pool-define x1.xml
> Pool x1 defined from x1.xml
> 
> # virsh pool-define x2.xml
> error: Failed to define pool from x2.xml
> error: operation failed: Storage source conflict with pool: 'x1'
> 

This is not what happens with current libvirt - the pool is defined successfully.

But if we did this for a directory pool it would be equally strange - the
paths are obviously different so there is no conflict.

> # virsh pool-start x1
> error: Failed to start pool x1
> error: cannot open path '/avr/lib/libvirt/images': No such file or directory
> 
> 
> While I see your point, I guess I would expect someone to edit their
> current incorrect definition rather than trying to create a new one and
> use that instead.

Reporting errors related to the old pool when trying to define a new one seems
confusing to me.

> I think this code follows the spirit of what other
> code does and enforces only 1 target per pool at definition time
> regardless of whether that target is valid.That's a different problem
> which is always a bit controversial - allowing bogus definitions to exist.

It's not foolproof - you can mount -o bind the directory somewhere else and
define the pool successfully.

Regarding bogus definitions and newly introduced checks, I think it's nice to
leave them defined and only report errors on startup (that way you can still
use management tools to fix/delete them after libvirt upgrade).

And a bogus pool definition shouldn't block the sane pool definitions, which
is what happens here.

Jan

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20141029/d6516825/attachment-0001.sig>