[libvirt] [PATCH v1 08/15] test: Introduce qemufirmwaretest

[Michal Privoznik]

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 at redhat.com>
>> ---
>>   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