[libvirt] [PATCH] virsh migrate: Require --tls for --tls-destination
--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
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?
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
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.
participants (2)
-
Jiri Denemark -
Peter Krempa