Re: [libvirt] [PATCH v2 3/3] storage_conf: Resolve libvirtd crash matching scsi_host
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'
# 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. 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.
John