[PATCH 0/2] qemu: Two almost trivial migration fixes

The report on libvirt-users [1] got me into testing and I've found another problem fixed in the other patch. 1: https://www.redhat.com/archives/libvirt-users/2020-October/msg00035.html Michal Prívozník (2): qemu: Don't try to start NBD server twice qemu_migration: Don't mangle NBD part of migration cookie src/qemu/qemu_migration.c | 8 +++++--- src/qemu/qemu_migration_cookie.c | 2 +- 2 files changed, 6 insertions(+), 4 deletions(-) -- 2.26.2

In one of recent patches the way that we start NBD server for incoming migration was reworked (v6.8.0-rc1~298). A new boolean was introduced that tracks whether the NBD server was started so that we don't start it twice nor record in the port in the port allocator twice. Well, this idea is good, but in the implementation the boolean is never set, so we are reserving the port twice and would be starting the NBD server twice too if it wasn't for port reservation fail. Fixes: e74d627bb3bc2684cbe3edc1e2f7cc745b4e1ff3 Reported-by: Vjaceslavs Klimovs <vklimovs@gmail.com> Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_migration.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 2f5d61f8e7..6f764b0c73 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -479,9 +479,11 @@ qemuMigrationDstStartNBDServer(virQEMUDriverPtr driver, QEMU_ASYNC_JOB_MIGRATION_IN) < 0) goto cleanup; - if (!server_started && - qemuMonitorNBDServerStart(priv->mon, &server, tls_alias) < 0) - goto exit_monitor; + if (!server_started) { + if (qemuMonitorNBDServerStart(priv->mon, &server, tls_alias) < 0) + goto exit_monitor; + server_started = true; + } if (qemuBlockExportAddNBD(vm, diskAlias, disk->src, diskAlias, true, NULL) < 0) goto exit_monitor; -- 2.26.2

On Mon, Oct 26, 2020 at 11:41:35 +0100, Michal Privoznik wrote:
In one of recent patches the way that we start NBD server for incoming migration was reworked (v6.8.0-rc1~298). A new boolean was introduced that tracks whether the NBD server was started so that we don't start it twice nor record in the port in the port allocator twice. Well, this idea is good, but in the implementation the boolean is never set, so we are reserving the port twice and would be starting the NBD server twice too if it wasn't for port reservation fail.
Fixes: e74d627bb3bc2684cbe3edc1e2f7cc745b4e1ff3 Reported-by: Vjaceslavs Klimovs <vklimovs@gmail.com> Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_migration.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>

In recent commit v6.8.0-135-g518be41aaa the formatting of NBD into migration cookie was moved into a separate function and with it it was switched from direct printing into the output buffer to virXMLFormatElement(). But there was a typo. The virXMLFormatElement() accepts two buffers on input, one for element attributes and another for child elements. Well, the line that was supposed to add NBD port into the attributes buffer printed the attribute directly into the output buffer which produced this mangled XML: <qemu-migration> port='49153'<nbd> <disk target='vda' capacity='8589934592'/> <disk target='vdb' capacity='12746752000'/> </nbd> </qemu-migration> Changing the incriminated line to print into the attributes buffer fixes the problem. Fixes: 518be41aaa3ebaac5f2307f268d24dc1b40b6b5c Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_migration_cookie.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_migration_cookie.c b/src/qemu/qemu_migration_cookie.c index 708c2cced7..39445ef8de 100644 --- a/src/qemu/qemu_migration_cookie.c +++ b/src/qemu/qemu_migration_cookie.c @@ -747,7 +747,7 @@ qemuMigrationCookieNBDXMLFormat(qemuMigrationCookieNBDPtr nbd, size_t i; if (nbd->port) - virBufferAsprintf(buf, " port='%d'", nbd->port); + virBufferAsprintf(&attrBuf, " port='%d'", nbd->port); for (i = 0; i < nbd->ndisks; i++) { virBufferEscapeString(&childBuf, "<disk target='%s'", nbd->disks[i].target); -- 2.26.2

On Mon, Oct 26, 2020 at 11:41:36 +0100, Michal Privoznik wrote:
In recent commit v6.8.0-135-g518be41aaa the formatting of NBD into migration cookie was moved into a separate function and with it it was switched from direct printing into the output buffer to virXMLFormatElement(). But there was a typo. The virXMLFormatElement() accepts two buffers on input, one for element attributes and another for child elements. Well, the line that was supposed to add NBD port into the attributes buffer printed the attribute directly into the output buffer which produced this mangled XML:
<qemu-migration> port='49153'<nbd> <disk target='vda' capacity='8589934592'/> <disk target='vdb' capacity='12746752000'/> </nbd> </qemu-migration>
Changing the incriminated line to print into the attributes buffer fixes the problem.
Fixes: 518be41aaa3ebaac5f2307f268d24dc1b40b6b5c Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_migration_cookie.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Oops! I'm planning on adding tests for the qemu-migration cookie XML since we don't have any and it would prevent this problem. Reviewed-by: Peter Krempa <pkrempa@redhat.com>
participants (2)
-
Michal Privoznik
-
Peter Krempa