[libvirt PATCH] virsh: domsetlaunchsecstate: mark options as mandatory

Ján Tomko posted 1 patch 2 years, 2 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/a9036060f13582282e2d40c5854c2bce3d845e73.1643211826.git.jtomko@redhat.com
tools/virsh-domain.c | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)
[libvirt PATCH] virsh: domsetlaunchsecstate: mark options as mandatory
Posted by Ján Tomko 2 years, 2 months ago
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

Re: [libvirt PATCH] virsh: domsetlaunchsecstate: mark options as mandatory
Posted by Michal Prívozník 2 years, 2 months ago
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

Re: [libvirt PATCH] virsh: domsetlaunchsecstate: mark options as mandatory
Posted by Andrea Bolognani 2 years, 2 months ago
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