[libvirt] [PATCH v6 9/9] virsh: implement new command for launch security

[Erik Skultety]

On Wed, May 23, 2018 at 04:18:34PM -0500, Brijesh Singh wrote:
> Add new 'launch-security' command, the command can be used to get or set
> the launch security information when booting encrypted VMs.
>
> Signed-off-by: Brijesh Singh <brijesh.singh at amd.com>
> ---
>  tools/virsh-domain.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  tools/virsh.pod      |  5 ++++
>  2 files changed, 86 insertions(+)
>
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index cfbbf5a7bc39..27bb702c8bb7 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -13870,6 +13870,81 @@ cmdDomFSInfo(vshControl *ctl, const vshCmd *cmd)
>      return ret >= 0;
>  }
>
> +/*
> + * "launch-security" command

Sigh... feature/command/API naming is soo tricky in libvirt, even though we
always try to be careful, we'll often hit the issues with commands being named
wrongly or just confusing, this is one of those cases, because the XML element
is called launch-security this really does make the impression the command
makes changes to the launch-security configuration for the guest, which is not
the case here. I think that if we ever find ourselves in that position that we
need to change the configuration of launch-security for a guest, then we need
to reserve this name for that, but now, launch-security-measure sounds more
descriptive of what it does right now to me, see my question about the setter
below, so that we can find a more suitable name for it.

This of course means the underlying APIs would need to change too, see my
comments to patches 6-8.

Any ideas for the naming guys?

> + */
> +static const vshCmdInfo info_launch_security[] = {
> +    {.name = "help",
> +        .data = N_("Get or set launch-security information")

So since we don't have the setter yet, it should only report as a getter,
because we're not introducing the setter switch as of now, once we do, we can
change the commentary as well...

> +    },
> +    {.name = "desc",
> +        .data = N_("Get or set the current launch-security information for "
> +                   "a guest domain.\n"

Here too...

> +                   "    To get the launch-security information use following"
> +                   "    command: \n\n"
> +                  "    virsh # launch-security <domain>")

^these 3 lines are unnecessary because that's contained in the SYNOPSIS
already.

> +    },
> +    {.name = NULL}
> +};
> +
> +static const vshCmdOptDef opts_launch_security[] = {
> +    VIRSH_COMMON_OPT_DOMAIN_FULL(0),
> +    VIRSH_COMMON_OPT_DOMAIN_CONFIG,
> +    VIRSH_COMMON_OPT_DOMAIN_LIVE,
> +    VIRSH_COMMON_OPT_DOMAIN_CURRENT,
> +    {.name = NULL}
> +};
> +
> +static void
> +virshPrintLaunchSecurityInfo(vshControl *ctl, virTypedParameterPtr params,
> +                             int nparams)
> +{
> +    size_t i;
> +
> +    for (i = 0; i < nparams; i++) {
> +        if (params[i].type == VIR_TYPED_PARAM_STRING)
> +            vshPrintExtra(ctl, "%-15s: %s\n", params[i].field, params[i].value.s);
> +    }
> +}

No need for ^this Print helper really...

> +
> +static bool
> +cmdLaunchSecurity(vshControl *ctl, const vshCmd *cmd)
> +{
> +    virDomainPtr dom;
> +    int nparams = 0;
> +    virTypedParameterPtr params = NULL;
> +    bool ret = false;
> +    unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT;
> +    bool current = vshCommandOptBool(cmd, "current");
> +    bool config = vshCommandOptBool(cmd, "config");
> +    bool live = vshCommandOptBool(cmd, "live");
> +
> +    VSH_EXCLUSIVE_OPTIONS_VAR(current, live);
> +    VSH_EXCLUSIVE_OPTIONS_VAR(current, config);
> +
> +    if (config)
> +        flags |= VIR_DOMAIN_AFFECT_CONFIG;
> +    if (live)
> +        flags |= VIR_DOMAIN_AFFECT_LIVE;
> +

So what should the setter then do, conceptually is enough for me as an answer?
I'm asking because I don't see a point in using VIR_DOMAIN_AFFECT_X at all
because those are for commands that do perform changes to the VM configuration,
either live or offline, getting a measurement doesn't touch the configuration
at all.

Erik

> +    if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
> +        return false;
> +
> +    if (virDomainGetLaunchSecurityInfo(dom, &params, &nparams, flags) != 0) {
> +        vshError(ctl, "%s", _("Unable to get launch security info"));
> +        goto cleanup;
> +    }
> +
> +    virshPrintLaunchSecurityInfo(ctl, params, nparams);
> +
> +    ret = true;
> + cleanup:
> +    virTypedParamsFree(params, nparams);
> +    virshDomainFree(dom);
> +    return ret;
> +}
> +
> +
>  const vshCmdDef domManagementCmds[] = {
>      {.name = "attach-device",
>       .handler = cmdAttachDevice,
> @@ -14485,5 +14560,11 @@ const vshCmdDef domManagementCmds[] = {
>       .info = info_domblkthreshold,
>       .flags = 0
>      },
> +    {.name = "launch-security-info",
> +     .handler = cmdLaunchSecurity,
> +     .opts = opts_launch_security,
> +     .info = info_launch_security,
> +     .flags = 0
> +    },
>      {.name = NULL}
>  };
> diff --git a/tools/virsh.pod b/tools/virsh.pod
> index 929958a9533c..31bb26bda2ac 100644
> --- a/tools/virsh.pod
> +++ b/tools/virsh.pod
> @@ -2899,6 +2899,11 @@ See B<vcpupin> for information on I<cpulist>.
>  Output the IP address and port number for the VNC display. If the information
>  is not available the processes will provide an exit code of 1.
>
> +=item B<launch-security-info> I<domain>
> +
> +Get the measurement of the memory contents encrypted through the launch
> +sequence when I<launch-security> is provided.
> +
>  =back
>
>  =head1 DEVICE COMMANDS
> --
> 2.14.3
>