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

Martin Kletzander posted 5 patches 5 years, 1 month ago
There is a newer version of this series
[libvirt PATCH v2 3/5] vmx: Make virVMXParseFileName return an integer
Posted by Martin Kletzander 5 years, 1 month ago
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

Re: [libvirt PATCH v2 3/5] vmx: Make virVMXParseFileName return an integer
Posted by Daniel Henrique Barboza 5 years, 1 month ago
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
> 

Re: [libvirt PATCH v2 3/5] vmx: Make virVMXParseFileName return an integer
Posted by Michal Privoznik 5 years, 1 month ago
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