[libvirt PATCH v3 0/8] vmx: Don't error out on missing filename for cdrom
This is perfectly valid in VMWare and the VM just boots with an empty drive. We used to just skip the whole drive before, but since we changed how we parse empty cdrom drives this now results in an error and the user not being able to even dump the XML. Instead of erroring out, just keep the drive empty. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1903953 v3: - Fixed the vmware driver - Bit of a clean-up - Few more tests v2: - Do not report and reset an error, but handle it more nicely. - https://www.redhat.com/archives/libvir-list/2020-December/msg00846.html v1: - https://www.redhat.com/archives/libvir-list/2020-December/msg00840.html Martin Kletzander (8): esx: Unindent unnecessary conditional branch tests: Use g_autofree in testParseVMXFileName vmx: Make virVMXParseFileName return an integer tests: Allow testing for parse failures in vmx2xmltest vmx: Allow missing cdrom image file in virVMXParseFileName tests: Test vmx files with missing images esx: Handle missing images in esxParseVMXFileName vmx: Treat missing cdrom-image as empty drive src/esx/esx_driver.c | 160 ++++++++++-------- src/vmware/vmware_conf.c | 21 ++- src/vmware/vmware_conf.h | 10 +- src/vmware/vmware_driver.c | 6 +- src/vmx/vmx.c | 27 +-- src/vmx/vmx.h | 5 +- ...x2xml-cdrom-ide-file-missing-datastore.vmx | 6 + .../vmx2xml-cdrom-ide-file-missing-file.vmx | 6 + ...ml-harddisk-ide-file-missing-datastore.vmx | 6 + ...mx2xml-harddisk-scsi-file-missing-file.vmx | 7 + tests/vmx2xmltest.c | 67 +++++--- 11 files changed, 203 insertions(+), 118 deletions(-) create mode 100644 tests/vmx2xmldata/vmx2xml-cdrom-ide-file-missing-datastore.vmx create mode 100644 tests/vmx2xmldata/vmx2xml-cdrom-ide-file-missing-file.vmx create mode 100644 tests/vmx2xmldata/vmx2xml-harddisk-ide-file-missing-datastore.vmx create mode 100644 tests/vmx2xmldata/vmx2xml-harddisk-scsi-file-missing-file.vmx -- 2.30.0
The positive branch can just return and the huge negative part does not need to be indented an extra level. Best viewed with `-w`. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/esx/esx_driver.c | 144 +++++++++++++++++++++---------------------- 1 file changed, 72 insertions(+), 72 deletions(-) diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index a17bf58a5124..51c26e29c65e 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -143,100 +143,100 @@ esxParseVMXFileName(const char *fileName, void *opaque) if (!strchr(fileName, '/') && !strchr(fileName, '\\')) { /* Plain file name, use same directory as for the .vmx file */ - result = g_strdup_printf("%s/%s", data->datastorePathWithoutFileName, - fileName); - } else { - if (esxVI_String_AppendValueToList(&propertyNameList, - "summary.name") < 0 || - esxVI_LookupDatastoreList(data->ctx, propertyNameList, - &datastoreList) < 0) { - return NULL; - } + return g_strdup_printf("%s/%s", data->datastorePathWithoutFileName, + fileName); + } - /* Search for datastore by mount path */ - for (datastore = datastoreList; datastore; - datastore = datastore->_next) { - esxVI_DatastoreHostMount_Free(&hostMount); - datastoreName = NULL; - - if (esxVI_LookupDatastoreHostMount(data->ctx, datastore->obj, - &hostMount, - esxVI_Occurrence_RequiredItem) < 0 || - esxVI_GetStringValue(datastore, "summary.name", &datastoreName, - esxVI_Occurrence_RequiredItem) < 0) { - goto cleanup; - } + if (esxVI_String_AppendValueToList(&propertyNameList, + "summary.name") < 0 || + esxVI_LookupDatastoreList(data->ctx, propertyNameList, + &datastoreList) < 0) { + return NULL; + } - tmp = (char *)STRSKIP(fileName, hostMount->mountInfo->path); + /* Search for datastore by mount path */ + for (datastore = datastoreList; datastore; + datastore = datastore->_next) { + esxVI_DatastoreHostMount_Free(&hostMount); + datastoreName = NULL; - if (!tmp) - continue; + if (esxVI_LookupDatastoreHostMount(data->ctx, datastore->obj, + &hostMount, + esxVI_Occurrence_RequiredItem) < 0 || + esxVI_GetStringValue(datastore, "summary.name", &datastoreName, + esxVI_Occurrence_RequiredItem) < 0) { + goto cleanup; + } - /* Found a match. Strip leading separators */ - while (*tmp == '/' || *tmp == '\\') - ++tmp; + tmp = (char *)STRSKIP(fileName, hostMount->mountInfo->path); - strippedFileName = g_strdup(tmp); + if (!tmp) + continue; - tmp = strippedFileName; + /* Found a match. Strip leading separators */ + while (*tmp == '/' || *tmp == '\\') + ++tmp; - /* Convert \ to / */ - while (*tmp != '\0') { - if (*tmp == '\\') - *tmp = '/'; + strippedFileName = g_strdup(tmp); - ++tmp; - } + tmp = strippedFileName; - result = g_strdup_printf("[%s] %s", datastoreName, strippedFileName); + /* Convert \ to / */ + while (*tmp != '\0') { + if (*tmp == '\\') + *tmp = '/'; - break; + ++tmp; } - /* Fallback to direct datastore name match */ - if (!result && STRPREFIX(fileName, "/vmfs/volumes/")) { - copyOfFileName = g_strdup(fileName); - - /* Expected format: '/vmfs/volumes/<datastore>/<path>' */ - if (!(tmp = STRSKIP(copyOfFileName, "/vmfs/volumes/")) || - !(datastoreName = strtok_r(tmp, "/", &saveptr)) || - !(directoryAndFileName = strtok_r(NULL, "", &saveptr))) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("File name '%s' doesn't have expected format " - "'/vmfs/volumes/<datastore>/<path>'"), fileName); - goto cleanup; - } - - esxVI_ObjectContent_Free(&datastoreList); + result = g_strdup_printf("[%s] %s", datastoreName, strippedFileName); - if (esxVI_LookupDatastoreByName(data->ctx, datastoreName, - NULL, &datastoreList, - esxVI_Occurrence_OptionalItem) < 0) { - goto cleanup; - } + break; + } - if (!datastoreList) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("File name '%s' refers to non-existing datastore '%s'"), - fileName, datastoreName); - goto cleanup; - } + /* Fallback to direct datastore name match */ + if (!result && STRPREFIX(fileName, "/vmfs/volumes/")) { + copyOfFileName = g_strdup(fileName); - result = g_strdup_printf("[%s] %s", datastoreName, - directoryAndFileName); + /* Expected format: '/vmfs/volumes/<datastore>/<path>' */ + if (!(tmp = STRSKIP(copyOfFileName, "/vmfs/volumes/")) || + !(datastoreName = strtok_r(tmp, "/", &saveptr)) || + !(directoryAndFileName = strtok_r(NULL, "", &saveptr))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("File name '%s' doesn't have expected format " + "'/vmfs/volumes/<datastore>/<path>'"), fileName); + goto cleanup; } - /* If it's an absolute path outside of a datastore just use it as is */ - if (!result && *fileName == '/') { - /* FIXME: need to deal with Windows paths here too */ - result = g_strdup(fileName); + esxVI_ObjectContent_Free(&datastoreList); + + if (esxVI_LookupDatastoreByName(data->ctx, datastoreName, + NULL, &datastoreList, + esxVI_Occurrence_OptionalItem) < 0) { + goto cleanup; } - if (!result) { + if (!datastoreList) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("Could not handle file name '%s'"), fileName); + _("File name '%s' refers to non-existing datastore '%s'"), + fileName, datastoreName); goto cleanup; } + + result = g_strdup_printf("[%s] %s", datastoreName, + directoryAndFileName); + } + + /* If it's an absolute path outside of a datastore just use it as is */ + if (!result && *fileName == '/') { + /* FIXME: need to deal with Windows paths here too */ + result = g_strdup(fileName); + } + + if (!result) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not handle file name '%s'"), fileName); + goto cleanup; } cleanup: -- 2.30.0
There's only one variable to clean-up, others are just tokens inside that variable, but it is nicer anyway. Positive returns have not been converted because the function will change soon and it would not make much sense. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- tests/vmx2xmltest.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/tests/vmx2xmltest.c b/tests/vmx2xmltest.c index 376116bb750a..8cc227bbedfc 100644 --- a/tests/vmx2xmltest.c +++ b/tests/vmx2xmltest.c @@ -130,7 +130,7 @@ testCompareHelper(const void *data) static char * testParseVMXFileName(const char *fileName, void *opaque G_GNUC_UNUSED) { - char *copyOfFileName = NULL; + g_autofree char *copyOfFileName = NULL; char *tmp = NULL; char *saveptr = NULL; char *datastoreName = NULL; @@ -145,7 +145,7 @@ testParseVMXFileName(const char *fileName, void *opaque G_GNUC_UNUSED) if ((tmp = STRSKIP(copyOfFileName, "/vmfs/volumes/")) == NULL || (datastoreName = strtok_r(tmp, "/", &saveptr)) == NULL || (directoryAndFileName = strtok_r(NULL, "", &saveptr)) == NULL) { - goto cleanup; + return NULL; } src = g_strdup_printf("[%s] %s", datastoreName, directoryAndFileName); @@ -154,15 +154,12 @@ testParseVMXFileName(const char *fileName, void *opaque G_GNUC_UNUSED) src = g_strdup(fileName); } else if (strchr(fileName, '/') != NULL) { /* Found relative path, this is not supported */ - src = NULL; + return NULL; } else { /* Found single file name referencing a file inside a datastore */ src = g_strdup_printf("[datastore] directory/%s", fileName); } - cleanup: - VIR_FREE(copyOfFileName); - return src; } -- 2.30.0
And return the actual extracted value in a parameter. This way we can later return success even without any extracted value. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/esx/esx_driver.c | 31 ++++++++++++++++++------------- src/vmware/vmware_conf.c | 20 ++++++++++++++------ src/vmware/vmware_conf.h | 9 ++++++++- src/vmware/vmware_driver.c | 6 +++--- src/vmx/vmx.c | 25 ++++++++++++------------- src/vmx/vmx.h | 2 +- tests/vmx2xmltest.c | 21 ++++++++++++--------- 7 files changed, 68 insertions(+), 46 deletions(-) diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index 51c26e29c65e..86d5396147a3 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -125,10 +125,11 @@ 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 char * -esxParseVMXFileName(const char *fileName, void *opaque) +static int +esxParseVMXFileName(const char *fileName, + void *opaque, + char **out) { - char *result = NULL; esxVMX_Data *data = opaque; esxVI_String *propertyNameList = NULL; esxVI_ObjectContent *datastoreList = NULL; @@ -140,18 +141,22 @@ esxParseVMXFileName(const char *fileName, void *opaque) char *strippedFileName = NULL; char *copyOfFileName = NULL; char *directoryAndFileName; + int ret = -1; + + *out = NULL; if (!strchr(fileName, '/') && !strchr(fileName, '\\')) { /* Plain file name, use same directory as for the .vmx file */ - return g_strdup_printf("%s/%s", data->datastorePathWithoutFileName, + *out = g_strdup_printf("%s/%s", data->datastorePathWithoutFileName, fileName); + return 0; } if (esxVI_String_AppendValueToList(&propertyNameList, "summary.name") < 0 || esxVI_LookupDatastoreList(data->ctx, propertyNameList, &datastoreList) < 0) { - return NULL; + return -1; } /* Search for datastore by mount path */ @@ -189,13 +194,13 @@ esxParseVMXFileName(const char *fileName, void *opaque) ++tmp; } - result = g_strdup_printf("[%s] %s", datastoreName, strippedFileName); + *out = g_strdup_printf("[%s] %s", datastoreName, strippedFileName); break; } /* Fallback to direct datastore name match */ - if (!result && STRPREFIX(fileName, "/vmfs/volumes/")) { + if (!*out && STRPREFIX(fileName, "/vmfs/volumes/")) { copyOfFileName = g_strdup(fileName); /* Expected format: '/vmfs/volumes/<datastore>/<path>' */ @@ -223,22 +228,22 @@ esxParseVMXFileName(const char *fileName, void *opaque) goto cleanup; } - result = g_strdup_printf("[%s] %s", datastoreName, - directoryAndFileName); + *out = g_strdup_printf("[%s] %s", datastoreName, directoryAndFileName); } /* If it's an absolute path outside of a datastore just use it as is */ - if (!result && *fileName == '/') { + if (!*out && *fileName == '/') { /* FIXME: need to deal with Windows paths here too */ - result = g_strdup(fileName); + *out = g_strdup(fileName); } - if (!result) { + if (!*out) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not handle file name '%s'"), fileName); goto cleanup; } + ret = 0; cleanup: esxVI_String_Free(&propertyNameList); esxVI_ObjectContent_Free(&datastoreList); @@ -246,7 +251,7 @@ esxParseVMXFileName(const char *fileName, void *opaque) VIR_FREE(strippedFileName); VIR_FREE(copyOfFileName); - return result; + return ret; } diff --git a/src/vmware/vmware_conf.c b/src/vmware/vmware_conf.c index e44673247ff1..4f7dc3001d2b 100644 --- a/src/vmware/vmware_conf.c +++ b/src/vmware/vmware_conf.c @@ -139,7 +139,7 @@ vmwareLoadDomains(struct vmware_driver *driver) char *saveptr = NULL; virCommandPtr cmd; - ctx.parseFileName = vmwareCopyVMXFileName; + ctx.parseFileName = vmwareParseVMXFileName; ctx.formatFileName = NULL; ctx.autodetectSCSIControllerModel = NULL; ctx.datacenterPath = NULL; @@ -507,11 +507,19 @@ vmwareExtractPid(const char * vmxPath) return pid_value; } -char * -vmwareCopyVMXFileName(const char *datastorePath, void *opaque G_GNUC_UNUSED) +int +vmwareParseVMXFileName(const char *datastorePath, + void *opaque G_GNUC_UNUSED, + char **out) { - char *path; + *out = g_strdup(datastorePath); + + return *out ? 0 : -1; +} - path = g_strdup(datastorePath); - return path; +char * +vmwareFormatVMXFileName(const char *datastorePath, + void *opaque G_GNUC_UNUSED) +{ + return g_strdup(datastorePath); } diff --git a/src/vmware/vmware_conf.h b/src/vmware/vmware_conf.h index 5e0ef3744f76..6f86983f511f 100644 --- a/src/vmware/vmware_conf.h +++ b/src/vmware/vmware_conf.h @@ -83,4 +83,11 @@ int vmwareMakePath(char *srcDir, char *srcName, char *srcExt, int vmwareExtractPid(const char * vmxPath); -char *vmwareCopyVMXFileName(const char *datastorePath, void *opaque); +int +vmwareParseVMXFileName(const char *datastorePath, + void *opaque, + char **out); + +char * +vmwareFormatVMXFileName(const char *datastorePath, + void *opaque); diff --git a/src/vmware/vmware_driver.c b/src/vmware/vmware_driver.c index 087664a34185..481c59ef3cce 100644 --- a/src/vmware/vmware_driver.c +++ b/src/vmware/vmware_driver.c @@ -409,7 +409,7 @@ vmwareDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int fla parse_flags |= VIR_DOMAIN_DEF_PARSE_VALIDATE_SCHEMA; ctx.parseFileName = NULL; - ctx.formatFileName = vmwareCopyVMXFileName; + ctx.formatFileName = vmwareFormatVMXFileName; ctx.autodetectSCSIControllerModel = NULL; ctx.datacenterPath = NULL; @@ -662,7 +662,7 @@ vmwareDomainCreateXML(virConnectPtr conn, const char *xml, parse_flags |= VIR_DOMAIN_DEF_PARSE_VALIDATE_SCHEMA; ctx.parseFileName = NULL; - ctx.formatFileName = vmwareCopyVMXFileName; + ctx.formatFileName = vmwareFormatVMXFileName; ctx.autodetectSCSIControllerModel = NULL; ctx.datacenterPath = NULL; @@ -950,7 +950,7 @@ vmwareConnectDomainXMLFromNative(virConnectPtr conn, const char *nativeFormat, return NULL; } - ctx.parseFileName = vmwareCopyVMXFileName; + ctx.parseFileName = vmwareParseVMXFileName; ctx.formatFileName = NULL; ctx.autodetectSCSIControllerModel = NULL; ctx.datacenterPath = NULL; diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c index b86dbe9ca267..97591842f789 100644 --- a/src/vmx/vmx.c +++ b/src/vmx/vmx.c @@ -2388,7 +2388,7 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virConfPtr con */ goto ignore; } else if (virStringHasCaseSuffix(fileName, ".vmdk")) { - char *tmp; + char *tmp = NULL; if (deviceType != NULL) { if (busType == VIR_DOMAIN_DISK_BUS_SCSI && @@ -2411,7 +2411,7 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virConfPtr con } virDomainDiskSetType(*def, VIR_STORAGE_TYPE_FILE); - if (!(tmp = ctx->parseFileName(fileName, ctx->opaque))) + if (ctx->parseFileName(fileName, ctx->opaque, &tmp) < 0) goto cleanup; virDomainDiskSetSource(*def, tmp); VIR_FREE(tmp); @@ -2438,7 +2438,7 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virConfPtr con */ goto ignore; } else if (fileName && virStringHasCaseSuffix(fileName, ".iso")) { - char *tmp; + char *tmp = NULL; if (deviceType && STRCASENEQ(deviceType, "cdrom-image")) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -2448,7 +2448,7 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virConfPtr con } virDomainDiskSetType(*def, VIR_STORAGE_TYPE_FILE); - if (!(tmp = ctx->parseFileName(fileName, ctx->opaque))) + if (ctx->parseFileName(fileName, ctx->opaque, &tmp) < 0) goto cleanup; virDomainDiskSetSource(*def, tmp); VIR_FREE(tmp); @@ -2514,7 +2514,8 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virConfPtr con char *tmp = NULL; virDomainDiskSetType(*def, VIR_STORAGE_TYPE_FILE); - if (fileName && !(tmp = ctx->parseFileName(fileName, ctx->opaque))) + if (fileName && + ctx->parseFileName(fileName, ctx->opaque, &tmp) < 0) goto cleanup; virDomainDiskSetSource(*def, tmp); VIR_FREE(tmp); @@ -2974,10 +2975,9 @@ virVMXParseSerial(virVMXContext *ctx, virConfPtr conf, int port, } else if (STRCASEEQ(fileType, "file")) { (*def)->target.port = port; (*def)->source->type = VIR_DOMAIN_CHR_TYPE_FILE; - (*def)->source->data.file.path = ctx->parseFileName(fileName, - ctx->opaque); - - if ((*def)->source->data.file.path == NULL) + if (ctx->parseFileName(fileName, + ctx->opaque, + &(*def)->source->data.file.path) < 0) goto cleanup; } else if (STRCASEEQ(fileType, "pipe")) { /* @@ -3140,10 +3140,9 @@ virVMXParseParallel(virVMXContext *ctx, virConfPtr conf, int port, } else if (STRCASEEQ(fileType, "file")) { (*def)->target.port = port; (*def)->source->type = VIR_DOMAIN_CHR_TYPE_FILE; - (*def)->source->data.file.path = ctx->parseFileName(fileName, - ctx->opaque); - - if ((*def)->source->data.file.path == NULL) + if (ctx->parseFileName(fileName, + ctx->opaque, + &(*def)->source->data.file.path) < 0) goto cleanup; } else { virReportError(VIR_ERR_INTERNAL_ERROR, diff --git a/src/vmx/vmx.h b/src/vmx/vmx.h index df5d39157b99..e5420c970a4b 100644 --- a/src/vmx/vmx.h +++ b/src/vmx/vmx.h @@ -36,7 +36,7 @@ virDomainXMLOptionPtr virVMXDomainXMLConfInit(virCapsPtr caps); * Context */ -typedef char * (*virVMXParseFileName)(const char *fileName, void *opaque); +typedef int (*virVMXParseFileName)(const char *fileName, void *opaque, char **src); typedef char * (*virVMXFormatFileName)(const char *src, void *opaque); typedef int (*virVMXAutodetectSCSIControllerModel)(virDomainDiskDefPtr def, int *model, void *opaque); diff --git a/tests/vmx2xmltest.c b/tests/vmx2xmltest.c index 8cc227bbedfc..bb7c498d1b41 100644 --- a/tests/vmx2xmltest.c +++ b/tests/vmx2xmltest.c @@ -127,15 +127,18 @@ testCompareHelper(const void *data) return ret; } -static char * -testParseVMXFileName(const char *fileName, void *opaque G_GNUC_UNUSED) +static int +testParseVMXFileName(const char *fileName, + void *opaque G_GNUC_UNUSED, + char **src) { g_autofree char *copyOfFileName = NULL; char *tmp = NULL; char *saveptr = NULL; char *datastoreName = NULL; char *directoryAndFileName = NULL; - char *src = NULL; + + *src = NULL; if (STRPREFIX(fileName, "/vmfs/volumes/")) { /* Found absolute path referencing a file inside a datastore */ @@ -145,22 +148,22 @@ testParseVMXFileName(const char *fileName, void *opaque G_GNUC_UNUSED) if ((tmp = STRSKIP(copyOfFileName, "/vmfs/volumes/")) == NULL || (datastoreName = strtok_r(tmp, "/", &saveptr)) == NULL || (directoryAndFileName = strtok_r(NULL, "", &saveptr)) == NULL) { - return NULL; + return -1; } - src = g_strdup_printf("[%s] %s", datastoreName, directoryAndFileName); + *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); + *src = g_strdup(fileName); } else if (strchr(fileName, '/') != NULL) { /* Found relative path, this is not supported */ - return NULL; + return -1; } else { /* Found single file name referencing a file inside a datastore */ - src = g_strdup_printf("[datastore] directory/%s", fileName); + *src = g_strdup_printf("[datastore] directory/%s", fileName); } - return src; + return 0; } static int -- 2.30.0
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- tests/vmx2xmltest.c | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/tests/vmx2xmltest.c b/tests/vmx2xmltest.c index bb7c498d1b41..7db2edb12c27 100644 --- a/tests/vmx2xmltest.c +++ b/tests/vmx2xmltest.c @@ -66,7 +66,7 @@ testCapsInit(void) } static int -testCompareFiles(const char *vmx, const char *xml) +testCompareFiles(const char *vmx, const char *xml, bool should_fail_parse) { int ret = -1; char *vmxData = NULL; @@ -74,9 +74,17 @@ testCompareFiles(const char *vmx, const char *xml) virDomainDefPtr def = NULL; if (virTestLoadFile(vmx, &vmxData) < 0) - goto cleanup; + return -1; - if (!(def = virVMXParseConfig(&ctx, xmlopt, caps, vmxData))) + def = virVMXParseConfig(&ctx, xmlopt, caps, vmxData); + if (should_fail_parse) { + if (!def) + ret = 0; + else + VIR_TEST_DEBUG("passed instead of expected failure"); + goto cleanup; + } + if (!def) goto cleanup; if (!virDomainDefCheckABIStability(def, def, xmlopt)) { @@ -104,6 +112,7 @@ testCompareFiles(const char *vmx, const char *xml) struct testInfo { const char *input; const char *output; + bool should_fail; }; static int @@ -119,7 +128,7 @@ testCompareHelper(const void *data) xml = g_strdup_printf("%s/vmx2xmldata/vmx2xml-%s.xml", abs_srcdir, info->output); - ret = testCompareFiles(vmx, xml); + ret = testCompareFiles(vmx, xml, info->should_fail); VIR_FREE(vmx); VIR_FREE(xml); @@ -171,9 +180,9 @@ mymain(void) { int ret = 0; -# define DO_TEST(_in, _out) \ +# define DO_TEST_FULL(_in, _out, _should_fail) \ do { \ - struct testInfo info = { _in, _out }; \ + struct testInfo info = { _in, _out, _should_fail }; \ virResetLastError(); \ if (virTestRun("VMware VMX-2-XML "_in" -> "_out, \ testCompareHelper, &info) < 0) { \ @@ -181,6 +190,9 @@ mymain(void) } \ } while (0) +# define DO_TEST(_in, _out) DO_TEST_FULL(_in, _out, false) +# define DO_TEST_FAIL(_in, _out) DO_TEST_FULL(_in, _out, true) + testCapsInit(); if (caps == NULL) -- 2.30.0
This will be used later. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/esx/esx_driver.c | 3 ++- src/vmware/vmware_conf.c | 3 ++- src/vmware/vmware_conf.h | 3 ++- src/vmx/vmx.c | 12 +++++++----- src/vmx/vmx.h | 5 ++++- tests/vmx2xmltest.c | 13 ++++++++++++- 6 files changed, 29 insertions(+), 10 deletions(-) diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index 86d5396147a3..dde51688f72f 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -128,7 +128,8 @@ esxFreePrivate(esxPrivate **priv) static int esxParseVMXFileName(const char *fileName, void *opaque, - char **out) + char **out, + bool allow_missing G_GNUC_UNUSED) { esxVMX_Data *data = opaque; esxVI_String *propertyNameList = NULL; diff --git a/src/vmware/vmware_conf.c b/src/vmware/vmware_conf.c index 4f7dc3001d2b..55cd1d6f2d36 100644 --- a/src/vmware/vmware_conf.c +++ b/src/vmware/vmware_conf.c @@ -510,7 +510,8 @@ vmwareExtractPid(const char * vmxPath) int vmwareParseVMXFileName(const char *datastorePath, void *opaque G_GNUC_UNUSED, - char **out) + char **out, + bool allow_missing G_GNUC_UNUSED) { *out = g_strdup(datastorePath); diff --git a/src/vmware/vmware_conf.h b/src/vmware/vmware_conf.h index 6f86983f511f..4974260e68f6 100644 --- a/src/vmware/vmware_conf.h +++ b/src/vmware/vmware_conf.h @@ -86,7 +86,8 @@ int vmwareExtractPid(const char * vmxPath); int vmwareParseVMXFileName(const char *datastorePath, void *opaque, - char **out); + char **out, + bool allow_missing); char * vmwareFormatVMXFileName(const char *datastorePath, diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c index 97591842f789..aa5d1d4eedea 100644 --- a/src/vmx/vmx.c +++ b/src/vmx/vmx.c @@ -2411,7 +2411,7 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virConfPtr con } virDomainDiskSetType(*def, VIR_STORAGE_TYPE_FILE); - if (ctx->parseFileName(fileName, ctx->opaque, &tmp) < 0) + if (ctx->parseFileName(fileName, ctx->opaque, &tmp, false) < 0) goto cleanup; virDomainDiskSetSource(*def, tmp); VIR_FREE(tmp); @@ -2448,7 +2448,7 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virConfPtr con } virDomainDiskSetType(*def, VIR_STORAGE_TYPE_FILE); - if (ctx->parseFileName(fileName, ctx->opaque, &tmp) < 0) + if (ctx->parseFileName(fileName, ctx->opaque, &tmp, false) < 0) goto cleanup; virDomainDiskSetSource(*def, tmp); VIR_FREE(tmp); @@ -2515,7 +2515,7 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virConfPtr con virDomainDiskSetType(*def, VIR_STORAGE_TYPE_FILE); if (fileName && - ctx->parseFileName(fileName, ctx->opaque, &tmp) < 0) + ctx->parseFileName(fileName, ctx->opaque, &tmp, false) < 0) goto cleanup; virDomainDiskSetSource(*def, tmp); VIR_FREE(tmp); @@ -2977,7 +2977,8 @@ virVMXParseSerial(virVMXContext *ctx, virConfPtr conf, int port, (*def)->source->type = VIR_DOMAIN_CHR_TYPE_FILE; if (ctx->parseFileName(fileName, ctx->opaque, - &(*def)->source->data.file.path) < 0) + &(*def)->source->data.file.path, + false) < 0) goto cleanup; } else if (STRCASEEQ(fileType, "pipe")) { /* @@ -3142,7 +3143,8 @@ virVMXParseParallel(virVMXContext *ctx, virConfPtr conf, int port, (*def)->source->type = VIR_DOMAIN_CHR_TYPE_FILE; if (ctx->parseFileName(fileName, ctx->opaque, - &(*def)->source->data.file.path) < 0) + &(*def)->source->data.file.path, + false) < 0) goto cleanup; } else { virReportError(VIR_ERR_INTERNAL_ERROR, diff --git a/src/vmx/vmx.h b/src/vmx/vmx.h index e5420c970a4b..550c1264f3b8 100644 --- a/src/vmx/vmx.h +++ b/src/vmx/vmx.h @@ -36,7 +36,10 @@ virDomainXMLOptionPtr virVMXDomainXMLConfInit(virCapsPtr caps); * Context */ -typedef int (*virVMXParseFileName)(const char *fileName, void *opaque, char **src); +typedef int (*virVMXParseFileName)(const char *fileName, + void *opaque, + char **src, + bool allow_missing); typedef char * (*virVMXFormatFileName)(const char *src, void *opaque); typedef int (*virVMXAutodetectSCSIControllerModel)(virDomainDiskDefPtr def, int *model, void *opaque); diff --git a/tests/vmx2xmltest.c b/tests/vmx2xmltest.c index 7db2edb12c27..3a11dfb41ce8 100644 --- a/tests/vmx2xmltest.c +++ b/tests/vmx2xmltest.c @@ -139,7 +139,8 @@ testCompareHelper(const void *data) static int testParseVMXFileName(const char *fileName, void *opaque G_GNUC_UNUSED, - char **src) + char **src, + bool allow_missing) { g_autofree char *copyOfFileName = NULL; char *tmp = NULL; @@ -160,6 +161,16 @@ testParseVMXFileName(const char *fileName, 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 */ -- 2.30.0
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- .../vmx2xml-cdrom-ide-file-missing-datastore.vmx | 6 ++++++ tests/vmx2xmldata/vmx2xml-cdrom-ide-file-missing-file.vmx | 6 ++++++ .../vmx2xml-harddisk-ide-file-missing-datastore.vmx | 6 ++++++ .../vmx2xml-harddisk-scsi-file-missing-file.vmx | 7 +++++++ tests/vmx2xmltest.c | 6 ++++++ 5 files changed, 31 insertions(+) create mode 100644 tests/vmx2xmldata/vmx2xml-cdrom-ide-file-missing-datastore.vmx create mode 100644 tests/vmx2xmldata/vmx2xml-cdrom-ide-file-missing-file.vmx create mode 100644 tests/vmx2xmldata/vmx2xml-harddisk-ide-file-missing-datastore.vmx create mode 100644 tests/vmx2xmldata/vmx2xml-harddisk-scsi-file-missing-file.vmx diff --git a/tests/vmx2xmldata/vmx2xml-cdrom-ide-file-missing-datastore.vmx b/tests/vmx2xmldata/vmx2xml-cdrom-ide-file-missing-datastore.vmx new file mode 100644 index 000000000000..8a8de892c88a --- /dev/null +++ b/tests/vmx2xmldata/vmx2xml-cdrom-ide-file-missing-datastore.vmx @@ -0,0 +1,6 @@ +config.version = "8" +virtualHW.version = "4" +ide0:0.present = "true" +ide0:0.deviceType = "cdrom-image" +ide0:0.fileName = "/vmfs/volumes/missing/cdrom.iso" +displayName = "test" diff --git a/tests/vmx2xmldata/vmx2xml-cdrom-ide-file-missing-file.vmx b/tests/vmx2xmldata/vmx2xml-cdrom-ide-file-missing-file.vmx new file mode 100644 index 000000000000..6ee2fb553ae4 --- /dev/null +++ b/tests/vmx2xmldata/vmx2xml-cdrom-ide-file-missing-file.vmx @@ -0,0 +1,6 @@ +config.version = "8" +virtualHW.version = "4" +ide0:0.present = "true" +ide0:0.deviceType = "cdrom-image" +ide0:0.fileName = "/vmfs/volumes/ds/missing.iso" +displayName = "test" diff --git a/tests/vmx2xmldata/vmx2xml-harddisk-ide-file-missing-datastore.vmx b/tests/vmx2xmldata/vmx2xml-harddisk-ide-file-missing-datastore.vmx new file mode 100644 index 000000000000..9d89ce7158a8 --- /dev/null +++ b/tests/vmx2xmldata/vmx2xml-harddisk-ide-file-missing-datastore.vmx @@ -0,0 +1,6 @@ +config.version = "8" +virtualHW.version = "4" +ide0:0.present = "true" +ide0:0.deviceType = "ata-hardDisk" +ide0:0.fileName = "/vmfs/volumes/missing/harddisk.vmdk" +displayName = "test" diff --git a/tests/vmx2xmldata/vmx2xml-harddisk-scsi-file-missing-file.vmx b/tests/vmx2xmldata/vmx2xml-harddisk-scsi-file-missing-file.vmx new file mode 100644 index 000000000000..d39f657e437f --- /dev/null +++ b/tests/vmx2xmldata/vmx2xml-harddisk-scsi-file-missing-file.vmx @@ -0,0 +1,7 @@ +config.version = "8" +virtualHW.version = "4" +scsi0.present = "true" +scsi0:0.present = "true" +scsi0:0.deviceType = "scsi-hardDisk" +scsi0:0.fileName = "/vmfs/volumes/ds/missing.vmdk" +displayName = "test" diff --git a/tests/vmx2xmltest.c b/tests/vmx2xmltest.c index 3a11dfb41ce8..d622e46fd563 100644 --- a/tests/vmx2xmltest.c +++ b/tests/vmx2xmltest.c @@ -247,6 +247,12 @@ mymain(void) DO_TEST("cdrom-ide-raw-auto-detect", "cdrom-ide-raw-auto-detect"); DO_TEST("cdrom-ide-raw-auto-detect", "cdrom-ide-raw-auto-detect"); + DO_TEST_FAIL("cdrom-ide-file-missing-datastore", "cdrom-ide-empty"); + DO_TEST_FAIL("cdrom-ide-file-missing-file", "cdrom-ide-empty"); + + DO_TEST_FAIL("harddisk-ide-file-missing-datastore", "harddisk-ide-file"); + DO_TEST_FAIL("harddisk-scsi-file-missing-file", "harddisk-scsi-file"); + DO_TEST("floppy-file", "floppy-file"); DO_TEST("floppy-device", "floppy-device"); -- 2.30.0
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/esx/esx_driver.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index dde51688f72f..0271f81a5655 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -129,7 +129,7 @@ static int esxParseVMXFileName(const char *fileName, void *opaque, char **out, - bool allow_missing G_GNUC_UNUSED) + bool allow_missing) { esxVMX_Data *data = opaque; esxVI_String *propertyNameList = NULL; @@ -223,9 +223,13 @@ esxParseVMXFileName(const char *fileName, } if (!datastoreList) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("File name '%s' refers to non-existing datastore '%s'"), - fileName, datastoreName); + if (allow_missing) { + ret = 0; + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("File name '%s' refers to non-existing datastore '%s'"), + fileName, datastoreName); + } goto cleanup; } -- 2.30.0
This is perfectly valid in VMWare and the VM just boots with an empty drive. We used to just skip the whole drive before, but since we changed how we parse empty cdrom drives this results in an error. Make it behave more closer to VMWare. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1903953 Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/vmx/vmx.c | 2 +- tests/vmx2xmltest.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c index aa5d1d4eedea..56318fc8b285 100644 --- a/src/vmx/vmx.c +++ b/src/vmx/vmx.c @@ -2448,7 +2448,7 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virConfPtr con } virDomainDiskSetType(*def, VIR_STORAGE_TYPE_FILE); - if (ctx->parseFileName(fileName, ctx->opaque, &tmp, false) < 0) + if (ctx->parseFileName(fileName, ctx->opaque, &tmp, true) < 0) goto cleanup; virDomainDiskSetSource(*def, tmp); VIR_FREE(tmp); diff --git a/tests/vmx2xmltest.c b/tests/vmx2xmltest.c index d622e46fd563..11739e6f3f51 100644 --- a/tests/vmx2xmltest.c +++ b/tests/vmx2xmltest.c @@ -247,8 +247,8 @@ mymain(void) DO_TEST("cdrom-ide-raw-auto-detect", "cdrom-ide-raw-auto-detect"); DO_TEST("cdrom-ide-raw-auto-detect", "cdrom-ide-raw-auto-detect"); - DO_TEST_FAIL("cdrom-ide-file-missing-datastore", "cdrom-ide-empty"); - DO_TEST_FAIL("cdrom-ide-file-missing-file", "cdrom-ide-empty"); + DO_TEST("cdrom-ide-file-missing-datastore", "cdrom-ide-empty"); + DO_TEST("cdrom-ide-file-missing-file", "cdrom-ide-empty"); DO_TEST_FAIL("harddisk-ide-file-missing-datastore", "harddisk-ide-file"); DO_TEST_FAIL("harddisk-scsi-file-missing-file", "harddisk-scsi-file"); -- 2.30.0
On 1/5/21 4:54 PM, Martin Kletzander wrote:
This is perfectly valid in VMWare and the VM just boots with an empty drive. We used to just skip the whole drive before, but since we changed how we parse empty cdrom drives this now results in an error and the user not being able to even dump the XML. Instead of erroring out, just keep the drive empty.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1903953
v3: - Fixed the vmware driver - Bit of a clean-up - Few more tests
v2: - Do not report and reset an error, but handle it more nicely. - https://www.redhat.com/archives/libvir-list/2020-December/msg00846.html
v1: - https://www.redhat.com/archives/libvir-list/2020-December/msg00840.html
Martin Kletzander (8): esx: Unindent unnecessary conditional branch tests: Use g_autofree in testParseVMXFileName vmx: Make virVMXParseFileName return an integer tests: Allow testing for parse failures in vmx2xmltest vmx: Allow missing cdrom image file in virVMXParseFileName tests: Test vmx files with missing images esx: Handle missing images in esxParseVMXFileName vmx: Treat missing cdrom-image as empty drive
src/esx/esx_driver.c | 160 ++++++++++-------- src/vmware/vmware_conf.c | 21 ++- src/vmware/vmware_conf.h | 10 +- src/vmware/vmware_driver.c | 6 +- src/vmx/vmx.c | 27 +-- src/vmx/vmx.h | 5 +- ...x2xml-cdrom-ide-file-missing-datastore.vmx | 6 + .../vmx2xml-cdrom-ide-file-missing-file.vmx | 6 + ...ml-harddisk-ide-file-missing-datastore.vmx | 6 + ...mx2xml-harddisk-scsi-file-missing-file.vmx | 7 + tests/vmx2xmltest.c | 67 +++++--- 11 files changed, 203 insertions(+), 118 deletions(-) create mode 100644 tests/vmx2xmldata/vmx2xml-cdrom-ide-file-missing-datastore.vmx create mode 100644 tests/vmx2xmldata/vmx2xml-cdrom-ide-file-missing-file.vmx create mode 100644 tests/vmx2xmldata/vmx2xml-harddisk-ide-file-missing-datastore.vmx create mode 100644 tests/vmx2xmldata/vmx2xml-harddisk-scsi-file-missing-file.vmx
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
On Tue, Jan 05, 2021 at 07:19:08PM +0100, Michal Privoznik wrote:
On 1/5/21 4:54 PM, Martin Kletzander wrote:
This is perfectly valid in VMWare and the VM just boots with an empty drive. We used to just skip the whole drive before, but since we changed how we parse empty cdrom drives this now results in an error and the user not being able to even dump the XML. Instead of erroring out, just keep the drive empty.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1903953
v3: - Fixed the vmware driver - Bit of a clean-up - Few more tests
v2: - Do not report and reset an error, but handle it more nicely. - https://www.redhat.com/archives/libvir-list/2020-December/msg00846.html
v1: - https://www.redhat.com/archives/libvir-list/2020-December/msg00840.html
Martin Kletzander (8): esx: Unindent unnecessary conditional branch tests: Use g_autofree in testParseVMXFileName vmx: Make virVMXParseFileName return an integer tests: Allow testing for parse failures in vmx2xmltest vmx: Allow missing cdrom image file in virVMXParseFileName tests: Test vmx files with missing images esx: Handle missing images in esxParseVMXFileName vmx: Treat missing cdrom-image as empty drive
src/esx/esx_driver.c | 160 ++++++++++-------- src/vmware/vmware_conf.c | 21 ++- src/vmware/vmware_conf.h | 10 +- src/vmware/vmware_driver.c | 6 +- src/vmx/vmx.c | 27 +-- src/vmx/vmx.h | 5 +- ...x2xml-cdrom-ide-file-missing-datastore.vmx | 6 + .../vmx2xml-cdrom-ide-file-missing-file.vmx | 6 + ...ml-harddisk-ide-file-missing-datastore.vmx | 6 + ...mx2xml-harddisk-scsi-file-missing-file.vmx | 7 + tests/vmx2xmltest.c | 67 +++++--- 11 files changed, 203 insertions(+), 118 deletions(-) create mode 100644 tests/vmx2xmldata/vmx2xml-cdrom-ide-file-missing-datastore.vmx create mode 100644 tests/vmx2xmldata/vmx2xml-cdrom-ide-file-missing-file.vmx create mode 100644 tests/vmx2xmldata/vmx2xml-harddisk-ide-file-missing-datastore.vmx create mode 100644 tests/vmx2xmldata/vmx2xml-harddisk-scsi-file-missing-file.vmx
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Thanks, although I *again* forgot to add your R-b to the patches... I wish gitlab took care of that. And I even have a function and a command for it: function rb --description 'Add Reviewed-by: to current commit' --argument who set git_cmd (command -s git) if ! test $git_cmd begin set_color red echo -n "error: " set_color normal echo "Command `git` not available" end >&2 return 1 end if ! test $who begin set_color red echo -n "error: " set_color normal echo "Missing parameter `who`" end >&2 return 1 end env VISUAL='git interpret-trailers --in-place --trailer "Reviewed-by='$who'"' $git_cmd commit --amend end Anyway, thanks, and sorry O:-)
Michal
participants (2)
-
Martin Kletzander -
Michal Privoznik