[libvirt] [PATCH v1 0/3] Check migration configuration
This version differs from the patch set "conf: Check migration_host is valid or not during libvirt restarts" I posted 2 weeks ago, I droped checking the migration_host on target host. and find an issue when setting migration_host. Chen Fan (3): migration: add migration_host support for Ipv6 address without brackets conf: Check migration_host is localhost or not during restart conf: Check migration_address is valid or not during restart src/qemu/qemu.conf | 2 +- src/qemu/qemu_conf.c | 21 +++++++++++++++++++++ src/qemu/qemu_migration.c | 19 +++++++++++++++---- src/qemu/test_libvirtd_qemu.aug.in | 2 +- 4 files changed, 38 insertions(+), 6 deletions(-) -- 1.9.3
when specifying migration_host to an Ipv6 address without brackets, it was resolved to an incorrect address, such as: tcp:2001:0DB8::1428:4444, but the correct address should be: tcp:[2001:0DB8::1428]:4444 so we should add brackets when parsing it. Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com> --- src/qemu/qemu_migration.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index e4b664b..c7eb305 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2850,11 +2850,22 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr driver, goto cleanup; if (migrateHost != NULL) { - if (virSocketAddrIsNumeric(migrateHost) && - virSocketAddrParse(NULL, migrateHost, AF_UNSPEC) < 0) - goto cleanup; + virSocketAddr migrateHostSocket; + bool migrateHostisIpv6Address = false; + + if (virSocketAddrIsNumeric(migrateHost)) { + if (virSocketAddrParse(&migrateHostSocket, migrateHost, AF_UNSPEC) < 0) + goto cleanup; + + if (VIR_SOCKET_ADDR_IS_FAMILY(&migrateHostSocket, AF_INET6)) { + migrateHostisIpv6Address = true; + } + } - if (VIR_STRDUP(hostname, migrateHost) < 0) + if ((migrateHostisIpv6Address && + virAsprintf(&hostname, "[%s]", migrateHost) < 0) || + (!migrateHostisIpv6Address && + virAsprintf(&hostname, "%s", migrateHost) < 0)) goto cleanup; } else { if ((hostname = virGetHostname()) == NULL) -- 1.9.3
On 09/12/2014 06:31 AM, Chen Fan wrote:
when specifying migration_host to an Ipv6 address without brackets, it was resolved to an incorrect address, such as: tcp:2001:0DB8::1428:4444, but the correct address should be: tcp:[2001:0DB8::1428]:4444 so we should add brackets when parsing it.
Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com> --- src/qemu/qemu_migration.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index e4b664b..c7eb305 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2850,11 +2850,22 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr driver, goto cleanup;
if (migrateHost != NULL) { - if (virSocketAddrIsNumeric(migrateHost) && - virSocketAddrParse(NULL, migrateHost, AF_UNSPEC) < 0) - goto cleanup;
Hmm, I'm not sure what this check was doing - if the address was succesfully parsed in AddrIsNumeric, it should be parsed by virSocketAddrParse as well.
+ virSocketAddr migrateHostSocket; + bool migrateHostisIpv6Address = false; + + if (virSocketAddrIsNumeric(migrateHost)) { + if (virSocketAddrParse(&migrateHostSocket, migrateHost, AF_UNSPEC) < 0) + goto cleanup; + + if (VIR_SOCKET_ADDR_IS_FAMILY(&migrateHostSocket, AF_INET6)) { + migrateHostisIpv6Address = true; + } + }
We also do this parsing to chceck for numeric IPv6 addresses in qemuMigrationPrepareAny. It would be nicer to create a new 'virSocketAddrIsNumericIPv6' function and use it in both of them. Jan
- if (VIR_STRDUP(hostname, migrateHost) < 0) + if ((migrateHostisIpv6Address && + virAsprintf(&hostname, "[%s]", migrateHost) < 0) || + (!migrateHostisIpv6Address && + virAsprintf(&hostname, "%s", migrateHost) < 0)) goto cleanup; } else { if ((hostname = virGetHostname()) == NULL)
On Mon, 2014-09-22 at 15:34 +0200, Ján Tomko wrote:
On 09/12/2014 06:31 AM, Chen Fan wrote:
when specifying migration_host to an Ipv6 address without brackets, it was resolved to an incorrect address, such as: tcp:2001:0DB8::1428:4444, but the correct address should be: tcp:[2001:0DB8::1428]:4444 so we should add brackets when parsing it.
Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com> --- src/qemu/qemu_migration.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index e4b664b..c7eb305 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2850,11 +2850,22 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr driver, goto cleanup;
if (migrateHost != NULL) { - if (virSocketAddrIsNumeric(migrateHost) && - virSocketAddrParse(NULL, migrateHost, AF_UNSPEC) < 0) - goto cleanup;
Hmm, I'm not sure what this check was doing - if the address was succesfully parsed in AddrIsNumeric, it should be parsed by virSocketAddrParse as well.
agreed.
+ virSocketAddr migrateHostSocket; + bool migrateHostisIpv6Address = false; + + if (virSocketAddrIsNumeric(migrateHost)) { + if (virSocketAddrParse(&migrateHostSocket, migrateHost, AF_UNSPEC) < 0) + goto cleanup; + + if (VIR_SOCKET_ADDR_IS_FAMILY(&migrateHostSocket, AF_INET6)) { + migrateHostisIpv6Address = true; + } + }
We also do this parsing to chceck for numeric IPv6 addresses in qemuMigrationPrepareAny. It would be nicer to create a new 'virSocketAddrIsNumericIPv6' function and use it in both of them.
I will follow this. Thanks, Chen
Jan
- if (VIR_STRDUP(hostname, migrateHost) < 0) + if ((migrateHostisIpv6Address && + virAsprintf(&hostname, "[%s]", migrateHost) < 0) || + (!migrateHostisIpv6Address && + virAsprintf(&hostname, "%s", migrateHost) < 0)) goto cleanup; } else { if ((hostname = virGetHostname()) == NULL)
Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com> --- src/qemu/qemu_conf.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index ac10b64..013f3de 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -707,6 +707,17 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, GET_VALUE_LONG("seccomp_sandbox", cfg->seccompSandbox); GET_VALUE_STR("migration_host", cfg->migrateHost); + if (cfg->migrateHost) { + if (STRPREFIX(cfg->migrateHost, "localhost") || + STREQ(cfg->migrateHost, "127.0.0.1") || + STREQ(cfg->migrateHost, "::1") || + STREQ(cfg->migrateHost, "[::1]")) { + virReportError(VIR_ERR_CONF_SYNTAX, "%s", + _("migration_host must be a valid address or hostname")); + goto cleanup; + } + } + GET_VALUE_STR("migration_address", cfg->migrationAddress); GET_VALUE_BOOL("log_timestamp", cfg->logTimestamp); -- 1.9.3
On 09/12/2014 06:33 AM, Chen Fan wrote:
Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com> --- src/qemu/qemu_conf.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index ac10b64..013f3de 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -707,6 +707,17 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, GET_VALUE_LONG("seccomp_sandbox", cfg->seccompSandbox);
GET_VALUE_STR("migration_host", cfg->migrateHost); + if (cfg->migrateHost) { + if (STRPREFIX(cfg->migrateHost, "localhost") || + STREQ(cfg->migrateHost, "127.0.0.1") || + STREQ(cfg->migrateHost, "::1") || + STREQ(cfg->migrateHost, "[::1]")) {\
I think we need a 'virSocketAddrIsLocalhost' function similar to virSocketAddrIsWildcard, which would check any numeric represnation of the address: if (STRPREFIX(cfg->migrateHost, "localhost") || virSocketAddrIsLocalhost(cfg->migrateHost))
+ virReportError(VIR_ERR_CONF_SYNTAX, "%s", + _("migration_host must be a valid address or hostname"));
Something more specific, like: "migration_host must not be localhost" is more user-friendly. Jan
+ goto cleanup; + } + } + GET_VALUE_STR("migration_address", cfg->migrationAddress);
GET_VALUE_BOOL("log_timestamp", cfg->logTimestamp);
On Mon, 2014-09-22 at 15:34 +0200, Ján Tomko wrote:
On 09/12/2014 06:33 AM, Chen Fan wrote:
Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com> --- src/qemu/qemu_conf.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index ac10b64..013f3de 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -707,6 +707,17 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, GET_VALUE_LONG("seccomp_sandbox", cfg->seccompSandbox);
GET_VALUE_STR("migration_host", cfg->migrateHost); + if (cfg->migrateHost) { + if (STRPREFIX(cfg->migrateHost, "localhost") || + STREQ(cfg->migrateHost, "127.0.0.1") || + STREQ(cfg->migrateHost, "::1") || + STREQ(cfg->migrateHost, "[::1]")) {\
I think we need a 'virSocketAddrIsLocalhost' function similar to virSocketAddrIsWildcard, which would check any numeric represnation of the address:
if (STRPREFIX(cfg->migrateHost, "localhost") || virSocketAddrIsLocalhost(cfg->migrateHost))
It's a good point.
+ virReportError(VIR_ERR_CONF_SYNTAX, "%s", + _("migration_host must be a valid address or hostname"));
Something more specific, like: "migration_host must not be localhost" is more user-friendly.
I will update the output. Thanks, Chen
Jan
+ goto cleanup; + } + } + GET_VALUE_STR("migration_address", cfg->migrationAddress);
GET_VALUE_BOOL("log_timestamp", cfg->logTimestamp);
When enabling the migration_address option, by default it is set to "127.0.0.1", but it's not a valid address for migration. so we should add verification and set the default migration_address to "0.0.0.0". Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com> --- src/qemu/qemu.conf | 2 +- src/qemu/qemu_conf.c | 10 ++++++++++ src/qemu/test_libvirtd_qemu.aug.in | 2 +- 3 files changed, 12 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index 79bba36..666c303 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -459,7 +459,7 @@ # Override the listen address for all incoming migrations. Defaults to # 0.0.0.0, or :: if both host and qemu are capable of IPv6. -#migration_address = "127.0.0.1" +#migration_address = "0.0.0.0" # The default hostname or IP address which will be used by a migration diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 013f3de..2cbf2a6 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -719,6 +719,16 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, } GET_VALUE_STR("migration_address", cfg->migrationAddress); + if (cfg->migrationAddress) { + if (STRPREFIX(cfg->migrationAddress, "localhost") || + STREQ(cfg->migrationAddress, "127.0.0.1") || + STREQ(cfg->migrationAddress, "::1") || + STREQ(cfg->migrationAddress, "[::1]")) { + virReportError(VIR_ERR_CONF_SYNTAX, "%s", + _("migration_address must be a valid address or hostname")); + goto cleanup; + } + } GET_VALUE_BOOL("log_timestamp", cfg->logTimestamp); diff --git a/src/qemu/test_libvirtd_qemu.aug.in b/src/qemu/test_libvirtd_qemu.aug.in index d2bc2c0..30fd27e 100644 --- a/src/qemu/test_libvirtd_qemu.aug.in +++ b/src/qemu/test_libvirtd_qemu.aug.in @@ -69,7 +69,7 @@ module Test_libvirtd_qemu = { "keepalive_interval" = "5" } { "keepalive_count" = "5" } { "seccomp_sandbox" = "1" } -{ "migration_address" = "127.0.0.1" } +{ "migration_address" = "0.0.0.0" } { "migration_host" = "host.example.com" } { "migration_port_min" = "49152" } { "migration_port_max" = "49215" } -- 1.9.3
Hi jiri, Please help to review this patches. Thanks, Chen On Fri, 2014-09-12 at 12:32 +0800, Chen Fan wrote:
This version differs from the patch set "conf: Check migration_host is valid or not during libvirt restarts" I posted 2 weeks ago, I droped checking the migration_host on target host. and find an issue when setting migration_host.
Chen Fan (3): migration: add migration_host support for Ipv6 address without brackets conf: Check migration_host is localhost or not during restart conf: Check migration_address is valid or not during restart
src/qemu/qemu.conf | 2 +- src/qemu/qemu_conf.c | 21 +++++++++++++++++++++ src/qemu/qemu_migration.c | 19 +++++++++++++++---- src/qemu/test_libvirtd_qemu.aug.in | 2 +- 4 files changed, 38 insertions(+), 6 deletions(-)
participants (3)
-
Chen Fan -
Chen, Fan
-
Ján Tomko