On Mon, Feb 23, 2026 at 19:52:22 -0300, Lucas Amaral wrote:
> Previously, users had to manually specify a TCP port when starting
> a pull-mode backup with an NBD server. A TODO comment in
> qemuBackupPrepare() noted this limitation and pointed toward using
> virPortAllocator, as done for migration, VNC, and SPICE ports.
>
> Add backup_port_min and backup_port_max configuration options to
> qemu.conf, defaulting to 10809-10872 (10809 is the IANA-assigned
> NBD port; range of 64 matches the migration port pattern).
>
> When a pull-mode backup is started without specifying a TCP port,
> a port is now acquired automatically from the configured range via
> virPortAllocatorAcquire(). The port is released when the backup
> ends or if startup fails.
>
> Signed-off-by: Lucas Amaral <[email protected]>
> ---
[...]
> docs/formatbackup.rst | 3 ++-
> src/qemu/libvirtd_qemu.aug | 2 ++
> src/qemu/qemu.conf.in | 9 ++++++++
> src/qemu/qemu_backup.c | 33 +++++++++++++++++++++---------
> src/qemu/qemu_conf.c | 25 ++++++++++++++++++++++
> src/qemu/qemu_conf.h | 6 ++++++
> src/qemu/qemu_domain.h | 1 +
> src/qemu/qemu_driver.c | 6 ++++++
> src/qemu/test_libvirtd_qemu.aug.in | 2 ++
> 9 files changed, 76 insertions(+), 11 deletions(-)
Sorry for the delay in reviewing this patch.
The code looks good, but normally we'd request splitting of the
'qemu.conf' related changes into a separate patch.
In this case I'll do the split (obviously preserving authorship) before
pushing so that you don't need to follow up with another version.
> diff --git a/docs/formatbackup.rst b/docs/formatbackup.rst
> index df6392e3bd..24857eaf72 100644
> --- a/docs/formatbackup.rst
> +++ b/docs/formatbackup.rst
> @@ -42,7 +42,8 @@ were supplied). The following child elements and attributes
> are supported:
> ```protocol`` element of a disk
> <formatdomain.html#hard-drives-floppy-disks-cdroms>`__ attached
> via NBD in the domain (such as transport, socket, name, port, or tls),
> necessary to set up an NBD server that exposes the content of each disk at
> - the time the backup is started.
> + the time the backup is started. For TCP transport, if ``port`` is
> omitted, a
> + port is allocated automatically from the range configured in
> ``/etc/libvirt/qemu.conf``.
>
> In addition to the above the NBD server used for backups allows using
> ``transport='fd' fdgroup='NAME'`` where ``NAME`` is the name used with
> diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug
> index 0286582169..eb790d48be 100644
> --- a/src/qemu/libvirtd_qemu.aug
> +++ b/src/qemu/libvirtd_qemu.aug
> @@ -146,6 +146,8 @@ module Libvirtd_qemu =
> | int_entry "migration_port_min"
> | int_entry "migration_port_max"
> | str_entry "migration_host"
> + | int_entry "backup_port_min"
> + | int_entry "backup_port_max"
>
> let log_entry = bool_entry "log_timestamp"
>
> diff --git a/src/qemu/qemu.conf.in b/src/qemu/qemu.conf.in
> index 2d1c67034d..5eacd70022 100644
> --- a/src/qemu/qemu.conf.in
> +++ b/src/qemu/qemu.conf.in
> @@ -969,6 +969,15 @@
> #migration_port_max = 49215
>
>
> +# Port range used for automatic allocation of NBD backup server ports.
> +# When a pull-mode backup is started without specifying a TCP port, a
> +# port from this range will be assigned automatically. The NBD standard
> +# port is 10809.
> +#
> +#backup_port_min = 10809
> +#backup_port_max = 10872
> +
> +
> # Timestamp QEMU's log messages (if QEMU supports it)
> #
> # Defaults to 1.
> diff --git a/src/qemu/qemu_backup.c b/src/qemu/qemu_backup.c
> index 44514d08fc..12ef64b14e 100644
> --- a/src/qemu/qemu_backup.c
> +++ b/src/qemu/qemu_backup.c
> @@ -32,6 +32,7 @@
> #include "storage_source.h"
> #include "storage_source_conf.h"
> #include "virerror.h"
> +#include "virportallocator.h"
> #include "virlog.h"
> #include "virbuffer.h"
> #include "backup_conf.h"
> @@ -71,16 +72,7 @@ qemuBackupPrepare(virDomainBackupDef *def)
>
> switch (def->server->transport) {
> case VIR_STORAGE_NET_HOST_TRANS_TCP:
> - /* TODO: Update qemu.conf to provide a port range,
> - * probably starting at 10809, for obtaining automatic
> - * port via virPortAllocatorAcquire, as well as store
> - * somewhere if we need to call virPortAllocatorRelease
> - * during BackupEnd. Until then, user must provide port */
> - if (!def->server->port) {
> - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> - _("<domainbackup> must specify TCP port for
> now"));
> - return -1;
> - }
> + /* port is auto-allocated if not set */
> break;
>
> case VIR_STORAGE_NET_HOST_TRANS_UNIX:
> @@ -841,6 +833,16 @@ qemuBackupBegin(virDomainObj *vm,
> if (qemuBackupPrepare(def) < 0)
> goto endjob;
>
> + if (pull && def->server &&
> + def->server->transport == VIR_STORAGE_NET_HOST_TRANS_TCP &&
> + !def->server->port) {
> + unsigned short port = 0;
> + if (virPortAllocatorAcquire(priv->driver->backupPorts, &port) < 0)
> + goto endjob;
> + def->server->port = port;
> + priv->backupNBDPort = port;
> + }
This code belongs to BackupPrepare instead of the comment.
> +
> if (qemuBackupBeginPrepareTLS(vm, cfg, def, &tlsProps, &tlsSecretProps)
> < 0)
> goto endjob;
>
> @@ -969,6 +971,11 @@ qemuBackupBegin(virDomainObj *vm,
> qemuDomainObjExitMonitor(vm);
> }
>
> + if (ret < 0 && priv->backupNBDPort) {
> + virPortAllocatorRelease(priv->backupNBDPort);
> + priv->backupNBDPort = 0;
> + }
> +
> if (ret < 0 && !job_started && priv->backup)
> def = g_steal_pointer(&priv->backup);
>
> @@ -1026,6 +1033,12 @@ qemuBackupNotifyBlockjobEndStopNBD(virDomainObj *vm,
> qemuDomainObjExitMonitor(vm);
>
> backup->nbdStopped = true;
> +
> + if (priv->backupNBDPort) {
> + virPortAllocatorRelease(priv->backupNBDPort);
> + priv->backupNBDPort = 0;
> + backup->server->port = 0;
> + }
> }
>
>
> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index de6e51177a..99e6a29148 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c
> @@ -74,6 +74,9 @@ VIR_LOG_INIT("qemu.qemu_conf");
> #define QEMU_MIGRATION_PORT_MIN 49152
> #define QEMU_MIGRATION_PORT_MAX 49215
>
> +#define QEMU_BACKUP_PORT_MIN 10809
> +#define QEMU_BACKUP_PORT_MAX 10872
> +
> VIR_ENUM_IMPL(virQEMUSchedCore,
> QEMU_SCHED_CORE_LAST,
> "none",
> @@ -265,6 +268,9 @@ virQEMUDriverConfig *virQEMUDriverConfigNew(bool
> privileged,
> cfg->migrationPortMin = QEMU_MIGRATION_PORT_MIN;
> cfg->migrationPortMax = QEMU_MIGRATION_PORT_MAX;
>
> + cfg->backupPortMin = QEMU_BACKUP_PORT_MIN;
> + cfg->backupPortMax = QEMU_BACKUP_PORT_MAX;
> +
> /* For privileged driver, try and find hugetlbfs mounts automatically.
> * Non-privileged driver requires admin to create a dir for the
> * user, chown it, and then let user configure it manually. */
> @@ -985,6 +991,25 @@ virQEMUDriverConfigLoadNetworkEntry(virQEMUDriverConfig
> *cfg,
> return -1;
> }
>
> + if (virConfGetValueUInt(conf, "backup_port_min", &cfg->backupPortMin) <
> 0)
> + return -1;
> + if (cfg->backupPortMin <= 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("%1$s: backup_port_min: port must be greater than
> 0"),
> + filename);
> + return -1;
> + }
> +
> + if (virConfGetValueUInt(conf, "backup_port_max", &cfg->backupPortMax) <
> 0)
> + return -1;
> + if (cfg->backupPortMax > 65535 ||
> + cfg->backupPortMax < cfg->backupPortMin) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("%1$s: backup_port_max: port must be between the
> minimal port %2$d and 65535"),
> + filename, cfg->backupPortMin);
Pre existing problem (in other port ranges) that I'll fix in a follow-up.
VIR_ERR_INTERNAL_ERROR is not appropriate error code for user
mis-configuration.
> + return -1;
> + }
> +
> if (virConfGetValueString(conf, "migration_host", &cfg->migrateHost) < 0)
> return -1;
> virStringStripIPv6Brackets(cfg->migrateHost);
Reviewed-by: Peter Krempa <[email protected]>
