[libvirt] [PATCH 1/1] libvirtd crash when defining scsi storage pool

[Pradipta Kumar Banerjee]

On 09/03/2014 03:28 PM, John Ferlan wrote:
> 
> [found this in my "probably should look at this one some day" pile..]
> 
> On 06/21/2014 12:57 PM, Pradipta Kr. Banerjee wrote:
>> libvirtd crashes  when  there is an existing SCSI pool
>> with adapter type as 'scsi_host' and defining a new SCSI pool with adapter
>> type as 'fc_host' and parent attribute missing.
>>
>> For eg when defining a storage-pool with the following XML will crash libvirtd
>> if there already exists a SCSI pool with adapter type 'scsi_host'
>>
>>   <pool type='scsi'>
>>     <name>TEST_SCSI_FC_POOL</name>
>>     <source>
>>        <adapter type='fc_host' wwnn='1234567890abcdef' wwpn='abcdef1234567890'/>
>>     </source>
>>     <target>
>>        <path>/dev/disk/by-path</path>
>>     </target>
>>   </pool>
>>
>> This happens because for fc_host, adapter 'name' is not relevant whereas
>> for scsi_host its mandatory attribute. However the check in libvirt for
>> finding duplicate storage pools doesn't take that into account while comparing,
>> resulting into crash
>>
>> This patch fixes the issue
> 
> Going to need to clean the above up, but if you also provided the existing
> scsi/scsi_host pool def that would have been good!
> 
>>
>> Signed-off-by: Pradipta Kr. Banerjee <bpradip at in.ibm.com>
>> ---
>>  src/conf/storage_conf.c | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
>> index 8b6fd79..54a4589 100644
>> --- a/src/conf/storage_conf.c
>> +++ b/src/conf/storage_conf.c
>> @@ -2126,8 +2126,10 @@ virStoragePoolSourceFindDuplicate(virStoragePoolObjListPtr pools,
>>                      STREQ(pool->def->source.adapter.data.fchost.wwpn,
>>                            def->source.adapter.data.fchost.wwpn))
>>                      matchpool = pool;
>> -            } else if (pool->def->source.adapter.type ==
>> -                       VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST){
>> +            } else if ((pool->def->source.adapter.type ==
>> +                        VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST)\
>> +                        && (def->source.adapter.type ==
>> +                           VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST)) {
> 
> My assumption is this issue is a result of commit id '9f781da'
> 
> Beyond the - it needs a bit a refresh due to other changes
> since that time - I'm wondering if you've only solved half
> the potential problem

You are right.
> 
> What if the fc_host is defined and you're coming in with
> a 'scsi_host' def->source.adapter.type?  (see a few lines
> earlier)
This will result in crash too.. I'll send a V2 based on your suggestion
> 
> 
>>                  if (STREQ(pool->def->source.adapter.data.name,
>>                            def->source.adapter.data.name))
>>                      matchpool = pool;
>>
> 
> So instead of what's here, how about the following diff:
> 
> 
>              break;
>          case VIR_STORAGE_POOL_SCSI:
>              if (pool->def->source.adapter.type ==
> +                VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST &&
> +                def->source.adapter.type ==
>                  VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) {
>                  if (STREQ(pool->def->source.adapter.data.fchost.wwnn,
>                            def->source.adapter.data.fchost.wwnn) &&
> @@ -2124,6 +2126,8 @@ virStoragePoolSourceFindDuplicate(virStoragePoolObjListPtr
>                            def->source.adapter.data.fchost.wwpn))
>                      matchpool = pool;
>              } else if (pool->def->source.adapter.type ==
> +                       VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST &&
> +                       def->source.adapter.type ==
>                         VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) {
>                  if (pool->def->source.adapter.data.scsi_host.name) {
>                      if (STREQ(pool->def->source.adapter.data.scsi_host.name,
> 
> 
> John
> 
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
> 


-- 
Regards,
Pradipta Kumar B(bpradip at in.ibm.com)
IBM Systems & Technology Labs,
India.