Re: [libvirt] [PATCH 1/1] libvirtd crash when defining scsi storage pool
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'
>>
>>
>> TEST_SCSI_FC_POOL
>>
>>> wwpn='abcdef1234567890'/>
>>
>>
>>/dev/disk/by-path
>>
>>
>>
>> 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
>> ---
>> 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(bpra...@in.ibm.com)
IBM Systems & Technology Labs,
India.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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'
>
>
> TEST_SCSI_FC_POOL
>
> wwpn='abcdef1234567890'/>
>
>
>/dev/disk/by-path
>
>
>
> 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
> ---
> 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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[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'
TEST_SCSI_FC_POOL
/dev/disk/by-path
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
---
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
