[libvirt PATCH v2 0/5] 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 v2: - Do not report and reset an error, but handle it more nicely. v1: - https://www.redhat.com/archives/libvir-list/2020-December/msg00840.html Martin Kletzander (5): esx: Unindent unnecessary conditional branch tests: Use g_autofree in testParseVMXFileName vmx: Make virVMXParseFileName return an integer vmx: Allow missing cdrom image file in virVMXParseFileName vmx: Treat missing cdrom-image as empty drive src/esx/esx_driver.c | 160 ++++++++++-------- src/vmware/vmware_conf.c | 10 +- src/vmx/vmx.c | 25 +-- src/vmx/vmx.h | 5 +- .../vmx2xmldata/vmx2xml-cdrom-ide-missing.vmx | 6 + tests/vmx2xmltest.c | 36 ++-- 6 files changed, 136 insertions(+), 106 deletions(-) create mode 100644 tests/vmx2xmldata/vmx2xml-cdrom-ide-missing.vmx -- 2.29.2
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.29.2
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.29.2
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 | 10 +++++----- src/vmx/vmx.c | 21 ++++++++++----------- src/vmx/vmx.h | 2 +- tests/vmx2xmltest.c | 19 ++++++++++--------- 5 files changed, 44 insertions(+), 39 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..c90cb10faf7c 100644 --- a/src/vmware/vmware_conf.c +++ b/src/vmware/vmware_conf.c @@ -507,11 +507,11 @@ vmwareExtractPid(const char * vmxPath) return pid_value; } -char * -vmwareCopyVMXFileName(const char *datastorePath, void *opaque G_GNUC_UNUSED) +int +vmwareCopyVMXFileName(const char *datastorePath, void *opaque G_GNUC_UNUSED, + char **out) { - char *path; + *out = g_strdup(datastorePath); - path = g_strdup(datastorePath); - return path; + return *out ? 0 : -1; } diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c index b86dbe9ca267..b2b2244415a1 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 (!(tmp = ctx->parseFileName(fileName, ctx->opaque))) + if (ctx->parseFileName(fileName, ctx->opaque, &tmp) < 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 (!(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..412b201f0242 100644 --- a/tests/vmx2xmltest.c +++ b/tests/vmx2xmltest.c @@ -127,15 +127,16 @@ 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; if (STRPREFIX(fileName, "/vmfs/volumes/")) { /* Found absolute path referencing a file inside a datastore */ @@ -145,22 +146,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.29.2
On 12/21/20 1:19 PM, Martin Kletzander wrote:
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 | 10 +++++----- src/vmx/vmx.c | 21 ++++++++++----------- src/vmx/vmx.h | 2 +- tests/vmx2xmltest.c | 19 ++++++++++--------- 5 files changed, 44 insertions(+), 39 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..c90cb10faf7c 100644 --- a/src/vmware/vmware_conf.c +++ b/src/vmware/vmware_conf.c @@ -507,11 +507,11 @@ vmwareExtractPid(const char * vmxPath) return pid_value; }
-char * -vmwareCopyVMXFileName(const char *datastorePath, void *opaque G_GNUC_UNUSED) +int +vmwareCopyVMXFileName(const char *datastorePath, void *opaque G_GNUC_UNUSED, + char **out) { - char *path; + *out = g_strdup(datastorePath);
- path = g_strdup(datastorePath); - return path; + return *out ? 0 : -1; } diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c index b86dbe9ca267..b2b2244415a1 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 (!(tmp = ctx->parseFileName(fileName, ctx->opaque))) + if (ctx->parseFileName(fileName, ctx->opaque, &tmp) < 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 (!(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);
This change is making the build break in my env: ../src/vmware/vmware_conf.c: In function ‘vmwareLoadDomains’: ../src/vmware/vmware_conf.c:142:23: error: assignment to ‘virVMXParseFileName’ {aka ‘int (*)(const char *, void *, char **)’} from incompatible pointer type ‘char * (*)(const char *, void *)’ [-Werror=incompatible-pointer-types] 142 | ctx.parseFileName = vmwareCopyVMXFileName; | ^ ../src/vmware/vmware_conf.c: At top level: ../src/vmware/vmware_conf.c:511:1: error: conflicting types for ‘vmwareCopyVMXFileName’ 511 | vmwareCopyVMXFileName(const char *datastorePath, void *opaque G_GNUC_UNUSED, | ^~~~~~~~~~~~~~~~~~~~~ In file included from ../src/vmware/vmware_conf.c:32: ../src/vmware/vmware_conf.h:86:7: note: previous declaration of ‘vmwareCopyVMXFileName’ was here 86 | char *vmwareCopyVMXFileName(const char *datastorePath, void *opaque); | ^~~~~~~~~~~~~~~~~~~~~ cc1: all warnings being treated as errors (...) ../src/vmware/vmware_driver.c: In function ‘vmwareConnectDomainXMLFromNative’: ../src/vmware/vmware_driver.c:953:23: error: assignment to ‘virVMXParseFileName’ {aka ‘int (*)(const char *, void *, char **)’} from incompatible pointer type ‘char * (*)(const char *, void *)’ [-Werror=incompatible-pointer-types] 953 | ctx.parseFileName = vmwareCopyVMXFileName; | ^ cc1: all warnings being treated as errors I believe there are a handful of virVMXParseFileName impl instances that needs to be changed as well. Thanks, DHB
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..412b201f0242 100644 --- a/tests/vmx2xmltest.c +++ b/tests/vmx2xmltest.c @@ -127,15 +127,16 @@ 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;
if (STRPREFIX(fileName, "/vmfs/volumes/")) { /* Found absolute path referencing a file inside a datastore */ @@ -145,22 +146,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
On 1/5/21 12:32 AM, Daniel Henrique Barboza wrote:
On 12/21/20 1:19 PM, Martin Kletzander wrote:
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 | 10 +++++----- src/vmx/vmx.c | 21 ++++++++++----------- src/vmx/vmx.h | 2 +- tests/vmx2xmltest.c | 19 ++++++++++--------- 5 files changed, 44 insertions(+), 39 deletions(-)
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);
This change is making the build break in my env:
../src/vmware/vmware_conf.c: In function ‘vmwareLoadDomains’: ../src/vmware/vmware_conf.c:142:23: error: assignment to ‘virVMXParseFileName’ {aka ‘int (*)(const char *, void *, char **)’} from incompatible pointer type ‘char * (*)(const char *, void *)’ [-Werror=incompatible-pointer-types] 142 | ctx.parseFileName = vmwareCopyVMXFileName; | ^ ../src/vmware/vmware_conf.c: At top level: ../src/vmware/vmware_conf.c:511:1: error: conflicting types for ‘vmwareCopyVMXFileName’ 511 | vmwareCopyVMXFileName(const char *datastorePath, void *opaque G_GNUC_UNUSED, | ^~~~~~~~~~~~~~~~~~~~~ In file included from ../src/vmware/vmware_conf.c:32: ../src/vmware/vmware_conf.h:86:7: note: previous declaration of ‘vmwareCopyVMXFileName’ was here 86 | char *vmwareCopyVMXFileName(const char *datastorePath, void *opaque); | ^~~~~~~~~~~~~~~~~~~~~ cc1: all warnings being treated as errors
(...)
../src/vmware/vmware_driver.c: In function ‘vmwareConnectDomainXMLFromNative’: ../src/vmware/vmware_driver.c:953:23: error: assignment to ‘virVMXParseFileName’ {aka ‘int (*)(const char *, void *, char **)’} from incompatible pointer type ‘char * (*)(const char *, void *)’ [-Werror=incompatible-pointer-types] 953 | ctx.parseFileName = vmwareCopyVMXFileName; | ^ cc1: all warnings being treated as errors
I believe there are a handful of virVMXParseFileName impl instances that needs to be changed as well.
Indeed. I think we will need to change virVMXFormatFileName() too at the same time, because of vmwareCopyVMXFileName() which is passed as formatFileName callback. BTW: I've found out that we don't automatically enable vmware driver. I had to enable it explicitly: meson -Dsystem=true -Ddriver_vmware=enabled _build I'm looking into that. Michal
On Tue, Jan 05, 2021 at 10:15:47AM +0100, Michal Privoznik wrote:
On 1/5/21 12:32 AM, Daniel Henrique Barboza wrote:
On 12/21/20 1:19 PM, Martin Kletzander wrote:
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 | 10 +++++----- src/vmx/vmx.c | 21 ++++++++++----------- src/vmx/vmx.h | 2 +- tests/vmx2xmltest.c | 19 ++++++++++--------- 5 files changed, 44 insertions(+), 39 deletions(-)
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);
This change is making the build break in my env:
../src/vmware/vmware_conf.c: In function ‘vmwareLoadDomains’: ../src/vmware/vmware_conf.c:142:23: error: assignment to ‘virVMXParseFileName’ {aka ‘int (*)(const char *, void *, char **)’} from incompatible pointer type ‘char * (*)(const char *, void *)’ [-Werror=incompatible-pointer-types] 142 | ctx.parseFileName = vmwareCopyVMXFileName; | ^ ../src/vmware/vmware_conf.c: At top level: ../src/vmware/vmware_conf.c:511:1: error: conflicting types for ‘vmwareCopyVMXFileName’ 511 | vmwareCopyVMXFileName(const char *datastorePath, void *opaque G_GNUC_UNUSED, | ^~~~~~~~~~~~~~~~~~~~~ In file included from ../src/vmware/vmware_conf.c:32: ../src/vmware/vmware_conf.h:86:7: note: previous declaration of ‘vmwareCopyVMXFileName’ was here 86 | char *vmwareCopyVMXFileName(const char *datastorePath, void *opaque); | ^~~~~~~~~~~~~~~~~~~~~ cc1: all warnings being treated as errors
(...)
../src/vmware/vmware_driver.c: In function ‘vmwareConnectDomainXMLFromNative’: ../src/vmware/vmware_driver.c:953:23: error: assignment to ‘virVMXParseFileName’ {aka ‘int (*)(const char *, void *, char **)’} from incompatible pointer type ‘char * (*)(const char *, void *)’ [-Werror=incompatible-pointer-types] 953 | ctx.parseFileName = vmwareCopyVMXFileName; | ^ cc1: all warnings being treated as errors
I believe there are a handful of virVMXParseFileName impl instances that needs to be changed as well.
Indeed. I think we will need to change virVMXFormatFileName() too at the same time, because of vmwareCopyVMXFileName() which is passed as formatFileName callback.
Not only that, but vmware_driver uses the smae function for parsing and formatting. They are, however, just g_strdup()s, so I can split them no problem. Thanks for noticing. Anyway, just looking at the commits they should be done a bit differently, I had some cock-ups there, I guess. I'll send a v3.
BTW: I've found out that we don't automatically enable vmware driver. I had to enable it explicitly:
meson -Dsystem=true -Ddriver_vmware=enabled _build
I'm looking into that.
We're not building it downstream even, I think, I managed to fix it for ESX where the issue was happening actually. The vmware driver just controls vmware player/workstation locally. Are there actually people using that?
Michal
This will be used later. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/esx/esx_driver.c | 13 +++++++++---- src/vmware/vmware_conf.c | 2 +- src/vmx/vmx.c | 12 +++++++----- src/vmx/vmx.h | 5 ++++- tests/vmx2xmltest.c | 13 ++++++++++++- 5 files changed, 33 insertions(+), 12 deletions(-) diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index 86d5396147a3..0271f81a5655 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) { esxVMX_Data *data = opaque; esxVI_String *propertyNameList = NULL; @@ -222,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; } diff --git a/src/vmware/vmware_conf.c b/src/vmware/vmware_conf.c index c90cb10faf7c..4b0832f50b3c 100644 --- a/src/vmware/vmware_conf.c +++ b/src/vmware/vmware_conf.c @@ -509,7 +509,7 @@ vmwareExtractPid(const char * vmxPath) int vmwareCopyVMXFileName(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/vmx/vmx.c b/src/vmx/vmx.c index b2b2244415a1..4d098a5fa4d6 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, true) < 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 412b201f0242..116d729a0147 100644 --- a/tests/vmx2xmltest.c +++ b/tests/vmx2xmltest.c @@ -130,7 +130,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; @@ -149,6 +150,16 @@ testParseVMXFileName(const char *fileName, return -1; } + if (STREQ(datastoreName, "missing") || + STREQ(directoryAndFileName, "missing")) { + if (allow_missing) + return 0; + + virReportError(VIR_ERR_INTERNAL_ERROR, + "Missing file name '%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.29.2
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/vmx2xmldata/vmx2xml-cdrom-ide-missing.vmx | 6 ++++++ tests/vmx2xmltest.c | 1 + 3 files changed, 8 insertions(+), 1 deletion(-) create mode 100644 tests/vmx2xmldata/vmx2xml-cdrom-ide-missing.vmx diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c index 4d098a5fa4d6..2c631e32e7df 100644 --- a/src/vmx/vmx.c +++ b/src/vmx/vmx.c @@ -2447,10 +2447,10 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virConfPtr con goto cleanup; } - virDomainDiskSetType(*def, VIR_STORAGE_TYPE_FILE); if (ctx->parseFileName(fileName, ctx->opaque, &tmp, true) < 0) goto cleanup; virDomainDiskSetSource(*def, tmp); + virDomainDiskSetType(*def, VIR_STORAGE_TYPE_FILE); VIR_FREE(tmp); } else if (deviceType && STRCASEEQ(deviceType, "atapi-cdrom")) { virDomainDiskSetType(*def, VIR_STORAGE_TYPE_BLOCK); diff --git a/tests/vmx2xmldata/vmx2xml-cdrom-ide-missing.vmx b/tests/vmx2xmldata/vmx2xml-cdrom-ide-missing.vmx new file mode 100644 index 000000000000..bef1ebbba272 --- /dev/null +++ b/tests/vmx2xmldata/vmx2xml-cdrom-ide-missing.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/dir/file.iso" +displayName = "test" diff --git a/tests/vmx2xmltest.c b/tests/vmx2xmltest.c index 116d729a0147..624ee14ece12 100644 --- a/tests/vmx2xmltest.c +++ b/tests/vmx2xmltest.c @@ -232,6 +232,7 @@ mymain(void) DO_TEST("cdrom-ide-raw-device", "cdrom-ide-raw-device"); 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("cdrom-ide-missing", "cdrom-ide-empty"); DO_TEST("floppy-file", "floppy-file"); DO_TEST("floppy-device", "floppy-device"); -- 2.29.2
participants (3)
-
Daniel Henrique Barboza -
Martin Kletzander -
Michal Privoznik