Re: [PATCH] vmx: Ensure unique disk targets when parsing

[Michal Prívozník <mprivozn@xxxxxxxxxx>]

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@xxxxxxxxxx>
> ---
>  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