[PATCH 0/9] esx: Allow disk images in subdirectories
*** BLURB HERE *** Michal Prívozník (9): libvirt_esx.syms: Put proper header file name into comment esx: Expose esxParseVMXFileName() for tests esx: Make esxVI_LookupDatastoreList() mockable esx: Make esxVI_LookupDatastoreHostMount() mockable esx: Make esxVI_LookupDatastoreByName() mockable tests: Introduce vmx2xmlmock vmx2xmltest: Drop custom file name parse function esx: Allow disk images in subdirectories vmx2xmltest: Add a test case for disks in subfolder src/esx/esx_driver.c | 24 +++-- src/esx/esx_driverpriv.h | 27 ++++++ src/esx/esx_vi.h | 6 +- src/libvirt_esx.syms | 23 +++++ tests/meson.build | 5 +- tests/vmx2xmldata/esx-in-the-wild-17.vmx | 106 +++++++++++++++++++++++ tests/vmx2xmldata/esx-in-the-wild-17.xml | 46 ++++++++++ tests/vmx2xmlmock.c | 79 +++++++++++++++++ tests/vmx2xmltest.c | 62 +++---------- 9 files changed, 308 insertions(+), 70 deletions(-) create mode 100644 src/esx/esx_driverpriv.h create mode 100644 tests/vmx2xmldata/esx-in-the-wild-17.vmx create mode 100644 tests/vmx2xmldata/esx-in-the-wild-17.xml create mode 100644 tests/vmx2xmlmock.c -- 2.51.0
From: Michal Privoznik <mprivozn@redhat.com> The esxVI_DateTime_ConvertToCalendarTime() symbol is declared in esx_vi_types.h header file. Reflect this in the corresponding .syms file. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_esx.syms | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/libvirt_esx.syms b/src/libvirt_esx.syms index 3c14b94aeb..023fecbe94 100644 --- a/src/libvirt_esx.syms +++ b/src/libvirt_esx.syms @@ -5,6 +5,9 @@ # esx/esx_util.h esxUtil_EscapeDatastoreItem; esxUtil_ParseDatastorePath; + + +# esx/esx_vi_types.h esxVI_DateTime_ConvertToCalendarTime; # Let emacs know we want case-insensitive sorting -- 2.51.0
From: Michal Privoznik <mprivozn@redhat.com> So far, our vmx2xmltest uses a custom .parseFileName callback. And it kind of makes sense because the one that's used in production (esxParseVMXFileName()) does some HTTP requests which we don't want to do in our test suite. But this creates other sorts of problems and the idea is to have the test ditch custom parse callback and stick with the production one. But for now, just expose it. With it, the esxVMX_Data struct is exposed too as it is passed into the function (via 'opaque' argument). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/esx/esx_driver.c | 13 +++---------- src/esx/esx_driverpriv.h | 27 +++++++++++++++++++++++++++ src/libvirt_esx.syms | 4 ++++ 3 files changed, 34 insertions(+), 10 deletions(-) create mode 100644 src/esx/esx_driverpriv.h diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index 34c0e28d31..6452a33b7c 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -33,11 +33,12 @@ #include "vmx.h" #include "virtypedparam.h" #include "esx_driver.h" +#define LIBVIRT_ESX_DRIVERPRIV_H_ALLOW +#include "esx_driverpriv.h" #include "esx_interface_driver.h" #include "esx_network_driver.h" #include "esx_storage_driver.h" #include "esx_private.h" -#include "esx_vi.h" #include "esx_vi_methods.h" #include "esx_util.h" #include "esx_stream.h" @@ -50,14 +51,6 @@ VIR_LOG_INIT("esx.esx_driver"); static int esxDomainGetMaxVcpus(virDomainPtr domain); -typedef struct _esxVMX_Data esxVMX_Data; - -struct _esxVMX_Data { - esxVI_Context *ctx; - char *datastorePathWithoutFileName; -}; - - static void esxFreePrivate(esxPrivate **priv) @@ -124,7 +117,7 @@ esxFreePrivate(esxPrivate **priv) * exception and need special handling. Parse the datastore name and use it * to lookup the datastore by name to verify that it exists. */ -static int +int esxParseVMXFileName(const char *fileName, void *opaque, char **out, diff --git a/src/esx/esx_driverpriv.h b/src/esx/esx_driverpriv.h new file mode 100644 index 0000000000..c947866d4e --- /dev/null +++ b/src/esx/esx_driverpriv.h @@ -0,0 +1,27 @@ +/* + * esx_driverpriv.h: private declarations for ESX driver + * + * SPDX-License-Identifier: LGPL-2.1-or-later + */ + +#ifndef LIBVIRT_ESX_DRIVERPRIV_H_ALLOW +# error "esx_driverpriv.h may only be included by esx_driver.c or test suites" +#endif /* LIBVIRT_ESX_DRIVERPRIV_H_ALLOW */ + +#pragma once + +#include "esx_vi.h" + +typedef struct _esxVMX_Data esxVMX_Data; + +struct _esxVMX_Data { + esxVI_Context *ctx; + char *datastorePathWithoutFileName; +}; + + +int +esxParseVMXFileName(const char *fileName, + void *opaque, + char **out, + bool allow_missing); diff --git a/src/libvirt_esx.syms b/src/libvirt_esx.syms index 023fecbe94..6a61b7019c 100644 --- a/src/libvirt_esx.syms +++ b/src/libvirt_esx.syms @@ -2,6 +2,10 @@ # These symbols are dependent upon --with-esx via WITH_ESX # +# esx/esx_driverpriv.h +esxParseVMXFileName; + + # esx/esx_util.h esxUtil_EscapeDatastoreItem; esxUtil_ParseDatastorePath; -- 2.51.0
From: Michal Privoznik <mprivozn@redhat.com> This function is going to be mocked soon. Annotate and export it. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/esx/esx_vi.h | 2 +- src/libvirt_esx.syms | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/esx/esx_vi.h b/src/esx/esx_vi.h index b5eeaa750e..854f3fc61a 100644 --- a/src/esx/esx_vi.h +++ b/src/esx/esx_vi.h @@ -397,7 +397,7 @@ int esxVI_LookupVirtualMachineByUuidAndPrepareForTask bool autoAnswer); int esxVI_LookupDatastoreList(esxVI_Context *ctx, esxVI_String *propertyNameList, - esxVI_ObjectContent **datastoreList); + esxVI_ObjectContent **datastoreList) ATTRIBUTE_MOCKABLE; int esxVI_LookupDatastoreByName(esxVI_Context *ctx, const char *name, esxVI_String *propertyNameList, diff --git a/src/libvirt_esx.syms b/src/libvirt_esx.syms index 6a61b7019c..30883ab629 100644 --- a/src/libvirt_esx.syms +++ b/src/libvirt_esx.syms @@ -11,6 +11,10 @@ esxUtil_EscapeDatastoreItem; esxUtil_ParseDatastorePath; +# esx/esx_vi.h +esxVI_LookupDatastoreList; + + # esx/esx_vi_types.h esxVI_DateTime_ConvertToCalendarTime; -- 2.51.0
From: Michal Privoznik <mprivozn@redhat.com> This function is going to be mocked soon. Annotate and export it. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/esx/esx_vi.h | 2 +- src/libvirt_esx.syms | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/esx/esx_vi.h b/src/esx/esx_vi.h index 854f3fc61a..a4ec0ab48c 100644 --- a/src/esx/esx_vi.h +++ b/src/esx/esx_vi.h @@ -413,7 +413,7 @@ int esxVI_LookupDatastoreByAbsolutePath(esxVI_Context *ctx, int esxVI_LookupDatastoreHostMount(esxVI_Context *ctx, esxVI_ManagedObjectReference *datastore, esxVI_DatastoreHostMount **hostMount, - esxVI_Occurrence occurrence); + esxVI_Occurrence occurrence) ATTRIBUTE_MOCKABLE; int esxVI_LookupTaskInfoByTask(esxVI_Context *ctx, esxVI_ManagedObjectReference *task, diff --git a/src/libvirt_esx.syms b/src/libvirt_esx.syms index 30883ab629..503a995aa5 100644 --- a/src/libvirt_esx.syms +++ b/src/libvirt_esx.syms @@ -12,6 +12,7 @@ esxUtil_ParseDatastorePath; # esx/esx_vi.h +esxVI_LookupDatastoreHostMount; esxVI_LookupDatastoreList; -- 2.51.0
From: Michal Privoznik <mprivozn@redhat.com> This function is going to be mocked soon. Annotate and export it. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/esx/esx_vi.h | 2 +- src/libvirt_esx.syms | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/esx/esx_vi.h b/src/esx/esx_vi.h index a4ec0ab48c..b083ef2b0c 100644 --- a/src/esx/esx_vi.h +++ b/src/esx/esx_vi.h @@ -402,7 +402,7 @@ int esxVI_LookupDatastoreList(esxVI_Context *ctx, esxVI_String *propertyNameList int esxVI_LookupDatastoreByName(esxVI_Context *ctx, const char *name, esxVI_String *propertyNameList, esxVI_ObjectContent **datastore, - esxVI_Occurrence occurrence); + esxVI_Occurrence occurrence) ATTRIBUTE_MOCKABLE; int esxVI_LookupDatastoreByAbsolutePath(esxVI_Context *ctx, const char *absolutePath, diff --git a/src/libvirt_esx.syms b/src/libvirt_esx.syms index 503a995aa5..d05684fd97 100644 --- a/src/libvirt_esx.syms +++ b/src/libvirt_esx.syms @@ -12,6 +12,7 @@ esxUtil_ParseDatastorePath; # esx/esx_vi.h +esxVI_LookupDatastoreByName; esxVI_LookupDatastoreHostMount; esxVI_LookupDatastoreList; -- 2.51.0
From: Michal Privoznik <mprivozn@redhat.com> If we want vmx2xmltest to use actual file name parser that's used in production (esxParseVMXFileName()) we need a mock to stop it from doing any HTTP requests and also to return predictable data. So far, the function can call three functions that do HTTP requests: esxVI_LookupDatastoreList(), esxVI_LookupDatastoreHostMount() and esxVI_LookupDatastoreByName(). Mock all three of them. And since their implementation uses some other symbols (like allocators or _AppendToList() helpers) we need to expose these symbols too. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_esx.syms | 10 ++++++ tests/meson.build | 3 ++ tests/vmx2xmlmock.c | 79 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 92 insertions(+) create mode 100644 tests/vmx2xmlmock.c diff --git a/src/libvirt_esx.syms b/src/libvirt_esx.syms index d05684fd97..d228e2bef7 100644 --- a/src/libvirt_esx.syms +++ b/src/libvirt_esx.syms @@ -18,8 +18,18 @@ esxVI_LookupDatastoreList; # esx/esx_vi_types.h +esxVI_AnyType_Alloc; esxVI_DateTime_ConvertToCalendarTime; + +# esx/esx/esx_vi_types.generated.h +esxVI_DatastoreHostMount_Alloc; +esxVI_DynamicProperty_Alloc; +esxVI_DynamicProperty_AppendToList; +esxVI_HostMountInfo_Alloc; +esxVI_ObjectContent_Alloc; +esxVI_ObjectContent_AppendToList; + # Let emacs know we want case-insensitive sorting # Local Variables: # sort-fold-case: t diff --git a/tests/meson.build b/tests/meson.build index 0d76d37959..9adf172b7f 100644 --- a/tests/meson.build +++ b/tests/meson.build @@ -553,6 +553,9 @@ if conf.has('WITH_VMX') { 'name': 'vmx2xmltest' }, { 'name': 'xml2vmxtest' }, ] + mock_libs += [ + { 'name': 'vmx2xmlmock', 'deps': [ esx_dep ] }, + ] endif if conf.has('WITH_JSON') diff --git a/tests/vmx2xmlmock.c b/tests/vmx2xmlmock.c new file mode 100644 index 0000000000..ce4a9a426b --- /dev/null +++ b/tests/vmx2xmlmock.c @@ -0,0 +1,79 @@ +#include <config.h> + +#include "internal.h" +#include "esx_vi.h" + +int +esxVI_LookupDatastoreList(esxVI_Context *ctx G_GNUC_UNUSED, + esxVI_String *propertyNameList, + esxVI_ObjectContent **datastoreList) +{ + esxVI_String *tmp; + + for (tmp = propertyNameList; tmp; tmp = tmp->_next) { + esxVI_ObjectContent *obj = NULL; + + if (STREQ(tmp->value, "summary.name")) { + esxVI_DynamicProperty *prop = NULL; + + esxVI_ObjectContent_Alloc(&obj); + + esxVI_DynamicProperty_Alloc(&prop); + prop->name = g_strdup("summary.name"); + + esxVI_AnyType_Alloc(&prop->val); + prop->val->type = esxVI_Type_String; + prop->val->other = g_strdup("xsd:string"); + prop->val->value = g_strdup("datastore"); + prop->val->string = prop->val->value; + esxVI_DynamicProperty_AppendToList(&obj->propSet, prop); + } + + if (obj) { + esxVI_ObjectContent_AppendToList(datastoreList, obj); + } + } + + return 0; +} + + +int +esxVI_LookupDatastoreHostMount(esxVI_Context *ctx G_GNUC_UNUSED, + esxVI_ManagedObjectReference *datastore G_GNUC_UNUSED, + esxVI_DatastoreHostMount **hostMount, + esxVI_Occurrence occurrence G_GNUC_UNUSED) +{ + esxVI_DatastoreHostMount *hm = NULL; + + esxVI_DatastoreHostMount_Alloc(&hm); + esxVI_HostMountInfo_Alloc(&hm->mountInfo); + hm->mountInfo->path = g_strdup("/non/existent"); + hm->mountInfo->accessMode = g_strdup("readWrite"); + hm->mountInfo->accessible = esxVI_Boolean_True; + + *hostMount = hm; + return 0; +} + + +int +esxVI_LookupDatastoreByName(esxVI_Context *ctx G_GNUC_UNUSED, + const char *name, + esxVI_String *propertyNameList G_GNUC_UNUSED, + esxVI_ObjectContent **datastore, + esxVI_Occurrence occurrence G_GNUC_UNUSED) +{ + esxVI_ObjectContent *obj = NULL; + + if (STREQ(name, "missing") || STREQ(name, "ds")) { + *datastore = NULL; + return 0; + } + + /* No need to return anything useful, empty object is fine. */ + esxVI_ObjectContent_Alloc(&obj); + esxVI_ObjectContent_AppendToList(datastore, obj); + + return 0; +} -- 2.51.0
On Thu, Nov 20, 2025 at 09:32:51AM +0100, Michal Privoznik via Devel wrote:
From: Michal Privoznik <mprivozn@redhat.com>
If we want vmx2xmltest to use actual file name parser that's used in production (esxParseVMXFileName()) we need a mock to stop it from doing any HTTP requests and also to return predictable data.
So far, the function can call three functions that do HTTP requests: esxVI_LookupDatastoreList(), esxVI_LookupDatastoreHostMount() and esxVI_LookupDatastoreByName().
Mock all three of them. And since their implementation uses some other symbols (like allocators or _AppendToList() helpers) we need to expose these symbols too.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_esx.syms | 10 ++++++ tests/meson.build | 3 ++ tests/vmx2xmlmock.c | 79 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 92 insertions(+) create mode 100644 tests/vmx2xmlmock.c
diff --git a/tests/vmx2xmlmock.c b/tests/vmx2xmlmock.c new file mode 100644 index 0000000000..ce4a9a426b --- /dev/null +++ b/tests/vmx2xmlmock.c @@ -0,0 +1,79 @@ +#include <config.h> + +#include "internal.h" +#include "esx_vi.h" + +int +esxVI_LookupDatastoreList(esxVI_Context *ctx G_GNUC_UNUSED, + esxVI_String *propertyNameList, + esxVI_ObjectContent **datastoreList) +{ + esxVI_String *tmp; + + for (tmp = propertyNameList; tmp; tmp = tmp->_next) { + esxVI_ObjectContent *obj = NULL; + + if (STREQ(tmp->value, "summary.name")) { + esxVI_DynamicProperty *prop = NULL; + + esxVI_ObjectContent_Alloc(&obj); + + esxVI_DynamicProperty_Alloc(&prop); + prop->name = g_strdup("summary.name"); + + esxVI_AnyType_Alloc(&prop->val); + prop->val->type = esxVI_Type_String; + prop->val->other = g_strdup("xsd:string"); + prop->val->value = g_strdup("datastore"); + prop->val->string = prop->val->value; + esxVI_DynamicProperty_AppendToList(&obj->propSet, prop); + } + + if (obj) { + esxVI_ObjectContent_AppendToList(datastoreList, obj);
This line, along with the declaration of @obj can be inside the previous body under the condition with STREQ, no need to separate it. Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
From: Michal Privoznik <mprivozn@redhat.com> Having a custom file name parsing function in vmx2xml that's different to the one used in production (esxParseVMXFileName()) might have served us well, but it also defeats the point of having a unit test. More specifically, if there's a bug in esxParseVMXFileName() then our unit test would not catch it. But now that we have vmx2xmlmock the custom parsing function can be dropped and the test can use the real one. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/meson.build | 2 +- tests/vmx2xmltest.c | 61 +++++++-------------------------------------- 2 files changed, 10 insertions(+), 53 deletions(-) diff --git a/tests/meson.build b/tests/meson.build index 9adf172b7f..cd53e48aa4 100644 --- a/tests/meson.build +++ b/tests/meson.build @@ -550,7 +550,7 @@ endif if conf.has('WITH_VMX') tests += [ - { 'name': 'vmx2xmltest' }, + { 'name': 'vmx2xmltest', 'include': [ esx_inc_dir ], 'link_with': [ esx_lib ] }, { 'name': 'xml2vmxtest' }, ] mock_libs += [ diff --git a/tests/vmx2xmltest.c b/tests/vmx2xmltest.c index bcd95ed87d..cb8e04af0d 100644 --- a/tests/vmx2xmltest.c +++ b/tests/vmx2xmltest.c @@ -8,6 +8,8 @@ # include "internal.h" # include "vmx/vmx.h" +# define LIBVIRT_ESX_DRIVERPRIV_H_ALLOW +# include "esx_driverpriv.h" # define VIR_FROM_THIS VIR_FROM_VMWARE @@ -104,60 +106,12 @@ testCompareHelper(const void *data) return ret; } -static int -testParseVMXFileName(const char *fileName, - void *opaque G_GNUC_UNUSED, - char **src, - bool allow_missing) -{ - g_autofree char *copyOfFileName = NULL; - char *tmp = NULL; - char *saveptr = NULL; - char *datastoreName = NULL; - char *directoryAndFileName = NULL; - - *src = NULL; - - if (STRPREFIX(fileName, "/vmfs/volumes/")) { - /* Found absolute path referencing a file inside a datastore */ - copyOfFileName = g_strdup(fileName); - - /* Expected format: '/vmfs/volumes/<datastore>/<path>' */ - if ((tmp = STRSKIP(copyOfFileName, "/vmfs/volumes/")) == NULL || - (datastoreName = strtok_r(tmp, "/", &saveptr)) == NULL || - (directoryAndFileName = strtok_r(NULL, "", &saveptr)) == NULL) { - return -1; - } - - if (STREQ(datastoreName, "missing") || - STRPREFIX(directoryAndFileName, "missing")) { - if (allow_missing) - return 0; - - virReportError(VIR_ERR_INTERNAL_ERROR, - "Referenced missing file '%s'", fileName); - return -1; - } - - *src = g_strdup_printf("[%s] %s", datastoreName, directoryAndFileName); - } else if (STRPREFIX(fileName, "/")) { - /* Found absolute path referencing a file outside a datastore */ - *src = g_strdup(fileName); - } else if (strchr(fileName, '/') != NULL) { - /* Found relative path, this is not supported */ - return -1; - } else { - /* Found single file name referencing a file inside a datastore */ - *src = g_strdup_printf("[datastore] directory/%s", fileName); - } - - return 0; -} static int mymain(void) { int ret = 0; + esxVMX_Data data = { 0 }; # define DO_TEST_FULL(file, should_fail) \ do { \ @@ -180,8 +134,10 @@ mymain(void) if (!(xmlopt = virVMXDomainXMLConfInit(caps))) return EXIT_FAILURE; - ctx.opaque = NULL; - ctx.parseFileName = testParseVMXFileName; + data.datastorePathWithoutFileName = (char*) "[datastore] directory"; + + ctx.opaque = &data; + ctx.parseFileName = esxParseVMXFileName; ctx.formatFileName = NULL; ctx.autodetectSCSIControllerModel = NULL; ctx.datacenterPath = NULL; @@ -296,7 +252,8 @@ mymain(void) return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -VIR_TEST_MAIN(mymain) +VIR_TEST_MAIN_PRELOAD(mymain, + VIR_TEST_MOCK("vmx2xml")) #else -- 2.51.0
From: Michal Privoznik <mprivozn@redhat.com> The esxParseVMXFileName() function parses path to a disk image trying to replace some "known" patterns (e.g. datastore paths). A simple filename is treated as a path relative to .vmx file. But disk images (and thus filenames) can be in a subdirectory, relative to the .vmx file. For instance: subfolder/disk.vmdk Adapt our parser to this fact. Resolves: https://issues.redhat.com/browse/RHEL-122751 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/esx/esx_driver.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index 6452a33b7c..9f965811b1 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -72,9 +72,11 @@ esxFreePrivate(esxPrivate **priv) * Parse a file name from a .vmx file and convert it to datastore path format * if possible. A .vmx file can contain file names in various formats: * - * - A single name referencing a file in the same directory as the .vmx file: + * - A single name referencing a file in the same directory as the .vmx file, + * or in a subdirectory: * * test1.vmdk + * subdir/test2.vmdk * * - An absolute file name referencing a file in a datastore that is mounted at * /vmfs/volumes/<datastore>: @@ -106,8 +108,9 @@ esxFreePrivate(esxPrivate **priv) * * Firstly this functions checks if the given file name contains a separator. * If it doesn't then the referenced file is in the same directory as the .vmx - * file. The datastore name and directory of the .vmx file are passed to this - * function via the opaque parameter by the caller of virVMXParseConfig. + * file, or in a subdirectory. The datastore name and directory of the .vmx + * file are passed to this function via the opaque parameter by the caller of + * virVMXParseConfig. * * Otherwise query for all known datastores and their mount directories. Then * try to find a datastore with a mount directory that is a prefix to the given @@ -138,7 +141,7 @@ esxParseVMXFileName(const char *fileName, *out = NULL; - if (!strchr(fileName, '/') && !strchr(fileName, '\\')) { + if (*fileName != '/' && !strchr(fileName, '\\')) { /* Plain file name, use same directory as for the .vmx file */ *out = g_strdup_printf("%s/%s", data->datastorePathWithoutFileName, fileName); -- 2.51.0
On Thu, Nov 20, 2025 at 09:32:53AM +0100, Michal Privoznik via Devel wrote:
From: Michal Privoznik <mprivozn@redhat.com>
The esxParseVMXFileName() function parses path to a disk image trying to replace some "known" patterns (e.g. datastore paths). A simple filename is treated as a path relative to .vmx file. But disk images (and thus filenames) can be in a subdirectory, relative to the .vmx file. For instance:
subfolder/disk.vmdk
Adapt our parser to this fact.
Resolves: https://issues.redhat.com/browse/RHEL-122751 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/esx/esx_driver.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index 6452a33b7c..9f965811b1 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -138,7 +141,7 @@ esxParseVMXFileName(const char *fileName,
*out = NULL;
- if (!strchr(fileName, '/') && !strchr(fileName, '\\')) { + if (*fileName != '/' && !strchr(fileName, '\\')) { /* Plain file name, use same directory as for the .vmx file */ *out = g_strdup_printf("%s/%s", data->datastorePathWithoutFileName, fileName);
Now when I'm thinking about it, there probably should be some check that it's not referring to too many parent directories? Although once we get it from the VMX it means it's already there... And we probably leave the checking to the server in the other (xml2vmx) case since we happily forward any subdir/../../../../filename.vmdk there. So I guess it's fine. Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
From: Michal Privoznik <mprivozn@redhat.com> This test case demonstrates correctness of the previous fix. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/vmx2xmldata/esx-in-the-wild-17.vmx | 106 +++++++++++++++++++++++ tests/vmx2xmldata/esx-in-the-wild-17.xml | 46 ++++++++++ tests/vmx2xmltest.c | 1 + 3 files changed, 153 insertions(+) create mode 100644 tests/vmx2xmldata/esx-in-the-wild-17.vmx create mode 100644 tests/vmx2xmldata/esx-in-the-wild-17.xml diff --git a/tests/vmx2xmldata/esx-in-the-wild-17.vmx b/tests/vmx2xmldata/esx-in-the-wild-17.vmx new file mode 100644 index 0000000000..ebbf76b449 --- /dev/null +++ b/tests/vmx2xmldata/esx-in-the-wild-17.vmx @@ -0,0 +1,106 @@ +.encoding = "UTF-8" +config.version = "8" +virtualHW.version = "20" +vmci0.present = "TRUE" +floppy0.present = "FALSE" +svga.vramSize = "8388608" +numvcpus = "2" +memSize = "4096" +tools.upgrade.policy = "manual" +sched.cpu.units = "mhz" +vm.createDate = "1760939758649227" +usb_xhci.present = "TRUE" +scsi0.virtualDev = "lsisas1068" +scsi0.present = "TRUE" +sata0.present = "TRUE" +sata0:0.deviceType = "cdrom-image" +sata0:0.fileName = "/vmfs/volumes/f84f070c-0d65498d/win-iso/windows_11_x64_official_dvd.iso" +sata0:0.present = "TRUE" +scsi0:0.deviceType = "scsi-hardDisk" +scsi0:0.fileName = "esx8.0-win11-with-second-disk-in-subfolder.vmdk" +sched.scsi0:0.shares = "normal" +sched.scsi0:0.throughputCap = "off" +scsi0:0.present = "TRUE" +ethernet0.allowGuestConnectionControl = "FALSE" +ethernet0.virtualDev = "e1000e" +ethernet0.networkName = "Mgmt Network" +ethernet0.addressType = "vpx" +ethernet0.generatedAddress = "00:50:56:a5:b8:68" +ethernet0.present = "TRUE" +displayName = "esx8.0-win11-with-second-disk-in-subfolder" +guestOS = "windows2019srvNext-64" +chipset.motherboardLayout = "i440bx" +toolScripts.afterPowerOn = "TRUE" +toolScripts.afterResume = "TRUE" +toolScripts.beforeSuspend = "TRUE" +toolScripts.beforePowerOff = "TRUE" +tools.syncTime = "FALSE" +tools.guest.desktop.autolock = "TRUE" +uuid.bios = "42 25 6e c1 e0 66 93 64-3d d1 36 a0 b7 52 63 dd" +vc.uuid = "50 25 0f 27 53 8b 50 91-a4 3b a5 0c e9 5f 23 82" +nvram = "esx8.0-win11-with-second-disk-in-subfolder.nvram" +svga.present = "TRUE" +pciBridge0.present = "TRUE" +pciBridge4.present = "TRUE" +pciBridge4.virtualDev = "pcieRootPort" +pciBridge4.functions = "8" +pciBridge5.present = "TRUE" +pciBridge5.virtualDev = "pcieRootPort" +pciBridge5.functions = "8" +pciBridge6.present = "TRUE" +pciBridge6.virtualDev = "pcieRootPort" +pciBridge6.functions = "8" +pciBridge7.present = "TRUE" +pciBridge7.virtualDev = "pcieRootPort" +pciBridge7.functions = "8" +hpet0.present = "TRUE" +RemoteDisplay.maxConnections = "-1" +sched.cpu.latencySensitivity = "normal" +disk.EnableUUID = "TRUE" +vmxstats.filename = "esx8.0-win11-x86_64-latest.scoreboard" +numa.autosize.cookie = "20012" +numa.autosize.vcpu.maxPerVirtualNode = "2" +cpuid.coresPerSocket.cookie = "2" +vm.genidX = "2627911129287880091" +pciBridge0.pciSlotNumber = "17" +pciBridge4.pciSlotNumber = "21" +pciBridge5.pciSlotNumber = "22" +pciBridge6.pciSlotNumber = "23" +pciBridge7.pciSlotNumber = "24" +scsi0.pciSlotNumber = "160" +ethernet0.pciSlotNumber = "192" +usb_xhci.pciSlotNumber = "224" +sata0.pciSlotNumber = "32" +scsi0.sasWWID = "50 05 05 61 e0 66 93 60" +vmotion.checkpointFBSize = "8388608" +vmotion.checkpointSVGAPrimarySize = "8388608" +vmotion.svga.mobMaxSize = "8388608" +vmotion.svga.graphicsMemoryKB = "8192" +monitor.phys_bits_used = "45" +softPowerOff = "TRUE" +tools.remindInstall = "TRUE" +migrate.hostLog = "esx8.0-win11-with-second-disk-in-subfolder-1b240d0d.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 = "52300e6b-6a64-4109-b144-f3efe9426650:vm-545:2azK3dLANCIEKysfZbstuwb/2iSvArcsaQpXPpR56RY=" +sched.swap.derivedName = "/vmfs/volumes/124778e2-48604a5f/esx8.0-win11-with-second-disk-in-subfolder/esx8.0-win11-with-second-disk-in-subfolder-5474484f.vswp" +uuid.location = "56 4d 73 7b a7 00 67 05-e5 0a b7 00 56 d4 1e 54" +vm.genid = "3571897048718482545" +scsi0:0.redo = "" +scsi0:1.deviceType = "scsi-hardDisk" +scsi0:1.fileName = "subfolder/esx8.0-win11-with-second-disk-in-subfolder_1.vmdk" +sched.scsi0:1.shares = "normal" +sched.scsi0:1.throughputCap = "off" +scsi0:1.present = "TRUE" +scsi0:1.redo = "" +vmci0.id = "-1219337251" +cleanShutdown = "TRUE" +usb_xhci:4.present = "TRUE" +usb_xhci:4.deviceType = "hid" +usb_xhci:4.port = "4" +usb_xhci:4.parent = "-1" diff --git a/tests/vmx2xmldata/esx-in-the-wild-17.xml b/tests/vmx2xmldata/esx-in-the-wild-17.xml new file mode 100644 index 0000000000..ae66de7431 --- /dev/null +++ b/tests/vmx2xmldata/esx-in-the-wild-17.xml @@ -0,0 +1,46 @@ +<domain type='vmware'> + <name>esx8.0-win11-with-second-disk-in-subfolder</name> + <uuid>42256ec1-e066-9364-3dd1-36a0b75263dd</uuid> + <genid>3191ed70-eb21-9c71-2478-373fb27fed9b</genid> + <memory unit='KiB'>4194304</memory> + <currentMemory unit='KiB'>4194304</currentMemory> + <vcpu placement='static'>2</vcpu> + <cputune> + <shares>2000</shares> + </cputune> + <os> + <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/esx8.0-win11-with-second-disk-in-subfolder.vmdk'/> + <target dev='sda' bus='scsi'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <disk type='file' device='disk'> + <source file='[datastore] directory/subfolder/esx8.0-win11-with-second-disk-in-subfolder_1.vmdk'/> + <target dev='sdb' bus='scsi'/> + <address type='drive' controller='0' bus='0' target='0' unit='1'/> + </disk> + <disk type='file' device='cdrom'> + <source file='[f84f070c-0d65498d] win-iso/windows_11_x64_official_dvd.iso'/> + <target dev='sda' bus='sata'/> + <readonly/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='scsi' index='0' model='lsisas1068'/> + <controller type='sata' index='0'/> + <interface type='bridge'> + <mac address='00:50:56:a5:b8:68' type='generated'/> + <source bridge='Mgmt Network'/> + <model type='e1000e'/> + </interface> + <video> + <model type='vmvga' vram='8192' primary='yes'/> + </video> + </devices> +</domain> diff --git a/tests/vmx2xmltest.c b/tests/vmx2xmltest.c index cb8e04af0d..fcca765bed 100644 --- a/tests/vmx2xmltest.c +++ b/tests/vmx2xmltest.c @@ -223,6 +223,7 @@ mymain(void) DO_TEST("esx-in-the-wild-14"); DO_TEST("esx-in-the-wild-15"); DO_TEST("esx-in-the-wild-16"); + DO_TEST("esx-in-the-wild-17"); DO_TEST("gsx-in-the-wild-1"); DO_TEST("gsx-in-the-wild-2"); -- 2.51.0
On Thu, Nov 20, 2025 at 09:32:45AM +0100, Michal Privoznik via Devel wrote:
*** BLURB HERE ***
Michal Prívozník (9): libvirt_esx.syms: Put proper header file name into comment esx: Expose esxParseVMXFileName() for tests esx: Make esxVI_LookupDatastoreList() mockable esx: Make esxVI_LookupDatastoreHostMount() mockable esx: Make esxVI_LookupDatastoreByName() mockable tests: Introduce vmx2xmlmock vmx2xmltest: Drop custom file name parse function esx: Allow disk images in subdirectories vmx2xmltest: Add a test case for disks in subfolder
And here's a Reviewed-by: Martin Kletzander <mkletzan@redhat.com> for patches 1-5,7,9. I'd suggest squashing 3-5 into one, but that's a nitpick.
FYI, this series seems to be breaking CI Segv on tests "23) VMware VMX-2-XML cdrom-ide-file-missing-datastore" On Thu, Nov 20, 2025 at 02:56:24PM +0100, Martin Kletzander via Devel wrote:
On Thu, Nov 20, 2025 at 09:32:45AM +0100, Michal Privoznik via Devel wrote:
*** BLURB HERE ***
Michal Prívozník (9): libvirt_esx.syms: Put proper header file name into comment esx: Expose esxParseVMXFileName() for tests esx: Make esxVI_LookupDatastoreList() mockable esx: Make esxVI_LookupDatastoreHostMount() mockable esx: Make esxVI_LookupDatastoreByName() mockable tests: Introduce vmx2xmlmock vmx2xmltest: Drop custom file name parse function esx: Allow disk images in subdirectories vmx2xmltest: Add a test case for disks in subfolder
And here's a
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
for patches 1-5,7,9. I'd suggest squashing 3-5 into one, but that's a nitpick.
With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Thu, Nov 20, 2025 at 05:56:04PM +0000, Daniel P. Berrangé via Devel wrote:
FYI, this series seems to be breaking CI
Segv on tests "23) VMware VMX-2-XML cdrom-ide-file-missing-datastore"
There's another breakage in the CI for language bindings tests/meson.build:553:72: ERROR: Unknown variable "esx_lib". eg the java bindings when run with meson build -Ddriver_libvirtd=disabled on the ubuntu 24.04 job With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
participants (3)
-
Daniel P. Berrangé -
Martin Kletzander -
Michal Privoznik