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

[Tony Krowiak <akrowiak@xxxxxxxxxxxxxxxxxx>]

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@xxxxxxxxxx>

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@xxxxxxxxxxxxxxxxxx>
Signed-off-by: Daniel Hansel <daniel.hansel@xxxxxxxxxxxxxxxxxx>
Signed-off-by: Boris Fiuczynski <fiuczy@xxxxxxxxxxxxxxxxxx>
Reviewed-by: Boris Fiuczynski <fiuczy@xxxxxxxxxxxxxxxxxx>
Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
---
 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'?

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?

 #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?

You have an ACK from me with the include removed. Please wait for
feedback from the author before pushing.

These changes will break the test cases, so they will need to be updated to reflect
these changes before pushing.

Jan

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list