On 29.03.2016 15:45, Maxim Nestratov wrote:
> This patch introduces a new 'vzConn' lockable object and provides
> helper functions to allocate/destroy it.
> Now we store domain related objects such as domain list, capabitilies
> etc. within a single vz_driver vzConn structure, which is shared by
> all driver connections. It is allocated in a lazy manner in any
> function that is trying to acces it. When a connection to vz daemon
> drops, vzDestroyConnection is called, which prevents further usage
> of vz_driver until a new connection to it is established.
>
> Signed-off-by: Maxim Nestratov <mnestratov@xxxxxxxxxxxxx>
> ---
> src/vz/vz_driver.c | 329 ++++++++++++++++++++++++++++++++++-------------------
> src/vz/vz_sdk.c | 9 +-
> src/vz/vz_utils.c | 13 ++-
> src/vz/vz_utils.h | 9 +-
> 4 files changed, 234 insertions(+), 126 deletions(-)
>
> diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c
> index 7de21d8..36030a7 100644
> --- a/src/vz/vz_driver.c
> +++ b/src/vz/vz_driver.c
> @@ -63,18 +63,24 @@ VIR_LOG_INIT("parallels.parallels_driver");
> #define PRLCTL "prlctl"
>
> static int vzConnectClose(virConnectPtr conn);
> +static virClassPtr vzConnClass;
>
> void
> vzDriverLock(vzConnPtr driver)
> {
> - virMutexLock(&driver->lock);
> + virObjectLock(driver);
> }
>
> void
> vzDriverUnlock(vzConnPtr driver)
> {
> - virMutexUnlock(&driver->lock);
> + virObjectUnlock(driver);
> }
> +static virMutex vz_driver_lock;
> +static vzConnPtr vz_driver;
> +
> +static vzConnPtr
> +vzConnObjNew(void);
>
> static int
> vzCapsAddGuestDomain(virCapsPtr caps,
> @@ -158,15 +164,67 @@ vzBuildCapabilities(void)
> goto cleanup;
> }
>
> +static void vzConnDispose(void * obj)
> +{
> + vzConnPtr conn = obj;
> +
> + if (conn->server) {
> + prlsdkUnsubscribeFromPCSEvents(conn);
> + prlsdkDisconnect(conn);
> + }
> +
> + virObjectUnref(conn->domains);
> + virObjectUnref(conn->caps);
> + virObjectUnref(conn->xmlopt);
> + virObjectEventStateFree(conn->domainEventState);
> +}
i've noticed that you removed support for close callback later but here
it looks like you miss it. I suggest you move drop close callback patch before
this one or keep it here nicely.
Also let's stick with privconn if we deal with vzConnPtr and we are in vz_driver.c
> +
> +static int vzConnOnceInit(void)
> +{
> + if (!(vzConnClass = virClassNew(virClassForObjectLockable(),
> + "vzConn",
> + sizeof(vzConn),
> + vzConnDispose)))
> + return -1;
> +
> + return 0;
> +}
> +VIR_ONCE_GLOBAL_INIT(vzConn)
> +
> +vzConnPtr
> +vzGetConnection(void)
> +{
> + virMutexLock(&vz_driver_lock);
> + if (!vz_driver)
> + vz_driver = vzConnObjNew();
> + virObjectRef(vz_driver);
> + virMutexUnlock(&vz_driver_lock);
> + return vz_driver;
> +}
> +
> +void
> +vzDestroyConnection(void)
> +{
> + vzConnPtr privconn;
> + virMutexLock(&vz_driver_lock);
> + privconn = vz_driver;
> + vz_driver = NULL;
> + virMutexUnlock(&vz_driver_lock);
> + virObjectUnref(privconn);
> +}
> +
> static char *
> -vzConnectGetCapabilities(virConnectPtr conn)
> +vzConnectGetCapabilities(virConnectPtr conn ATTRIBUTE_UNUSED)
> {
> - vzConnPtr privconn = conn->privateData;
> + vzConnPtr privconn = vzGetConnection();
> + if (!privconn)
> + return NULL;
> char *xml;
>
> vzDriverLock(privconn);
> xml = virCapabilitiesFormatXML(privconn->caps);
> vzDriverUnlock(privconn);
> + virObjectUnref(privconn);
> return xml;
> }
>
> @@ -214,78 +272,42 @@ virDomainDefParserConfig vzDomainDefParserConfig = {
> .domainPostParseCallback = vzDomainDefPostParse,
> };
>
> -
> -static int
> -vzOpenDefault(virConnectPtr conn)
> +static vzConnPtr
> +vzConnObjNew(void)
> {
> - vzConnPtr privconn;
> -
> - if (VIR_ALLOC(privconn) < 0)
> - return VIR_DRV_OPEN_ERROR;
> - if (virMutexInit(&privconn->lock) < 0) {
> - virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("cannot initialize mutex"));
> - goto err_free;
> - }
> -
> - if (prlsdkInit()) {
> - VIR_DEBUG("%s", _("Can't initialize Parallels SDK"));
> - goto err_free;
> - }
> -
> - if (prlsdkConnect(privconn) < 0)
> - goto err_free;
> -
> - if (vzInitVersion(privconn) < 0)
> - goto error;
> -
> - if (!(privconn->caps = vzBuildCapabilities()))
> - goto error;
> + vzConnPtr conn;
>
> - vzDomainDefParserConfig.priv = &privconn->vzCaps;
> - if (!(privconn->xmlopt = virDomainXMLOptionNew(&vzDomainDefParserConfig,
> - NULL, NULL)))
> - goto error;
> -
> - if (!(privconn->domains = virDomainObjListNew()))
> - goto error;
> -
> - if (!(privconn->domainEventState = virObjectEventStateNew()))
> - goto error;
> -
> - if (prlsdkSubscribeToPCSEvents(privconn))
> - goto error;
> -
> - if (!(privconn->closeCallback = virNewConnectCloseCallbackData()))
> - goto error;
> -
> - conn->privateData = privconn;
> + if (vzConnInitialize() < 0)
> + return NULL;
>
> - if (prlsdkLoadDomains(privconn))
> - goto error;
> + if (!(conn = virObjectLockableNew(vzConnClass)))
> + return NULL;
>
> - return VIR_DRV_OPEN_SUCCESS;
> + vzDomainDefParserConfig.priv = &conn->vzCaps;
> +
> + if (!(conn->caps = vzBuildCapabilities()) ||
> + !(conn->xmlopt = virDomainXMLOptionNew(&vzDomainDefParserConfig,
> + NULL, NULL)) ||
> + !(conn->domainEventState = virObjectEventStateNew()) ||
> + !(conn->domains = virDomainObjListNew()) ||
> + (vzInitVersion(conn) < 0) ||
> + (prlsdkConnect(conn) < 0) ||
> + (prlsdkSubscribeToPCSEvents(conn) < 0) ||
> + (prlsdkLoadDomains(conn) < 0)
> + ) {
I'd better move vzInitVersion, prlsdkConnect etc out of new. Looks like it is typical
for constructors in libvirt not to have side effects.
> + virObjectUnref(conn);
> + return NULL;
> + }
Let's leave previous code structure alone here. This will make this patch more comprehensible.
We can move from bunch of 'if's to long || sequence in different patch if we want to.
>
> - error:
> - virObjectUnref(privconn->closeCallback);
> - privconn->closeCallback = NULL;
> - virObjectUnref(privconn->domains);
> - virObjectUnref(privconn->caps);
> - virObjectEventStateFree(privconn->domainEventState);
> - prlsdkDisconnect(privconn);
> - prlsdkDeinit();
> - err_free:
> - conn->privateData = NULL;
> - VIR_FREE(privconn);
> - return VIR_DRV_OPEN_ERROR;
> + return conn;
> }
>
> static virDrvOpenStatus
> -vzConnectOpen(virConnectPtr conn,
> +vzConnectOpen(virConnectPtr conn ATTRIBUTE_UNUSED,
> virConnectAuthPtr auth ATTRIBUTE_UNUSED,
> unsigned int flags)
> {
> - int ret;
> + vzConnPtr privconn = NULL;
>
> virCheckFlags(VIR_CONNECT_RO, VIR_DRV_OPEN_ERROR);
>
> @@ -317,44 +339,33 @@ vzConnectOpen(virConnectPtr conn,
> return VIR_DRV_OPEN_ERROR;
> }
>
> - if ((ret = vzOpenDefault(conn)) != VIR_DRV_OPEN_SUCCESS)
> - return ret;
> + if (!(privconn = vzGetConnection()))
> + return VIR_DRV_OPEN_ERROR;
>
> + virObjectUnref(privconn);
> return VIR_DRV_OPEN_SUCCESS;
> }
>
> static int
> -vzConnectClose(virConnectPtr conn)
> +vzConnectClose(virConnectPtr conn ATTRIBUTE_UNUSED)
> {
> - vzConnPtr privconn = conn->privateData;
> -
> + vzConnPtr privconn = vzGetConnection();
> if (!privconn)
> return 0;
>
> - vzDriverLock(privconn);
> - prlsdkUnsubscribeFromPCSEvents(privconn);
> - virObjectUnref(privconn->caps);
> - virObjectUnref(privconn->xmlopt);
> - virObjectUnref(privconn->domains);
> - virObjectUnref(privconn->closeCallback);
> - privconn->closeCallback = NULL;
> - virObjectEventStateFree(privconn->domainEventState);
> - prlsdkDisconnect(privconn);
> - conn->privateData = NULL;
> - prlsdkDeinit();
>
> - vzDriverUnlock(privconn);
> - virMutexDestroy(&privconn->lock);
> -
> - VIR_FREE(privconn);
> + virObjectUnref(privconn);
> return 0;
> }
>
> static int
> -vzConnectGetVersion(virConnectPtr conn, unsigned long *hvVer)
> +vzConnectGetVersion(virConnectPtr conn ATTRIBUTE_UNUSED, unsigned long *hvVer)
> {
> - vzConnPtr privconn = conn->privateData;
> + vzConnPtr privconn = vzGetConnection();
> + if (!privconn)
> + return -1;
> *hvVer = privconn->vzVersion;
> + virObjectUnref(privconn);
> return 0;
> }
I find it too strange that privcon can change from call to call while
we stay with same conn. Moreover it just won't work. privconn holds
state like domainEventState for example and if connection to VZ
is broken we loose all subscriptions without any notice from client.
(taking into account that later you remove close callback implementation).
...
> @@ -1564,6 +1635,10 @@ static virConnectDriver vzConnectDriver = {
> static int
> vzStateCleanup(void)
> {
> + prlsdkDeinit();
> + virObjectUnref(vz_driver);
> + vz_driver = NULL;
> + virMutexDestroy(&vz_driver_lock);
> return 0;
> }
>
> @@ -1572,7 +1647,25 @@ vzStateInitialize(bool privileged ATTRIBUTE_UNUSED,
> virStateInhibitCallback callback ATTRIBUTE_UNUSED,
> void *opaque ATTRIBUTE_UNUSED)
> {
> + if (!privileged) {
> + VIR_INFO("Not running privileged, disabling driver");
> + return 0;
> + }
Is it correct? Driver init result is ok so someone could call it's methods...
...
> diff --git a/src/vz/vz_utils.h b/src/vz/vz_utils.h
> index b415b0f..00e795f 100644
> --- a/src/vz/vz_utils.h
> +++ b/src/vz/vz_utils.h
> @@ -61,7 +61,7 @@ typedef struct _vzCapabilities vzCapabilities;
> typedef struct _vzCapabilities *vzCapabilitiesPtr;
>
> struct _vzConn {
> - virMutex lock;
> + virObjectLockable parent;
>
> /* Immutable pointer, self-locking APIs */
> virDomainObjListPtr domains;
> @@ -105,6 +105,13 @@ char * vzGetOutput(const char *binary, ...)
> ATTRIBUTE_NONNULL(1) ATTRIBUTE_SENTINEL;
> void vzDriverLock(vzConnPtr driver);
> void vzDriverUnlock(vzConnPtr driver);
> +
> +vzConnPtr
> +vzGetConnection(void);
> +
> +void
> +vzDestroyConnection(void);
> +
odd code
> virDomainObjPtr
> vzNewDomain(vzConnPtr privconn,
> char *name,
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list