Re: [libvirt PATCH v2 3/5] vmx: Make virVMXParseFileName return an integer

[Daniel Henrique Barboza]

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 <mklet...@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