On 2/28/19 10:42 AM, Laszlo Ersek wrote:
On 02/27/19 11:04, Michal Privoznik wrote:
Test firmware description parsing so far.
Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
---
tests/Makefile.am | 9 ++++
tests/qemufirmwaredata/40-bios.json | 35 +++++++++++++
tests/qemufirmwaredata/50-ovmf-sb.json | 36 +++++++++++++
tests/qemufirmwaredata/60-ovmf.json | 35 +++++++++++++
tests/qemufirmwaredata/70-aavmf.json | 35 +++++++++++++
tests/qemufirmwaretest.c | 70 ++++++++++++++++++++++++++
6 files changed, 220 insertions(+)
create mode 100644 tests/qemufirmwaredata/40-bios.json
create mode 100644 tests/qemufirmwaredata/50-ovmf-sb.json
create mode 100644 tests/qemufirmwaredata/60-ovmf.json
create mode 100644 tests/qemufirmwaredata/70-aavmf.json
create mode 100644 tests/qemufirmwaretest.c
If the test data files added in this patch are from the examples in
QEMU's "docs/interop/firmware.json", I suggest stating so in the commit
message, also identifying the QEMU commit that added those examples.
It's a mixture. Some files (50-ovmf-sb.json and 60-ovmf.json) were taken
from OVMF package from RHEL-7 and some were taken from firmware.json
which has some examples in the comments. I'll put that into the commit
message.
In addition, I wonder if the filenames should carry the double-digit
priority prefixes at once in this patch.
Even if they do, I'm thinking that aavmf shouldn't be ordered (by
priority prefix) behind ovmf, given that the qemu targets / machine
types they are suitable for are mutually exclusive.
At this point, the code doesn't care about priority just yet. It is
implemented in next patches. But I'm not sure I understand what you
mean. You mean that 70-aavmf.json should be renamed to just 'aavmf.json'?
Other than that, I have two superficial comments (questions) below,
because my knowledge of the libvirt test infrastructure is nonexistent
to minimal:
diff --git a/tests/qemufirmwaretest.c b/tests/qemufirmwaretest.c
new file mode 100644
index 0000000000..0c5fb1e55a
--- /dev/null
+++ b/tests/qemufirmwaretest.c
@@ -0,0 +1,70 @@
+#include <config.h>
+
+#include "testutils.h"
+#include "qemu/qemu_firmware.h"
+
+#define VIR_FROM_THIS VIR_FROM_QEMU
+
+static int
+testParseFormatFW(const void *opaque)
+{
+ const char *filename = opaque;
+ VIR_AUTOFREE(char *) path = NULL;
+ VIR_AUTOPTR(qemuFirmware) fw = NULL;
+ VIR_AUTOFREE(char *) buf = NULL;
+ VIR_AUTOPTR(virJSONValue) json = NULL;
+ VIR_AUTOFREE(char *) expected = NULL;
+ VIR_AUTOFREE(char *) actual = NULL;
+
+ if (virAsprintf(&path, "%s/qemufirmwaredata/%s",
+ abs_srcdir, filename) < 0)
+ return -1;
+
+ if (!(fw = qemuFirmwareParse(path)))
+ return -1;
+
+ if (virFileReadAll(path,
+ 1024 * 1024, /* 1MiB */
+ &buf) < 0)
+ return -1;
+
+ if (!(json = virJSONValueFromString(buf)))
+ return -1;
+
+ /* Description and tags are not parsed. */
+ if (virJSONValueObjectRemoveKey(json, "description", NULL) < 0 ||
+ virJSONValueObjectRemoveKey(json, "tags", NULL) < 0)
+ return -1;
+
+ if (!(expected = virJSONValueToString(json, true)))
+ return -1;
+
+ if (!(actual = qemuFirmwareFormat(fw)))
+ return -1;
+
+ return virTestCompareToString(expected, actual);
+}
Can you add a comment to the function about the general idea? Or is this
approach common for libvirt tests? I mean, to make heads or tails of
this function, I have to decipher what each step does, and what the
final comparison compares. It would be easier to read if you explained
the steps in a comment, and then we'd only have to verify the steps
against that documentation.
Okay. Long story short, this test tries to ensure that firmware parsing
code does parse given files correctly. qemuFirmwareParse() parses JSON
into some internal structre and then qemuFirmwareFormat() formats the
structure back into a JSON string. This output should be identical to
taking the JSON file and removing "description" and "tags" which are not
meant to be parsed.
My other question: when does virJSONValueObjectRemoveKey() fail? I
assume it doesn't fail when the key isn't present.
The function removes 'key:pair' from a JSON object. But it has to really
be object, not anything else. With our internal JSON APIs it is possible
to get poitner to virtually anything within a JSON object. For instance,
if you'd have the following document:
{
"arr": [1,2,3]
}
it is possible to obtain a pointer to the array [1,2,3]. Obviously,
RemoveKey() should fail if such pointer would be pased. But if *p is
poitner to the whole JSON document then virJSONValueObjectRemoveKey(p,
"arr", NULL) should not fail and leave us with empty document.
No, the function doesn't fail if key is not found.
Michal
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
