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

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(a)redhat.com&gt; >> --- >> 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 ;)