6:11 p.m.
For now, we set the migration URI via command line '--migrate_uri' or
construct the URI by looking up the dest host's hostname which could be
solved by DNS automatically.
But in cases the dest host have two or more NICs to reach, we may need to
send the migration data over a specific NIC which is different from the
automatically resloved one for some reason like performance, security, etc.
thus we must explicitly specify the migrateuri in command line everytime,
but it is too troublesome if there are many such hosts(and don't forget
virt-manager).
This patches add a configuration file option on dest host to save the
default migrate uri which explicitly specify which of this host's
addresses is used for transferring data, thus user doesn't boring
to specify it in command line everytime.
Any comments are welcome.
Thank you very much.
Chen Fan (2):
Add support for migration URI configuration
add inotify handler to qemu driver
src/qemu/qemu.conf | 5 ++-
src/qemu/qemu_conf.c | 89 +++++++++++++++++++++++++++++++++++++++++++++++
src/qemu/qemu_conf.h | 11 +++++-
src/qemu/qemu_driver.c | 25 ++++++++++++-
src/qemu/qemu_migration.c | 5 +++
5 files changed, 132 insertions(+), 3 deletions(-)
--
1.8.1.4
6:11 p.m.
New subject: [libvirt] [patch v3 1/2] Add support for migration URI configuration
Signed-off-by: Chen Fan <chen.fan.fnst(a)cn.fujitsu.com>
---
src/qemu/qemu.conf | 5 ++++-
src/qemu/qemu_conf.c | 2 ++
src/qemu/qemu_conf.h | 2 ++
src/qemu/qemu_migration.c | 5 +++++
4 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
index f0e802f..2973631 100644
--- a/src/qemu/qemu.conf
+++ b/src/qemu/qemu.conf
@@ -449,7 +449,10 @@
#
#seccomp_sandbox = 1
-
+# Override the URI used for any specific migration URI to be sent.
+# Defaults to NULL, will be set to as "tcp://hostIP[:port]".
+#
+#migrate_uri = "tcp://hostIP:port"
# Override the listen address for all incoming migrations. Defaults to
# 0.0.0.0, or :: if both host and qemu are capable of IPv6.
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 198ee2f..0bd943d 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -576,6 +576,8 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
GET_VALUE_STR("migration_address", cfg->migrationAddress);
+ GET_VALUE_STR("migrate_uri", cfg->migrateUri);
+
ret = 0;
cleanup:
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index a36ea63..2e45421 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -167,6 +167,8 @@ struct _virQEMUDriverConfig {
char *migrationAddress;
int migrationPortMin;
int migrationPortMax;
+
+ char *migrateUri;
};
/* Main driver state */
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index a9f7fea..bcf966a 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -2639,6 +2639,7 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr driver,
int ret = -1;
virURIPtr uri = NULL;
bool well_formed_uri = true;
+ virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
VIR_DEBUG("driver=%p, dconn=%p, cookiein=%s, cookieinlen=%d, "
"cookieout=%p, cookieoutlen=%p, uri_in=%s, uri_out=%p, "
@@ -2649,6 +2650,10 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr driver,
*uri_out = NULL;
+ if (uri_in == NULL && cfg->migrateUri) {
+ uri_in = cfg->migrateUri;
+ }
+
/* The URI passed in may be NULL or a string "tcp://somehostname:port".
*
* If the URI passed in is NULL then we allocate a port number
--
1.8.1.4
10:21 a.m.
New subject: [libvirt] [patch v3 1/2] Add support for migration URI configuration
I think *cfg* should be unref by virObjectUnref(cfg) .
And so does patch 2/2.
-----Original Message-----
From: libvir-list-bounces(a)redhat.com [mailto:libvir-list-bounces@redhat.com]
On Behalf Of Chen Fan
Sent: Wednesday, May 07, 2014 6:12 PM
To: libvir-list(a)redhat.com
Subject: [libvirt] [patch v3 1/2] Add support for migration URI configuration
Signed-off-by: Chen Fan <chen.fan.fnst(a)cn.fujitsu.com>
---
src/qemu/qemu.conf | 5 ++++-
src/qemu/qemu_conf.c | 2 ++
src/qemu/qemu_conf.h | 2 ++
src/qemu/qemu_migration.c | 5 +++++
4 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
index f0e802f..2973631 100644
--- a/src/qemu/qemu.conf
+++ b/src/qemu/qemu.conf
@@ -449,7 +449,10 @@
#
#seccomp_sandbox = 1
-
+# Override the URI used for any specific migration URI to be sent.
+# Defaults to NULL, will be set to as "tcp://hostIP[:port]".
+#
+#migrate_uri = "tcp://hostIP:port"
# Override the listen address for all incoming migrations. Defaults to
# 0.0.0.0, or :: if both host and qemu are capable of IPv6.
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 198ee2f..0bd943d 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -576,6 +576,8 @@ int
virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
GET_VALUE_STR("migration_address", cfg->migrationAddress);
+ GET_VALUE_STR("migrate_uri", cfg->migrateUri);
+
ret = 0;
cleanup:
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index a36ea63..2e45421 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -167,6 +167,8 @@ struct _virQEMUDriverConfig {
char *migrationAddress;
int migrationPortMin;
int migrationPortMax;
+
+ char *migrateUri;
};
/* Main driver state */
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index a9f7fea..bcf966a 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -2639,6 +2639,7 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr
driver,
int ret = -1;
virURIPtr uri = NULL;
bool well_formed_uri = true;
+ virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
VIR_DEBUG("driver=%p, dconn=%p, cookiein=%s, cookieinlen=%d, "
"cookieout=%p, cookieoutlen=%p, uri_in=%s, uri_out=%p, "
@@ -2649,6 +2650,10 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr
driver,
*uri_out = NULL;
+ if (uri_in == NULL && cfg->migrateUri) {
+ uri_in = cfg->migrateUri;
+ }
+
/* The URI passed in may be NULL or a string
"tcp://somehostname:port".
*
* If the URI passed in is NULL then we allocate a port number
--
1.8.1.4
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
11:17 a.m.
New subject: [libvirt] [patch v3 1/2] Add support for migration URI configuration
On Thu, 2014-05-08 at 02:21 +0000, Wangrui (K) wrote:
I think *cfg* should be unref by virObjectUnref(cfg) .
And so does patch 2/2.
Thanks for pointing this out!
I will fix this and post them soon.
Thanks,
Chen
> -----Original Message-----
> From: libvir-list-bounces(a)redhat.com [mailto:libvir-list-bounces@redhat.com]
> On Behalf Of Chen Fan
> Sent: Wednesday, May 07, 2014 6:12 PM
> To: libvir-list(a)redhat.com
> Subject: [libvirt] [patch v3 1/2] Add support for migration URI configuration
>
> Signed-off-by: Chen Fan <chen.fan.fnst(a)cn.fujitsu.com>
> ---
> src/qemu/qemu.conf | 5 ++++-
> src/qemu/qemu_conf.c | 2 ++
> src/qemu/qemu_conf.h | 2 ++
> src/qemu/qemu_migration.c | 5 +++++
> 4 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
> index f0e802f..2973631 100644
> --- a/src/qemu/qemu.conf
> +++ b/src/qemu/qemu.conf
> @@ -449,7 +449,10 @@
> #
> #seccomp_sandbox = 1
>
> -
> +# Override the URI used for any specific migration URI to be sent.
> +# Defaults to NULL, will be set to as "tcp://hostIP[:port]".
> +#
> +#migrate_uri = "tcp://hostIP:port"
>
> # Override the listen address for all incoming migrations. Defaults to
> # 0.0.0.0, or :: if both host and qemu are capable of IPv6.
> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index 198ee2f..0bd943d 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c
> @@ -576,6 +576,8 @@ int
> virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
>
> GET_VALUE_STR("migration_address", cfg->migrationAddress);
>
> + GET_VALUE_STR("migrate_uri", cfg->migrateUri);
> +
> ret = 0;
>
> cleanup:
> diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
> index a36ea63..2e45421 100644
> --- a/src/qemu/qemu_conf.h
> +++ b/src/qemu/qemu_conf.h
> @@ -167,6 +167,8 @@ struct _virQEMUDriverConfig {
> char *migrationAddress;
> int migrationPortMin;
> int migrationPortMax;
> +
> + char *migrateUri;
> };
>
> /* Main driver state */
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index a9f7fea..bcf966a 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -2639,6 +2639,7 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr
> driver,
> int ret = -1;
> virURIPtr uri = NULL;
> bool well_formed_uri = true;
> + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>
> VIR_DEBUG("driver=%p, dconn=%p, cookiein=%s, cookieinlen=%d, "
> "cookieout=%p, cookieoutlen=%p, uri_in=%s, uri_out=%p, "
> @@ -2649,6 +2650,10 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr
> driver,
>
> *uri_out = NULL;
>
> + if (uri_in == NULL && cfg->migrateUri) {
> + uri_in = cfg->migrateUri;
> + }
> +
> /* The URI passed in may be NULL or a string
> "tcp://somehostname:port".
> *
> * If the URI passed in is NULL then we allocate a port number
> --
> 1.8.1.4
>
> --
> libvir-list mailing list
> libvir-list(a)redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
6:11 p.m.
New subject: [libvirt] [patch v3 2/2] add inotify handler to qemu driver
we don't expect to reload 'migrate_uri' with restarting libvirtd everytime
while
updating the URI, so adding inotify handler to reload 'migrate_uri' configuration
without restarting libvirtd, it will be also helpful for virt-manager to get
'migrate_uri'.
Signed-off-by: Chen Fan <chen.fan.fnst(a)cn.fujitsu.com>
---
src/qemu/qemu_conf.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++++++
src/qemu/qemu_conf.h | 9 +++++-
src/qemu/qemu_driver.c | 25 ++++++++++++++-
3 files changed, 119 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 0bd943d..e50fdb8 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 <sys/inotify.h>
#include "virerror.h"
#include "qemu_conf.h"
@@ -1431,3 +1432,89 @@ qemuTranslateSnapshotDiskSourcePool(virConnectPtr conn
ATTRIBUTE_UNUSED,
_("Snapshots are not yet supported with 'pool'
volumes"));
return -1;
}
+
+void
+qemuInotifyEvent(int watch,
+ int fd,
+ int events ATTRIBUTE_UNUSED,
+ void *data)
+{
+ char buf[1024];
+ char filename[1024];
+ struct inotify_event *e;
+ int got;
+ char *tmp, *name;
+ virQEMUDriverPtr driver = data;
+ virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
+ virConfPtr conf;
+ virConfValuePtr p;
+
+ qemuDriverLock(driver);
+ if (watch != driver->inotifyWatch)
+ goto cleanup;
+
+ reread:
+ got = read(fd, buf, sizeof(buf));
+ if (got == -1) {
+ if (errno == EINTR)
+ goto reread;
+ goto cleanup;
+ }
+
+ tmp = buf;
+ while (got) {
+ if (got < sizeof(struct inotify_event))
+ goto cleanup; /* bad */
+
+ VIR_WARNINGS_NO_CAST_ALIGN
+ e = (struct inotify_event *)tmp;
+ VIR_WARNINGS_RESET
+
+ tmp += sizeof(struct inotify_event);
+ got -= sizeof(struct inotify_event);
+
+ if (got < e->len)
+ goto cleanup;
+
+ tmp += e->len;
+ got -= e->len;
+
+ name = (char *)&(e->name);
+
+ if (STRNEQ(name, "qemu.conf")) {
+ continue;
+ }
+
+ snprintf(filename, 1024, "%s/qemu.conf",
+ cfg->configBaseDir);
+
+ if (e->mask & (IN_CREATE | IN_CLOSE_WRITE | IN_MOVED_TO)) {
+ char *str;
+ VIR_DEBUG("Got inotify '%s' modified", filename);
+ if (!(conf = virConfReadFile(filename, 0)))
+ goto cleanup;
+
+ p = virConfGetValue(conf, "migrate_uri");
+ if (p && p->type != VIR_CONF_STRING) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Expected a string for 'migrate_uri' config
parameter"));
+ virConfFree(conf);
+ goto cleanup;
+ }
+
+ if (p) {
+ str = p->str;
+ } else {
+ str = NULL;
+ }
+
+ VIR_FREE(cfg->migrateUri);
+ if (VIR_STRDUP(cfg->migrateUri, str) < 0)
+ virConfFree(conf);
+ goto cleanup;
+ }
+ }
+
+ cleanup:
+ qemuDriverUnlock(driver);
+}
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index 2e45421..2cc62f9 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -240,6 +240,10 @@ struct _virQEMUDriver {
/* Immutable pointer, self-clocking APIs */
virCloseCallbacksPtr closeCallbacks;
+
+ /* The inotify fd */
+ int inotifyFD;
+ int inotifyWatch;
};
typedef struct _qemuDomainCmdlineDef qemuDomainCmdlineDef;
@@ -309,5 +313,8 @@ int qemuTranslateDiskSourcePool(virConnectPtr conn,
int qemuTranslateSnapshotDiskSourcePool(virConnectPtr conn,
virDomainSnapshotDiskDefPtr def);
-
+void qemuInotifyEvent(int watch,
+ int fd,
+ int events ATTRIBUTE_UNUSED,
+ void *data);
#endif /* __QEMUD_CONF_H */
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index fca1a91..83122d8 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -42,7 +42,7 @@
#include <sys/ioctl.h>
#include <sys/un.h>
#include <byteswap.h>
-
+#include <sys/inotify.h>
#include "qemu_driver.h"
#include "qemu_agent.h"
@@ -596,6 +596,7 @@ qemuStateInitialize(bool privileged,
/* Don't have a dom0 so start from 1 */
qemu_driver->nextvmid = 1;
+ qemu_driver->inotifyWatch = -1;
if (!(qemu_driver->domains = virDomainObjListNew()))
goto error;
@@ -750,6 +751,28 @@ qemuStateInitialize(bool privileged,
if (!(qemu_driver->xmlopt = virQEMUDriverCreateXMLConf(qemu_driver)))
goto error;
+ if ((qemu_driver->inotifyFD = inotify_init()) < 0) {
+ VIR_ERROR(_("cannot initialize inotify"));
+ goto error;
+ }
+
+ VIR_INFO("Adding inotify watch on %s", cfg->configBaseDir);
+ if (inotify_add_watch(qemu_driver->inotifyFD,
+ cfg->configBaseDir,
+ IN_CREATE |
+ IN_CLOSE_WRITE | IN_DELETE |
+ IN_MOVED_TO | IN_MOVED_FROM) < 0) {
+ VIR_ERROR(_("Failed to create inotify watch on %s: %s"),
+ cfg->configBaseDir,
+ virStrerror(errno, ebuf, sizeof(ebuf)));
+ goto error;
+ }
+
+ if ((qemu_driver->inotifyWatch =
+ virEventAddHandle(qemu_driver->inotifyFD, VIR_EVENT_HANDLE_READABLE,
+ qemuInotifyEvent, qemu_driver, NULL)) < 0)
+ goto error;
+
/* If hugetlbfs is present, then we need to create a sub-directory within
* it, since we can't assume the root mount point has permissions that
* will let our spawned QEMU instances use it.
--
1.8.1.4
5:25 p.m.
New subject: [libvirt] [patch v3 2/2] add inotify handler to qemu driver
On Wed, May 07, 2014 at 18:11:50 +0800, Chen Fan wrote:
we don't expect to reload 'migrate_uri' with restarting
libvirtd everytime while
updating the URI, so adding inotify handler to reload 'migrate_uri'
configuration
without restarting libvirtd, it will be also helpful for virt-manager to get
'migrate_uri'.
NACK, if we ever want configuration to be automatically reloaded when
configuration file changes (which I seriously doubt, SIGHUP is the
standard way of reloading configuration files), we certainly would not
want to do it this hacky way and only for one specific option. Not to
mention that the content of virQEMUDriverConfig must not change, a
completely new structure has to be created with the changed values.
Anyway, it's good you removed it from v4.
Jirka
10:07 a.m.
New subject: [libvirt] [patch v3 2/2] add inotify handler to qemu driver
On Thu, 2014-05-15 at 11:25 +0200, Jiri Denemark wrote:
On Wed, May 07, 2014 at 18:11:50 +0800, Chen Fan wrote:
> we don't expect to reload 'migrate_uri' with restarting libvirtd
everytime while
> updating the URI, so adding inotify handler to reload 'migrate_uri'
configuration
> without restarting libvirtd, it will be also helpful for virt-manager to get
> 'migrate_uri'.
NACK, if we ever want configuration to be automatically reloaded when
configuration file changes (which I seriously doubt, SIGHUP is the
standard way of reloading configuration files), we certainly would not
want to do it this hacky way and only for one specific option. Not to
mention that the content of virQEMUDriverConfig must not change, a
completely new structure has to be created with the changed values.
Hi Jiri,
thanks for reminding me, actually, I knew the virQEMUDriverConfig
without lock protect to reload is wrong. except that I think there are
some options in virQEMUDriverConfig can be reloaded. as you said, If
we want to reload them, Do we must create a new structure to save the
changes, then we must use 'lock' to make code thread safe and override
the qemuDriverCfg values everywhere while we use them. right?
or are there any other ideas?
Thanks,
Chen
Anyway, it's good you removed it from v4.
Jirka
7:12 p.m.
New subject: [libvirt] [patch v3 2/2] add inotify handler to qemu driver
On Thu, May 22, 2014 at 02:07:32 +0000, chen.fan.fnst(a)cn.fujitsu.com wrote:
On Thu, 2014-05-15 at 11:25 +0200, Jiri Denemark wrote:
> On Wed, May 07, 2014 at 18:11:50 +0800, Chen Fan wrote:
> > we don't expect to reload 'migrate_uri' with restarting libvirtd
everytime while
> > updating the URI, so adding inotify handler to reload 'migrate_uri'
configuration
> > without restarting libvirtd, it will be also helpful for virt-manager to get
> > 'migrate_uri'.
>
> NACK, if we ever want configuration to be automatically reloaded when
> configuration file changes (which I seriously doubt, SIGHUP is the
> standard way of reloading configuration files), we certainly would not
> want to do it this hacky way and only for one specific option. Not to
> mention that the content of virQEMUDriverConfig must not change, a
> completely new structure has to be created with the changed values.
>
Hi Jiri,
thanks for reminding me, actually, I knew the virQEMUDriverConfig
without lock protect to reload is wrong. except that I think there are
some options in virQEMUDriverConfig can be reloaded. as you said, If
we want to reload them, Do we must create a new structure to save the
changes, then we must use 'lock' to make code thread safe and override
the qemuDriverCfg values everywhere while we use them. right?
or are there any other ideas?
virQEMUDriverConfig is immutable so the structure returned by
virQEMUDriverGetConfig(qemu_driver) must never be changed. You have to
create a copy of it, update the values in the new structure, take a
driver lock, unref the original driver->config and put the new pointer
to driver->config. This way, any code that already got
virQEMUDriverConfig pointer will be using the old value and all calls to
virQEMUDriverGetConfig will return the updated configuration. The old
one will be freed when its last user unrefs it.
But anyway, we should not be updating the config whenever the
configuration file changes. We should rather do that in qemuStateReload.
However, I'm not sure what to do if there are options that just cannot
be changed while libvirtd is running. Applying some changes while
ignoring others when SIGHUP is sent to libvirtd would certainly lead to
confusion.
Jirka
6:21 p.m.
New subject: [libvirt] [patch v3 2/2] add inotify handler to qemu driver
On Thu, 2014-05-22 at 13:12 +0200, Jiri Denemark wrote:
On Thu, May 22, 2014 at 02:07:32 +0000, chen.fan.fnst(a)cn.fujitsu.com
wrote:
> On Thu, 2014-05-15 at 11:25 +0200, Jiri Denemark wrote:
> > On Wed, May 07, 2014 at 18:11:50 +0800, Chen Fan wrote:
> > > we don't expect to reload 'migrate_uri' with restarting
libvirtd everytime while
> > > updating the URI, so adding inotify handler to reload
'migrate_uri' configuration
> > > without restarting libvirtd, it will be also helpful for virt-manager to
get
> > > 'migrate_uri'.
> >
> > NACK, if we ever want configuration to be automatically reloaded when
> > configuration file changes (which I seriously doubt, SIGHUP is the
> > standard way of reloading configuration files), we certainly would not
> > want to do it this hacky way and only for one specific option. Not to
> > mention that the content of virQEMUDriverConfig must not change, a
> > completely new structure has to be created with the changed values.
> >
> Hi Jiri,
> thanks for reminding me, actually, I knew the virQEMUDriverConfig
> without lock protect to reload is wrong. except that I think there are
> some options in virQEMUDriverConfig can be reloaded. as you said, If
> we want to reload them, Do we must create a new structure to save the
> changes, then we must use 'lock' to make code thread safe and override
> the qemuDriverCfg values everywhere while we use them. right?
> or are there any other ideas?
virQEMUDriverConfig is immutable so the structure returned by
virQEMUDriverGetConfig(qemu_driver) must never be changed. You have to
create a copy of it, update the values in the new structure, take a
driver lock, unref the original driver->config and put the new pointer
to driver->config. This way, any code that already got
virQEMUDriverConfig pointer will be using the old value and all calls to
virQEMUDriverGetConfig will return the updated configuration. The old
one will be freed when its last user unrefs it.
But anyway, we should not be updating the config whenever the
configuration file changes. We should rather do that in qemuStateReload.
However, I'm not sure what to do if there are options that just cannot
be changed while libvirtd is running. Applying some changes while
ignoring others when SIGHUP is sent to libvirtd would certainly lead to
confusion.
How about dividing this qemu_config into two parts, one is used for the
immutable values, and the other is used for the mutable values? then if
SIGHUP is sent to libvirtd, we can use the above method you mentioned to
update the mutable configuration. maybe we can add an independent
structure pointer in virQEMUDriverConfig to save them. and only make a
copy of this mutable config rather than the entire virQEMUDriverConfig.
Thanks,
Chen
Jirka
5:17 p.m.
New subject: [libvirt] [patch v3 2/2] add inotify handler to qemu driver
On Mon, May 26, 2014 at 10:21:09 +0000, chen.fan.fnst(a)cn.fujitsu.com wrote:
On Thu, 2014-05-22 at 13:12 +0200, Jiri Denemark wrote:
> On Thu, May 22, 2014 at 02:07:32 +0000, chen.fan.fnst(a)cn.fujitsu.com wrote:
> > On Thu, 2014-05-15 at 11:25 +0200, Jiri Denemark wrote:
> > > On Wed, May 07, 2014 at 18:11:50 +0800, Chen Fan wrote:
> > > > we don't expect to reload 'migrate_uri' with restarting
libvirtd everytime while
> > > > updating the URI, so adding inotify handler to reload
'migrate_uri' configuration
> > > > without restarting libvirtd, it will be also helpful for virt-manager
to get
> > > > 'migrate_uri'.
> > >
> > > NACK, if we ever want configuration to be automatically reloaded when
> > > configuration file changes (which I seriously doubt, SIGHUP is the
> > > standard way of reloading configuration files), we certainly would not
> > > want to do it this hacky way and only for one specific option. Not to
> > > mention that the content of virQEMUDriverConfig must not change, a
> > > completely new structure has to be created with the changed values.
> > >
> > Hi Jiri,
> > thanks for reminding me, actually, I knew the virQEMUDriverConfig
> > without lock protect to reload is wrong. except that I think there are
> > some options in virQEMUDriverConfig can be reloaded. as you said, If
> > we want to reload them, Do we must create a new structure to save the
> > changes, then we must use 'lock' to make code thread safe and override
> > the qemuDriverCfg values everywhere while we use them. right?
> > or are there any other ideas?
>
> virQEMUDriverConfig is immutable so the structure returned by
> virQEMUDriverGetConfig(qemu_driver) must never be changed. You have to
> create a copy of it, update the values in the new structure, take a
> driver lock, unref the original driver->config and put the new pointer
> to driver->config. This way, any code that already got
> virQEMUDriverConfig pointer will be using the old value and all calls to
> virQEMUDriverGetConfig will return the updated configuration. The old
> one will be freed when its last user unrefs it.
>
> But anyway, we should not be updating the config whenever the
> configuration file changes. We should rather do that in qemuStateReload.
> However, I'm not sure what to do if there are options that just cannot
> be changed while libvirtd is running. Applying some changes while
> ignoring others when SIGHUP is sent to libvirtd would certainly lead to
> confusion.
How about dividing this qemu_config into two parts, one is used for the
immutable values, and the other is used for the mutable values? then if
SIGHUP is sent to libvirtd, we can use the above method you mentioned to
update the mutable configuration. maybe we can add an independent
structure pointer in virQEMUDriverConfig to save them. and only make a
copy of this mutable config rather than the entire virQEMUDriverConfig.
As I said, I don't really like the idea of applying changes to selected
options while ignoring the rest of the options. Moreover since some
options cannot be easily changed even on SIGHUP and since changing
libvirt's configuration is not something anyone would do every minute, I
think the requirement to restart libvirtd to apply configuration changes
is good enough.
Jirka
11:44 a.m.
New subject: [libvirt] [patch v3 2/2] add inotify handler to qemu driver
On Tue, 2014-05-27 at 11:17 +0200, Jiri Denemark wrote:
On Mon, May 26, 2014 at 10:21:09 +0000, chen.fan.fnst(a)cn.fujitsu.com
wrote:
> On Thu, 2014-05-22 at 13:12 +0200, Jiri Denemark wrote:
> > On Thu, May 22, 2014 at 02:07:32 +0000, chen.fan.fnst(a)cn.fujitsu.com wrote:
> > > On Thu, 2014-05-15 at 11:25 +0200, Jiri Denemark wrote:
> > > > On Wed, May 07, 2014 at 18:11:50 +0800, Chen Fan wrote:
> > > > > we don't expect to reload 'migrate_uri' with
restarting libvirtd everytime while
> > > > > updating the URI, so adding inotify handler to reload
'migrate_uri' configuration
> > > > > without restarting libvirtd, it will be also helpful for
virt-manager to get
> > > > > 'migrate_uri'.
> > > >
> > > > NACK, if we ever want configuration to be automatically reloaded
when
> > > > configuration file changes (which I seriously doubt, SIGHUP is the
> > > > standard way of reloading configuration files), we certainly would
not
> > > > want to do it this hacky way and only for one specific option. Not
to
> > > > mention that the content of virQEMUDriverConfig must not change, a
> > > > completely new structure has to be created with the changed values.
> > > >
> > > Hi Jiri,
> > > thanks for reminding me, actually, I knew the virQEMUDriverConfig
> > > without lock protect to reload is wrong. except that I think there are
> > > some options in virQEMUDriverConfig can be reloaded. as you said, If
> > > we want to reload them, Do we must create a new structure to save the
> > > changes, then we must use 'lock' to make code thread safe and
override
> > > the qemuDriverCfg values everywhere while we use them. right?
> > > or are there any other ideas?
> >
> > virQEMUDriverConfig is immutable so the structure returned by
> > virQEMUDriverGetConfig(qemu_driver) must never be changed. You have to
> > create a copy of it, update the values in the new structure, take a
> > driver lock, unref the original driver->config and put the new pointer
> > to driver->config. This way, any code that already got
> > virQEMUDriverConfig pointer will be using the old value and all calls to
> > virQEMUDriverGetConfig will return the updated configuration. The old
> > one will be freed when its last user unrefs it.
> >
> > But anyway, we should not be updating the config whenever the
> > configuration file changes. We should rather do that in qemuStateReload.
> > However, I'm not sure what to do if there are options that just cannot
> > be changed while libvirtd is running. Applying some changes while
> > ignoring others when SIGHUP is sent to libvirtd would certainly lead to
> > confusion.
> How about dividing this qemu_config into two parts, one is used for the
> immutable values, and the other is used for the mutable values? then if
> SIGHUP is sent to libvirtd, we can use the above method you mentioned to
> update the mutable configuration. maybe we can add an independent
> structure pointer in virQEMUDriverConfig to save them. and only make a
> copy of this mutable config rather than the entire virQEMUDriverConfig.
As I said, I don't really like the idea of applying changes to selected
options while ignoring the rest of the options. Moreover since some
options cannot be easily changed even on SIGHUP and since changing
libvirt's configuration is not something anyone would do every minute, I
think the requirement to restart libvirtd to apply configuration changes
is good enough.
I'm thinking more about virt-manager, If having to restart libvirtd to
apply changed configuration, the virt-manager must reconnect with this
host's libvirtd every time. if support this without restart libvirtd,
isn't it better?
Thanks,
Chen
Jirka
3754
days inactive
3776
days old
10 comments
4 participants
participants (4)
-
Chen Fan -
chen.fan.fnst＠cn.fujitsu.com
-
Jiri Denemark -
Wangrui (K)