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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list