[prev in list] [next in list] [prev in thread] [next in thread] 

List:       libvir-list
Subject:    Re: [libvirt PATCH] qemu: Fix domain ID allocation
From:       Michal Privoznik <mprivozn () redhat ! com>
Date:       2020-01-31 15:55:41
Message-ID: 0269aff0-ff9e-892d-966e-e6e33c2ddae1 () redhat ! com
[Download RAW message or body]

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

[prev in list] [next in list] [prev in thread] [next in thread]