8:19 a.m.
The first patch fixes an issue.
The last one - I am not sure if it's not going to break something and
thus it's optional.
Michal Prívozník (4):
vmx: Accept empty fileName for cdrom-image
vmx2xmltest: Add another test case
vmx: Separate disk target name generation into a function
vmx: Ensure unique disk targets when parsing
src/vmx/vmx.c | 199 +++++++++++++++--------
tests/vmx2xmldata/esx-in-the-wild-12.vmx | 86 ++++++++++
tests/vmx2xmldata/esx-in-the-wild-12.xml | 39 +++++
tests/vmx2xmldata/esx-in-the-wild-8.xml | 4 +-
tests/vmx2xmltest.c | 1 +
5 files changed, 261 insertions(+), 68 deletions(-)
create mode 100644 tests/vmx2xmldata/esx-in-the-wild-12.vmx
create mode 100644 tests/vmx2xmldata/esx-in-the-wild-12.xml
--
2.41.0
8:19 a.m.
New subject: [PATCH 1/4] vmx: Accept empty fileName for cdrom-image
Turns out, there are two ways to specify an empty CD-ROM drive in
a .vmx file:
1) .fileName = "emptyBackingString"
2) .fileName = ""
While we do parse 1) successfully, the code does not accept 2)
and an error is reported. Modify the code to treat both cases the
same.
Resolves: https://issues.redhat.com/browse/RHEL-19380
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/vmx/vmx.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
index 26b89776e1..af1c1640ae 100644
--- a/src/vmx/vmx.c
+++ b/src/vmx/vmx.c
@@ -2483,7 +2483,8 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOption *xmlopt,
virConf *conf,
*/
goto ignore;
}
- } else if (fileName && STREQ(fileName, "emptyBackingString"))
{
+ } else if (fileName && (STREQ(fileName, "") ||
+ STREQ(fileName, "emptyBackingString"))) {
if (deviceType && STRCASENEQ(deviceType, "cdrom-image")) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Expecting VMX entry '%1$s' to be
'cdrom-image' but found '%2$s'"),
--
2.41.0
8:20 a.m.
New subject: [PATCH 1/4] vmx: Accept empty fileName for cdrom-image
On a Friday in 2023, Michal Privoznik wrote:
Turns out, there are two ways to specify an empty CD-ROM drive in
a .vmx file:
1) .fileName = "emptyBackingString"
2) .fileName = ""
While we do parse 1) successfully, the code does not accept 2)
and an error is reported. Modify the code to treat both cases the
same.
Resolves: https://issues.redhat.com/browse/RHEL-19380
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/vmx/vmx.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
8:19 a.m.
New subject: [PATCH 2/4] vmx2xmltest: Add another test case
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
tests/vmx2xmldata/esx-in-the-wild-12.vmx | 86 ++++++++++++++++++++++++
tests/vmx2xmldata/esx-in-the-wild-12.xml | 39 +++++++++++
tests/vmx2xmltest.c | 1 +
3 files changed, 126 insertions(+)
create mode 100644 tests/vmx2xmldata/esx-in-the-wild-12.vmx
create mode 100644 tests/vmx2xmldata/esx-in-the-wild-12.xml
diff --git a/tests/vmx2xmldata/esx-in-the-wild-12.vmx
b/tests/vmx2xmldata/esx-in-the-wild-12.vmx
new file mode 100644
index 0000000000..31963ffb12
--- /dev/null
+++ b/tests/vmx2xmldata/esx-in-the-wild-12.vmx
@@ -0,0 +1,86 @@
+.encoding = "UTF-8"
+config.version = "8"
+virtualHW.version = "20"
+vmci0.present = "TRUE"
+floppy0.present = "FALSE"
+svga.vramSize = "8388608"
+memSize = "2048"
+firmware = "efi"
+tools.upgrade.policy = "manual"
+sched.cpu.units = "mhz"
+vm.createDate = "1702444883756312"
+scsi0.virtualDev = "pvscsi"
+scsi0.present = "TRUE"
+sata0.present = "TRUE"
+sata0:0.startConnected = "FALSE"
+sata0:0.deviceType = "cdrom-image"
+sata0:0.fileName = ""
+sata0:0.present = "TRUE"
+scsi0:0.deviceType = "scsi-hardDisk"
+scsi0:0.fileName = "Auto-esx8.0-rhell9.3-efi-with-empty-cdrom.vmdk"
+sched.scsi0:0.shares = "normal"
+sched.scsi0:0.throughputCap = "off"
+scsi0:0.present = "TRUE"
+ethernet0.allowGuestConnectionControl = "FALSE"
+ethernet0.virtualDev = "vmxnet3"
+ethernet0.networkName = "VM Network"
+ethernet0.addressType = "vpx"
+ethernet0.generatedAddress = "00:50:56:a0:cf:2f"
+ethernet0.uptCompatibility = "TRUE"
+ethernet0.present = "TRUE"
+displayName = "Auto-esx8.0-rhell9.3-efi-with-empty-cdrom"
+guestOS = "rhel9-64"
+chipset.motherboardLayout = "acpi"
+uefi.secureBoot.enabled = "TRUE"
+toolScripts.afterPowerOn = "TRUE"
+toolScripts.afterResume = "TRUE"
+toolScripts.beforeSuspend = "TRUE"
+toolScripts.beforePowerOff = "TRUE"
+tools.syncTime = "FALSE"
+tools.guest.desktop.autolock = "TRUE"
+uuid.bios = "42 20 fc a7 11 dd d6 7e-19 cc fc ad 0a 37 c3 42"
+vc.uuid = "50 20 ca 5b 7a c1 5c 44-cf a1 9e 76 2e 2f 93 3b"
+nvram = "Auto-esx8.0-rhell9.3-efi-with-empty-cdrom.nvram"
+svga.present = "TRUE"
+hpet0.present = "TRUE"
+RemoteDisplay.maxConnections = "-1"
+sched.cpu.latencySensitivity = "normal"
+numa.autosize.cookie = "10012"
+numa.autosize.vcpu.maxPerVirtualNode = "1"
+cpuid.coresPerSocket.cookie = "1"
+pciBridge1.present = "TRUE"
+pciBridge1.virtualDev = "pciRootBridge"
+pciBridge1.functions = "1"
+pciBridge1:0.pxm = "0"
+pciBridge0.present = "TRUE"
+pciBridge0.virtualDev = "pciRootBridge"
+pciBridge0.functions = "1"
+pciBridge0.pxm = "-1"
+scsi0.pciSlotNumber = "32"
+ethernet0.pciSlotNumber = "33"
+sata0.pciSlotNumber = "34"
+scsi0.sasWWID = "50 05 05 67 11 dd d6 70"
+vmotion.checkpointFBSize = "4194304"
+vmotion.checkpointSVGAPrimarySize = "8388608"
+vmotion.svga.mobMaxSize = "8388608"
+vmotion.svga.graphicsMemoryKB = "8192"
+monitor.phys_bits_used = "45"
+softPowerOff = "FALSE"
+tools.remindInstall = "TRUE"
+svga.guestBackedPrimaryAware = "TRUE"
+guestInfo.detailed.data = "architecture='X86' bitness='64'
cpeString='cpe:/o:redhat:enterprise_linux:9::baseos' distroAddlVersion='9.3
(Plow)' distroName='Red Hat Enterprise Linux' distroVersion='9.3'
familyName='Linux' kernelVersion='5.14.0-362.8.1.el9_3.x86_64'
prettyName='Red Hat Enterprise Linux 9.3 (Plow)'"
+vmxstats.filename = "esx8.0-rhel9.3-x86_64.scoreboard"
+migrate.hostLog = "Auto-esx8.0-rhell9.3-efi-with-empty-cdrom-3ea47ba9.hlog"
+sched.cpu.min = "0"
+sched.cpu.shares = "normal"
+sched.mem.min = "0"
+sched.mem.minSize = "0"
+sched.mem.shares = "normal"
+migrate.encryptionMode = "opportunistic"
+ftcpt.ftEncryptionMode = "ftEncryptionOpportunistic"
+viv.moid =
"e00e5864-a7ed-4b49-b99a-71da911f679c:vm-1008:OCn73UgW4dBwF0sgk/5SmLb24snLgdgGTDWbdB/O5g4="
+sched.swap.derivedName =
"/vmfs/volumes/c97af7e9-686a74ad/Auto-esx8.0-rhell9.3-efi-with-empty-cdrom/Auto-esx8.0-rhell9.3-efi-with-empty-cdrom-155bb14b.vswp"
+uuid.location = "56 4d 4b 9e d2 17 63 9b-98 0e 92 5d 77 2a a3 83"
+scsi0:0.redo = ""
+vmci0.id = "171426626"
+cleanShutdown = "FALSE"
diff --git a/tests/vmx2xmldata/esx-in-the-wild-12.xml
b/tests/vmx2xmldata/esx-in-the-wild-12.xml
new file mode 100644
index 0000000000..42184501d0
--- /dev/null
+++ b/tests/vmx2xmldata/esx-in-the-wild-12.xml
@@ -0,0 +1,39 @@
+<domain type='vmware'>
+ <name>Auto-esx8.0-rhell9.3-efi-with-empty-cdrom</name>
+ <uuid>4220fca7-11dd-d67e-19cc-fcad0a37c342</uuid>
+ <memory unit='KiB'>2097152</memory>
+ <currentMemory unit='KiB'>2097152</currentMemory>
+ <vcpu placement='static'>1</vcpu>
+ <cputune>
+ <shares>1000</shares>
+ </cputune>
+ <os firmware='efi'>
+ <type arch='x86_64'>hvm</type>
+ </os>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <disk type='file' device='disk'>
+ <source file='[datastore]
directory/Auto-esx8.0-rhell9.3-efi-with-empty-cdrom.vmdk'/>
+ <target dev='sda' bus='scsi'/>
+ <address type='drive' controller='0' bus='0'
target='0' unit='0'/>
+ </disk>
+ <disk type='file' device='cdrom'>
+ <target dev='sda' bus='sata'/>
+ <readonly/>
+ <address type='drive' controller='0' bus='0'
target='0' unit='0'/>
+ </disk>
+ <controller type='scsi' index='0' model='vmpvscsi'/>
+ <controller type='sata' index='0'/>
+ <interface type='bridge'>
+ <mac address='00:50:56:a0:cf:2f' type='generated'/>
+ <source bridge='VM Network'/>
+ <model type='vmxnet3'/>
+ </interface>
+ <video>
+ <model type='vmvga' vram='8192' primary='yes'/>
+ </video>
+ </devices>
+</domain>
diff --git a/tests/vmx2xmltest.c b/tests/vmx2xmltest.c
index a43cab3108..785eee6d30 100644
--- a/tests/vmx2xmltest.c
+++ b/tests/vmx2xmltest.c
@@ -262,6 +262,7 @@ mymain(void)
DO_TEST("esx-in-the-wild-9");
DO_TEST("esx-in-the-wild-10");
DO_TEST("esx-in-the-wild-11");
+ DO_TEST("esx-in-the-wild-12");
DO_TEST("gsx-in-the-wild-1");
DO_TEST("gsx-in-the-wild-2");
--
2.41.0
8:22 a.m.
New subject: [PATCH 2/4] vmx2xmltest: Add another test case
On a Friday in 2023, Michal Privoznik wrote:
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
tests/vmx2xmldata/esx-in-the-wild-12.vmx | 86 ++++++++++++++++++++++++
tests/vmx2xmldata/esx-in-the-wild-12.xml | 39 +++++++++++
tests/vmx2xmltest.c | 1 +
3 files changed, 126 insertions(+)
create mode 100644 tests/vmx2xmldata/esx-in-the-wild-12.vmx
create mode 100644 tests/vmx2xmldata/esx-in-the-wild-12.xml
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
8:19 a.m.
New subject: [PATCH 3/4] vmx: Separate disk target name generation into a function
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/vmx/vmx.c | 175 +++++++++++++++++++++++++++++++-------------------
1 file changed, 110 insertions(+), 65 deletions(-)
diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
index af1c1640ae..399f03b419 100644
--- a/src/vmx/vmx.c
+++ b/src/vmx/vmx.c
@@ -2136,6 +2136,113 @@ virVMXParseSATAController(virConf *conf, int controller, bool
*present)
}
+static int
+virXMXGenerateDiskTarget(virDomainDiskDef *def,
+ virDomainDef *vmdef,
+ int controllerOrBus,
+ int unit)
+{
+ const char *prefix = NULL;
+ unsigned int idx = 0;
+
+ switch (def->bus) {
+ case VIR_DOMAIN_DISK_BUS_SCSI:
+ if (controllerOrBus < 0 || controllerOrBus > 3) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("SCSI controller index %1$d out of [0..3]
range"),
+ controllerOrBus);
+ return -1;
+ }
+
+ if (unit < 0 || unit > vmdef->scsiBusMaxUnit || unit == 7) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("SCSI unit index %1$d out of [0..6,8..%2$u]
range"),
+ unit, vmdef->scsiBusMaxUnit);
+ return -1;
+ }
+
+ idx = controllerOrBus * 15 + (unit < 7 ? unit : unit - 1);
+ prefix = "sd";
+ break;
+
+ case VIR_DOMAIN_DISK_BUS_SATA:
+ if (controllerOrBus < 0 || controllerOrBus > 3) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("SATA controller index %1$d out of [0..3]
range"),
+ controllerOrBus);
+ return -1;
+ }
+
+ if (unit < 0 || unit >= 30) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("SATA unit index %1$d out of [0..29] range"),
+ unit);
+ return -1;
+ }
+
+ idx = controllerOrBus * 30 + unit;
+ prefix = "sd";
+ break;
+
+ case VIR_DOMAIN_DISK_BUS_IDE:
+ if (controllerOrBus < 0 || controllerOrBus > 1) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("IDE bus index %1$d out of [0..1] range"),
+ controllerOrBus);
+ return -1;
+ }
+
+ if (unit < 0 || unit > 1) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("IDE unit index %1$d out of [0..1] range"),
unit);
+ return -1;
+ }
+ idx = controllerOrBus * 2 + unit;
+ prefix = "hd";
+ break;
+
+ case VIR_DOMAIN_DISK_BUS_FDC:
+ if (controllerOrBus != 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("FDC controller index %1$d out of [0] range"),
+ controllerOrBus);
+ return -1;
+ }
+
+ if (unit < 0 || unit > 1) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("FDC unit index %1$d out of [0..1] range"),
+ unit);
+ return -1;
+ }
+
+ idx = unit;
+ prefix = "fd";
+ break;
+
+ case VIR_DOMAIN_DISK_BUS_VIRTIO:
+ case VIR_DOMAIN_DISK_BUS_XEN:
+ case VIR_DOMAIN_DISK_BUS_USB:
+ case VIR_DOMAIN_DISK_BUS_UML:
+ case VIR_DOMAIN_DISK_BUS_SD:
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Unsupported bus type '%1$s' for device type
'%2$s'"),
+ virDomainDiskBusTypeToString(def->bus),
+ virDomainDiskDeviceTypeToString(def->device));
+ return -1;
+ break;
+
+ case VIR_DOMAIN_DISK_BUS_NONE:
+ case VIR_DOMAIN_DISK_BUS_LAST:
+ virReportEnumRangeError(virDomainDiskBus, def->bus);
+ return -1;
+ break;
+ }
+
+ def->dst = virIndexToDiskName(idx, prefix);
+ return 0;
+}
+
static int
virVMXParseDisk(virVMXContext *ctx, virDomainXMLOption *xmlopt, virConf *conf,
@@ -2208,60 +2315,11 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOption *xmlopt,
virConf *conf,
if (device == VIR_DOMAIN_DISK_DEVICE_DISK ||
device == VIR_DOMAIN_DISK_DEVICE_CDROM) {
if (busType == VIR_DOMAIN_DISK_BUS_SCSI) {
- if (controllerOrBus < 0 || controllerOrBus > 3) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("SCSI controller index %1$d out of [0..3]
range"),
- controllerOrBus);
- goto cleanup;
- }
-
- if (unit < 0 || unit > vmdef->scsiBusMaxUnit || unit == 7) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("SCSI unit index %1$d out of [0..6,8..%2$u]
range"),
- unit, vmdef->scsiBusMaxUnit);
- goto cleanup;
- }
-
prefix = g_strdup_printf("scsi%d:%d", controllerOrBus, unit);
-
- (*def)->dst =
- virIndexToDiskName
- (controllerOrBus * 15 + (unit < 7 ? unit : unit - 1),
"sd");
} else if (busType == VIR_DOMAIN_DISK_BUS_SATA) {
- if (controllerOrBus < 0 || controllerOrBus > 3) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("SATA controller index %1$d out of [0..3]
range"),
- controllerOrBus);
- goto cleanup;
- }
-
- if (unit < 0 || unit >= 30) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("SATA unit index %1$d out of [0..29] range"),
- unit);
- goto cleanup;
- }
-
prefix = g_strdup_printf("sata%d:%d", controllerOrBus, unit);
-
- (*def)->dst = virIndexToDiskName(controllerOrBus * 30 + unit,
"sd");
} else if (busType == VIR_DOMAIN_DISK_BUS_IDE) {
- if (controllerOrBus < 0 || controllerOrBus > 1) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("IDE bus index %1$d out of [0..1] range"),
- controllerOrBus);
- goto cleanup;
- }
-
- if (unit < 0 || unit > 1) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("IDE unit index %1$d out of [0..1] range"),
unit);
- goto cleanup;
- }
-
prefix = g_strdup_printf("ide%d:%d", controllerOrBus, unit);
-
- (*def)->dst = virIndexToDiskName(controllerOrBus * 2 + unit,
"hd");
} else {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("Unsupported bus type '%1$s' for device type
'%2$s'"),
@@ -2271,23 +2329,7 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOption *xmlopt,
virConf *conf,
}
} else if (device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) {
if (busType == VIR_DOMAIN_DISK_BUS_FDC) {
- if (controllerOrBus != 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("FDC controller index %1$d out of [0]
range"),
- controllerOrBus);
- goto cleanup;
- }
-
- if (unit < 0 || unit > 1) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("FDC unit index %1$d out of [0..1] range"),
- unit);
- goto cleanup;
- }
-
prefix = g_strdup_printf("floppy%d", unit);
-
- (*def)->dst = virIndexToDiskName(unit, "fd");
} else {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("Unsupported bus type '%1$s' for device type
'%2$s'"),
@@ -2302,6 +2344,9 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOption *xmlopt,
virConf *conf,
goto cleanup;
}
+ if (virXMXGenerateDiskTarget(*def, vmdef, controllerOrBus, unit) < 0)
+ goto cleanup;
+
VMX_BUILD_NAME(present);
VMX_BUILD_NAME(startConnected);
VMX_BUILD_NAME(deviceType);
--
2.41.0
8:22 a.m.
New subject: [PATCH 3/4] vmx: Separate disk target name generation into a
function
On a Friday in 2023, Michal Privoznik wrote:
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/vmx/vmx.c | 175 +++++++++++++++++++++++++++++++-------------------
1 file changed, 110 insertions(+), 65 deletions(-)
diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
index af1c1640ae..399f03b419 100644
--- a/src/vmx/vmx.c
+++ b/src/vmx/vmx.c
@@ -2136,6 +2136,113 @@ virVMXParseSATAController(virConf *conf, int controller, bool
*present)
}
+static int
+virXMXGenerateDiskTarget(virDomainDiskDef *def,
s/XMX/VMX/
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
8:19 a.m.
New subject: [PATCH 4/4] vmx: Ensure unique disk targets when parsing
When parsing disks from a vmx file, the target name is generated
based on disk bus, controller the disk is attached to, and its
unit. But in case of SCSI and SATA attached disks this does not
guarantee the target name uniqueness. If there are two disks, one
attached to scsi.0 and the other to sata.0 both end up with the
same "sda" target name. And because of the way their drive
address is derived, they end up with the same address too.
Try harder to generate an unique disk target.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/vmx/vmx.c | 189 +++++++++++++----------
tests/vmx2xmldata/esx-in-the-wild-12.xml | 4 +-
tests/vmx2xmldata/esx-in-the-wild-8.xml | 4 +-
3 files changed, 109 insertions(+), 88 deletions(-)
diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
index 399f03b419..7c752c72f9 100644
--- a/src/vmx/vmx.c
+++ b/src/vmx/vmx.c
@@ -2142,105 +2142,126 @@ virXMXGenerateDiskTarget(virDomainDiskDef *def,
int controllerOrBus,
int unit)
{
- const char *prefix = NULL;
- unsigned int idx = 0;
-
- switch (def->bus) {
- case VIR_DOMAIN_DISK_BUS_SCSI:
- if (controllerOrBus < 0 || controllerOrBus > 3) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("SCSI controller index %1$d out of [0..3]
range"),
- controllerOrBus);
- return -1;
- }
+ unsigned int tries = 0;
- if (unit < 0 || unit > vmdef->scsiBusMaxUnit || unit == 7) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("SCSI unit index %1$d out of [0..6,8..%2$u]
range"),
- unit, vmdef->scsiBusMaxUnit);
- return -1;
- }
+ for (tries = 0; tries < 10; tries++) {
+ g_autofree char *dst = NULL;
+ const char *prefix = NULL;
+ unsigned int idx = 0;
+ size_t i;
- idx = controllerOrBus * 15 + (unit < 7 ? unit : unit - 1);
- prefix = "sd";
- break;
+ switch (def->bus) {
+ case VIR_DOMAIN_DISK_BUS_SCSI:
+ if (controllerOrBus < 0 || controllerOrBus > 3) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("SCSI controller index %1$d out of [0..3]
range"),
+ controllerOrBus);
+ return -1;
+ }
- case VIR_DOMAIN_DISK_BUS_SATA:
- if (controllerOrBus < 0 || controllerOrBus > 3) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("SATA controller index %1$d out of [0..3]
range"),
- controllerOrBus);
- return -1;
- }
+ if (unit < 0 || unit > vmdef->scsiBusMaxUnit || unit == 7) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("SCSI unit index %1$d out of [0..6,8..%2$u]
range"),
+ unit, vmdef->scsiBusMaxUnit);
+ return -1;
+ }
- if (unit < 0 || unit >= 30) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("SATA unit index %1$d out of [0..29] range"),
- unit);
- return -1;
- }
+ idx = controllerOrBus * 15 + (unit < 7 ? unit : unit - 1);
+ prefix = "sd";
+ break;
- idx = controllerOrBus * 30 + unit;
- prefix = "sd";
- break;
+ case VIR_DOMAIN_DISK_BUS_SATA:
+ if (controllerOrBus < 0 || controllerOrBus > 3) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("SATA controller index %1$d out of [0..3]
range"),
+ controllerOrBus);
+ return -1;
+ }
- case VIR_DOMAIN_DISK_BUS_IDE:
- if (controllerOrBus < 0 || controllerOrBus > 1) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("IDE bus index %1$d out of [0..1] range"),
- controllerOrBus);
- return -1;
- }
+ if (unit < 0 || unit >= 30) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("SATA unit index %1$d out of [0..29] range"),
+ unit);
+ return -1;
+ }
+
+ idx = controllerOrBus * 30 + unit;
+ prefix = "sd";
+ break;
- if (unit < 0 || unit > 1) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("IDE unit index %1$d out of [0..1] range"),
unit);
+ case VIR_DOMAIN_DISK_BUS_IDE:
+ if (controllerOrBus < 0 || controllerOrBus > 1) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("IDE bus index %1$d out of [0..1] range"),
+ controllerOrBus);
+ return -1;
+ }
+
+ if (unit < 0 || unit > 1) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("IDE unit index %1$d out of [0..1] range"),
unit);
+ return -1;
+ }
+ idx = controllerOrBus * 2 + unit;
+ prefix = "hd";
+ break;
+
+ case VIR_DOMAIN_DISK_BUS_FDC:
+ if (controllerOrBus != 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("FDC controller index %1$d out of [0]
range"),
+ controllerOrBus);
+ return -1;
+ }
+
+ if (unit < 0 || unit > 1) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("FDC unit index %1$d out of [0..1] range"),
+ unit);
+ return -1;
+ }
+
+ idx = unit;
+ prefix = "fd";
+ break;
+
+ case VIR_DOMAIN_DISK_BUS_VIRTIO:
+ case VIR_DOMAIN_DISK_BUS_XEN:
+ case VIR_DOMAIN_DISK_BUS_USB:
+ case VIR_DOMAIN_DISK_BUS_UML:
+ case VIR_DOMAIN_DISK_BUS_SD:
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Unsupported bus type '%1$s' for device type
'%2$s'"),
+ virDomainDiskBusTypeToString(def->bus),
+ virDomainDiskDeviceTypeToString(def->device));
return -1;
- }
- idx = controllerOrBus * 2 + unit;
- prefix = "hd";
- break;
-
- case VIR_DOMAIN_DISK_BUS_FDC:
- if (controllerOrBus != 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("FDC controller index %1$d out of [0] range"),
- controllerOrBus);
+ break;
+
+ case VIR_DOMAIN_DISK_BUS_NONE:
+ case VIR_DOMAIN_DISK_BUS_LAST:
+ virReportEnumRangeError(virDomainDiskBus, def->bus);
return -1;
+ break;
}
- if (unit < 0 || unit > 1) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("FDC unit index %1$d out of [0..1] range"),
- unit);
- return -1;
+ /* Now generate target candidate and check for its uniqueness. */
+
+ dst = virIndexToDiskName(idx + tries, prefix);
+
+ for (i = 0; i < vmdef->ndisks; i++) {
+ if (STREQ(vmdef->disks[i]->dst, dst))
+ break;
}
- idx = unit;
- prefix = "fd";
- break;
-
- case VIR_DOMAIN_DISK_BUS_VIRTIO:
- case VIR_DOMAIN_DISK_BUS_XEN:
- case VIR_DOMAIN_DISK_BUS_USB:
- case VIR_DOMAIN_DISK_BUS_UML:
- case VIR_DOMAIN_DISK_BUS_SD:
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("Unsupported bus type '%1$s' for device type
'%2$s'"),
- virDomainDiskBusTypeToString(def->bus),
- virDomainDiskDeviceTypeToString(def->device));
- return -1;
- break;
-
- case VIR_DOMAIN_DISK_BUS_NONE:
- case VIR_DOMAIN_DISK_BUS_LAST:
- virReportEnumRangeError(virDomainDiskBus, def->bus);
- return -1;
- break;
+ if (i == vmdef->ndisks) {
+ def->dst = g_steal_pointer(&dst);
+ return 0;
+ }
}
- def->dst = virIndexToDiskName(idx, prefix);
- return 0;
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Unable to generate disk target name"));
+ return -1;
}
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 0eea610709..4e3f986e38 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.41.0
8:48 a.m.
New subject: [PATCH 4/4] vmx: Ensure unique disk targets when parsing
On a Friday in 2023, Michal Privoznik wrote:
When parsing disks from a vmx file, the target name is generated
based on disk bus, controller the disk is attached to, and its
unit. But in case of SCSI and SATA attached disks this does not
guarantee the target name uniqueness. If there are two disks, one
attached to scsi.0 and the other to sata.0 both end up with the
same "sda" target name. And because of the way their drive
address is derived, they end up with the same address too.
Try harder to generate an unique disk target.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/vmx/vmx.c | 189 +++++++++++++----------
tests/vmx2xmldata/esx-in-the-wild-12.xml | 4 +-
tests/vmx2xmldata/esx-in-the-wild-8.xml | 4 +-
3 files changed, 109 insertions(+), 88 deletions(-)
diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
index 399f03b419..7c752c72f9 100644
--- a/src/vmx/vmx.c
+++ b/src/vmx/vmx.c
@@ -2142,105 +2142,126 @@ virXMXGenerateDiskTarget(virDomainDiskDef *def,
int controllerOrBus,
int unit)
{
- const char *prefix = NULL;
- unsigned int idx = 0;
-
- switch (def->bus) {
- case VIR_DOMAIN_DISK_BUS_SCSI:
- if (controllerOrBus < 0 || controllerOrBus > 3) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("SCSI controller index %1$d out of [0..3]
range"),
- controllerOrBus);
- return -1;
- }
+ unsigned int tries = 0;
- if (unit < 0 || unit > vmdef->scsiBusMaxUnit || unit == 7) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("SCSI unit index %1$d out of [0..6,8..%2$u]
range"),
- unit, vmdef->scsiBusMaxUnit);
- return -1;
- }
+ for (tries = 0; tries < 10; tries++) {
It seems strange to me to use 'tries' to try to compute something that
is already known.
Wouldn't generating the indexes in two passes work here?
First pass takes cares of only SATA disks, for example,
and the second pass will take care of all the other disks,
using the highest index used for a SATA disk as an offset.
Jano
9:26 a.m.
New subject: [PATCH 4/4] vmx: Ensure unique disk targets when parsing
On 1/25/24 15:48, Ján Tomko wrote:
On a Friday in 2023, Michal Privoznik wrote:
> When parsing disks from a vmx file, the target name is generated
> based on disk bus, controller the disk is attached to, and its
> unit. But in case of SCSI and SATA attached disks this does not
> guarantee the target name uniqueness. If there are two disks, one
> attached to scsi.0 and the other to sata.0 both end up with the
> same "sda" target name. And because of the way their drive
> address is derived, they end up with the same address too.
>
> Try harder to generate an unique disk target.
>
> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> ---
> src/vmx/vmx.c | 189 +++++++++++++----------
> tests/vmx2xmldata/esx-in-the-wild-12.xml | 4 +-
> tests/vmx2xmldata/esx-in-the-wild-8.xml | 4 +-
> 3 files changed, 109 insertions(+), 88 deletions(-)
>
> diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
> index 399f03b419..7c752c72f9 100644
> --- a/src/vmx/vmx.c
> +++ b/src/vmx/vmx.c
> @@ -2142,105 +2142,126 @@ virXMXGenerateDiskTarget(virDomainDiskDef *def,
> int controllerOrBus,
> int unit)
> {
> - const char *prefix = NULL;
> - unsigned int idx = 0;
> -
> - switch (def->bus) {
> - case VIR_DOMAIN_DISK_BUS_SCSI:
> - if (controllerOrBus < 0 || controllerOrBus > 3) {
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("SCSI controller index %1$d out of
> [0..3] range"),
> - controllerOrBus);
> - return -1;
> - }
> + unsigned int tries = 0;
>
> - if (unit < 0 || unit > vmdef->scsiBusMaxUnit || unit == 7) {
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("SCSI unit index %1$d out of
> [0..6,8..%2$u] range"),
> - unit, vmdef->scsiBusMaxUnit);
> - return -1;
> - }
> + for (tries = 0; tries < 10; tries++) {
It seems strange to me to use 'tries' to try to compute something that
is already known.
Wouldn't generating the indexes in two passes work here?
First pass takes cares of only SATA disks, for example,
and the second pass will take care of all the other disks,
using the highest index used for a SATA disk as an offset.
That might work. Let me see if I can write such patch.
Michal
332
days inactive
373
days old
9 comments
3 participants
participants (3)
-
Ján Tomko -
Michal Privoznik -
Michal Prívozník