On 10/06/2014 01:23 PM, John Ferlan wrote: > > > On 10/03/2014 09:20 AM, Ján Tomko wrote: >> On 09/30/2014 11:35 PM, John Ferlan wrote: >>> https://bugzilla.redhat.com/show_bug.cgi?id=1146837 >>> >>> Resolve a crash in libvirtd resulting from commit id 'a4bd62ad' (1.0.6) >>> which added parentaddr and unique_id to allow unique identification of >>> a scsi_host, but assumed that all the pool entries and the incoming >>> definition would be similarly defined. If the existing pool uses the >>> 'name' attribute and an incoming pool is using the parentaddr/unique_id, >>> then the code will attempt to compare the existing name string against >>> the incoming name string which doesn't exist (is NULL) and results in >>> a core (STREQ). >>> >> >> Fixing this crash would be nicer in a separate patch. >> > > This patch does fix the crash and it must fix the side effect to having > that check (e.g. both pool and incoming def use name). The crash is the > condition where incoming definition doesn't use the same XML format as > the already defined pool. Adding in the mismatched definition checks in > a prior or future patch doesn't make sense mainly because all that was > considered previously was matching definitions. > >>> Conversely, if the existing pool used the parentaddr/unique_id and the >>> to be defined pool used the name, then the comparison would be against >>> the parentaddr, but since the incoming pool doesn't have one - that would >>> leave the comparison against a parentaddr of all 0's and a unique_id of 0, >>> which will always comparison to fail. This means someone could define the >>> same source adapter for two pools >> >> When defining a storage pool, we don't check if the adapter name or >> parentaddr/unique_id is valid, so I don't think we should require it to be >> valid to detect duplicates. > > If you mean we don't check that the name starts with 'scsi' or > 'scsi_host', then sure I agree, but that would be a different bug or > issue. I can certainly add a check if that's desired to ensure prefix > is correct. Of course, the docs : > > http://libvirt.org/formatstorage.html > > do provide the rules for the name property (and less so for the parent). >
I mean we don't check if a scsi controller is present on the specified PCI
address when the pool is defined (so the definiton does not depend on host
hardware). After this patch, I can successfully define a pool with:
<adapter type='scsi_host'>
<parentaddr unique_id='1'>
<address domain='0x0000' bus='0x00' slot='0x1f' function='0x0'/>
</parentaddr>
</adapter>
(where 00:1f.0 is some ISA bridge on my system)
But defining another pool with <adapter type='scsi_host'> and name specified
now depends on the host hardware, i.e. it always fails:
<adapter type='scsi_host' name='host1'/>
error: Failed to define pool from pool.xml
error: operation failed: Storage source conflict with pool: 'scsi-pool'
Depending on the host hardware for duplicate detection during definition seems
weird to me, considering we don't do that for the first definition on the pool
either.
Jan
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
