[libvirt] [PATCH 2/4] qemu: Save qemu driver in qemuDomainObjPrivateData

[Michal Privoznik]

On 07/25/2017 03:48 PM, Martin Kletzander wrote:
> On Tue, Jul 25, 2017 at 03:20:48PM +0200, Michal Privoznik wrote:
>> On 07/24/2017 04:09 PM, Martin Kletzander wrote:
>>> This way we can finally make it static and not use any externs anywhere.
>>>
>>> Signed-off-by: Martin Kletzander <mkletzan at redhat.com>
>>> ---
>>>  src/qemu/qemu_domain.c  | 3 ++-
>>>  src/qemu/qemu_domain.h  | 2 ++
>>>  src/qemu/qemu_driver.c  | 2 +-
>>>  src/qemu/qemu_process.c | 5 +----
>>>  4 files changed, 6 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>>> index f1e144d92b64..0b7c45280321 100644
>>> --- a/src/qemu/qemu_domain.c
>>> +++ b/src/qemu/qemu_domain.c
>>> @@ -1662,7 +1662,7 @@ qemuDomainClearPrivatePaths(virDomainObjPtr vm)
>>>
>>>
>>>  static void *
>>> -qemuDomainObjPrivateAlloc(void *opaque ATTRIBUTE_UNUSED)
>>> +qemuDomainObjPrivateAlloc(void *opaque)
>>>  {
>>>      qemuDomainObjPrivatePtr priv;
>>>
>>> @@ -1679,6 +1679,7 @@ qemuDomainObjPrivateAlloc(void *opaque
>>> ATTRIBUTE_UNUSED)
>>>          goto error;
>>>
>>>      priv->migMaxBandwidth = QEMU_DOMAIN_MIG_BANDWIDTH_MAX;
>>> +    priv->driver = opaque;
>>>
>>>      return priv;
>>>
>>> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
>>> index 365b23c96167..71567af034f5 100644
>>> --- a/src/qemu/qemu_domain.h
>>> +++ b/src/qemu/qemu_domain.h
>>> @@ -217,6 +217,8 @@ struct _qemuDomainSecretInfo {
>>>  typedef struct _qemuDomainObjPrivate qemuDomainObjPrivate;
>>>  typedef qemuDomainObjPrivate *qemuDomainObjPrivatePtr;
>>>  struct _qemuDomainObjPrivate {
>>> +    virQEMUDriverPtr driver;
>>> +
>>>      struct qemuDomainJobObj job;
>>>
>>>      virBitmapPtr namespaces;
>>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>>> index 6568def156f4..9c54571cf078 100644
>>> --- a/src/qemu/qemu_driver.c
>>> +++ b/src/qemu/qemu_driver.c
>>> @@ -159,7 +159,7 @@ static int qemuGetDHCPInterfaces(virDomainPtr dom,
>>>                                   virDomainObjPtr vm,
>>>                                   virDomainInterfacePtr **ifaces);
>>>
>>> -virQEMUDriverPtr qemu_driver = NULL;
>>> +static virQEMUDriverPtr qemu_driver;
>>>
>>>
>>>  static void
>>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>>> index 525521aaf0ca..757f5d95e0b7 100644
>>> --- a/src/qemu/qemu_process.c
>>> +++ b/src/qemu/qemu_process.c
>>> @@ -120,9 +120,6 @@ qemuProcessRemoveDomainStatus(virQEMUDriverPtr
>>> driver,
>>>  }
>>>
>>>
>>> -/* XXX figure out how to remove this */
>>> -extern virQEMUDriverPtr qemu_driver;
>>> -
>>>  /*
>>>   * This is a callback registered with a qemuAgentPtr instance,
>>>   * and to be invoked when the agent console hits an end of file
>>> @@ -518,9 +515,9 @@ qemuProcessHandleReset(qemuMonitorPtr mon
>>> ATTRIBUTE_UNUSED,
>>>  static void
>>>  qemuProcessFakeReboot(void *opaque)
>>>  {
>>> -    virQEMUDriverPtr driver = qemu_driver;
>>>      virDomainObjPtr vm = opaque;
>>>      qemuDomainObjPrivatePtr priv = vm->privateData;
>>> +    virQEMUDriverPtr driver = priv->driver;
>>
>> Well, storing driver in private data looks like overkill for this
>> purpose. qemuProcessFakeReboot() runs in a thread (which explains weird
>> function arguments). But in order to pass qemu driver into this function
>> we can make the function take a struct.
>> On the other hand, it might come handy (even right now) as it enables us
>> to clean up some code where we already have both priv and driver in
>> function arguments. Frankly, I'm torn. So it's up to you whether you
>> want to go this way or the one I'm suggesting.
>>
> 
> I'm perfectly fine with passing the struct and I have no problem with
> changing this that way.  The clean up of qemuProcessFakeReboot is not the
> main purpose of this clean up; it's the last patch and enabling future
> clean ups.  However, I have thought about it and when I pass the struct,
> it will eventually still get removed and it will end up in this form
> during the future clean ups.  In other words, I can pass the struct, but
> it's not needed any more since the driver will be available even without
> that =)
> 
> So I'll stick with this since you left me decide ;)

Right. We don't need both approaches. ACK to this and to 1/4 too then.
Although, let me just point out that it was difficult for me to track
that it is indeed driver that is passed as @opaque to
qemuDomainObjPrivateAlloc(). It isn't visible at the first glance.

Michal