On Tue, Jan 16, 2024 at 04:25:03PM +0100, Peter Krempa wrote: > Originally the migration code didn't register the NBD disk port with the > port allocator when it was manually specified. Later when commit > e74d627bb3bc2684cbe3 refactored the code and started registering the old > logic which was clearing 'priv->nbdPort' in case when it was manually > specified was not removed. s/registering/registering it, / > This caused following problems: > - the port was not released after successful migration > - the port was released even when it was not allocated on failures > regarding the NBD server start > - the port was not released on other failures of the migration after > NBD server startup > > To address this we remove the assumption that 'priv->nbdPort' is used > only for auto-allocated port and fill it only once the port is > allocated and make the caller of qemuMigrationDstStartNBDServer > responsilbe for releasing it. *responsible > +++ b/src/qemu/qemu_migration.c > @@ -627,6 +629,9 @@ qemuMigrationDstStartNBDServer(virQEMUDriver *driver, > > server.port = port; > } > + > + /* caller will release the port on failure */ > + priv->nbdPort = server.port; The port is ultimately released even on success, though it will happen further up the stack. Still, it feels like the comment would be more accurate and consistent with the function's documentation if you dropped the "on failure" part. With the commit message fixed and the comment optionally tweaked, Reviewed-by: Andrea Bolognani <abologna@xxxxxxxxxx> -- Andrea Bolognani / Red Hat / Virtualization _______________________________________________ Devel mailing list -- devel@xxxxxxxxxxxxxxxxx To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx
