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

Daniel Henrique Barboza danielhb413 at gmail.com
Mon Jan 4 23:32:35 UTC 2021


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 at 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
> 




More information about the libvir-list mailing list