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