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

[John Ferlan]

[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

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)


>                  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