[libvirt] [PATCH V1 1/6] Add QMP probing for TPM

[Stefan Berger]

On 03/14/2013 10:20 AM, Daniel P. Berrange wrote:
> On Wed, Mar 13, 2013 at 12:03:49PM -0400, Stefan Berger wrote:
>> Probe for QEMU's QMP TPM support by querying the lists of
>> supported TPM models (query-tpm-models) and backend types
>> (query-tpm-types).
>>
>> The setting of the capability flags following the strings
>> returned from the commands above is only provided in the
>> patch where domain_conf.c gets TPM support due to dependencies
>> on functions only introduced there.
>>
>> Signed-off-by: Stefan Berger <stefanb at linux.vnet.ibm.com>
>>
>> ---
>>   src/libvirt_private.syms     |    1
>>   src/qemu/qemu_capabilities.c |   61 ++++++++++++++++++++++++++++
>>   src/qemu/qemu_capabilities.h |    3 +
>>   src/qemu/qemu_monitor.c      |   44 ++++++++++++++++++++
>>   src/qemu/qemu_monitor.h      |    6 ++
>>   src/qemu/qemu_monitor_json.c |   91 +++++++++++++++++++++++++++++++++++++++++++
>>   src/qemu/qemu_monitor_json.h |    8 +++
>>   src/util/virutil.c           |   14 ++++++
>>   src/util/virutil.h           |    3 +
>>   9 files changed, 231 insertions(+)
>>
>> Index: libvirt/src/qemu/qemu_capabilities.c
>> ===================================================================
>> --- libvirt.orig/src/qemu/qemu_capabilities.c
>> +++ libvirt/src/qemu/qemu_capabilities.c
>> @@ -210,6 +210,8 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAS
>>   
>>                 "rng-random", /* 130 */
>>                 "rng-egd",
>> +              "tpm-passthrough",
>> +              "tpm-tis",
>>       );
>>   
>>   struct _virQEMUCaps {
>> Index: libvirt/src/qemu/qemu_capabilities.h
>> ===================================================================
>> --- libvirt.orig/src/qemu/qemu_capabilities.h
>> +++ libvirt/src/qemu/qemu_capabilities.h
>> @@ -171,6 +171,8 @@ enum virQEMUCapsFlags {
>>       QEMU_CAPS_OBJECT_RNG_RANDOM  = 130, /* the rng-random backend for
>>                                              virtio rng */
>>       QEMU_CAPS_OBJECT_RNG_EGD     = 131, /* EGD protocol daemon for rng */
>> +    QEMU_CAPS_DEVICE_TPM_PASSTHROUGH = 132, /* -tpmdev passthrough */
>> +    QEMU_CAPS_DEVICE_TPM_TIS     = 133, /* -device tpm_tis */
> I'm wondering whether we really need to spend 2 capability bits on
> this. Is it possible to build QEMU such that we have one, but not
> the other ? If not then one capability would suffice

I had it with a single capability bit QEMU_CAPS_DEVICE_TPM *and* was 
reading the strings about the supported models and backend types from 
QEMU via QMP and held them in qemuCaps. When building the command line I 
then compared the intended model and backend type to be used against the 
list held in qemuCaps. That went fine until I wrote the test. There we 
end up using a pseudo executable (/usr/bin/qemu or so) as well and the 
test failed because it wasn't probed for the models and types. So I 
ended up changing the code so that I can set those bits inside the tests 
rather than having to set the needed strings in the array.

Presumably query-tpm-models and query-tpm-lists allows us to report an 
error from libvirt about non-availability of a model/backend type rather 
than having QEMU report an error which we reflect to the user.


+
+static int
+qemuMonitorJSONGetStringArray(qemuMonitorPtr mon, const char *qmpCmd,
+                              char ***array)
+{
+    int ret;
+    virJSONValuePtr cmd;
+    virJSONValuePtr reply = NULL;
+    virJSONValuePtr data;
+    char **list = NULL;
+    int n = 0;
+    size_t i;
+
+    *array = NULL;
+
+    if (!(cmd = qemuMonitorJSONMakeCommand(qmpCmd, NULL)))
+        return -1;
+
+    ret = qemuMonitorJSONCommand(mon, cmd, &reply);
+
+    if (ret == 0)
+        ret = qemuMonitorJSONCheckError(cmd, reply);
+
+    if (ret < 0)
+        goto cleanup;
+
+    ret = -1;
+
+    if (!(data = virJSONValueObjectGet(reply, "return"))) {
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       _("%s reply was missing return data"),
+                       qmpCmd);
+        goto cleanup;
+    }

> I think everything above this point should be in the main monitor APIs,
> and this method concern itself solely with converting the virJSONValuePtr
> into a string array, not actually executing command. This makes the code
> more portable for use in commands which need control over the command
> execution to handle errors / pass arguments.

Hm, this surprises me since this would put JSON specific code into the 
monitor APIs (qemu_monitor.c) while qemu_monitor.c typically only serves 
as a dispatcher for either text monitor or json monitor.


>> Index: libvirt/src/libvirt_private.syms===================================================================
>> --- libvirt.orig/src/libvirt_private.syms
>> +++ libvirt/src/libvirt_private.syms
>> @@ -1886,6 +1886,7 @@ virSetUIDGIDWithCaps;
>>   virSkipSpaces;
>>   virSkipSpacesAndBackslash;
>>   virSkipSpacesBackwards;
>> +virStrArrayHasString;
>>   virStrcpy;
>>   virStrIsPrint;
>>   virStrncpy;
>> Index: libvirt/src/util/virutil.c
>> ===================================================================
>> --- libvirt.orig/src/util/virutil.c
>> +++ libvirt/src/util/virutil.c
>> @@ -3379,3 +3379,17 @@ cleanup:
>>       VIR_FREE(buf);
>>       return ret;
>>   }
>> +
>> +
>> +bool
>> +virStrArrayHasString(char **strings, size_t n_strings, const char *needle)
>> +{
>> +    size_t i;
>> +
>> +    for (i = 0; i < n_strings; i++) {
>> +        if (STREQ(strings[i], needle))
>> +            return true;
>> +    }
>> +
>> +    return false;
>> +}
>> Index: libvirt/src/util/virutil.h
>> ===================================================================
>> --- libvirt.orig/src/util/virutil.h
>> +++ libvirt/src/util/virutil.h
>> @@ -297,4 +297,7 @@ int virGetDeviceUnprivSGIO(const char *p
>>   char * virGetUnprivSGIOSysfsPath(const char *path,
>>                                    const char *sysfs_dir);
>>   
>> +bool virStrArrayHasString(char **strings, size_t n_strings,
>> +                          const char *needle);
>> +
>>   #endif /* __VIR_UTIL_H__ */
> The virStrArrayHasString method doesn't appear to be used in this patch.
> If some other patch needs this still, then it should be a separate patch
> from the monitor code.
Ok, I'll split this off into a patch of its own.

    Stefan