[libvirt PATCH] virsh: domsetlaunchsecstate: mark options as mandatory
We exit if they are not present. Let the virsh option parser do the checking instead of checking it manually. Change the type to OT_DATA (i.e. a mandatory string), mark them as required and remove VSH_OFLAG_REQ_OPT so that the header file and the secret file can be specified without the option names. https://bugzilla.redhat.com/show_bug.cgi?id=2046024 Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tools/virsh-domain.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index b56f6a90f5..d279af68b2 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -9587,13 +9587,13 @@ static const vshCmdInfo info_domsetlaunchsecstate[] = { static const vshCmdOptDef opts_domsetlaunchsecstate[] = { VIRSH_COMMON_OPT_DOMAIN_FULL(0), {.name = "secrethdr", - .type = VSH_OT_STRING, - .flags = VSH_OFLAG_REQ_OPT, + .type = VSH_OT_DATA, + .flags = VSH_OFLAG_REQ, .help = N_("path to file containing the secret header"), }, {.name = "secret", - .type = VSH_OT_STRING, - .flags = VSH_OFLAG_REQ_OPT, + .type = VSH_OT_DATA, + .flags = VSH_OFLAG_REQ, .help = N_("path to file containing the secret"), }, {.name = "set-address", @@ -9627,9 +9627,6 @@ cmdDomSetLaunchSecState(vshControl * ctl, const vshCmd * cmd) if (vshCommandOptStringReq(ctl, cmd, "secret", &secfile) < 0) return false; - if (sechdrfile == NULL || secfile == NULL) - return false; - if (virFileReadAll(sechdrfile, 1024*64, &sechdr) < 0) { vshSaveLibvirtError(); return false; -- 2.31.1
On Wed, Jan 26, 2022 at 16:43:52 +0100, Ján Tomko wrote:
We exit if they are not present.
Let the virsh option parser do the checking instead of checking it manually. Change the type to OT_DATA (i.e. a mandatory string), mark them as required and remove VSH_OFLAG_REQ_OPT so that the header file and the secret file can be specified without the option names.
https://bugzilla.redhat.com/show_bug.cgi?id=2046024
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tools/virsh-domain.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index b56f6a90f5..d279af68b2 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -9587,13 +9587,13 @@ static const vshCmdInfo info_domsetlaunchsecstate[] = { static const vshCmdOptDef opts_domsetlaunchsecstate[] = { VIRSH_COMMON_OPT_DOMAIN_FULL(0), {.name = "secrethdr", - .type = VSH_OT_STRING, - .flags = VSH_OFLAG_REQ_OPT, + .type = VSH_OT_DATA, + .flags = VSH_OFLAG_REQ, .help = N_("path to file containing the secret header"), }, {.name = "secret", - .type = VSH_OT_STRING, - .flags = VSH_OFLAG_REQ_OPT, + .type = VSH_OT_DATA, + .flags = VSH_OFLAG_REQ, .help = N_("path to file containing the secret"), }, {.name = "set-address",
Won't this prevent us from being able to specify a different set of parameters to the API if e.g. the header/secret combination comes out of fashion?
On 1/26/22 16:43, Ján Tomko wrote:
We exit if they are not present.
Let the virsh option parser do the checking instead of checking it manually. Change the type to OT_DATA (i.e. a mandatory string), mark them as required and remove VSH_OFLAG_REQ_OPT so that the header file and the secret file can be specified without the option names.
https://bugzilla.redhat.com/show_bug.cgi?id=2046024
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tools/virsh-domain.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)
@@ -9627,9 +9627,6 @@ cmdDomSetLaunchSecState(vshControl * ctl, const vshCmd * cmd) if (vshCommandOptStringReq(ctl, cmd, "secret", &secfile) < 0) return false;
- if (sechdrfile == NULL || secfile == NULL) - return false; - if (virFileReadAll(sechdrfile, 1024*64, &sechdr) < 0) { vshSaveLibvirtError(); return false;
In a follow up patch this check should be reintroduced to qemu driver impl because there's no way it can work if neither is passed to the API. But for this patch as is: Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
On Wed, Jan 26, 2022 at 04:43:52PM +0100, Ján Tomko wrote:
We exit if they are not present.
Let the virsh option parser do the checking instead of checking it manually. Change the type to OT_DATA (i.e. a mandatory string), mark them as required and remove VSH_OFLAG_REQ_OPT so that the header file and the secret file can be specified without the option names.
https://bugzilla.redhat.com/show_bug.cgi?id=2046024
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tools/virsh-domain.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)
You should update virsh.rst accordingly with something along the lines of the diff included below. With that taken care of, Reviewed-by: Andrea Bolognani <abologna@redhat.com> diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index e28927ed6c..65002f968a 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -2095,18 +2095,17 @@ domsetlaunchsecstate :: - domsetlaunchsecstate domain --secrethdr hdr-filename - --secret secret-filename [--set-address address] + domsetlaunchsecstate domain secrethdr secret [--set-address address] Set a launch security secret in the guest's memory. The guest must have a launchSecurity type enabled in its configuration and be in a paused state. On success, the guest can be transitioned to a running state. On failure, the guest should be destroyed. -*--secrethdr* specifies a filename containing the base64-encoded secret header. +*secrethdr* is the path to a file containing the base64-encoded secret header. The header includes artifacts needed by the hypervisor firmware to recover the -plain text of the launch secret. *--secret* specifies the filename containing -the base64-encoded encrypted launch secret. +plain text of the launch secret. *secret* is the path to a file containing the +base64-encoded encrypted launch secret. The *--set-address* option can be used to specify a physical address within the guest's memory to set the secret. If not specified, the address will be -- Andrea Bolognani / Red Hat / Virtualization
participants (4)
-
Andrea Bolognani -
Ján Tomko -
Michal Prívozník -
Peter Krempa