Re: [libvirt] [PATCH v2 3/3] storage_conf: Resolve libvirtd crash matching scsi_host

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(a)redhat.com&gt; > --- > 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.