virsh migrate: Require --tls for --tls-destination

[libvirt] [PATCH] virsh migrate: Require --tls for --tls-destination

Posted by Jiri Denemark 4 years, 4 months ago

--tls-destination would be just ignored unless --tls is not specified,
which is correct, but let's provide a bit of a guidance is a user
forgets to add --tls.

https://bugzilla.redhat.com/show_bug.cgi?id=1784345

Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
---
 tools/virsh-domain.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 56137bdd74..e7e92ee60d 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -10966,6 +10966,7 @@ cmdMigrate(vshControl *ctl, const vshCmd *cmd)
     VSH_REQUIRE_OPTION("postcopy-after-precopy", "postcopy");
     VSH_REQUIRE_OPTION("timeout-postcopy", "postcopy");
     VSH_REQUIRE_OPTION("persistent-xml", "persistent");
+    VSH_REQUIRE_OPTION("tls-destination", "tls");
 
     if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
         return false;
-- 
2.24.1

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

Re: [libvirt] [PATCH] virsh migrate: Require --tls for --tls-destination

Posted by Peter Krempa 4 years, 4 months ago

On Tue, Dec 17, 2019 at 14:34:12 +0100, Jiri Denemark wrote:
> --tls-destination would be just ignored unless --tls is not specified,
> which is correct, but let's provide a bit of a guidance is a user
> forgets to add --tls.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1784345
> 
> Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
> ---
>  tools/virsh-domain.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index 56137bdd74..e7e92ee60d 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -10966,6 +10966,7 @@ cmdMigrate(vshControl *ctl, const vshCmd *cmd)
>      VSH_REQUIRE_OPTION("postcopy-after-precopy", "postcopy");
>      VSH_REQUIRE_OPTION("timeout-postcopy", "postcopy");
>      VSH_REQUIRE_OPTION("persistent-xml", "persistent");
> +    VSH_REQUIRE_OPTION("tls-destination", "tls");

This fixes just virsh users. What about direct API users? Shouldn't the
bug be fixed at API level too?

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

Re: [libvirt] [PATCH] virsh migrate: Require --tls for --tls-destination

Posted by Jiri Denemark 4 years, 4 months ago

On Tue, Dec 17, 2019 at 14:46:24 +0100, Peter Krempa wrote:
> On Tue, Dec 17, 2019 at 14:34:12 +0100, Jiri Denemark wrote:
> > --tls-destination would be just ignored unless --tls is not specified,
> > which is correct, but let's provide a bit of a guidance is a user
> > forgets to add --tls.
> > 
> > https://bugzilla.redhat.com/show_bug.cgi?id=1784345
> > 
> > Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
> > ---
> >  tools/virsh-domain.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> > index 56137bdd74..e7e92ee60d 100644
> > --- a/tools/virsh-domain.c
> > +++ b/tools/virsh-domain.c
> > @@ -10966,6 +10966,7 @@ cmdMigrate(vshControl *ctl, const vshCmd *cmd)
> >      VSH_REQUIRE_OPTION("postcopy-after-precopy", "postcopy");
> >      VSH_REQUIRE_OPTION("timeout-postcopy", "postcopy");
> >      VSH_REQUIRE_OPTION("persistent-xml", "persistent");
> > +    VSH_REQUIRE_OPTION("tls-destination", "tls");
> 
> This fixes just virsh users. What about direct API users? Shouldn't the
> bug be fixed at API level too?

We currently don't do such checks at API level. Migration parameters
which depend on a flag are checked only if the flag is set, otherwise
they are just ignored. This is probably not optimal, but it's not
incorrect :-)

Jirka

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

Re: [libvirt] [PATCH] virsh migrate: Require --tls for --tls-destination

Posted by Peter Krempa 4 years, 4 months ago

On Tue, Dec 17, 2019 at 15:02:24 +0100, Jiri Denemark wrote:
> On Tue, Dec 17, 2019 at 14:46:24 +0100, Peter Krempa wrote:
> > On Tue, Dec 17, 2019 at 14:34:12 +0100, Jiri Denemark wrote:
> > > --tls-destination would be just ignored unless --tls is not specified,
> > > which is correct, but let's provide a bit of a guidance is a user
> > > forgets to add --tls.
> > > 
> > > https://bugzilla.redhat.com/show_bug.cgi?id=1784345
> > > 
> > > Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
> > > ---
> > >  tools/virsh-domain.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> > > index 56137bdd74..e7e92ee60d 100644
> > > --- a/tools/virsh-domain.c
> > > +++ b/tools/virsh-domain.c
> > > @@ -10966,6 +10966,7 @@ cmdMigrate(vshControl *ctl, const vshCmd *cmd)
> > >      VSH_REQUIRE_OPTION("postcopy-after-precopy", "postcopy");
> > >      VSH_REQUIRE_OPTION("timeout-postcopy", "postcopy");
> > >      VSH_REQUIRE_OPTION("persistent-xml", "persistent");
> > > +    VSH_REQUIRE_OPTION("tls-destination", "tls");
> > 
> > This fixes just virsh users. What about direct API users? Shouldn't the
> > bug be fixed at API level too?
> 
> We currently don't do such checks at API level. Migration parameters
> which depend on a flag are checked only if the flag is set, otherwise
> they are just ignored. This is probably not optimal, but it's not
> incorrect :-)

While I think that what you wrote is a ridiculously weak excuse, I see
that we do such a weird thing in other instances as well.

Note that the documentation for the parameter does't mention the above
either.

ACK if you amend the commit message stating that this is a virsh-only
hack done on purpose, so that it doesn't look like I forgot to mention
the API-level check during review.

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