Re: [PATCH v8 3/8] parallels: implement functions for domain life cycle management

[Peter Krempa <pkrempa@xxxxxxxxxx>]

On 07/04/12 19:42, Dmitry Guryanov wrote:
> Add functions for create/shutdown/destroy and suspend/resume domain.
>
> Signed-off-by: Dmitry Guryanov <dguryanov@xxxxxxxxxxxxx>
> ---
>   src/parallels/parallels_driver.c |  149 ++++++++++++++++++++++++++++++++++++++
>   src/parallels/parallels_driver.h |    1 +
>   src/parallels/parallels_utils.c  |   18 +++++
>   3 files changed, 168 insertions(+), 0 deletions(-)
>
> diff --git a/src/parallels/parallels_driver.c b/src/parallels/parallels_driver.c
> index 7424c20..8fadd79 100644
> --- a/src/parallels/parallels_driver.c
> +++ b/src/parallels/parallels_driver.c
> @@ -59,6 +59,11 @@
>   static void parallelsFreeDomObj(void *p);
>   static virCapsPtr parallelsBuildCapabilities(void);
>   static int parallelsClose(virConnectPtr conn);
> +static int parallelsPause(virDomainObjPtr privdom);
> +static int parallelsResume(virDomainObjPtr privdom);
> +static int parallelsStart(virDomainObjPtr privdom);
> +static int parallelsKill(virDomainObjPtr privdom);
> +static int parallelsStop(virDomainObjPtr privdom);
>
>   static void
>   parallelsDriverLock(parallelsConnPtr driver)
> @@ -84,6 +89,12 @@ parallelsFreeDomObj(void *p)
>       VIR_FREE(p);
>   };
>
> +static void
> +parallelsDomainEventQueue(parallelsConnPtr driver, virDomainEventPtr event)
> +{
> +    virDomainEventStateQueue(driver->domainEventState, event);
> +}
> +
>   static virCapsPtr
>   parallelsBuildCapabilities(void)
>   {
> @@ -761,6 +772,139 @@ parallelsDomainGetAutostart(virDomainPtr domain, int *autostart)
>       return ret;
>   }
>
> +typedef int (*parallelsChangeState)    (virDomainObjPtr privdom);
> +#define PARALLELS_UUID(x)     (((parallelsDomObjPtr)(x->privateData))->uuid)
> +
> +static int
> +parallelsDomainChangeState(virDomainPtr domain,

Alignment. I'm not going to bug anymore about this. Please align them 
with the first argument type.

> +                     virDomainState req_state, const char *req_state_name,
> +                     parallelsChangeState chstate,
> +                     virDomainState new_state, int reason,
> +                     int event_type, int event_detail)
> +{
> +    parallelsConnPtr privconn = domain->conn->privateData;
> +    virDomainObjPtr privdom;
> +    virDomainEventPtr event = NULL;
> +    int state;
> +    int ret = -1;
> +
> +    parallelsDriverLock(privconn);
> +    privdom = virDomainFindByName(&privconn->domains, domain->name);

I noticed you're using FindByName to lookup the domain. Some drivers are 
using the "more robust":

vm = virDomainFindByUUID(&driver->domains, dom->uuid);

Although this is not a show-stopper.


> +    parallelsDriverUnlock(privconn);
> +
> +    if (privdom == NULL) {
> +        parallelsError(VIR_ERR_NO_DOMAIN,
> +                 _("no domain with matching name '%s'"), domain->name);
> +        goto cleanup;
> +    }
> +
> +    state = virDomainObjGetState(privdom, NULL);
> +    if (state != req_state) {
> +        parallelsError(VIR_ERR_INTERNAL_ERROR, _("domain '%s' not %s"),
> +                 privdom->def->name, req_state_name);
> +        goto cleanup;
> +    }
> +
> +    if (chstate(privdom))
> +        goto cleanup;
> +
> +    virDomainObjSetState(privdom, new_state, reason);
> +
> +    event = virDomainEventNewFromObj(privdom, event_type, event_detail);
> +    ret = 0;
> +
> +  cleanup:
> +    if (privdom)
> +        virDomainObjUnlock(privdom);
> +
> +    if (event) {
> +        parallelsDriverLock(privconn);
> +        parallelsDomainEventQueue(privconn, event);
> +        parallelsDriverUnlock(privconn);
> +    }
> +    return ret;
> +}
> +
> +static int parallelsPause(virDomainObjPtr privdom)
> +{
> +    return parallelsCmdRun(PRLCTL, "pause", PARALLELS_UUID(privdom), NULL);
> +}
> +

Otherwise this patch looks OK.

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