Disk targets were generated in virVMXParseConfig() with
virVMXGenerateDiskTarget(). It works on combination of
controller, fix offset, unit and prefix. While SCSI and SATA have
same prefix "sd", function virVMXGenerateDiskTarget() could
returned in some cases same targets.

In this patch, after loaded SCSI and SATA disks to the def, it
checks if in array exists any SATA disks after SCSI. If not,
nothing happened. If yes, targets of all SATA disks are changed.
Disk target is calculated as: last SCSI target value + n, where n
is position of disk in array after last SCSI (1,2,..).

Because assigned addresses of disks are generated from their
indexes, for every changed SATA disk is called
virDomainDiskDefAssignAddress() with the updated value.

The corresponding tests have been modified to match the index
changes.

Signed-off-by: Adam Julis <ajulis@redhat.com>
---
 src/vmx/vmx.c                            | 32 ++++++++++++++++++++++++
 tests/vmx2xmldata/esx-in-the-wild-12.xml |  4 +--
 tests/vmx2xmldata/esx-in-the-wild-8.xml  |  4 +--
 3 files changed, 36 insertions(+), 4 deletions(-)

diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
index 227744d062..9fdf0f1cd4 100644
--- a/src/vmx/vmx.c
+++ b/src/vmx/vmx.c
@@ -1389,6 +1389,9 @@ virVMXParseConfig(virVMXContext *ctx,
     bool smbios_reflecthost = false;
     int controller;
     int bus;
+    int ndisk;
+    int last_scsi;
+    int offset = -1;
     int port;
     bool present;
     int scsi_virtualDev[4] = { -1, -1, -1, -1 };
@@ -1805,6 +1808,35 @@ virVMXParseConfig(virVMXContext *ctx,
         }
     }
 
+    /* now disks contain only SCSI and SATA, SATA could have same index (dst) as SCSI
+     * find last SCSI index in array and use it as offset for all SATA indexes
+     * (overwrite old values)
+     * finally, regenerate correct addresses, while it depends on the index */
+    for (ndisk = 0; ndisk < def->ndisks; ndisk++) {
+        virDomainDiskDef *dsc = def->disks[ndisk];
+
+        if (dsc->bus == VIR_DOMAIN_DISK_BUS_SCSI) {
+            offset = virDiskNameToIndex(dsc->dst);
+            last_scsi = ndisk;
+            continue;
+        }
+
+        if (offset > -1) {
+            VIR_FREE(def->disks[ndisk]->dst);
+            def->disks[ndisk]->dst = virIndexToDiskName(offset + ndisk - last_scsi, "sd");
+
+            if (virDomainDiskDefAssignAddress(NULL, def->disks[ndisk], def) < 0) {
+                virReportError(VIR_ERR_INTERNAL_ERROR,
+                               _("Could not assign address to disk '%1$s'"),
+                               virDomainDiskGetSource(dsc));
+                goto cleanup;
+            }
+        }
+
+        else
+            break;
+    }
+
     /* def:disks (ide) */
     for (bus = 0; bus < 2; ++bus) {
         for (unit = 0; unit < 2; ++unit) {
diff --git a/tests/vmx2xmldata/esx-in-the-wild-12.xml b/tests/vmx2xmldata/esx-in-the-wild-12.xml
index 42184501d0..a7730845ee 100644
--- a/tests/vmx2xmldata/esx-in-the-wild-12.xml
+++ b/tests/vmx2xmldata/esx-in-the-wild-12.xml
@@ -21,9 +21,9 @@
       <address type='drive' controller='0' bus='0' target='0' unit='0'/>
     </disk>
     <disk type='file' device='cdrom'>
-      <target dev='sda' bus='sata'/>
+      <target dev='sdb' bus='sata'/>
       <readonly/>
-      <address type='drive' controller='0' bus='0' target='0' unit='0'/>
+      <address type='drive' controller='0' bus='0' target='0' unit='1'/>
     </disk>
     <controller type='scsi' index='0' model='vmpvscsi'/>
     <controller type='sata' index='0'/>
diff --git a/tests/vmx2xmldata/esx-in-the-wild-8.xml b/tests/vmx2xmldata/esx-in-the-wild-8.xml
index 47d22ced2a..d5356bda34 100644
--- a/tests/vmx2xmldata/esx-in-the-wild-8.xml
+++ b/tests/vmx2xmldata/esx-in-the-wild-8.xml
@@ -36,9 +36,9 @@
     </disk>
     <disk type='file' device='cdrom'>
       <source file='[692eb778-2d4937fe] CentOS-4.7.ServerCD-x86_64.iso'/>
-      <target dev='sda' bus='sata'/>
+      <target dev='sdd' bus='sata'/>
       <readonly/>
-      <address type='drive' controller='0' bus='0' target='0' unit='0'/>
+      <address type='drive' controller='0' bus='0' target='0' unit='3'/>
     </disk>
     <controller type='scsi' index='0' model='vmpvscsi'/>
     <controller type='sata' index='0'/>
-- 
2.45.2

On 7/19/24 15:25, Adam Julis wrote:
> Disk targets were generated in virVMXParseConfig() with
> virVMXGenerateDiskTarget(). It works on combination of
> controller, fix offset, unit and prefix. While SCSI and SATA have
> same prefix "sd", function virVMXGenerateDiskTarget() could
> returned in some cases same targets.
> 
> In this patch, after loaded SCSI and SATA disks to the def, it
> checks if in array exists any SATA disks after SCSI. If not,
> nothing happened. If yes, targets of all SATA disks are changed.
> Disk target is calculated as: last SCSI target value + n, where n
> is position of disk in array after last SCSI (1,2,..).
> 
> Because assigned addresses of disks are generated from their
> indexes, for every changed SATA disk is called
> virDomainDiskDefAssignAddress() with the updated value.
> 
> The corresponding tests have been modified to match the index
> changes.
> 
> Signed-off-by: Adam Julis <ajulis@redhat.com>
> ---
>  src/vmx/vmx.c                            | 32 ++++++++++++++++++++++++
>  tests/vmx2xmldata/esx-in-the-wild-12.xml |  4 +--
>  tests/vmx2xmldata/esx-in-the-wild-8.xml  |  4 +--
>  3 files changed, 36 insertions(+), 4 deletions(-)

It's nice to see this in action (i.e. test cases being fixed).

> 
> diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
> index 227744d062..9fdf0f1cd4 100644
> --- a/src/vmx/vmx.c
> +++ b/src/vmx/vmx.c
> @@ -1389,6 +1389,9 @@ virVMXParseConfig(virVMXContext *ctx,
>      bool smbios_reflecthost = false;
>      int controller;
>      int bus;
> +    int ndisk;
> +    int last_scsi;
> +    int offset = -1;
>      int port;
>      bool present;
>      int scsi_virtualDev[4] = { -1, -1, -1, -1 };
> @@ -1805,6 +1808,35 @@ virVMXParseConfig(virVMXContext *ctx,
>          }
>      }
>  
> +    /* now disks contain only SCSI and SATA, SATA could have same index (dst) as SCSI
> +     * find last SCSI index in array and use it as offset for all SATA indexes
> +     * (overwrite old values)
> +     * finally, regenerate correct addresses, while it depends on the index */
> +    for (ndisk = 0; ndisk < def->ndisks; ndisk++) {
> +        virDomainDiskDef *dsc = def->disks[ndisk];
> +
> +        if (dsc->bus == VIR_DOMAIN_DISK_BUS_SCSI) {
> +            offset = virDiskNameToIndex(dsc->dst);
> +            last_scsi = ndisk;
> +            continue;
> +        }
> +
> +        if (offset > -1) {
> +            VIR_FREE(def->disks[ndisk]->dst);
> +            def->disks[ndisk]->dst = virIndexToDiskName(offset + ndisk - last_scsi, "sd");
> +
> +            if (virDomainDiskDefAssignAddress(NULL, def->disks[ndisk], def) < 0) {
> +                virReportError(VIR_ERR_INTERNAL_ERROR,
> +                               _("Could not assign address to disk '%1$s'"),
> +                               virDomainDiskGetSource(dsc));
> +                goto cleanup;
> +            }
> +        }
> +
> +        else
> +            break;
> +    }

I'm wondering whether we actually need this logic. At this point, we
know SCSI disks are at the beginning of def->disks array, followed by
SATA disks. I wonder we should just do plain:

    for (i = 0; i < def->ndisks; i++) {
        virDomainDiskDef *disk = def->disks[i];

        VIR_FREE(disk->dst);
        disk->dst = virIndexToDiskName(i, "sd");

        if (virDomainDiskDefAssignAddress(NULL, disk, def) < 0) {
            virReportError(VIR_ERR_INTERNAL_ERROR,
                           _("Could not assign address to disk '%1$s'"),
                           virDomainDiskGetSource(disk));
            goto cleanup;
        }
    }

This has a downside of overwriting more targets. But then again, disk
target is just an unique identifier and we do not (can not) guarantee
the same naming inside guest.

Michal