[libvirt] [RFC 3/3] qemu: Introduce qemuBuildMasterKeyCommandLine

[John Ferlan]

On 03/22/2016 09:01 AM, Peter Krempa wrote:
> On Mon, Mar 21, 2016 at 14:29:02 -0400, John Ferlan wrote:
>> Using the masterKey and if the capability exists build an -object secret
>> command line to pass a masterKey file to qemu. The qemuBuildHasMasterKey
>> is expected to be reused by other objects which would need to know not
>> only if the masterKey has been provided, but if the secret object can be
>> used for their own purposes.
>>
>> Additionally, generate qemuDomainGetMasterKeyAlias which will be used by
>> later patches to define the 'keyid' (eg, masterKey) to be used by other
>> secrets to provide the id to qemu for the master key.
>>
>> Although the path to the domain libDir and the masterKey file are created
>> after qemuBuildCommandLine completes successfully, it's still possible to
>> generate the path and use it. No different than code paths that use the
>> domain libDir to create socket files (e.g. monitor.sock).
>>
>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>> ---
>>  src/qemu/qemu_command.c                            | 67 ++++++++++++++++++++++
>>  src/qemu/qemu_domain.c                             | 17 ++++++
>>  src/qemu/qemu_domain.h                             |  2 +
>>  .../qemuxml2argvdata/qemuxml2argv-master-key.args  | 23 ++++++++
>>  tests/qemuxml2argvdata/qemuxml2argv-master-key.xml | 30 ++++++++++
>>  tests/qemuxml2argvtest.c                           |  2 +
>>  6 files changed, 141 insertions(+)
>>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-master-key.args
>>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-master-key.xml
>>
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index eb02553..04e75fd 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -151,6 +151,70 @@ VIR_ENUM_IMPL(qemuNumaPolicy, VIR_DOMAIN_NUMATUNE_MEM_LAST,
>>                "interleave");
>>  
>>  /**
>> + * qemuBuildHasMasterKey:
>> + * @qemuCaps: QEMU binary capabilities
>> + *
>> + * Return true if this binary supports the secret -object, false otherwise.
>> + */
>> +static bool
>> +qemuBuildHasMasterKey(virQEMUCapsPtr qemuCaps)
>> +{
>> +    return virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_SECRET);
>> +}
> 
> This doesn't seem to be terribly useful. Especially since you have to
> check for priv->masterKey anyways.
> 

I thought it might be useful for other CommmandLine functions that may
someday need to determine if the secret -object is supported before
making decisions whether to use it or go with the existing mechanism.

But given other feedback, this'll probably be moot anyway.

>> +
>> +
>> +/**
>> + * qemuBuildMasterKeyCommandLine:
>> + * @cmd: the command to modify
>> + * @qemuCaps qemu capabilities object
>> + * @domainLibDir: location to find the master key
>> +
>> + * Formats the command line for a master key if available
>> + *
>> + * Returns 0 on success, -1 w/ error message on failure
>> + */
>> +static int
>> +qemuBuildMasterKeyCommandLine(virCommandPtr cmd,
>> +                              virQEMUCapsPtr qemuCaps,
>> +                              const char *domainLibDir)
>> +{
>> +    int ret = -1;
>> +    char *alias = NULL;
>> +    char *path = NULL;
>> +
>> +    /* If the -object secret does not exist, then just return. This just
>> +     * means the domain won't be able to use a secret master key and is
>> +     * not a failure.
>> +     */
>> +    if (!qemuBuildHasMasterKey(qemuCaps)) {
>> +        VIR_INFO("secret object is not supported by this QEMU binary");
>> +        return 0;
> 
> Here is the correct place to generate the key. I don't really see the
> benefit of doing it even for qemu that does not support this feature.
> 

Cannot disagree, but my conundrum was not having the priv object in
qemuBuildCommandLine, so choose something (libDir) that I can use.
This'll all get flipped in the next pass, but figured I'd at least try
to explain what was rattling through the rafters while making an initial
pass.

>> +    }
>> +
>> +    if (!(alias = qemuDomainGetMasterKeyAlias()))
>> +        return -1;
>> +
>> +    /* Get the path... NB, the path will not exist yet as domainLibDir
>> +     * and the master secret file gets created after we've successfully
>> +     * built the command line
>> +     */
>> +    if (!(path = qemuDomainGetMasterKeyFilePath(domainLibDir)))
>> +        goto cleanup;
>> +
>> +    virCommandAddArg(cmd, "-object");
>> +    virCommandAddArgFormat(cmd, "secret,id=%s,format=base64,file=%s",
>> +                           alias, path);
>> +
>> +    ret = 0;
>> +
>> + cleanup:
>> +    VIR_FREE(alias);
>> +    VIR_FREE(path);
>> +    return ret;
>> +}
>> +
>> +
>> +/**
>>   * qemuVirCommandGetFDSet:
>>   * @cmd: the command to modify
>>   * @fd: fd to reassign to the child
> 
> [...]
> 
>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> index 507ae9e..53d6705 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -468,6 +468,23 @@ qemuDomainJobInfoToParams(qemuDomainJobInfoPtr jobInfo,
>>  }
>>  
>>  
>> +/* qemuDomainGetMasterKeyAlias:
>> + *
>> + * Generate and return the masterKey alias
>> + *
>> + * Returns NULL or a string containing the master key alias
>> + */
>> +char *
>> +qemuDomainGetMasterKeyAlias(void)
> 
> You've added src/qemu/qemu_alias.c, so this belongs there.
> 

Coin flip - I chose here mainly because qemu_alias.c is primarily
dealing with Device aliases, but also because qemuDomainStorageAlias is
here.  IDC either way and will move it there.

Tks -

John
>> +{
>> +    char *alias;
>> +
>> +    ignore_value(VIR_STRDUP(alias, "masterKey0"));
>> +
>> +    return alias;
>> +}
>> +
>> +
>>  /* qemuDomainGetMasterKeyFilePath:
>>   * @libDir: Directory path to domain lib files
>>   *
> 
> Peter
>