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