[libvirt] [PATCH] esx: Rework datastore path parsing and handling
Instead of splitting the path part of a datastore path into directory and file name, keep this in one piece. An example: "[datastore] directory/file" was split into this before: datastoreName = "datastore" directoryName = "directory" fileName = "file" Now it's split into this: datastoreName = "datastore" directoryName = "directory" directoryAndFileName = "directory/file" This simplifies code using esxUtil_ParseDatastorePath, because directoryAndFileName is used more often than fileName. Also the old approach expected the datastore path to reference a actual file, but this isn't always correct, especially when listing volume. In that case esxUtil_ParseDatastorePath is used to parse a path that references a directory. This fails for a vpx:// connection because the vCenter returns directory paths with a trailing '/'. The new approach is robust against this and the actual decision if the datastore path should reference a file or a directory is up to the caller of esxUtil_ParseDatastorePath. Update the tests accordingly. --- src/esx/esx_driver.c | 90 +++++++++++++++++++----------------------- src/esx/esx_storage_driver.c | 70 +++++++++----------------------- src/esx/esx_util.c | 71 +++++++++++++++++++-------------- src/esx/esx_util.h | 2 +- src/esx/esx_vi.c | 30 +++++++++++++- tests/esxutilstest.c | 32 ++++++++------ tests/xml2vmxtest.c | 19 +++------ 7 files changed, 155 insertions(+), 159 deletions(-) diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index 50d5bc0..eda3fc2 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -52,8 +52,7 @@ typedef struct _esxVMX_Data esxVMX_Data; struct _esxVMX_Data { esxVI_Context *ctx; - const char *datastoreName; - const char *directoryName; + char *datastorePathWithoutFileName; }; @@ -117,8 +116,8 @@ esxParseVMXFileName(const char *fileName, void *opaque) if (strchr(fileName, '/') == NULL && strchr(fileName, '\\') == NULL) { /* Plain file name, use same directory as for the .vmx file */ - if (virAsprintf(&datastorePath, "[%s] %s/%s", data->datastoreName, - data->directoryName, fileName) < 0) { + if (virAsprintf(&datastorePath, "%s/%s", + data->datastorePathWithoutFileName, fileName) < 0) { virReportOOMError(); goto cleanup; } @@ -254,8 +253,7 @@ esxFormatVMXFileName(const char *datastorePath, void *opaque) bool success = false; esxVMX_Data *data = opaque; char *datastoreName = NULL; - char *directoryName = NULL; - char *fileName = NULL; + char *directoryAndFileName = NULL; esxVI_ObjectContent *datastore = NULL; esxVI_DatastoreHostMount *hostMount = NULL; char separator = '/'; @@ -265,13 +263,12 @@ esxFormatVMXFileName(const char *datastorePath, void *opaque) char *absolutePath = NULL; /* Parse datastore path and lookup datastore */ - if (esxUtil_ParseDatastorePath(datastorePath, &datastoreName, - &directoryName, &fileName) < 0) { + if (esxUtil_ParseDatastorePath(datastorePath, &datastoreName, NULL, + &directoryAndFileName) < 0) { goto cleanup; } - if (esxVI_LookupDatastoreByName(data->ctx, datastoreName, - NULL, &datastore, + if (esxVI_LookupDatastoreByName(data->ctx, datastoreName, NULL, &datastore, esxVI_Occurrence_RequiredItem) < 0 || esxVI_LookupDatastoreHostMount(data->ctx, datastore->obj, &hostMount) < 0) { @@ -290,29 +287,23 @@ esxFormatVMXFileName(const char *datastorePath, void *opaque) --length; } - /* Format as <mount>[/<directory>]/<file> */ + /* Format as <mount>[/<directory>]/<file>, convert / to \ when necessary */ virBufferAdd(&buffer, hostMount->mountInfo->path, length); - if (directoryName != NULL) { - /* Convert / to \ when necessary */ - if (separator != '/') { - tmp = directoryName; - - while (*tmp != '\0') { - if (*tmp == '/') { - *tmp = separator; - } + if (separator != '/') { + tmp = directoryAndFileName; - ++tmp; + while (*tmp != '\0') { + if (*tmp == '/') { + *tmp = separator; } - } - virBufferAddChar(&buffer, separator); - virBufferAdd(&buffer, directoryName, -1); + ++tmp; + } } virBufferAddChar(&buffer, separator); - virBufferAdd(&buffer, fileName, -1); + virBufferAdd(&buffer, directoryAndFileName, -1); if (virBufferError(&buffer)) { virReportOOMError(); @@ -332,8 +323,7 @@ esxFormatVMXFileName(const char *datastorePath, void *opaque) } VIR_FREE(datastoreName); - VIR_FREE(directoryName); - VIR_FREE(fileName); + VIR_FREE(directoryAndFileName); esxVI_ObjectContent_Free(&datastore); esxVI_DatastoreHostMount_Free(&hostMount); @@ -2527,7 +2517,7 @@ esxDomainDumpXML(virDomainPtr domain, int flags) char *vmPathName = NULL; char *datastoreName = NULL; char *directoryName = NULL; - char *fileName = NULL; + char *directoryAndFileName = NULL; virBuffer buffer = VIR_BUFFER_INITIALIZER; char *url = NULL; char *vmx = NULL; @@ -2554,19 +2544,13 @@ esxDomainDumpXML(virDomainPtr domain, int flags) } if (esxUtil_ParseDatastorePath(vmPathName, &datastoreName, &directoryName, - &fileName) < 0) { + &directoryAndFileName) < 0) { goto cleanup; } virBufferVSprintf(&buffer, "%s://%s:%d/folder/", priv->transport, domain->conn->uri->server, domain->conn->uri->port); - - if (directoryName != NULL) { - virBufferURIEncodeString(&buffer, directoryName); - virBufferAddChar(&buffer, '/'); - } - - virBufferURIEncodeString(&buffer, fileName); + virBufferURIEncodeString(&buffer, directoryAndFileName); virBufferAddLit(&buffer, "?dcPath="); virBufferURIEncodeString(&buffer, priv->primary->datacenter->name); virBufferAddLit(&buffer, "&dsName="); @@ -2584,8 +2568,20 @@ esxDomainDumpXML(virDomainPtr domain, int flags) } data.ctx = priv->primary; - data.datastoreName = datastoreName; - data.directoryName = directoryName; + + if (directoryName == NULL) { + if (virAsprintf(&data.datastorePathWithoutFileName, "[%s]", + datastoreName) < 0) { + virReportOOMError(); + goto cleanup; + } + } else { + if (virAsprintf(&data.datastorePathWithoutFileName, "[%s] %s", + datastoreName, directoryName) < 0) { + virReportOOMError(); + goto cleanup; + } + } ctx.opaque = &data; ctx.parseFileName = esxParseVMXFileName; @@ -2612,8 +2608,9 @@ esxDomainDumpXML(virDomainPtr domain, int flags) esxVI_ObjectContent_Free(&virtualMachine); VIR_FREE(datastoreName); VIR_FREE(directoryName); - VIR_FREE(fileName); + VIR_FREE(directoryAndFileName); VIR_FREE(url); + VIR_FREE(data.datastorePathWithoutFileName); VIR_FREE(vmx); virDomainDefFree(def); @@ -2640,8 +2637,7 @@ esxDomainXMLFromNative(virConnectPtr conn, const char *nativeFormat, } data.ctx = priv->primary; - data.datastoreName = "?"; - data.directoryName = "?"; + data.datastorePathWithoutFileName = (char *)"[?] ?"; ctx.opaque = &data; ctx.parseFileName = esxParseVMXFileName; @@ -2686,8 +2682,7 @@ esxDomainXMLToNative(virConnectPtr conn, const char *nativeFormat, } data.ctx = priv->primary; - data.datastoreName = NULL; - data.directoryName = NULL; + data.datastorePathWithoutFileName = NULL; ctx.opaque = &data; ctx.parseFileName = NULL; @@ -2887,7 +2882,6 @@ esxDomainDefineXML(virConnectPtr conn, const char *xml ATTRIBUTE_UNUSED) esxVMX_Data data; char *datastoreName = NULL; char *directoryName = NULL; - char *fileName = NULL; virBuffer buffer = VIR_BUFFER_INITIALIZER; char *url = NULL; char *datastoreRelatedPath = NULL; @@ -2927,8 +2921,7 @@ esxDomainDefineXML(virConnectPtr conn, const char *xml ATTRIBUTE_UNUSED) /* Build VMX from domain XML */ data.ctx = priv->primary; - data.datastoreName = NULL; - data.directoryName = NULL; + data.datastorePathWithoutFileName = NULL; ctx.opaque = &data; ctx.parseFileName = NULL; @@ -2979,11 +2972,11 @@ esxDomainDefineXML(virConnectPtr conn, const char *xml ATTRIBUTE_UNUSED) } if (esxUtil_ParseDatastorePath(disk->src, &datastoreName, &directoryName, - &fileName) < 0) { + NULL) < 0) { goto cleanup; } - if (! virFileHasSuffix(fileName, ".vmdk")) { + if (! virFileHasSuffix(disk->src, ".vmdk")) { ESX_ERROR(VIR_ERR_INTERNAL_ERROR, _("Expecting source '%s' of first file-based harddisk to " "be a VMDK image"), disk->src); @@ -3067,7 +3060,6 @@ esxDomainDefineXML(virConnectPtr conn, const char *xml ATTRIBUTE_UNUSED) VIR_FREE(vmx); VIR_FREE(datastoreName); VIR_FREE(directoryName); - VIR_FREE(fileName); VIR_FREE(url); VIR_FREE(datastoreRelatedPath); esxVI_ObjectContent_Free(&virtualMachine); diff --git a/src/esx/esx_storage_driver.c b/src/esx/esx_storage_driver.c index af8876a..d2d8f22 100644 --- a/src/esx/esx_storage_driver.c +++ b/src/esx/esx_storage_driver.c @@ -611,10 +611,8 @@ esxStoragePoolListStorageVolumes(virStoragePoolPtr pool, char **const names, esxVI_HostDatastoreBrowserSearchResults *searchResultsList = NULL; esxVI_HostDatastoreBrowserSearchResults *searchResults = NULL; esxVI_FileInfo *fileInfo = NULL; - char *datastoreName = NULL; - char *directoryName = NULL; - char *fileName = NULL; - char *prefix = NULL; + char *directoryAndFileName = NULL; + int length; int count = 0; int i; @@ -639,40 +637,32 @@ esxStoragePoolListStorageVolumes(virStoragePoolPtr pool, char **const names, /* Interpret search result */ for (searchResults = searchResultsList; searchResults != NULL; searchResults = searchResults->_next) { - VIR_FREE(datastoreName); - VIR_FREE(directoryName); - VIR_FREE(fileName); - VIR_FREE(prefix); + VIR_FREE(directoryAndFileName); - if (esxUtil_ParseDatastorePath(searchResults->folderPath, &datastoreName, - &directoryName, &fileName) < 0) { + if (esxUtil_ParseDatastorePath(searchResults->folderPath, NULL, NULL, + &directoryAndFileName) < 0) { goto cleanup; } - if (directoryName != NULL) { - if (virAsprintf(&prefix, "%s/%s", directoryName, fileName) < 0) { - virReportOOMError(); - goto cleanup; - } - } else { - prefix = strdup(fileName); + /* Strip trailing separators */ + length = strlen(directoryAndFileName); - if (prefix == NULL) { - virReportOOMError(); - goto cleanup; - } + while (length > 0 && directoryAndFileName[length - 1] == '/') { + directoryAndFileName[length - 1] = '\0'; + --length; } + /* Build volume names */ for (fileInfo = searchResults->file; fileInfo != NULL; fileInfo = fileInfo->_next) { - if (*prefix == '\0') { + if (length < 1) { names[count] = strdup(fileInfo->path); if (names[count] == NULL) { virReportOOMError(); goto cleanup; } - } else if (virAsprintf(&names[count], "%s/%s", prefix, + } else if (virAsprintf(&names[count], "%s/%s", directoryAndFileName, fileInfo->path) < 0) { virReportOOMError(); goto cleanup; @@ -694,10 +684,7 @@ esxStoragePoolListStorageVolumes(virStoragePoolPtr pool, char **const names, } esxVI_HostDatastoreBrowserSearchResults_Free(&searchResultsList); - VIR_FREE(datastoreName); - VIR_FREE(directoryName); - VIR_FREE(fileName); - VIR_FREE(prefix); + VIR_FREE(directoryAndFileName); return count; } @@ -744,46 +731,29 @@ esxStorageVolumeLookupByKeyOrPath(virConnectPtr conn, const char *keyOrPath) virStorageVolPtr volume = NULL; esxPrivate *priv = conn->storagePrivateData; char *datastoreName = NULL; - char *directoryName = NULL; - char *fileName = NULL; - char *volumeName = NULL; + char *directoryAndFileName = NULL; esxVI_FileInfo *fileInfo = NULL; if (esxVI_EnsureSession(priv->primary) < 0) { return NULL; } - if (esxUtil_ParseDatastorePath(keyOrPath, &datastoreName, &directoryName, - &fileName) < 0) { + if (esxUtil_ParseDatastorePath(keyOrPath, &datastoreName, NULL, + &directoryAndFileName) < 0) { goto cleanup; } - if (directoryName != NULL) { - if (virAsprintf(&volumeName, "%s/%s", directoryName, fileName) < 0) { - virReportOOMError(); - goto cleanup; - } - } else { - volumeName = strdup(fileName); - - if (volumeName == NULL) { - virReportOOMError(); - goto cleanup; - } - } - if (esxVI_LookupFileInfoByDatastorePath(priv->primary, keyOrPath, &fileInfo, esxVI_Occurrence_RequiredItem) < 0) { goto cleanup; } - volume = virGetStorageVol(conn, datastoreName, volumeName, keyOrPath); + volume = virGetStorageVol(conn, datastoreName, directoryAndFileName, + keyOrPath); cleanup: VIR_FREE(datastoreName); - VIR_FREE(directoryName); - VIR_FREE(fileName); - VIR_FREE(volumeName); + VIR_FREE(directoryAndFileName); esxVI_FileInfo_Free(&fileInfo); return volume; diff --git a/src/esx/esx_util.c b/src/esx/esx_util.c index 7c7c841..08c6c46 100644 --- a/src/esx/esx_util.c +++ b/src/esx/esx_util.c @@ -275,19 +275,19 @@ esxUtil_ParseVirtualMachineIDString(const char *id_string, int *id) int esxUtil_ParseDatastorePath(const char *datastorePath, char **datastoreName, - char **directoryName, char **fileName) + char **directoryName, char **directoryAndFileName) { int result = -1; char *copyOfDatastorePath = NULL; char *tmp = NULL; char *saveptr = NULL; char *preliminaryDatastoreName = NULL; - char *directoryAndFileName = NULL; - char *separator = NULL; + char *preliminaryDirectoryAndFileName = NULL; + char *preliminaryFileName = NULL; - if (datastoreName == NULL || *datastoreName != NULL || - directoryName == NULL || *directoryName != NULL || - fileName == NULL || *fileName != NULL) { + if ((datastoreName != NULL && *datastoreName != NULL) || + (directoryName != NULL && *directoryName != NULL) || + (directoryAndFileName != NULL && *directoryAndFileName != NULL)) { ESX_ERROR(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument")); return -1; } @@ -296,43 +296,46 @@ esxUtil_ParseDatastorePath(const char *datastorePath, char **datastoreName, goto cleanup; } - /* Expected format: '[<datastore>] <path>' */ - if ((tmp = STRSKIP(copyOfDatastorePath, "[")) == NULL || - (preliminaryDatastoreName = strtok_r(tmp, "]", &saveptr)) == NULL || - (directoryAndFileName = strtok_r(NULL, "", &saveptr)) == NULL) { + /* Expected format: '[<datastore>] <path>' where <path> is optional */ + if ((tmp = STRSKIP(copyOfDatastorePath, "[")) == NULL || *tmp == ']' || + (preliminaryDatastoreName = strtok_r(tmp, "]", &saveptr)) == NULL) { ESX_ERROR(VIR_ERR_INTERNAL_ERROR, _("Datastore path '%s' doesn't have expected format " "'[<datastore>] <path>'"), datastorePath); goto cleanup; } - if (esxVI_String_DeepCopyValue(datastoreName, + if (datastoreName != NULL && + esxVI_String_DeepCopyValue(datastoreName, preliminaryDatastoreName) < 0) { goto cleanup; } - directoryAndFileName += strspn(directoryAndFileName, " "); + preliminaryDirectoryAndFileName = strtok_r(NULL, "", &saveptr); - /* Split <path> into <directory>/<file>, where <directory> is optional */ - separator = strrchr(directoryAndFileName, '/'); + if (preliminaryDirectoryAndFileName == NULL) { + preliminaryDirectoryAndFileName = (char *)""; + } else { + preliminaryDirectoryAndFileName += + strspn(preliminaryDirectoryAndFileName, " "); + } + + if (directoryAndFileName != NULL && + esxVI_String_DeepCopyValue(directoryAndFileName, + preliminaryDirectoryAndFileName) < 0) { + goto cleanup; + } - if (separator != NULL) { - *separator++ = '\0'; + if (directoryName != NULL) { + /* Split <path> into <directory>/<file> */ + preliminaryFileName = strrchr(preliminaryDirectoryAndFileName, '/'); - if (*separator == '\0') { - ESX_ERROR(VIR_ERR_INTERNAL_ERROR, - _("Datastore path '%s' doesn't reference a file"), - datastorePath); - goto cleanup; + if (preliminaryFileName != NULL) { + *preliminaryFileName++ = '\0'; } if (esxVI_String_DeepCopyValue(directoryName, - directoryAndFileName) < 0 || - esxVI_String_DeepCopyValue(fileName, separator) < 0) { - goto cleanup; - } - } else { - if (esxVI_String_DeepCopyValue(fileName, directoryAndFileName) < 0) { + preliminaryDirectoryAndFileName) < 0) { goto cleanup; } } @@ -341,9 +344,17 @@ esxUtil_ParseDatastorePath(const char *datastorePath, char **datastoreName, cleanup: if (result < 0) { - VIR_FREE(*datastoreName); - VIR_FREE(*directoryName); - VIR_FREE(*fileName); + if (datastoreName != NULL) { + VIR_FREE(*datastoreName); + } + + if (directoryName != NULL) { + VIR_FREE(*directoryName); + } + + if (directoryAndFileName != NULL) { + VIR_FREE(*directoryAndFileName); + } } VIR_FREE(copyOfDatastorePath); diff --git a/src/esx/esx_util.h b/src/esx/esx_util.h index 5799730..4d92333 100644 --- a/src/esx/esx_util.h +++ b/src/esx/esx_util.h @@ -52,7 +52,7 @@ void esxUtil_FreeParsedUri(esxUtil_ParsedUri **parsedUri); int esxUtil_ParseVirtualMachineIDString(const char *id_string, int *id); int esxUtil_ParseDatastorePath(const char *datastorePath, char **datastoreName, - char **directoryName, char **fileName); + char **directoryName, char **directoryAndFileName); int esxUtil_ResolveHostname(const char *hostname, char *ipAddress, size_t ipAddress_length); diff --git a/src/esx/esx_vi.c b/src/esx/esx_vi.c index 2359e8f..a6950fd 100644 --- a/src/esx/esx_vi.c +++ b/src/esx/esx_vi.c @@ -2946,7 +2946,9 @@ esxVI_LookupFileInfoByDatastorePath(esxVI_Context *ctx, int result = -1; char *datastoreName = NULL; char *directoryName = NULL; + char *directoryAndFileName = NULL; char *fileName = NULL; + int length; char *datastorePathWithoutFileName = NULL; esxVI_String *propertyNameList = NULL; esxVI_ObjectContent *datastore = NULL; @@ -2966,22 +2968,45 @@ esxVI_LookupFileInfoByDatastorePath(esxVI_Context *ctx, } if (esxUtil_ParseDatastorePath(datastorePath, &datastoreName, - &directoryName, &fileName) < 0) { + &directoryName, &directoryAndFileName) < 0) { goto cleanup; } - if (directoryName == NULL) { + if (STREQ(directoryName, directoryAndFileName)) { + /* + * The <path> part of the datatore path didn't contain a '/', assume + * that the <path> part is actually the file name. + */ if (virAsprintf(&datastorePathWithoutFileName, "[%s]", datastoreName) < 0) { virReportOOMError(); goto cleanup; } + + if (esxVI_String_DeepCopyValue(&fileName, directoryAndFileName) < 0) { + goto cleanup; + } } else { if (virAsprintf(&datastorePathWithoutFileName, "[%s] %s", datastoreName, directoryName) < 0) { virReportOOMError(); goto cleanup; } + + length = strlen(directoryName); + + if (directoryAndFileName[length] != '/' || + directoryAndFileName[length + 1] == '\0') { + ESX_VI_ERROR(VIR_ERR_INTERNAL_ERROR, + _("Datastore path '%s' doesn't reference a file"), + datastorePath); + goto cleanup; + } + + if (esxVI_String_DeepCopyValue(&fileName, + directoryAndFileName + length + 1) < 0) { + goto cleanup; + } } /* Lookup HostDatastoreBrowser */ @@ -3087,6 +3112,7 @@ esxVI_LookupFileInfoByDatastorePath(esxVI_Context *ctx, VIR_FREE(datastoreName); VIR_FREE(directoryName); + VIR_FREE(directoryAndFileName); VIR_FREE(fileName); VIR_FREE(datastorePathWithoutFileName); esxVI_String_Free(&propertyNameList); diff --git a/tests/esxutilstest.c b/tests/esxutilstest.c index 09470ed..4906ad4 100644 --- a/tests/esxutilstest.c +++ b/tests/esxutilstest.c @@ -99,14 +99,18 @@ struct testPath { int result; const char *datastoreName; const char *directoryName; - const char *fileName; + const char *directoryAndFileName; }; static struct testPath paths[] = { - { "[datastore] directory/file", 0, "datastore", "directory", "file" }, - { "[datastore] file", 0, "datastore", NULL, "file" }, + { "[datastore] directory/file", 0, "datastore", "directory", + "directory/file" }, + { "[datastore] directory1/directory2/file", 0, "datastore", + "directory1/directory2", "directory1/directory2/file" }, + { "[datastore] file", 0, "datastore", "file", "file" }, + { "[datastore] directory/", 0, "datastore", "directory", "directory/" }, + { "[datastore]", 0, "datastore", "", "" }, { "[] directory/file", -1, NULL, NULL, NULL }, - { "[datastore] directory/", -1, NULL, NULL, NULL }, { "directory/file", -1, NULL, NULL, NULL }, }; @@ -116,16 +120,16 @@ testParseDatastorePath(const void *data ATTRIBUTE_UNUSED) int i, result = 0; char *datastoreName = NULL; char *directoryName = NULL; - char *fileName = NULL; + char *directoryAndFileName = NULL; for (i = 0; i < ARRAY_CARDINALITY(paths); ++i) { VIR_FREE(datastoreName); VIR_FREE(directoryName); - VIR_FREE(fileName); + VIR_FREE(directoryAndFileName); - if (esxUtil_ParseDatastorePath(paths[i].datastorePath, - &datastoreName, &directoryName, - &fileName) != paths[i].result) { + if (esxUtil_ParseDatastorePath + (paths[i].datastorePath, &datastoreName, &directoryName, + &directoryAndFileName) != paths[i].result) { goto failure; } @@ -138,14 +142,14 @@ testParseDatastorePath(const void *data ATTRIBUTE_UNUSED) goto failure; } - if (paths[i].directoryName != NULL && - STRNEQ(paths[i].directoryName, directoryName)) { + if (STRNEQ(paths[i].directoryName, directoryName)) { virtTestDifference(stderr, paths[i].directoryName, directoryName); goto failure; } - if (STRNEQ(paths[i].fileName, fileName)) { - virtTestDifference(stderr, paths[i].fileName, fileName); + if (STRNEQ(paths[i].directoryAndFileName, directoryAndFileName)) { + virtTestDifference(stderr, paths[i].directoryAndFileName, + directoryAndFileName); goto failure; } } @@ -153,7 +157,7 @@ testParseDatastorePath(const void *data ATTRIBUTE_UNUSED) cleanup: VIR_FREE(datastoreName); VIR_FREE(directoryName); - VIR_FREE(fileName); + VIR_FREE(directoryAndFileName); return result; diff --git a/tests/xml2vmxtest.c b/tests/xml2vmxtest.c index eed3ac0..71ce14a 100644 --- a/tests/xml2vmxtest.c +++ b/tests/xml2vmxtest.c @@ -148,24 +148,18 @@ testFormatVMXFileName(const char *src, void *opaque ATTRIBUTE_UNUSED) { bool success = false; char *datastoreName = NULL; - char *directoryName = NULL; - char *fileName = NULL; + char *directoryAndFileName = NULL; char *absolutePath = NULL; if (STRPREFIX(src, "[")) { /* Found potential datastore path */ - if (esxUtil_ParseDatastorePath(src, &datastoreName, &directoryName, - &fileName) < 0) { + if (esxUtil_ParseDatastorePath(src, &datastoreName, NULL, + &directoryAndFileName) < 0) { goto cleanup; } - if (directoryName == NULL) { - virAsprintf(&absolutePath, "/vmfs/volumes/%s/%s", datastoreName, - fileName); - } else { - virAsprintf(&absolutePath, "/vmfs/volumes/%s/%s/%s", datastoreName, - directoryName, fileName); - } + virAsprintf(&absolutePath, "/vmfs/volumes/%s/%s", datastoreName, + directoryAndFileName); } else if (STRPREFIX(src, "/")) { /* Found absolute path */ absolutePath = strdup(src); @@ -182,8 +176,7 @@ testFormatVMXFileName(const char *src, void *opaque ATTRIBUTE_UNUSED) } VIR_FREE(datastoreName); - VIR_FREE(directoryName); - VIR_FREE(fileName); + VIR_FREE(directoryAndFileName); return absolutePath; } -- 1.7.0.4
On 08/26/2010 04:37 PM, Matthias Bolte wrote:
Instead of splitting the path part of a datastore path into directory and file name, keep this in one piece. An example:
"[datastore] directory/file"
was split into this before:
datastoreName = "datastore" directoryName = "directory" fileName = "file"
Now it's split into this:
datastoreName = "datastore" directoryName = "directory" directoryAndFileName = "directory/file"
Seems reasonable.
This simplifies code using esxUtil_ParseDatastorePath, because directoryAndFileName is used more often than fileName. Also the old approach expected the datastore path to reference a actual
s/ a / an /
@@ -52,8 +52,7 @@ typedef struct _esxVMX_Data esxVMX_Data;
struct _esxVMX_Data { esxVI_Context *ctx; - const char *datastoreName; - const char *directoryName; + char *datastorePathWithoutFileName;
I guess losing the const here is okay.
@@ -2612,8 +2608,9 @@ esxDomainDumpXML(virDomainPtr domain, int flags) esxVI_ObjectContent_Free(&virtualMachine); VIR_FREE(datastoreName); VIR_FREE(directoryName); - VIR_FREE(fileName); + VIR_FREE(directoryAndFileName); VIR_FREE(url); + VIR_FREE(data.datastorePathWithoutFileName); VIR_FREE(vmx); virDomainDefFree(def);
@@ -2640,8 +2637,7 @@ esxDomainXMLFromNative(virConnectPtr conn, const char *nativeFormat, }
data.ctx = priv->primary; - data.datastoreName = "?"; - data.directoryName = "?"; + data.datastorePathWithoutFileName = (char *)"[?] ?";
Are you missing a strdup() here? I'm worried that the VIR_FREE(data.datastorePathWithoutFileName) in esxDomainDumpXML will now try to free static storage.
diff --git a/src/esx/esx_storage_driver.c b/src/esx/esx_storage_driver.c index af8876a..d2d8f22 100644 --- a/src/esx/esx_storage_driver.c +++ b/src/esx/esx_storage_driver.c @@ -611,10 +611,8 @@ esxStoragePoolListStorageVolumes(virStoragePoolPtr pool, char **const names, esxVI_HostDatastoreBrowserSearchResults *searchResultsList = NULL; esxVI_HostDatastoreBrowserSearchResults *searchResults = NULL; esxVI_FileInfo *fileInfo = NULL; - char *datastoreName = NULL; - char *directoryName = NULL; - char *fileName = NULL; - char *prefix = NULL; + char *directoryAndFileName = NULL; + int length;
s/int/size_t/
+++ b/src/esx/esx_vi.c @@ -2946,7 +2946,9 @@ esxVI_LookupFileInfoByDatastorePath(esxVI_Context *ctx, int result = -1; char *datastoreName = NULL; char *directoryName = NULL; + char *directoryAndFileName = NULL; char *fileName = NULL; + int length;
s/int/size_t/ -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
2010/9/1 Eric Blake <eblake@redhat.com>:
On 08/26/2010 04:37 PM, Matthias Bolte wrote:
Instead of splitting the path part of a datastore path into directory and file name, keep this in one piece. An example:
"[datastore] directory/file"
was split into this before:
datastoreName = "datastore" directoryName = "directory" fileName = "file"
Now it's split into this:
datastoreName = "datastore" directoryName = "directory" directoryAndFileName = "directory/file"
Seems reasonable.
This simplifies code using esxUtil_ParseDatastorePath, because directoryAndFileName is used more often than fileName. Also the old approach expected the datastore path to reference a actual
s/ a / an /
I'll fix that before pushing.
@@ -52,8 +52,7 @@ typedef struct _esxVMX_Data esxVMX_Data;
struct _esxVMX_Data { esxVI_Context *ctx; - const char *datastoreName; - const char *directoryName; + char *datastorePathWithoutFileName;
I guess losing the const here is okay.
It should have never been const in the first place.
@@ -2612,8 +2608,9 @@ esxDomainDumpXML(virDomainPtr domain, int flags) esxVI_ObjectContent_Free(&virtualMachine); VIR_FREE(datastoreName); VIR_FREE(directoryName); - VIR_FREE(fileName); + VIR_FREE(directoryAndFileName); VIR_FREE(url); + VIR_FREE(data.datastorePathWithoutFileName); VIR_FREE(vmx); virDomainDefFree(def);
@@ -2640,8 +2637,7 @@ esxDomainXMLFromNative(virConnectPtr conn, const char *nativeFormat, }
data.ctx = priv->primary; - data.datastoreName = "?"; - data.directoryName = "?"; + data.datastorePathWithoutFileName = (char *)"[?] ?";
Are you missing a strdup() here? I'm worried that the VIR_FREE(data.datastorePathWithoutFileName) in esxDomainDumpXML will now try to free static storage.
This is in esxDomainXMLFromNative, data.datastorePathWithoutFileName doesn't get freed here, so not strdup'ing is fine here. In esxDomainDumpXML data.datastorePathWithoutFileName is allocated via virAsprintf and therefore it need to be freed in esxDomainDumpXML. So nothing to change here.
diff --git a/src/esx/esx_storage_driver.c b/src/esx/esx_storage_driver.c index af8876a..d2d8f22 100644 --- a/src/esx/esx_storage_driver.c +++ b/src/esx/esx_storage_driver.c @@ -611,10 +611,8 @@ esxStoragePoolListStorageVolumes(virStoragePoolPtr pool, char **const names, esxVI_HostDatastoreBrowserSearchResults *searchResultsList = NULL; esxVI_HostDatastoreBrowserSearchResults *searchResults = NULL; esxVI_FileInfo *fileInfo = NULL; - char *datastoreName = NULL; - char *directoryName = NULL; - char *fileName = NULL; - char *prefix = NULL; + char *directoryAndFileName = NULL; + int length;
s/int/size_t/
+++ b/src/esx/esx_vi.c @@ -2946,7 +2946,9 @@ esxVI_LookupFileInfoByDatastorePath(esxVI_Context *ctx, int result = -1; char *datastoreName = NULL; char *directoryName = NULL; + char *directoryAndFileName = NULL; char *fileName = NULL; + int length;
s/int/size_t/
I'll fix those two before pushing. Matthias
On 09/02/2010 04:00 PM, Matthias Bolte wrote:
Are you missing a strdup() here? I'm worried that the VIR_FREE(data.datastorePathWithoutFileName) in esxDomainDumpXML will now try to free static storage.
This is in esxDomainXMLFromNative, data.datastorePathWithoutFileName doesn't get freed here, so not strdup'ing is fine here.
In esxDomainDumpXML data.datastorePathWithoutFileName is allocated via virAsprintf and therefore it need to be freed in esxDomainDumpXML.
So nothing to change here.
Okay; your explanations make sense. ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
2010/9/3 Eric Blake <eblake@redhat.com>:
On 09/02/2010 04:00 PM, Matthias Bolte wrote:
Are you missing a strdup() here? I'm worried that the VIR_FREE(data.datastorePathWithoutFileName) in esxDomainDumpXML will now try to free static storage.
This is in esxDomainXMLFromNative, data.datastorePathWithoutFileName doesn't get freed here, so not strdup'ing is fine here.
In esxDomainDumpXML data.datastorePathWithoutFileName is allocated via virAsprintf and therefore it need to be freed in esxDomainDumpXML.
So nothing to change here.
Okay; your explanations make sense.
ACK.
Thanks, pushed. Matthias
participants (2)
-
Eric Blake -
Matthias Bolte