Resolve issue with duplicated SCSI hostdev addresses

[libvirt] [PATCH 3/4] conf: Use existing SCSI hostdev model to create new

Posted by John Ferlan 7 years, 5 months ago

In virDomainDefMaybeAddHostdevSCSIcontroller when we add a new
controller because someone neglected to add one or we're adding
one because the existing one is full, we should copy over the
model number from the existing controller since whatever we
create should at least have the same characteristics as the one
we cannot use because it's full.

NB: This affects the existing hostdev-scsi-autogen-address test
which would add a default ('lsi') SCSI controller for the various
scsi_host's that would create a controller for the hostdev.

Signed-off-by: John Ferlan <jferlan@redhat.com>
---
 src/conf/domain_conf.c                                    | 13 ++++++++++++-
 tests/qemuxml2xmloutdata/hostdev-scsi-autogen-address.xml |  2 +-
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 66e21c4bd..61b4a0075 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -17689,12 +17689,22 @@ virDomainDefMaybeAddHostdevSCSIcontroller(virDomainDefPtr def)
     size_t i;
     int maxController = -1;
     virDomainHostdevDefPtr hostdev;
+    virDomainControllerModelSCSI model = -1;
+    virDomainControllerModelSCSI newModel = -1;
 
     for (i = 0; i < def->nhostdevs; i++) {
         hostdev = def->hostdevs[i];
         if (virHostdevIsSCSIDevice(hostdev) &&
             (int)hostdev->info->addr.drive.controller > maxController) {
             maxController = hostdev->info->addr.drive.controller;
+            /* We may be creating a new controller because this one is full.
+             * So let's grab the model from it and update the model we're
+             * going to add as long as this one isn't undefined. The premise
+             * being keeping the same controller model for all SCSI hostdevs. */
+            model = virDomainDeviceFindControllerModel(def, hostdev->info,
+                                                       VIR_DOMAIN_CONTROLLER_TYPE_SCSI);
+            if (model != -1)
+                newModel = model;
         }
     }
 
@@ -17702,7 +17712,8 @@ virDomainDefMaybeAddHostdevSCSIcontroller(virDomainDefPtr def)
         return 0;
 
     for (i = 0; i <= maxController; i++) {
-        if (virDomainDefMaybeAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_SCSI, i, -1) < 0)
+        if (virDomainDefMaybeAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_SCSI,
+                                           i, newModel) < 0)
             return -1;
     }
 
diff --git a/tests/qemuxml2xmloutdata/hostdev-scsi-autogen-address.xml b/tests/qemuxml2xmloutdata/hostdev-scsi-autogen-address.xml
index 8e93056ee..cea212b64 100644
--- a/tests/qemuxml2xmloutdata/hostdev-scsi-autogen-address.xml
+++ b/tests/qemuxml2xmloutdata/hostdev-scsi-autogen-address.xml
@@ -29,7 +29,7 @@
       <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/>
     </controller>
     <controller type='pci' index='0' model='pci-root'/>
-    <controller type='scsi' index='1'>
+    <controller type='scsi' index='1' model='virtio-scsi'>
       <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/>
     </controller>
     <input type='mouse' bus='ps2'/>
-- 
2.13.6

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 3/4] conf: Use existing SCSI hostdev model to create new

Posted by Eric Farman 7 years, 5 months ago


On 12/06/2017 08:08 AM, John Ferlan wrote:
> In virDomainDefMaybeAddHostdevSCSIcontroller when we add a new
> controller because someone neglected to add one or we're adding
> one because the existing one is full, we should copy over the
> model number from the existing controller since whatever we
> create should at least have the same characteristics as the one
> we cannot use because it's full.
> 
> NB: This affects the existing hostdev-scsi-autogen-address test
> which would add a default ('lsi') SCSI controller for the various
> scsi_host's that would create a controller for the hostdev.
> 
> Signed-off-by: John Ferlan <jferlan@redhat.com>
> ---
>   src/conf/domain_conf.c                                    | 13 ++++++++++++-
>   tests/qemuxml2xmloutdata/hostdev-scsi-autogen-address.xml |  2 +-
>   2 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 66e21c4bd..61b4a0075 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -17689,12 +17689,22 @@ virDomainDefMaybeAddHostdevSCSIcontroller(virDomainDefPtr def)
>       size_t i;
>       int maxController = -1;
>       virDomainHostdevDefPtr hostdev;
> +    virDomainControllerModelSCSI model = -1;
> +    virDomainControllerModelSCSI newModel = -1;
> 
>       for (i = 0; i < def->nhostdevs; i++) {
>           hostdev = def->hostdevs[i];
>           if (virHostdevIsSCSIDevice(hostdev) &&
>               (int)hostdev->info->addr.drive.controller > maxController) {
>               maxController = hostdev->info->addr.drive.controller;
> +            /* We may be creating a new controller because this one is full.
> +             * So let's grab the model from it and update the model we're
> +             * going to add as long as this one isn't undefined. The premise
> +             * being keeping the same controller model for all SCSI hostdevs. */
> +            model = virDomainDeviceFindControllerModel(def, hostdev->info,
> +                                                       VIR_DOMAIN_CONTROLLER_TYPE_SCSI);
> +            if (model != -1)
> +                newModel = model;

What's the harm if the last controller were undefined?  Wouldn't it get 
populated down the road anyway if one were not set at this point (we 
send -1 today, after all).  That could eliminate one of the two 
virDomainControllerModelSCSI variables here, I would think.

But, either way I think it's fine.

Reviewed-by: Eric Farman <farman@linux.vnet.ibm.com>

>           }
>       }
> 
> @@ -17702,7 +17712,8 @@ virDomainDefMaybeAddHostdevSCSIcontroller(virDomainDefPtr def)
>           return 0;
> 
>       for (i = 0; i <= maxController; i++) {
> -        if (virDomainDefMaybeAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_SCSI, i, -1) < 0)
> +        if (virDomainDefMaybeAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_SCSI,
> +                                           i, newModel) < 0)
>               return -1;
>       }
> 
> diff --git a/tests/qemuxml2xmloutdata/hostdev-scsi-autogen-address.xml b/tests/qemuxml2xmloutdata/hostdev-scsi-autogen-address.xml
> index 8e93056ee..cea212b64 100644
> --- a/tests/qemuxml2xmloutdata/hostdev-scsi-autogen-address.xml
> +++ b/tests/qemuxml2xmloutdata/hostdev-scsi-autogen-address.xml
> @@ -29,7 +29,7 @@
>         <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/>
>       </controller>
>       <controller type='pci' index='0' model='pci-root'/>
> -    <controller type='scsi' index='1'>
> +    <controller type='scsi' index='1' model='virtio-scsi'>
>         <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/>
>       </controller>
>       <input type='mouse' bus='ps2'/>
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 3/4] conf: Use existing SCSI hostdev model to create new

Posted by John Ferlan 7 years, 4 months ago


On 12/15/2017 11:32 AM, Eric Farman wrote:
> 
> 
> On 12/06/2017 08:08 AM, John Ferlan wrote:
>> In virDomainDefMaybeAddHostdevSCSIcontroller when we add a new
>> controller because someone neglected to add one or we're adding
>> one because the existing one is full, we should copy over the
>> model number from the existing controller since whatever we
>> create should at least have the same characteristics as the one
>> we cannot use because it's full.
>>
>> NB: This affects the existing hostdev-scsi-autogen-address test
>> which would add a default ('lsi') SCSI controller for the various
>> scsi_host's that would create a controller for the hostdev.
>>
>> Signed-off-by: John Ferlan <jferlan@redhat.com>
>> ---
>>   src/conf/domain_conf.c                                    | 13
>> ++++++++++++-
>>   tests/qemuxml2xmloutdata/hostdev-scsi-autogen-address.xml |  2 +-
>>   2 files changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 66e21c4bd..61b4a0075 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -17689,12 +17689,22 @@
>> virDomainDefMaybeAddHostdevSCSIcontroller(virDomainDefPtr def)
>>       size_t i;
>>       int maxController = -1;
>>       virDomainHostdevDefPtr hostdev;
>> +    virDomainControllerModelSCSI model = -1;
>> +    virDomainControllerModelSCSI newModel = -1;
>>
>>       for (i = 0; i < def->nhostdevs; i++) {
>>           hostdev = def->hostdevs[i];
>>           if (virHostdevIsSCSIDevice(hostdev) &&
>>               (int)hostdev->info->addr.drive.controller >
>> maxController) {
>>               maxController = hostdev->info->addr.drive.controller;
>> +            /* We may be creating a new controller because this one
>> is full.
>> +             * So let's grab the model from it and update the model
>> we're
>> +             * going to add as long as this one isn't undefined. The
>> premise
>> +             * being keeping the same controller model for all SCSI
>> hostdevs. */
>> +            model = virDomainDeviceFindControllerModel(def,
>> hostdev->info,
>> +                                                      
>> VIR_DOMAIN_CONTROLLER_TYPE_SCSI);
>> +            if (model != -1)
>> +                newModel = model;
> 
> What's the harm if the last controller were undefined?  Wouldn't it get
> populated down the road anyway if one were not set at this point (we
> send -1 today, after all).  That could eliminate one of the two
> virDomainControllerModelSCSI variables here, I would think.

Yes, a -1 is passed along which (for now) means a SCSI LSI controller
would be created except for pseries which would generate something
different.

John

> 
> But, either way I think it's fine.
> 
> Reviewed-by: Eric Farman <farman@linux.vnet.ibm.com>
> 
>>           }
>>       }
>>
>> @@ -17702,7 +17712,8 @@
>> virDomainDefMaybeAddHostdevSCSIcontroller(virDomainDefPtr def)
>>           return 0;
>>
>>       for (i = 0; i <= maxController; i++) {
>> -        if (virDomainDefMaybeAddController(def,
>> VIR_DOMAIN_CONTROLLER_TYPE_SCSI, i, -1) < 0)
>> +        if (virDomainDefMaybeAddController(def,
>> VIR_DOMAIN_CONTROLLER_TYPE_SCSI,
>> +                                           i, newModel) < 0)
>>               return -1;
>>       }
>>
>> diff --git a/tests/qemuxml2xmloutdata/hostdev-scsi-autogen-address.xml
>> b/tests/qemuxml2xmloutdata/hostdev-scsi-autogen-address.xml
>> index 8e93056ee..cea212b64 100644
>> --- a/tests/qemuxml2xmloutdata/hostdev-scsi-autogen-address.xml
>> +++ b/tests/qemuxml2xmloutdata/hostdev-scsi-autogen-address.xml
>> @@ -29,7 +29,7 @@
>>         <address type='pci' domain='0x0000' bus='0x00' slot='0x01'
>> function='0x1'/>
>>       </controller>
>>       <controller type='pci' index='0' model='pci-root'/>
>> -    <controller type='scsi' index='1'>
>> +    <controller type='scsi' index='1' model='virtio-scsi'>
>>         <address type='pci' domain='0x0000' bus='0x00' slot='0x04'
>> function='0x0'/>
>>       </controller>
>>       <input type='mouse' bus='ps2'/>
>>
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 3/4] conf: Use existing SCSI hostdev model to create new

Posted by Ján Tomko 7 years, 4 months ago

On Wed, Dec 06, 2017 at 08:08:05AM -0500, John Ferlan wrote:
>In virDomainDefMaybeAddHostdevSCSIcontroller when we add a new
>controller because someone neglected to add one or we're adding
>one because the existing one is full, we should copy over the
>model number from the existing controller since whatever we
>create should at least have the same characteristics as the one
>we cannot use because it's full.
>
>NB: This affects the existing hostdev-scsi-autogen-address test
>which would add a default ('lsi') SCSI controller for the various
>scsi_host's that would create a controller for the hostdev.
>

Yet the change adds 'virtio-scsi'.

>Signed-off-by: John Ferlan <jferlan@redhat.com>
>---
> src/conf/domain_conf.c                                    | 13 ++++++++++++-
> tests/qemuxml2xmloutdata/hostdev-scsi-autogen-address.xml |  2 +-
> 2 files changed, 13 insertions(+), 2 deletions(-)
>
>diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>index 66e21c4bd..61b4a0075 100644
>--- a/src/conf/domain_conf.c
>+++ b/src/conf/domain_conf.c
>@@ -17689,12 +17689,22 @@ virDomainDefMaybeAddHostdevSCSIcontroller(virDomainDefPtr def)
>     size_t i;
>     int maxController = -1;
>     virDomainHostdevDefPtr hostdev;
>+    virDomainControllerModelSCSI model = -1;

This declaration can be moved inside the if to reduce its scope. (I'm
with Andrea on this [0])

>+    virDomainControllerModelSCSI newModel = -1;
>
>     for (i = 0; i < def->nhostdevs; i++) {
>         hostdev = def->hostdevs[i];
>         if (virHostdevIsSCSIDevice(hostdev) &&
>             (int)hostdev->info->addr.drive.controller > maxController) {
>             maxController = hostdev->info->addr.drive.controller;
>+            /* We may be creating a new controller because this one is full.
>+             * So let's grab the model from it and update the model we're
>+             * going to add as long as this one isn't undefined. The premise
>+             * being keeping the same controller model for all SCSI hostdevs. */
>+            model = virDomainDeviceFindControllerModel(def, hostdev->info,
>+                                                       VIR_DOMAIN_CONTROLLER_TYPE_SCSI);
>+            if (model != -1)
>+                newModel = model;
>         }
>     }
>

ACK

Jan

[0] https://www.redhat.com/archives/libvir-list/2017-December/msg00563.html
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 3/4] conf: Use existing SCSI hostdev model to create new

Posted by Ján Tomko 7 years, 4 months ago

On Wed, Dec 20, 2017 at 01:52:16PM +0100, Ján Tomko wrote:
>On Wed, Dec 06, 2017 at 08:08:05AM -0500, John Ferlan wrote:
>>In virDomainDefMaybeAddHostdevSCSIcontroller when we add a new
>>controller because someone neglected to add one or we're adding
>>one because the existing one is full, we should copy over the
>>model number from the existing controller since whatever we
>>create should at least have the same characteristics as the one
>>we cannot use because it's full.
>>
>>NB: This affects the existing hostdev-scsi-autogen-address test
>>which would add a default ('lsi') SCSI controller for the various
>>scsi_host's that would create a controller for the hostdev.
>>
>
>Yet the change adds 'virtio-scsi'.
>
>>Signed-off-by: John Ferlan <jferlan@redhat.com>
>>---
>> src/conf/domain_conf.c                                    | 13 ++++++++++++-
>> tests/qemuxml2xmloutdata/hostdev-scsi-autogen-address.xml |  2 +-
>> 2 files changed, 13 insertions(+), 2 deletions(-)
>>
>>diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>>index 66e21c4bd..61b4a0075 100644
>>--- a/src/conf/domain_conf.c
>>+++ b/src/conf/domain_conf.c
>>@@ -17689,12 +17689,22 @@ virDomainDefMaybeAddHostdevSCSIcontroller(virDomainDefPtr def)
>>     size_t i;
>>     int maxController = -1;
>>     virDomainHostdevDefPtr hostdev;
>>+    virDomainControllerModelSCSI model = -1;
>
>This declaration can be moved inside the if to reduce its scope. (I'm
>with Andrea on this [0])
>

Also, assigning -1 to an unsigned variable won't stick.

>>+    virDomainControllerModelSCSI newModel = -1;
>>
>>     for (i = 0; i < def->nhostdevs; i++) {
>>         hostdev = def->hostdevs[i];
>>         if (virHostdevIsSCSIDevice(hostdev) &&
>>             (int)hostdev->info->addr.drive.controller > maxController) {
>>             maxController = hostdev->info->addr.drive.controller;
>>+            /* We may be creating a new controller because this one is full.
>>+             * So let's grab the model from it and update the model we're
>>+             * going to add as long as this one isn't undefined. The premise
>>+             * being keeping the same controller model for all SCSI hostdevs. */
>>+            model = virDomainDeviceFindControllerModel(def, hostdev->info,
>>+                                                       VIR_DOMAIN_CONTROLLER_TYPE_SCSI);
>>+            if (model != -1)

This condition is always true according to my compiler.

Jan

>>+                newModel = model;
>>         }
>>     }
>>
>
>ACK
>
>Jan
>
>[0] https://www.redhat.com/archives/libvir-list/2017-December/msg00563.html



>--
>libvir-list mailing list
>libvir-list@redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 1/4] qemu: Tolerate storage source private data being NULL for hotplug SCSI hostdev
[libvirt] [PATCH 2/4] qemu: Use same model when adding hostdev SCSI controller
[libvirt] [PATCH 3/4] conf: Use existing SCSI hostdev model to create new
[libvirt] [PATCH 4/4] conf: Fix generating addresses for SCSI hostdev