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.