Re: [PATCH 1/9] vz: expand start/stop/... APIs for ACL checks

[Maxim Nestratov <mnestratov@xxxxxxxxxxxxx>]

24-Jun-16 17:32, Nikolay Shirokovskiy пишет:

>    The original motivation is to expand API calls like start/stop etc so that
> the ACL checks could be added. But this patch has its own befenits.
>
> 1. functions like prlsdkStart/Stop use common routine to wait for
> job without domain lock. They become more self contained and do
> not return intermediate PRL_RESULT.
>
> 2. vzDomainManagedSave do not update cache twice.
>
> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@xxxxxxxxxxxxx>
> ---
>   src/vz/vz_driver.c | 216 ++++++++++++++++++++++++++++++++++++++++++++++++-----
>   src/vz/vz_sdk.c    | 172 +++++++++++++++++++++---------------------
>   src/vz/vz_sdk.h    |  23 ++----
>   3 files changed, 291 insertions(+), 120 deletions(-)
>
> diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c
> index faa1f56..0079384 100644
> --- a/src/vz/vz_driver.c
> +++ b/src/vz/vz_driver.c
> @@ -957,36 +957,210 @@ vzConnectDomainEventDeregisterAny(virConnectPtr conn,
>       return 0;
>   }
>   
> -static int vzDomainSuspend(virDomainPtr domain)
> +static int
> +vzDomainSuspend(virDomainPtr domain)
>   {
> -    return prlsdkDomainChangeState(domain, prlsdkPause);
> +    vzConnPtr privconn = domain->conn->privateData;
> +    virDomainObjPtr dom;
> +    int ret = -1;
> +    bool job = false;
> +
> +    if (!(dom = vzDomObjFromDomainRef(domain)))
> +        return -1;
> +
> +    if (vzDomainObjBeginJob(dom) < 0)
> +        goto cleanup;
> +    job = true;
> +
> +    if (!vzDomainObjIsExist(dom))
> +        goto cleanup;
> +

s/vzDomainObjIsExist/vzEnsureDomainExists


> +    if (prlsdkPause(dom) < 0)
> +        goto cleanup;
> +
> +    if (prlsdkUpdateDomain(privconn->driver, dom) < 0)
> +        goto cleanup;
> +
> +    ret = 0;
> +
> + cleanup:
> +    if (job)
> +        vzDomainObjEndJob(dom);
> +    virDomainObjEndAPI(&dom);
> +
> +    return ret;
>   }
>   
> -static int vzDomainResume(virDomainPtr domain)
> +static int
> +vzDomainResume(virDomainPtr domain)
>   {
> -    return prlsdkDomainChangeState(domain, prlsdkResume);
> +    vzConnPtr privconn = domain->conn->privateData;
> +    virDomainObjPtr dom;
> +    int ret = -1;
> +    bool job = false;
> +
> +    if (!(dom = vzDomObjFromDomainRef(domain)))
> +        return -1;
> +
> +    if (vzDomainObjBeginJob(dom) < 0)
> +        goto cleanup;
> +    job = true;
> +
> +    if (!vzDomainObjIsExist(dom))
> +        goto cleanup;
> +

same here and below

> +    if (prlsdkResume(dom) < 0)
> +        goto cleanup;
> +
> +    if (prlsdkUpdateDomain(privconn->driver, dom) < 0)
> +        goto cleanup;
> +
> +    ret = 0;
> +
> + cleanup:
> +    if (job)
> +        vzDomainObjEndJob(dom);
> +    virDomainObjEndAPI(&dom);
> +
> +    return ret;
>   }
>   
> -static int vzDomainCreate(virDomainPtr domain)
> +static int
> +vzDomainCreate(virDomainPtr domain)
>   {
> -    return prlsdkDomainChangeState(domain, prlsdkStart);
> +    vzConnPtr privconn = domain->conn->privateData;
> +    virDomainObjPtr dom;
> +    int ret = -1;
> +    bool job = false;
> +
> +    if (!(dom = vzDomObjFromDomainRef(domain)))
> +        return -1;
> +
> +    if (vzDomainObjBeginJob(dom) < 0)
> +        goto cleanup;
> +    job = true;
> +
> +    if (!vzDomainObjIsExist(dom))
> +        goto cleanup;
> +
> +    if (prlsdkStart(dom) < 0)
> +        goto cleanup;
> +
> +    if (prlsdkUpdateDomain(privconn->driver, dom) < 0)
> +        goto cleanup;
> +
> +    ret = 0;
> +
> + cleanup:
> +    if (job)
> +        vzDomainObjEndJob(dom);
> +    virDomainObjEndAPI(&dom);
> +
> +    return ret;
>   }
>   
> -static int vzDomainDestroy(virDomainPtr domain)
> +static int
> +vzDomainDestroy(virDomainPtr domain)
>   {
> -    return prlsdkDomainChangeState(domain, prlsdkKill);
> +    vzConnPtr privconn = domain->conn->privateData;
> +    virDomainObjPtr dom;
> +    int ret = -1;
> +    bool job = false;
> +
> +    if (!(dom = vzDomObjFromDomainRef(domain)))
> +        return -1;
> +
> +    if (vzDomainObjBeginJob(dom) < 0)
> +        goto cleanup;
> +    job = true;
> +
> +    if (!vzDomainObjIsExist(dom))
> +        goto cleanup;
> +
> +    if (prlsdkKill(dom) < 0)
> +        goto cleanup;
> +
> +    if (prlsdkUpdateDomain(privconn->driver, dom) < 0)
> +        goto cleanup;
> +
> +    ret = 0;
> +
> + cleanup:
> +    if (job)
> +        vzDomainObjEndJob(dom);
> +    virDomainObjEndAPI(&dom);
> +
> +    return ret;
>   }
>   
> -static int vzDomainShutdown(virDomainPtr domain)
> +static int
> +vzDomainShutdown(virDomainPtr domain)
>   {
> -    return prlsdkDomainChangeState(domain, prlsdkStop);
> +    vzConnPtr privconn = domain->conn->privateData;
> +    virDomainObjPtr dom;
> +    int ret = -1;
> +    bool job = false;
> +
> +    if (!(dom = vzDomObjFromDomainRef(domain)))
> +        return -1;
> +
> +    if (vzDomainObjBeginJob(dom) < 0)
> +        goto cleanup;
> +    job = true;
> +
> +    if (!vzDomainObjIsExist(dom))
> +        goto cleanup;
> +
> +    if (prlsdkStop(dom) < 0)
> +        goto cleanup;
> +
> +    if (prlsdkUpdateDomain(privconn->driver, dom) < 0)
> +        goto cleanup;
> +
> +    ret = 0;
> +
> + cleanup:
> +    if (job)
> +        vzDomainObjEndJob(dom);
> +    virDomainObjEndAPI(&dom);
> +
> +    return ret;
>   }
>   
> -static int vzDomainReboot(virDomainPtr domain,
> -                          unsigned int flags)
> +static int
> +vzDomainReboot(virDomainPtr domain, unsigned int flags)
>   {
> +    vzConnPtr privconn = domain->conn->privateData;
> +    virDomainObjPtr dom;
> +    int ret = -1;
> +    bool job = false;
> +
>       virCheckFlags(0, -1);
> -    return prlsdkDomainChangeState(domain, prlsdkRestart);
> +
> +    if (!(dom = vzDomObjFromDomainRef(domain)))
> +        return -1;
> +
> +    if (vzDomainObjBeginJob(dom) < 0)
> +        goto cleanup;
> +    job = true;
> +
> +    if (!vzDomainObjIsExist(dom))
> +        goto cleanup;
> +
> +    if (prlsdkRestart(dom) < 0)
> +        goto cleanup;
> +
> +    if (prlsdkUpdateDomain(privconn->driver, dom) < 0)
> +        goto cleanup;
> +
> +    ret = 0;
> +
> + cleanup:
> +    if (job)
> +        vzDomainObjEndJob(dom);
> +    virDomainObjEndAPI(&dom);
> +
> +    return ret;
>   }
>   
>   static int vzDomainIsActive(virDomainPtr domain)
> @@ -1095,13 +1269,17 @@ vzDomainManagedSave(virDomainPtr domain, unsigned int flags)
>   
>       state = virDomainObjGetState(dom, &reason);
>   
> -    if (state == VIR_DOMAIN_RUNNING && (flags & VIR_DOMAIN_SAVE_PAUSED)) {
> -        ret = prlsdkDomainChangeStateLocked(privconn->driver, dom, prlsdkPause);
> -        if (ret)
> -            goto cleanup;
> -    }
> +    if (state == VIR_DOMAIN_RUNNING && (flags & VIR_DOMAIN_SAVE_PAUSED) &&
> +        prlsdkPause(dom) < 0)
> +        goto cleanup;
>   
> -    ret = prlsdkDomainChangeStateLocked(privconn->driver, dom, prlsdkSuspend);
> +    if (prlsdkSuspend(dom) < 0)
> +        goto cleanup;
> +
> +    if (prlsdkUpdateDomain(privconn->driver, dom) < 0)
> +        goto cleanup;
> +
> +    ret = 0;
>   
>    cleanup:
>       if (job)
> diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c
> index 77193e6..02cbb3b 100644
> --- a/src/vz/vz_sdk.c
> +++ b/src/vz/vz_sdk.c
> @@ -40,6 +40,8 @@
>   
>   static int
>   prlsdkUUIDParse(const char *uuidstr, unsigned char *uuid);
> +static void
> +prlsdkConvertError(PRL_RESULT pret);
>   
>   VIR_LOG_INIT("parallels.sdk");
>   
> @@ -2004,131 +2006,129 @@ void prlsdkUnsubscribeFromPCSEvents(vzDriverPtr driver)
>           logPrlError(ret);
>   }
>   
> -PRL_RESULT prlsdkStart(PRL_HANDLE sdkdom)
> +int prlsdkStart(virDomainObjPtr dom)
>   {
>       PRL_HANDLE job = PRL_INVALID_HANDLE;
> +    vzDomObjPtr privdom = dom->privateData;
> +    PRL_RESULT pret;
> +
> +    job = PrlVm_StartEx(privdom->sdkdom, PSM_VM_START, 0);
> +    if (PRL_FAILED(pret = waitDomainJob(job, dom))) {
> +        prlsdkConvertError(pret);
> +        return -1;
> +    }
>   
> -    job = PrlVm_StartEx(sdkdom, PSM_VM_START, 0);
> -    return waitJob(job);
> +    return 0;
>   }
>   
> -static PRL_RESULT prlsdkStopEx(PRL_HANDLE sdkdom, PRL_UINT32 mode)
> +int prlsdkKill(virDomainObjPtr dom)
>   {
>       PRL_HANDLE job = PRL_INVALID_HANDLE;
> +    vzDomObjPtr privdom = dom->privateData;
> +    PRL_RESULT pret;
>   
> -    job = PrlVm_StopEx(sdkdom, mode, 0);
> -    return waitJob(job);
> -}
> +    job = PrlVm_StopEx(privdom->sdkdom, PSM_KILL, 0);
> +    if (PRL_FAILED(pret = waitDomainJob(job, dom))) {
> +        prlsdkConvertError(pret);
> +        return -1;
> +    }
>   
> -PRL_RESULT prlsdkKill(PRL_HANDLE sdkdom)
> -{
> -    return prlsdkStopEx(sdkdom, PSM_KILL);
> +    return 0;
>   }
>   
> -PRL_RESULT prlsdkStop(PRL_HANDLE sdkdom)
> +int prlsdkStop(virDomainObjPtr dom)
>   {
> -    return prlsdkStopEx(sdkdom, PSM_SHUTDOWN);
> +    PRL_HANDLE job = PRL_INVALID_HANDLE;
> +    vzDomObjPtr privdom = dom->privateData;
> +    PRL_RESULT pret;
> +
> +    job = PrlVm_StopEx(privdom->sdkdom, PSM_SHUTDOWN, 0);
> +    if (PRL_FAILED(pret = waitDomainJob(job, dom))) {
> +        prlsdkConvertError(pret);
> +        return -1;
> +    }
> +
> +    return 0;
>   }
>   
> -PRL_RESULT prlsdkPause(PRL_HANDLE sdkdom)
> +int prlsdkPause(virDomainObjPtr dom)
>   {
>       PRL_HANDLE job = PRL_INVALID_HANDLE;
> +    vzDomObjPtr privdom = dom->privateData;
> +    PRL_RESULT pret;
> +
> +    job = PrlVm_Pause(privdom->sdkdom, false);
> +    if (PRL_FAILED(pret = waitDomainJob(job, dom))) {
> +        prlsdkConvertError(pret);
> +        return -1;
> +    }
>   
> -    job = PrlVm_Pause(sdkdom, false);
> -    return waitJob(job);
> +    return 0;
>   }
>   
> -PRL_RESULT prlsdkResume(PRL_HANDLE sdkdom)
> +int prlsdkResume(virDomainObjPtr dom)
>   {
>       PRL_HANDLE job = PRL_INVALID_HANDLE;
> +    vzDomObjPtr privdom = dom->privateData;
> +    PRL_RESULT pret;
>   
> -    job = PrlVm_Resume(sdkdom);
> -    return waitJob(job);
> +    job = PrlVm_Resume(privdom->sdkdom);
> +    if (PRL_FAILED(pret = waitDomainJob(job, dom))) {
> +        prlsdkConvertError(pret);
> +        return -1;
> +    }
> +
> +    return 0;
>   }
>   
> -PRL_RESULT prlsdkSuspend(PRL_HANDLE sdkdom)
> +int prlsdkSuspend(virDomainObjPtr dom)
>   {
>       PRL_HANDLE job = PRL_INVALID_HANDLE;
> +    vzDomObjPtr privdom = dom->privateData;
> +    PRL_RESULT pret;
>   
> -    job = PrlVm_Suspend(sdkdom);
> -    return waitJob(job);
> +    job = PrlVm_Suspend(privdom->sdkdom);
> +    if (PRL_FAILED(pret = waitDomainJob(job, dom))) {
> +        prlsdkConvertError(pret);
> +        return -1;
> +    }
> +
> +    return 0;
>   }
>   
> -PRL_RESULT prlsdkRestart(PRL_HANDLE sdkdom)
> +int prlsdkRestart(virDomainObjPtr dom)
>   {
>       PRL_HANDLE job = PRL_INVALID_HANDLE;
> +    vzDomObjPtr privdom = dom->privateData;
> +    PRL_RESULT pret;
>   
> -    job = PrlVm_Restart(sdkdom);
> -    return waitJob(job);
> +    job = PrlVm_Restart(privdom->sdkdom);
> +    if (PRL_FAILED(pret = waitDomainJob(job, dom))) {
> +        prlsdkConvertError(pret);
> +        return -1;
> +    }
> +
> +    return 0;
>   }
>   
> -int
> -prlsdkDomainChangeStateLocked(vzDriverPtr driver,
> -                              virDomainObjPtr dom,
> -                              prlsdkChangeStateFunc chstate)
> +static void
> +prlsdkConvertError(PRL_RESULT pret)
>   {
> -    vzDomObjPtr pdom;
> -    PRL_RESULT pret;
>       virErrorNumber virerr;
>   
> -    pdom = dom->privateData;
> -    virObjectUnlock(dom);
> -    pret = chstate(pdom->sdkdom);
> -    virObjectLock(dom);
> -    if (PRL_FAILED(pret)) {
> -        virResetLastError();
> -
> -        switch (pret) {
> -        case PRL_ERR_DISP_VM_IS_NOT_STARTED:
> -        case PRL_ERR_DISP_VM_IS_NOT_STOPPED:
> -        case PRL_ERR_INVALID_ACTION_REQUESTED:
> -        case PRL_ERR_UNIMPLEMENTED:
> -            virerr = VIR_ERR_OPERATION_INVALID;
> -            break;
> -        default:
> -            virerr = VIR_ERR_OPERATION_FAILED;
> -        }
> -
> -        virReportError(virerr, "%s", _("Can't change domain state."));
> -        return -1;
> -    }
> -
> -    return prlsdkUpdateDomain(driver, dom);
> -}
> -
> -int
> -prlsdkDomainChangeState(virDomainPtr domain,
> -                        prlsdkChangeStateFunc chstate)
> -{
> -    vzConnPtr privconn = domain->conn->privateData;
> -    virDomainObjPtr dom;
> -    int ret = -1;
> -    bool job = false;
> -
> -    if (!(dom = vzDomObjFromDomainRef(domain)))
> -        return -1;
> -
> -    if (vzDomainObjBeginJob(dom) < 0)
> -        goto cleanup;
> -    job = true;
> -
> -    if (dom->removing) {
> -        char uuidstr[VIR_UUID_STRING_BUFLEN];
> -
> -        virUUIDFormat(dom->def->uuid, uuidstr);
> -        virReportError(VIR_ERR_NO_DOMAIN,
> -                       _("no domain with matching uuid '%s' (%s)"),
> -                       uuidstr, dom->def->name);
> -        goto cleanup;
> +    switch (pret) {
> +    case PRL_ERR_DISP_VM_IS_NOT_STARTED:
> +    case PRL_ERR_DISP_VM_IS_NOT_STOPPED:
> +    case PRL_ERR_INVALID_ACTION_REQUESTED:
> +    case PRL_ERR_UNIMPLEMENTED:
> +        virerr = VIR_ERR_OPERATION_INVALID;
> +        break;
> +    default:
> +        virerr = VIR_ERR_OPERATION_FAILED;
>       }
>   
> -    ret = prlsdkDomainChangeStateLocked(privconn->driver, dom, chstate);
> -
> - cleanup:
> -    if (job)
> -        vzDomainObjEndJob(dom);
> -    virDomainObjEndAPI(&dom);
> -    return ret;
> +    virResetLastError();
> +    virReportError(virerr, "%s", _("Can't change domain state."));
>   }
>   
>   static int
> diff --git a/src/vz/vz_sdk.h b/src/vz/vz_sdk.h
> index e32001a..e9d7169 100644
> --- a/src/vz/vz_sdk.h
> +++ b/src/vz/vz_sdk.h
> @@ -37,22 +37,15 @@ prlsdkAddDomainByName(vzDriverPtr driver, const char *name);
>   int prlsdkUpdateDomain(vzDriverPtr driver, virDomainObjPtr dom);
>   int prlsdkSubscribeToPCSEvents(vzDriverPtr driver);
>   void prlsdkUnsubscribeFromPCSEvents(vzDriverPtr driver);
> -PRL_RESULT prlsdkStart(PRL_HANDLE sdkdom);
> -PRL_RESULT prlsdkKill(PRL_HANDLE sdkdom);
> -PRL_RESULT prlsdkStop(PRL_HANDLE sdkdom);
> -PRL_RESULT prlsdkPause(PRL_HANDLE sdkdom);
> -PRL_RESULT prlsdkResume(PRL_HANDLE sdkdom);
> -PRL_RESULT prlsdkSuspend(PRL_HANDLE sdkdom);
> -PRL_RESULT prlsdkRestart(PRL_HANDLE sdkdom);
>   
> -typedef PRL_RESULT (*prlsdkChangeStateFunc)(PRL_HANDLE sdkdom);
> -int
> -prlsdkDomainChangeState(virDomainPtr domain,
> -                        prlsdkChangeStateFunc chstate);
> -int
> -prlsdkDomainChangeStateLocked(vzDriverPtr driver,
> -                              virDomainObjPtr dom,
> -                              prlsdkChangeStateFunc chstate);
> +int prlsdkStart(virDomainObjPtr dom);
> +int prlsdkKill(virDomainObjPtr dom);
> +int prlsdkStop(virDomainObjPtr dom);
> +int prlsdkPause(virDomainObjPtr dom);
> +int prlsdkResume(virDomainObjPtr dom);
> +int prlsdkSuspend(virDomainObjPtr dom);
> +int prlsdkRestart(virDomainObjPtr dom);
> +
>   int
>   prlsdkApplyConfig(vzDriverPtr driver,
>                     virDomainObjPtr dom,
ACK with mentioned changes

Maxim

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list