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

[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(a)in.ibm.com&gt; > --- > 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 -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list