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

[Laszlo Ersek]

On 03/05/19 15:38, Michal Privoznik wrote:
> 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'?

My first suggestion is to drop the prio prefixes (in this patch only).

If you prefer to keep them, then my second suggestion is to give the
same prio to the sole aavmf descriptor as to the main (highest prio)
ovmf descriptor.

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

Ah, that's what I missed, i.e. how virJSONValueObjectRemoveKey() worked.

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

Thanks for explaining.

Laszlo