[libvirt] [PATCH 1/1] libvirtd crash when defining scsi storage pool
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 Signed-off-by: Pradipta Kr. Banerjee <bpradip@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)) { if (STREQ(pool->def->source.adapter.data.name, def->source.adapter.data.name)) matchpool = pool; -- 1.9.3
[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@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
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@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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- Regards, Pradipta Kumar B(bpradip@in.ibm.com) IBM Systems & Technology Labs, India.
participants (3)
-
John Ferlan -
Pradipta Kr. Banerjee -
Pradipta Kumar Banerjee