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

[John Ferlan]

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