On 03/09/19 15:32, Markus Armbruster wrote:
> Laszlo Ersek <ler...@redhat.com> writes:
>> (5) Most importantly: I don't think we need this patch.
>>
>> First, AFAICS, the unhex function is never used in the series, and no
>> unit test is being added for it. That makes it a bad candidate for
>> "include/qemu/cutils.h".
>>
>> Second, while the hex function is used in PATCH v2 13/18
>> ("hw/nvram/fw_cfg: Add QMP 'info fw_cfg' command"), the documentation in
>> that patch and the logic in the patch are inconsistent. The
>> documentation -- i.e. both the commit message and the "misc.json" change
>> -- say that "FirmwareConfigurationItem.data" is unused (not populated).
>> However, that's exactly what create_qmp_fw_cfg_item() uses the hex
>> function for.
>>
>> Third, if we do decide that the QMP command should output the fw_cfg
>> binary data, then the QMP tradition (to my knowledge) has been to use
>> base64 encoding. GLib provides helpers for base64:
>>
>> https://developer.gnome.org/glib/stable/glib-Base64-Encoding.html
>>
>> and you can see examples of it being used in e.g.
>>
>> (a) qmp_ringbuf_read() [chardev/char-ringbuf.c] -- the @ringbuf-read
>> command is defined in "qapi/char.json"
>>
>> (b) qmp_guest_exec_status() [qga/commands.c] -- the @guest-exec-status
>> command is defined in "qga/qapi-schema.json".
>
> Yes. I wish you had wrote that first, saving me the trouble of looking
> at the patch.
>
You are right, I'm sorry! :(
Laszlo
