date:20201221

Re: RFC PATCH: Issue 90 (Test Clarification)

2020-12-21 Thread Ryan Gahagan

We were able to get the testing environment running after some
trial-and-error. The issue was that the version of libtirpc was out of
date, causing an error in meson.build when the XDR dependency was being
read. By updating the libtirpc-dev package we were able to start building
locally. However, we did have some questions about our tests. For now we
wanted to focus specifically on the qemublocktest.c file.

We have the function virDomainDiskSourceNetworkParse in
src/conf/domain_conf.c where XML is parsed into virStorageSource data and
the function qemuBlockStorageSourceCreateGetStorageProps under
src/qemu/qemu_block.c where this data is converted into a JSON object. Our
understanding of the tests/qemublocktest.c file is that we provide an XML
string example via TEST_JSON_FORMAT_NET and that this string was parsed
into JSON then formatted back into XML to make sure the process produced
the same output as the input. One issue is that for some reason our user
and group strings (i.e. “”) are not getting
parsed into integers. The nfs_uid and nfs_gid fields are never set and
therefore result in a 0 every time. We have code to fix this in a method
under qemuDomainPrepareStorageSourceBlockdev in the file
src/qemu/qemu_domain.c which looks up the user and group and stores the
values we expect, but this method isn’t getting called. Where in this
process have we missed something? Should we move the virGetUserID and
virGetGroupID method calls to somewhere else?
We were also curious about the methods which parse the JSON values back
into data. In our case, this is virStorageSourceParseBackingJSONNFS in
src/util/virstoragefile.c. This method tries to retrieve the user and group
integers from the NFS JSON object. However, when constructing the JSON
object in the getStorageProps method we don't actually add those values if
they're "default" values. In JSON terms this means that the integers are
undefined, but how does C interpret it? Does the retrieve method return 0
or simply fail?

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

2020-12-21 Thread Martin Kletzander

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 
---
 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//' */
@@ -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, virDomainXMLOptionP

[libvirt PATCH v2 1/5] esx: Unindent unnecessary conditional branch

2020-12-21 Thread Martin Kletzander

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 
---
 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//' */
-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//'"), 
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;

[libvirt PATCH v2 5/5] vmx: Treat missing cdrom-image as empty drive

2020-12-21 Thread Martin Kletzander

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

[libvirt PATCH v2 0/5] vmx: Don't error out on missing filename for cdrom

2020-12-21 Thread Martin Kletzander

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

[libvirt PATCH v2 2/5] tests: Use g_autofree in testParseVMXFileName

2020-12-21 Thread Martin Kletzander

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

[libvirt PATCH v2 4/5] vmx: Allow missing cdrom image file in virVMXParseFileName

2020-12-21 Thread Martin Kletzander

This will be used later.

Signed-off-by: Martin Kletzander 
---
 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)

Re: [libvirt PATCH] vmx: Don't error out on missing filename for cdrom

2020-12-21 Thread Martin Kletzander


On Mon, Dec 21, 2020 at 01:09:13PM +0100, Ján Tomko wrote:

On a Monday in 2020, Martin Kletzander wrote:

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

Signed-off-by: Martin Kletzander 
---
src/vmx/vmx.c | 14 +++---
1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
index b86dbe9ca267..40e4ef962992 100644
--- a/src/vmx/vmx.c
+++ b/src/vmx/vmx.c
@@ -2447,10 +2447,18 @@ virVMXParseDisk(virVMXContext *ctx, 
virDomainXMLOptionPtr xmlopt, virConfPtr con
goto cleanup;
}

+tmp = ctx->parseFileName(fileName, ctx->opaque);
virDomainDiskSetType(*def, VIR_STORAGE_TYPE_FILE);
-if (!(tmp = ctx->parseFileName(fileName, ctx->opaque)))
-goto cleanup;
-virDomainDiskSetSource(*def, tmp);
+/* It is easily possible to have a cdrom with non-existing filename
+ * as the image and vmware just provides an empty cdrom.
+ *
+ * See: https://bugzilla.redhat.com/1903953
+ */
+if (tmp) {
+virDomainDiskSetSource(*def, tmp);
+} else {
+virResetLastError();


Using virResetLastError elsewhere than beginning of a libvirt API is
suspicious. If it's not an error, we should not have logged it in the
first place.



Yeah, I did not like that either, but esx, you know...

Since it's you, asking co nicely, I'll give it another shot.


Jano


+}
VIR_FREE(tmp);
} else if (deviceType && STRCASEEQ(deviceType, "atapi-cdrom")) {
virDomainDiskSetType(*def, VIR_STORAGE_TYPE_BLOCK);
--
2.29.2






signature.asc
Description: PGP signature

Re: [libvirt PATCH] vmx: Don't error out on missing filename for cdrom

2020-12-21 Thread Tim Wiederhake

On Mon, 2020-12-21 at 12:48 +0100, Martin Kletzander wrote:
> 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
> 
> Signed-off-by: Martin Kletzander 

Reviewed-by: Tim Wiederhake 

> ---
>  src/vmx/vmx.c | 14 +++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
> index b86dbe9ca267..40e4ef962992 100644
> --- a/src/vmx/vmx.c
> +++ b/src/vmx/vmx.c
> @@ -2447,10 +2447,18 @@ virVMXParseDisk(virVMXContext *ctx,
> virDomainXMLOptionPtr xmlopt, virConfPtr con
>  goto cleanup;
>  }
>  
> +tmp = ctx->parseFileName(fileName, ctx->opaque);
>  virDomainDiskSetType(*def, VIR_STORAGE_TYPE_FILE);
> -if (!(tmp = ctx->parseFileName(fileName, ctx->opaque)))
> -goto cleanup;
> -virDomainDiskSetSource(*def, tmp);
> +/* It is easily possible to have a cdrom with non-
> existing filename
> + * as the image and vmware just provides an empty cdrom.
> + *
> + * See: https://bugzilla.redhat.com/1903953
> + */
> +if (tmp) {
> +virDomainDiskSetSource(*def, tmp);
> +} else {
> +virResetLastError();
> +}
>  VIR_FREE(tmp);
>  } else if (deviceType && STRCASEEQ(deviceType, "atapi-
> cdrom")) {
>  virDomainDiskSetType(*def, VIR_STORAGE_TYPE_BLOCK);

Re: [libvirt PATCH] vmx: Don't error out on missing filename for cdrom

2020-12-21 Thread Ján Tomko


On a Monday in 2020, Martin Kletzander wrote:

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

Signed-off-by: Martin Kletzander 
---
src/vmx/vmx.c | 14 +++---
1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
index b86dbe9ca267..40e4ef962992 100644
--- a/src/vmx/vmx.c
+++ b/src/vmx/vmx.c
@@ -2447,10 +2447,18 @@ virVMXParseDisk(virVMXContext *ctx, 
virDomainXMLOptionPtr xmlopt, virConfPtr con
goto cleanup;
}

+tmp = ctx->parseFileName(fileName, ctx->opaque);
virDomainDiskSetType(*def, VIR_STORAGE_TYPE_FILE);
-if (!(tmp = ctx->parseFileName(fileName, ctx->opaque)))
-goto cleanup;
-virDomainDiskSetSource(*def, tmp);
+/* It is easily possible to have a cdrom with non-existing filename
+ * as the image and vmware just provides an empty cdrom.
+ *
+ * See: https://bugzilla.redhat.com/1903953
+ */
+if (tmp) {
+virDomainDiskSetSource(*def, tmp);
+} else {
+virResetLastError();


Using virResetLastError elsewhere than beginning of a libvirt API is
suspicious. If it's not an error, we should not have logged it in the
first place.

Jano


+}
VIR_FREE(tmp);
} else if (deviceType && STRCASEEQ(deviceType, "atapi-cdrom")) {
virDomainDiskSetType(*def, VIR_STORAGE_TYPE_BLOCK);
--
2.29.2



signature.asc
Description: PGP signature

Re: [libvirt PATCH] docs: Fix dead link

2020-12-21 Thread Martin Kletzander


On Mon, Dec 21, 2020 at 09:01:36AM +0100, Tim Wiederhake wrote:

Signed-off-by: Tim Wiederhake 


Reviewed-by: Martin Kletzander 


---
docs/coding-style.rst | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/coding-style.rst b/docs/coding-style.rst
index b3ac070fac..55dfa196a2 100644
--- a/docs/coding-style.rst
+++ b/docs/coding-style.rst
@@ -939,7 +939,7 @@ ok:
Although libvirt does not encourage the Linux kernel wind/unwind
style of multiple labels, there's a good general discussion of the
issue archived at
-`KernelTrap `__
+`KernelTrap 
`__

When using goto, please use one of these standard labels if it
makes sense:
--
2.26.2



signature.asc
Description: PGP signature

[libvirt PATCH] vmx: Don't error out on missing filename for cdrom

2020-12-21 Thread Martin Kletzander

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

Signed-off-by: Martin Kletzander 
---
 src/vmx/vmx.c | 14 +++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
index b86dbe9ca267..40e4ef962992 100644
--- a/src/vmx/vmx.c
+++ b/src/vmx/vmx.c
@@ -2447,10 +2447,18 @@ virVMXParseDisk(virVMXContext *ctx, 
virDomainXMLOptionPtr xmlopt, virConfPtr con
 goto cleanup;
 }
 
+tmp = ctx->parseFileName(fileName, ctx->opaque);
 virDomainDiskSetType(*def, VIR_STORAGE_TYPE_FILE);
-if (!(tmp = ctx->parseFileName(fileName, ctx->opaque)))
-goto cleanup;
-virDomainDiskSetSource(*def, tmp);
+/* It is easily possible to have a cdrom with non-existing filename
+ * as the image and vmware just provides an empty cdrom.
+ *
+ * See: https://bugzilla.redhat.com/1903953
+ */
+if (tmp) {
+virDomainDiskSetSource(*def, tmp);
+} else {
+virResetLastError();
+}
 VIR_FREE(tmp);
 } else if (deviceType && STRCASEEQ(deviceType, "atapi-cdrom")) {
 virDomainDiskSetType(*def, VIR_STORAGE_TYPE_BLOCK);
-- 
2.29.2

Re: RFC PATCH: Issue 90 (Test Clarification)

2020-12-21 Thread Peter Krempa

On Fri, Dec 18, 2020 at 15:14:01 -0600, Ryan Gahagan wrote:
> On Thu, Dec 17, 2020 at 8:00 AM Peter Krempa  wrote:
> 
> > Also note that the upstream test-suite run in the CI does actually
> > provide the expected output. Obviously you can't use the ENV variable to
> > automatically overwrite your files, but you certainly can copy out the
> > diffs from the CI. Just commit empty files in place for the output files
> > and the CI will complain that they differ including the full difference.
> >
> 
> We tried doing this and we failed on CI, but the pipeline jobs didn't
> output a diff, and instead simply errored out with the message "Full log
> written to
> /builds/bschoney/libvirt/build/meson-private/dist-build/meson-logs/testlog.txt".
> How can we actually view the diff against our blank file? We're trying to
> find the output for the qemuxml2argvdata args file for our new test but
> it's not being printed anywhere.

You can actually see some of the test output in your CI pipeline runs:

https://gitlab.com/bschoney/libvirt/-/jobs/922060289#L2961

anyways, it's not worth for me to debug why some of the output might be
skipped (perhaps missing empty output files).

Please make sure your local env is able to build libvirt, so that you
can use VIR_TEST_REGENERATE_OUTPUT. That is certainly the simplest way.
In addition you certainly want to test your changes in action, and our
CI AFAIK doesn't provide built packages.

[libvirt PATCH] docs: Fix dead link

2020-12-21 Thread Tim Wiederhake

Signed-off-by: Tim Wiederhake 
---
 docs/coding-style.rst | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/coding-style.rst b/docs/coding-style.rst
index b3ac070fac..55dfa196a2 100644
--- a/docs/coding-style.rst
+++ b/docs/coding-style.rst
@@ -939,7 +939,7 @@ ok:
 Although libvirt does not encourage the Linux kernel wind/unwind
 style of multiple labels, there's a good general discussion of the
 issue archived at
-`KernelTrap `__
+`KernelTrap 
`__
 
 When using goto, please use one of these standard labels if it
 makes sense:
-- 
2.26.2

Re: RFC PATCH: Issue 90 (Test Clarification)

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

[libvirt PATCH v2 1/5] esx: Unindent unnecessary conditional branch

[libvirt PATCH v2 5/5] vmx: Treat missing cdrom-image as empty drive

[libvirt PATCH v2 0/5] vmx: Don't error out on missing filename for cdrom

[libvirt PATCH v2 2/5] tests: Use g_autofree in testParseVMXFileName

[libvirt PATCH v2 4/5] vmx: Allow missing cdrom image file in virVMXParseFileName

Re: [libvirt PATCH] vmx: Don't error out on missing filename for cdrom

Re: [libvirt PATCH] vmx: Don't error out on missing filename for cdrom

Re: [libvirt PATCH] vmx: Don't error out on missing filename for cdrom

Re: [libvirt PATCH] docs: Fix dead link

[libvirt PATCH] vmx: Don't error out on missing filename for cdrom

Re: RFC PATCH: Issue 90 (Test Clarification)

[libvirt PATCH] docs: Fix dead link

14 matches

Site Navigation

Mail list logo

Footer information