[libvirt] [PATCH v2 3/4] libvirt: qemu: enable/disable protected key management ops

[Tony Krowiak]

On 05/15/2015 12:23 PM, Ján Tomko wrote:
> On Fri, May 15, 2015 at 04:43:29PM +0200, Michal Privoznik wrote:
>> From: Tony Krowiak <aekrowia at us.ibm.com>
>>
>> Introduces two new -machine option parameters to the QEMU command to
>> enable/disable the CPACF protected key management operations for a guest:
>>
>>      aes-key-wrap='on|off'
>>      dea-key-wrap='on|off'
>>
>> The QEMU code maps the corresponding domain configuration elements to the
>> QEMU -machine option parameters to create the QEMU command:
>>
>>      <cipher name='aes' state='on'>   --> aes-key-wrap=on
>>      <cipher name='aes' state='off'>  --> aes-key-wrap=off
>>      <cipher name='dea' state='on'>   --> dea-key-wrap=on
>>      <cipher name='dea' state='off'>  --> dea-key-wrap=off
>>
>> Signed-off-by: Tony Krowiak <akrowiak at linux.vnet.ibm.com>
>> Signed-off-by: Daniel Hansel <daniel.hansel at linux.vnet.ibm.com>
>> Signed-off-by: Boris Fiuczynski <fiuczy at linux.vnet.ibm.com>
>> Reviewed-by: Boris Fiuczynski <fiuczy at linux.vnet.ibm.com>
>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>> ---
>>   src/qemu/qemu_capabilities.c |  4 +++
>>   src/qemu/qemu_capabilities.h |  2 ++
>>   src/qemu/qemu_command.c      | 73 ++++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 79 insertions(+)
>>
> So the difference to v1 is that they are no longer turned on by default
> if QEMU supports it. (I hope I did not miss anything else; it would
> have been helpful if you listed the important changes)
>
> I agree that this should not be done on XML parsing as was done in v1.
> Would it make sense to treat the missing option (STATE_ABSENT) as
> 'turn it on if qemu supports it'?
Some background:
My original design was similar to Michal's in that if key wrapping was not
configured for the guest in the domain XML, then the machine options 
would not
be inserted into the QEMU command line. Our internal reviewers commented 
that
there should be default values for libvirt that match the QEMU defaults, so
I did exactly as you suggested here, inserting default values into the QEMU
command line on STATE_ABSENT. Our internal reviewers then pointed out 
that the
dumpxml command would return a configuration that did not match that of 
the running
guest, so I added the XML post parsing piece to set default values into 
virDomainDef if
QEMU supports the key wrapping machine options. In any case, I'm not 
married to
any of these ideas, so you have my ACK pending Jan's suggestions.
>
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index 2939f8d..98fc5f8 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -38,6 +38,7 @@
>>   #include "virnetdevbridge.h"
>>   #include "virstring.h"
>>   #include "virtime.h"
>> +#include "virutil.h"
> Why is this include needed?
I believe that this is no longer needed and is a remnant of an earlier 
iteration that I failed to
remove. My bad.
>
>>   #include "viruuid.h"
>>   #include "c-ctype.h"
>>   #include "domain_nwfilter.h"
>> @@ -7286,6 +7287,39 @@ qemuBuildObsoleteAccelArg(virCommandPtr cmd,
>>       return 0;
>>   }
>>   
>> +static bool
>> +qemuAppendKeyWrapMachineParm(virBuffer *buf, virQEMUCapsPtr qemuCaps,
>> +                             int flag, const char *pname, int pstate)
>> +{
>> +    if (pstate != VIR_TRISTATE_SWITCH_ABSENT) {
>> +        if (!virQEMUCapsGet(qemuCaps, flag)) {
>> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> +                           _("%s is not available with this QEMU binary"), pname);
>> +            return false;
>> +        }
> Can't we allow state='off' with QEMU that does not support it?
We can certainly bypass the appending of the machine option if 
state='off', but if I am
not mistaken, sending a machine option to QEMU that it does not support 
will cause
QEMU to throw an error. I think it is wisest to inform the user of a 
configuration
error here.
>
> You have an ACK from me with the include removed. Please wait for
> feedback from the author before pushing.
>
> Jan
>
>
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150515/11d7ec02/attachment-0001.htm>