Re: [libvirt] [PATCH] conf: Check migration_host is valid or not during libvirt restarts

2014-08-31 Thread chen.fan.f...@cn.fujitsu.com 
On Fri, 2014-08-29 at 14:52 +0200, Jiri Denemark wrote: 
 On Fri, Aug 29, 2014 at 19:20:58 +0800, Chen Fan wrote:
  if user specified an invalid strings as migration hostname,
  like setting: migration_host = XXX, libvirt should check
  it and return error during lbivirt restart.
  
  Signed-off-by: Chen Fan chen.fan.f...@cn.fujitsu.com
  ---
   src/qemu/qemu_conf.c | 40 
   1 file changed, 40 insertions(+)
  
  diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
  index e2ec54f..450ac5b 100644
  --- a/src/qemu/qemu_conf.c
  +++ b/src/qemu/qemu_conf.c
  @@ -33,6 +33,7 @@
   #include fcntl.h
   #include sys/wait.h
   #include arpa/inet.h
  +#include netdb.h
   
   #include virerror.h
   #include qemu_conf.h
  @@ -650,6 +651,45 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr 
  cfg,
   GET_VALUE_LONG(seccomp_sandbox, cfg-seccompSandbox);
   
   GET_VALUE_STR(migration_host, cfg-migrateHost);
  +if (cfg-migrateHost) {
  +struct addrinfo hints;
  +struct addrinfo *res;
  +
  +memset(hints, 0, sizeof(hints));
  +hints.ai_flags = AI_ADDRCONFIG;
  +hints.ai_family = AF_UNSPEC;
  +
  +if (getaddrinfo(cfg-migrateHost, NULL, hints, res) != 0) {
  +virReportError(VIR_ERR_CONF_SYNTAX,
  +   _(migration_host: '%s' is not a valid 
  hostname),
  +   cfg-migrateHost);
  +goto cleanup;
  +}
  +
  +if (res == NULL) {
  +virReportError(VIR_ERR_CONF_SYNTAX,
  +   _(No IP address for host '%s' found),
  +   cfg-migrateHost);
  +goto cleanup;
  +}
  +
  +freeaddrinfo(res);
 
 I don't think this is a good idea. What if it's in fact valid and just
 can't be resolved due to a temporary issue? It should only fail at the
 time someone actually tries to migrate a domain.

What are these issues? usually, I think migration hosts always at the
same network, a valid hostname should be resolved by DNS. if can't be
resolved, libvirt should tell user the error at the time of libvirt
restart, Maybe we can output a warning or info to let user know that but
restart failure. what do you think?

Thanks,
Chen


 
  +
  +if (STRPREFIX(cfg-migrateHost, localhost)) {
  +virReportError(VIR_ERR_CONF_SYNTAX, %s,
  +   _(setting migration_host to 'localhost' is not 
  allowed));
  +goto cleanup;
  +}
  +
  +if (STREQ(cfg-migrateHost, 127.0.0.1) ||
  +STREQ(cfg-migrateHost, ::1)) {
  +virReportError(VIR_ERR_CONF_SYNTAX, %s,
  +   _(setting migration_host to '127.0.0.1' or 
  '::1' 
  + is not allowed));
  +goto cleanup;
  +}
  +}
  +
 
 Checking for localhost could make sense.
 
   GET_VALUE_STR(migration_address, cfg-migrationAddress);
   
   GET_VALUE_BOOL(log_timestamp, cfg-logTimestamp);
 
 Jirka


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

[libvirt] [PATCH] conf: Check migration_host is valid or not during libvirt restarts

2014-08-29 Thread Chen Fan 
if user specified an invalid strings as migration hostname,
like setting: migration_host = XXX, libvirt should check
it and return error during lbivirt restart.

Signed-off-by: Chen Fan chen.fan.f...@cn.fujitsu.com
---
 src/qemu/qemu_conf.c | 40 
 1 file changed, 40 insertions(+)

diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index e2ec54f..450ac5b 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -33,6 +33,7 @@
 #include fcntl.h
 #include sys/wait.h
 #include arpa/inet.h
+#include netdb.h
 
 #include virerror.h
 #include qemu_conf.h
@@ -650,6 +651,45 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
 GET_VALUE_LONG(seccomp_sandbox, cfg-seccompSandbox);
 
 GET_VALUE_STR(migration_host, cfg-migrateHost);
+if (cfg-migrateHost) {
+struct addrinfo hints;
+struct addrinfo *res;
+
+memset(hints, 0, sizeof(hints));
+hints.ai_flags = AI_ADDRCONFIG;
+hints.ai_family = AF_UNSPEC;
+
+if (getaddrinfo(cfg-migrateHost, NULL, hints, res) != 0) {
+virReportError(VIR_ERR_CONF_SYNTAX,
+   _(migration_host: '%s' is not a valid hostname),
+   cfg-migrateHost);
+goto cleanup;
+}
+
+if (res == NULL) {
+virReportError(VIR_ERR_CONF_SYNTAX,
+   _(No IP address for host '%s' found),
+   cfg-migrateHost);
+goto cleanup;
+}
+
+freeaddrinfo(res);
+
+if (STRPREFIX(cfg-migrateHost, localhost)) {
+virReportError(VIR_ERR_CONF_SYNTAX, %s,
+   _(setting migration_host to 'localhost' is not 
allowed));
+goto cleanup;
+}
+
+if (STREQ(cfg-migrateHost, 127.0.0.1) ||
+STREQ(cfg-migrateHost, ::1)) {
+virReportError(VIR_ERR_CONF_SYNTAX, %s,
+   _(setting migration_host to '127.0.0.1' or '::1' 
+ is not allowed));
+goto cleanup;
+}
+}
+
 GET_VALUE_STR(migration_address, cfg-migrationAddress);
 
 GET_VALUE_BOOL(log_timestamp, cfg-logTimestamp);
-- 
1.9.3

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

Re: [libvirt] [PATCH] conf: Check migration_host is valid or not during libvirt restarts

2014-08-29 Thread Jiri Denemark 
On Fri, Aug 29, 2014 at 19:20:58 +0800, Chen Fan wrote:
 if user specified an invalid strings as migration hostname,
 like setting: migration_host = XXX, libvirt should check
 it and return error during lbivirt restart.
 
 Signed-off-by: Chen Fan chen.fan.f...@cn.fujitsu.com
 ---
  src/qemu/qemu_conf.c | 40 
  1 file changed, 40 insertions(+)
 
 diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
 index e2ec54f..450ac5b 100644
 --- a/src/qemu/qemu_conf.c
 +++ b/src/qemu/qemu_conf.c
 @@ -33,6 +33,7 @@
  #include fcntl.h
  #include sys/wait.h
  #include arpa/inet.h
 +#include netdb.h
  
  #include virerror.h
  #include qemu_conf.h
 @@ -650,6 +651,45 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr 
 cfg,
  GET_VALUE_LONG(seccomp_sandbox, cfg-seccompSandbox);
  
  GET_VALUE_STR(migration_host, cfg-migrateHost);
 +if (cfg-migrateHost) {
 +struct addrinfo hints;
 +struct addrinfo *res;
 +
 +memset(hints, 0, sizeof(hints));
 +hints.ai_flags = AI_ADDRCONFIG;
 +hints.ai_family = AF_UNSPEC;
 +
 +if (getaddrinfo(cfg-migrateHost, NULL, hints, res) != 0) {
 +virReportError(VIR_ERR_CONF_SYNTAX,
 +   _(migration_host: '%s' is not a valid hostname),
 +   cfg-migrateHost);
 +goto cleanup;
 +}
 +
 +if (res == NULL) {
 +virReportError(VIR_ERR_CONF_SYNTAX,
 +   _(No IP address for host '%s' found),
 +   cfg-migrateHost);
 +goto cleanup;
 +}
 +
 +freeaddrinfo(res);

I don't think this is a good idea. What if it's in fact valid and just
can't be resolved due to a temporary issue? It should only fail at the
time someone actually tries to migrate a domain.

 +
 +if (STRPREFIX(cfg-migrateHost, localhost)) {
 +virReportError(VIR_ERR_CONF_SYNTAX, %s,
 +   _(setting migration_host to 'localhost' is not 
 allowed));
 +goto cleanup;
 +}
 +
 +if (STREQ(cfg-migrateHost, 127.0.0.1) ||
 +STREQ(cfg-migrateHost, ::1)) {
 +virReportError(VIR_ERR_CONF_SYNTAX, %s,
 +   _(setting migration_host to '127.0.0.1' or '::1' 
 
 + is not allowed));
 +goto cleanup;
 +}
 +}
 +

Checking for localhost could make sense.

  GET_VALUE_STR(migration_address, cfg-migrationAddress);
  
  GET_VALUE_BOOL(log_timestamp, cfg-logTimestamp);

Jirka

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