qemu: Fix domain ID allocation

[libvirt PATCH] qemu: Fix domain ID allocation

Posted by Ján Tomko 4 years, 2 months ago

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

Re: [libvirt PATCH] qemu: Fix domain ID allocation

Posted by Peter Krempa 4 years, 2 months ago

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>

Re: [libvirt PATCH] qemu: Fix domain ID allocation

Posted by Michal Privoznik 4 years, 2 months ago

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

Re: [libvirt PATCH] qemu: Fix domain ID allocation

Posted by Daniel P. Berrangé 4 years, 2 months ago

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 :|

Re: [libvirt PATCH] qemu: Fix domain ID allocation

Posted by Ján Tomko 4 years, 2 months ago

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

Re: [libvirt PATCH] qemu: Fix domain ID allocation

Posted by Michal Privoznik 4 years, 2 months ago

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