8:45 a.m.
We added support for dula <graphics>, but there were places in the
code which still needed to be adapted. Two tiny patches seem to fix
it all. Tested, works fine.
Martin Kletzander (2):
qemu: Don't miss errors when changing graphics passwords
qemu: Allow seamless migration for domains with multiple graphics
src/qemu/qemu_migration.c | 29 ++++++++++++++++++-----------
src/qemu/qemu_process.c | 6 +++---
2 files changed, 21 insertions(+), 14 deletions(-)
--
1.8.2.1
8:46 a.m.
New subject: [libvirt] [PATCH 1/2] qemu: Don't miss errors when changing graphics passwords
Commit 23e8b5d8e7a92bac85b7fd2aca8992501bf680ee forgot to check the
return value for all calls to qemuDomainChangeGraphicsPasswords().
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
src/qemu/qemu_process.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index ac5ffcf..7dc7f65 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -2015,10 +2015,10 @@ qemuProcessInitPasswords(virConnectPtr conn,
&graphics->data.spice.auth,
cfg->spicePassword);
}
- }
- if (ret < 0)
- goto cleanup;
+ if (ret < 0)
+ goto cleanup;
+ }
if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) {
for (i = 0; i < vm->def->ndisks; i++) {
--
1.8.2.1
6:10 a.m.
New subject: [libvirt] [PATCH 1/2] qemu: Don't miss errors when changing graphics passwords
On Tue, Jul 02, 2013 at 03:46:00PM +0200, Martin Kletzander wrote:
Commit 23e8b5d8e7a92bac85b7fd2aca8992501bf680ee forgot to check the
return value for all calls to qemuDomainChangeGraphicsPasswords().
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
src/qemu/qemu_process.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index ac5ffcf..7dc7f65 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -2015,10 +2015,10 @@ qemuProcessInitPasswords(virConnectPtr conn,
&graphics->data.spice.auth,
cfg->spicePassword);
}
- }
- if (ret < 0)
- goto cleanup;
+ if (ret < 0)
+ goto cleanup;
+ }
if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) {
for (i = 0; i < vm->def->ndisks; i++) {
ACK
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
8:46 a.m.
New subject: [libvirt] [PATCH 2/2] qemu: Allow seamless migration for domains with multiple graphics
Since commit 23e8b5d8, the code is refactored in a way that supports
domains with multiple graphics elements and commit 37b415200 allows
starting such domains. However none of those commits take migration
into account. Even though qemu doesn't support relocation for
anything else than VNC and for no more than one graphics, there is no
reason to hardcode one graphics into this part of the code as well.
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
src/qemu/qemu_migration.c | 29 ++++++++++++++++++-----------
1 file changed, 18 insertions(+), 11 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 69d5398..d1a86b7 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -427,19 +427,21 @@ qemuMigrationCookieAddGraphics(qemuMigrationCookiePtr mig,
virQEMUDriverPtr driver,
virDomainObjPtr dom)
{
+ size_t i = 0;
+
if (mig->flags & QEMU_MIGRATION_COOKIE_GRAPHICS) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Migration graphics data already present"));
return -1;
}
- if (dom->def->ngraphics == 1 &&
- (dom->def->graphics[0]->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC ||
- dom->def->graphics[0]->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE)) {
- if (!(mig->graphics =
- qemuMigrationCookieGraphicsAlloc(driver, dom->def->graphics[0])))
- return -1;
- mig->flags |= QEMU_MIGRATION_COOKIE_GRAPHICS;
+ for (i = 0; i < dom->def->ngraphics; i++) {
+ if (dom->def->graphics[i]->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE) {
+ if (!(mig->graphics =
+ qemuMigrationCookieGraphicsAlloc(driver, dom->def->graphics[i])))
+ return -1;
+ mig->flags |= QEMU_MIGRATION_COOKIE_GRAPHICS;
+ }
}
return 0;
@@ -1602,11 +1604,16 @@ qemuMigrationWaitForSpice(virQEMUDriverPtr driver,
qemuDomainObjPrivatePtr priv = vm->privateData;
bool wait_for_spice = false;
bool spice_migrated = false;
+ size_t i = 0;
- if (vm->def->ngraphics == 1 &&
- vm->def->graphics[0]->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE &&
- virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_SEAMLESS_MIGRATION))
- wait_for_spice = true;
+ if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_SEAMLESS_MIGRATION)) {
+ for (i = 0; i < vm->def->ngraphics; i++) {
+ if (vm->def->graphics[i]->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE) {
+ wait_for_spice = true;
+ break;
+ }
+ }
+ }
if (!wait_for_spice)
return 0;
--
1.8.2.1
6:10 a.m.
New subject: [libvirt] [PATCH 2/2] qemu: Allow seamless migration for domains with multiple graphics
On Tue, Jul 02, 2013 at 03:46:01PM +0200, Martin Kletzander wrote:
Since commit 23e8b5d8, the code is refactored in a way that supports
domains with multiple graphics elements and commit 37b415200 allows
starting such domains. However none of those commits take migration
into account. Even though qemu doesn't support relocation for
anything else than VNC and for no more than one graphics, there is no
reason to hardcode one graphics into this part of the code as well.
I think you mean s/VNC/SPICE/ here.
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
src/qemu/qemu_migration.c | 29 ++++++++++++++++++-----------
1 file changed, 18 insertions(+), 11 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 69d5398..d1a86b7 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -427,19 +427,21 @@ qemuMigrationCookieAddGraphics(qemuMigrationCookiePtr mig,
virQEMUDriverPtr driver,
virDomainObjPtr dom)
{
+ size_t i = 0;
+
if (mig->flags & QEMU_MIGRATION_COOKIE_GRAPHICS) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Migration graphics data already present"));
return -1;
}
- if (dom->def->ngraphics == 1 &&
- (dom->def->graphics[0]->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC ||
- dom->def->graphics[0]->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE)) {
- if (!(mig->graphics =
- qemuMigrationCookieGraphicsAlloc(driver, dom->def->graphics[0])))
- return -1;
- mig->flags |= QEMU_MIGRATION_COOKIE_GRAPHICS;
+ for (i = 0; i < dom->def->ngraphics; i++) {
+ if (dom->def->graphics[i]->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE) {
+ if (!(mig->graphics =
+ qemuMigrationCookieGraphicsAlloc(driver,
dom->def->graphics[i])))
+ return -1;
+ mig->flags |= QEMU_MIGRATION_COOKIE_GRAPHICS;
+ }
}
return 0;
@@ -1602,11 +1604,16 @@ qemuMigrationWaitForSpice(virQEMUDriverPtr driver,
qemuDomainObjPrivatePtr priv = vm->privateData;
bool wait_for_spice = false;
bool spice_migrated = false;
+ size_t i = 0;
- if (vm->def->ngraphics == 1 &&
- vm->def->graphics[0]->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE
&&
- virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_SEAMLESS_MIGRATION))
- wait_for_spice = true;
+ if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_SEAMLESS_MIGRATION)) {
+ for (i = 0; i < vm->def->ngraphics; i++) {
+ if (vm->def->graphics[i]->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE)
{
+ wait_for_spice = true;
+ break;
+ }
+ }
+ }
if (!wait_for_spice)
return 0;
ACK with subject fix
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
7:11 a.m.
New subject: [libvirt] [PATCH 2/2] qemu: Allow seamless migration for domains with multiple graphics
On Wed, Jul 03, 2013 at 12:10:23 +0100, Daniel Berrange wrote:
On Tue, Jul 02, 2013 at 03:46:01PM +0200, Martin Kletzander wrote:
> Since commit 23e8b5d8, the code is refactored in a way that supports
> domains with multiple graphics elements and commit 37b415200 allows
> starting such domains. However none of those commits take migration
> into account. Even though qemu doesn't support relocation for
> anything else than VNC and for no more than one graphics, there is no
> reason to hardcode one graphics into this part of the code as well.
I think you mean s/VNC/SPICE/ here.
>
> Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
> ---
> src/qemu/qemu_migration.c | 29 ++++++++++++++++++-----------
> 1 file changed, 18 insertions(+), 11 deletions(-)
>
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 69d5398..d1a86b7 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -427,19 +427,21 @@ qemuMigrationCookieAddGraphics(qemuMigrationCookiePtr mig,
> virQEMUDriverPtr driver,
> virDomainObjPtr dom)
> {
> + size_t i = 0;
> +
> if (mig->flags & QEMU_MIGRATION_COOKIE_GRAPHICS) {
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> _("Migration graphics data already present"));
> return -1;
> }
>
> - if (dom->def->ngraphics == 1 &&
> - (dom->def->graphics[0]->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC ||
> - dom->def->graphics[0]->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE))
{
> - if (!(mig->graphics =
> - qemuMigrationCookieGraphicsAlloc(driver,
dom->def->graphics[0])))
> - return -1;
> - mig->flags |= QEMU_MIGRATION_COOKIE_GRAPHICS;
> + for (i = 0; i < dom->def->ngraphics; i++) {
> + if (dom->def->graphics[i]->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE)
{
> + if (!(mig->graphics =
> + qemuMigrationCookieGraphicsAlloc(driver,
dom->def->graphics[i])))
> + return -1;
> + mig->flags |= QEMU_MIGRATION_COOKIE_GRAPHICS;
> + }
> }
I think you should break the loop inside the if statement to make it
clear qemuMigrationCookieGraphicsAlloc won't be called more than once
causing a memory leak.
Jirka
8:05 a.m.
New subject: [libvirt] [PATCH 2/2] qemu: Allow seamless migration for domains with multiple graphics
On 07/03/2013 02:11 PM, Jiri Denemark wrote:
On Wed, Jul 03, 2013 at 12:10:23 +0100, Daniel Berrange wrote:
> On Tue, Jul 02, 2013 at 03:46:01PM +0200, Martin Kletzander wrote:
>> Since commit 23e8b5d8, the code is refactored in a way that supports
>> domains with multiple graphics elements and commit 37b415200 allows
>> starting such domains. However none of those commits take migration
>> into account. Even though qemu doesn't support relocation for
>> anything else than VNC and for no more than one graphics, there is no
>> reason to hardcode one graphics into this part of the code as well.
>
> I think you mean s/VNC/SPICE/ here.
>
>>
>> Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
>> ---
>> src/qemu/qemu_migration.c | 29 ++++++++++++++++++-----------
>> 1 file changed, 18 insertions(+), 11 deletions(-)
>>
>> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
>> index 69d5398..d1a86b7 100644
>> --- a/src/qemu/qemu_migration.c
>> +++ b/src/qemu/qemu_migration.c
>> @@ -427,19 +427,21 @@ qemuMigrationCookieAddGraphics(qemuMigrationCookiePtr mig,
>> virQEMUDriverPtr driver,
>> virDomainObjPtr dom)
>> {
>> + size_t i = 0;
>> +
>> if (mig->flags & QEMU_MIGRATION_COOKIE_GRAPHICS) {
>> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> _("Migration graphics data already present"));
>> return -1;
>> }
>>
>> - if (dom->def->ngraphics == 1 &&
>> - (dom->def->graphics[0]->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC
||
>> - dom->def->graphics[0]->type ==
VIR_DOMAIN_GRAPHICS_TYPE_SPICE)) {
>> - if (!(mig->graphics =
>> - qemuMigrationCookieGraphicsAlloc(driver,
dom->def->graphics[0])))
>> - return -1;
>> - mig->flags |= QEMU_MIGRATION_COOKIE_GRAPHICS;
>> + for (i = 0; i < dom->def->ngraphics; i++) {
>> + if (dom->def->graphics[i]->type ==
VIR_DOMAIN_GRAPHICS_TYPE_SPICE) {
>> + if (!(mig->graphics =
>> + qemuMigrationCookieGraphicsAlloc(driver,
dom->def->graphics[i])))
>> + return -1;
>> + mig->flags |= QEMU_MIGRATION_COOKIE_GRAPHICS;
>> + }
>> }
I think you should break the loop inside the if statement to make it
clear qemuMigrationCookieGraphicsAlloc won't be called more than once
causing a memory leak.
Jirka
You're right, thanks for finding that out. I fixed this, the commit
message and pushed the series.
Thank you both for review,
Martin
4230
days inactive
4231
days old
6 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Jiri Denemark -
Martin Kletzander