On 10/29/2014 01:49 PM, John Ferlan wrote: > > > On 10/29/2014 07:31 AM, Ján Tomko wrote: >> On 10/06/2014 11:49 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). >>> >>> 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 >>> >>> In order to resolve this, adjust the code to get the 'host#' to be used >>> by the storage scsi backend in order to check/start the pool and make sure >>> the incoming definition doesn't match any of the existing pool defs. >>> >>> Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx> >>> --- >>> src/conf/storage_conf.c | 69 ++++++++++++++++++++++++++++++------------------- >>> 1 file changed, 42 insertions(+), 27 deletions(-) >>> >>> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c >>> index 36696a4..19c452b 100644 >>> --- a/src/conf/storage_conf.c >>> +++ b/src/conf/storage_conf.c >>> @@ -2062,26 +2062,37 @@ virStoragePoolObjIsDuplicate(virStoragePoolObjListPtr pools, >>> return ret; >>> } >>> >>> -static bool >>> -matchSCSIAdapterParent(virStoragePoolObjPtr pool, >>> - virStoragePoolDefPtr def) >>> +static int >>> +getSCSIHostNumber(virStoragePoolSourceAdapter adapter, >>> + unsigned int *hostnum) >>> { >>> - virDevicePCIAddressPtr pooladdr = >>> - &pool->def->source.adapter.data.scsi_host.parentaddr; >>> - virDevicePCIAddressPtr defaddr = >>> - &def->source.adapter.data.scsi_host.parentaddr; >>> - int pool_unique_id = >>> - pool->def->source.adapter.data.scsi_host.unique_id; >>> - int def_unique_id = >>> - def->source.adapter.data.scsi_host.unique_id; >>> - if (pooladdr->domain == defaddr->domain && >>> - pooladdr->bus == defaddr->bus && >>> - pooladdr->slot == defaddr->slot && >>> - pooladdr->function == defaddr->function && >>> - pool_unique_id == def_unique_id) { >>> - return true; >>> - } >>> - return false; >>> + int ret = -1; >>> + unsigned int num; >>> + char *name = NULL; >>> + >>> + if (adapter.data.scsi_host.has_parent) { >>> + virDevicePCIAddress addr = adapter.data.scsi_host.parentaddr; >>> + unsigned int unique_id = adapter.data.scsi_host.unique_id; >>> + >>> + if (!(name = virGetSCSIHostNameByParentaddr(addr.domain, >>> + addr.bus, >>> + addr.slot, >>> + addr.function, >>> + unique_id))) >>> + goto cleanup; >> >> This still has the same problem v1 does: >> https://www.redhat.com/archives/libvir-list/2014-October/msg00117.html >> >> A valid pool definition for an existing device can be refused because another >> definition, the one we already accepted, is invalid. I think this is strange >> behavior and I would rather allow duplicit pools if the user went through the >> trouble of bypassing our checks. >> > > 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' Jan
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list