[libvirt PATCH] qemu: Fix domain ID allocation
The rewrite to use GLib's atomic ops functions changed the behavior of virAtomicIntInc - before it returned the pre-increment value. Most of the callers using its value were adjusted, but the one in qemuDriverAllocateID was not. If libvirtd would reconnect to a running domain during startup, the next started domain would get the same ID: $ virsh list Id Name State -------------------------- 1 f28live running 1 f28live1 running Use the g_atomic_add function directly (as recommended in viratomic.h) and add 1 to the result. This also restores the usual numbering from 1 instead of 0. Signed-off-by: Ján Tomko <jtomko@redhat.com> Fixes: 7b9645a7d127a374b8d1c83fdf9789706dbab2c9 --- src/qemu/qemu_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index e5051027fc..0b119cbe78 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1858,7 +1858,7 @@ qemuSetUnprivSGIO(virDomainDeviceDefPtr dev) int qemuDriverAllocateID(virQEMUDriverPtr driver) { - return virAtomicIntInc(&driver->lastvmid); + return g_atomic_int_add(&driver->lastvmid, 1) + 1; } -- 2.21.0
On Fri, Jan 31, 2020 at 15:43:14 +0100, Ján Tomko wrote:
The rewrite to use GLib's atomic ops functions changed the behavior of virAtomicIntInc - before it returned the pre-increment value.
Most of the callers using its value were adjusted, but the one in qemuDriverAllocateID was not. If libvirtd would reconnect to a running domain during startup, the next started domain would get the same ID:
$ virsh list Id Name State -------------------------- 1 f28live running 1 f28live1 running
Use the g_atomic_add function directly (as recommended in viratomic.h) and add 1 to the result.
This also restores the usual numbering from 1 instead of 0.
Signed-off-by: Ján Tomko <jtomko@redhat.com> Fixes: 7b9645a7d127a374b8d1c83fdf9789706dbab2c9 --- src/qemu/qemu_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
On 1/31/20 3:43 PM, Ján Tomko wrote:
The rewrite to use GLib's atomic ops functions changed the behavior of virAtomicIntInc - before it returned the pre-increment value.
Most of the callers using its value were adjusted, but the one in qemuDriverAllocateID was not. If libvirtd would reconnect to a running domain during startup, the next started domain would get the same ID:
$ virsh list Id Name State -------------------------- 1 f28live running 1 f28live1 running
Use the g_atomic_add function directly (as recommended in viratomic.h) and add 1 to the result.
This also restores the usual numbering from 1 instead of 0.
Signed-off-by: Ján Tomko <jtomko@redhat.com> Fixes: 7b9645a7d127a374b8d1c83fdf9789706dbab2c9 --- src/qemu/qemu_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index e5051027fc..0b119cbe78 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1858,7 +1858,7 @@ qemuSetUnprivSGIO(virDomainDeviceDefPtr dev)
int qemuDriverAllocateID(virQEMUDriverPtr driver) { - return virAtomicIntInc(&driver->lastvmid); + return g_atomic_int_add(&driver->lastvmid, 1) + 1; }
Does it makes sense to replace all virAtomic with g_atomic or do we still pretend that we care about client library and we don't crash on OOM in it? Michal
On Fri, Jan 31, 2020 at 04:33:50PM +0100, Michal Privoznik wrote:
On 1/31/20 3:43 PM, Ján Tomko wrote:
The rewrite to use GLib's atomic ops functions changed the behavior of virAtomicIntInc - before it returned the pre-increment value.
Most of the callers using its value were adjusted, but the one in qemuDriverAllocateID was not. If libvirtd would reconnect to a running domain during startup, the next started domain would get the same ID:
$ virsh list Id Name State -------------------------- 1 f28live running 1 f28live1 running
Use the g_atomic_add function directly (as recommended in viratomic.h) and add 1 to the result.
This also restores the usual numbering from 1 instead of 0.
Signed-off-by: Ján Tomko <jtomko@redhat.com> Fixes: 7b9645a7d127a374b8d1c83fdf9789706dbab2c9 --- src/qemu/qemu_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index e5051027fc..0b119cbe78 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1858,7 +1858,7 @@ qemuSetUnprivSGIO(virDomainDeviceDefPtr dev) int qemuDriverAllocateID(virQEMUDriverPtr driver) { - return virAtomicIntInc(&driver->lastvmid); + return g_atomic_int_add(&driver->lastvmid, 1) + 1; }
Does it makes sense to replace all virAtomic with g_atomic or do we
That does make sense. It is encouraged in viratomic.h, as I said in the commit message. The reason I did not cleanup all of them is the bugfix nature of this patch.
still pretend that we care about client library and we don't crash on OOM in it?
I sincerely hope that none of the g_atomic functions allocate memory. Jano
Michal
On 1/31/20 4:45 PM, Ján Tomko wrote:
On Fri, Jan 31, 2020 at 04:33:50PM +0100, Michal Privoznik wrote:
On 1/31/20 3:43 PM, Ján Tomko wrote:
The rewrite to use GLib's atomic ops functions changed the behavior of virAtomicIntInc - before it returned the pre-increment value.
Most of the callers using its value were adjusted, but the one in qemuDriverAllocateID was not. If libvirtd would reconnect to a running domain during startup, the next started domain would get the same ID:
$ virsh list Id Name State -------------------------- 1 f28live running 1 f28live1 running
Use the g_atomic_add function directly (as recommended in viratomic.h) and add 1 to the result.
This also restores the usual numbering from 1 instead of 0.
Signed-off-by: Ján Tomko <jtomko@redhat.com> Fixes: 7b9645a7d127a374b8d1c83fdf9789706dbab2c9 --- src/qemu/qemu_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index e5051027fc..0b119cbe78 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1858,7 +1858,7 @@ qemuSetUnprivSGIO(virDomainDeviceDefPtr dev) int qemuDriverAllocateID(virQEMUDriverPtr driver) { - return virAtomicIntInc(&driver->lastvmid); + return g_atomic_int_add(&driver->lastvmid, 1) + 1; }
Does it makes sense to replace all virAtomic with g_atomic or do we
That does make sense. It is encouraged in viratomic.h, as I said in the commit message. The reason I did not cleanup all of them is the bugfix nature of this patch.
Aha! So we are already doing that. From our src/util/viratomic.h: #define virAtomicIntInc(i) g_atomic_int_add(i, 1) So all we need to do is just drop viratomic. Michal
On Fri, Jan 31, 2020 at 04:33:50PM +0100, Michal Privoznik wrote:
On 1/31/20 3:43 PM, Ján Tomko wrote:
The rewrite to use GLib's atomic ops functions changed the behavior of virAtomicIntInc - before it returned the pre-increment value.
Most of the callers using its value were adjusted, but the one in qemuDriverAllocateID was not. If libvirtd would reconnect to a running domain during startup, the next started domain would get the same ID:
$ virsh list Id Name State -------------------------- 1 f28live running 1 f28live1 running
Use the g_atomic_add function directly (as recommended in viratomic.h) and add 1 to the result.
This also restores the usual numbering from 1 instead of 0.
Signed-off-by: Ján Tomko <jtomko@redhat.com> Fixes: 7b9645a7d127a374b8d1c83fdf9789706dbab2c9 --- src/qemu/qemu_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index e5051027fc..0b119cbe78 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1858,7 +1858,7 @@ qemuSetUnprivSGIO(virDomainDeviceDefPtr dev) int qemuDriverAllocateID(virQEMUDriverPtr driver) { - return virAtomicIntInc(&driver->lastvmid); + return g_atomic_int_add(&driver->lastvmid, 1) + 1; }
Does it makes sense to replace all virAtomic with g_atomic or do we still pretend that we care about client library and we don't crash on OOM in it?
We explicitly abort-on-OOM *everywhere*, the moment we started using g_new for VIR_ALLOC, and that includs the client library. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
participants (4)
-
Daniel P. Berrangé -
Ján Tomko -
Michal Privoznik -
Peter Krempa