On 2013年01月16日 07:15, Jim Fehlig wrote:

Commit dfa1e1dd added functions whose definitions do not conform to the style used in the libxl driver. Change these functions to be consistent throughout the driver. --- src/libxl/libxl_driver.c | 79 ++++++++++++++++++++++++++---------------------- 1 file changed, 43 insertions(+), 36 deletions(-)

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index a956188..4b8f205 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -1,6 +1,6 @@ /*---------------------------------------------------------------------------*/ /* Copyright (C) 2006-2012 Red Hat, Inc. - * Copyright (c) 2011 SUSE LINUX Products GmbH, Nuernberg, Germany. + * Copyright (C) 2011-2013 SUSE LINUX Products GmbH, Nuernberg, Germany. * Copyright (C) 2011 Univention GmbH. * * This library is free software; you can redistribute it and/or @@ -71,15 +71,14 @@ struct libxlOSEventHookTimerInfo { int id; };

- -static void libxlDomainManagedSaveLoad(void *payload, - const void *n ATTRIBUTE_UNUSED, - void *opaque); - - static libxlDriverPrivatePtr libxl_driver = NULL;

/* Function declarations */ +static void +libxlDomainManagedSaveLoad(void *payload, + const void *n ATTRIBUTE_UNUSED, + void *opaque); + static int libxlVmStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm, bool start_paused, int restore_fd); @@ -97,11 +96,11 @@ libxlDriverUnlock(libxlDriverPrivatePtr driver) virMutexUnlock(&driver->lock); }

- -static void libxlFDEventCallback(int watch ATTRIBUTE_UNUSED, - int fd, - int vir_events, - void *fdinfo) +static void +libxlFDEventCallback(int watch ATTRIBUTE_UNUSED, + int fd, + int vir_events, + void *fdinfo) { struct libxlOSEventHookFDInfo *info = fdinfo; int events = 0; @@ -118,13 +117,15 @@ static void libxlFDEventCallback(int watch ATTRIBUTE_UNUSED, libxl_osevent_occurred_fd(info->priv->ctx, info->xl_priv, fd, 0, events); }

-static void libxlFreeFDInfo(void *obj) +static void +libxlFreeFDInfo(void *obj) { VIR_FREE(obj); }

-static int libxlFDRegisterEventHook(void *priv, int fd, void **hndp, - short events, void *xl_priv) +static int +libxlFDRegisterEventHook(void *priv, int fd, void **hndp, + short events, void *xl_priv) { int vir_events = VIR_EVENT_HANDLE_ERROR; struct libxlOSEventHookFDInfo *fdinfo; @@ -152,10 +153,11 @@ static int libxlFDRegisterEventHook(void *priv, int fd, void **hndp, return 0; }

-static int libxlFDModifyEventHook(void *priv ATTRIBUTE_UNUSED, - int fd ATTRIBUTE_UNUSED, - void **hndp, - short events) +static int +libxlFDModifyEventHook(void *priv ATTRIBUTE_UNUSED, + int fd ATTRIBUTE_UNUSED, + void **hndp, + short events) { struct libxlOSEventHookFDInfo *fdinfo = *hndp; int vir_events = VIR_EVENT_HANDLE_ERROR; @@ -169,32 +171,35 @@ static int libxlFDModifyEventHook(void *priv ATTRIBUTE_UNUSED, return 0; }

-static void libxlFDDeregisterEventHook(void *priv ATTRIBUTE_UNUSED, - int fd ATTRIBUTE_UNUSED, - void *hnd) +static void +libxlFDDeregisterEventHook(void *priv ATTRIBUTE_UNUSED, + int fd ATTRIBUTE_UNUSED, + void *hnd) { struct libxlOSEventHookFDInfo *fdinfo = hnd;

virEventRemoveHandle(fdinfo->watch); }

- -static void libxlTimerCallback(int timer ATTRIBUTE_UNUSED, void *timer_v) +static void +libxlTimerCallback(int timer ATTRIBUTE_UNUSED, void *timer_v) { struct libxlOSEventHookTimerInfo *timer_info = timer_v;

libxl_osevent_occurred_timeout(timer_info->priv->ctx, timer_info->xl_priv); }

-static void libxlTimerInfoFree(void* obj) +static void +libxlTimerInfoFree(void* obj) { VIR_FREE(obj); }

-static int libxlTimeoutRegisterEventHook(void *priv, - void **hndp, - struct timeval abs_t, - void *for_libxl) +static int +libxlTimeoutRegisterEventHook(void *priv, + void **hndp, + struct timeval abs_t, + void *for_libxl) { struct timeval now; struct libxlOSEventHookTimerInfo *timer_info; @@ -222,9 +227,10 @@ static int libxlTimeoutRegisterEventHook(void *priv, return 0; }

-static int libxlTimeoutModifyEventHook(void *priv ATTRIBUTE_UNUSED, - void **hndp, - struct timeval abs_t) +static int +libxlTimeoutModifyEventHook(void *priv ATTRIBUTE_UNUSED, + void **hndp, + struct timeval abs_t) { struct timeval now; int timeout; @@ -237,8 +243,9 @@ static int libxlTimeoutModifyEventHook(void *priv ATTRIBUTE_UNUSED, return 0; }

-static void libxlTimeoutDeregisterEventHook(void *priv ATTRIBUTE_UNUSED, - void *hnd) +static void +libxlTimeoutDeregisterEventHook(void *priv ATTRIBUTE_UNUSED, + void *hnd) { struct libxlOSEventHookTimerInfo *timer_info = hnd;

@@ -283,7 +290,6 @@ libxlDomainObjPrivateFree(void *data) VIR_FREE(priv); }

- /* driver must be locked before calling */ static void libxlDomainEventQueue(libxlDriverPrivatePtr driver, virDomainEventPtr event) @@ -521,7 +527,8 @@ libxlVmReap(libxlDriverPrivatePtr driver, /* * Handle previously registered event notification from libxenlight */ -static void libxlEventHandler(void *data, const libxl_event *event) +static void +libxlEventHandler(void *data, const libxl_event *event) { libxlDriverPrivatePtr driver = libxl_driver; virDomainObjPtr vm = data;

ACK

On 2013年01月16日 07:15, Jim Fehlig wrote:

Use consistent parameter names throughout the libxl timeout and fd event functions. --- src/libxl/libxl_driver.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 4b8f205..94a78e2 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -100,9 +100,9 @@ static void libxlFDEventCallback(int watch ATTRIBUTE_UNUSED, int fd, int vir_events, - void *fdinfo) + void *fd_info) { - struct libxlOSEventHookFDInfo *info = fdinfo; + struct libxlOSEventHookFDInfo *info = fd_info; int events = 0;

if (vir_events& VIR_EVENT_HANDLE_READABLE) @@ -182,11 +182,11 @@ libxlFDDeregisterEventHook(void *priv ATTRIBUTE_UNUSED, }

static void -libxlTimerCallback(int timer ATTRIBUTE_UNUSED, void *timer_v) +libxlTimerCallback(int timer ATTRIBUTE_UNUSED, void *timer_info) { - struct libxlOSEventHookTimerInfo *timer_info = timer_v; + struct libxlOSEventHookTimerInfo *info = timer_info;

- libxl_osevent_occurred_timeout(timer_info->priv->ctx, timer_info->xl_priv); + libxl_osevent_occurred_timeout(info->priv->ctx, info->xl_priv); }

static void @@ -199,7 +199,7 @@ static int libxlTimeoutRegisterEventHook(void *priv, void **hndp, struct timeval abs_t, - void *for_libxl) + void *xl_priv) { struct timeval now; struct libxlOSEventHookTimerInfo *timer_info; @@ -221,7 +221,7 @@ libxlTimeoutRegisterEventHook(void *priv, }

timer_info->priv = priv; - timer_info->xl_priv = for_libxl; + timer_info->xl_priv = xl_priv; timer_info->id = timer_id; *hndp = timer_info; return 0;

ACK

On 2013年01月16日 07:15, Jim Fehlig wrote:

Callers should not free death events provided by libxl_evdisable_FOO(). --- src/libxl/libxl_driver.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 94a78e2..61ceae1 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -270,7 +270,6 @@ libxlDomainObjPrivateAlloc(void) return NULL;

libxl_ctx_alloc(&priv->ctx, LIBXL_VERSION, 0, libxl_driver->logger); - priv->deathW = NULL; libxl_osevent_register_hooks(priv->ctx,&libxl_event_callbacks, priv);

return priv; @@ -281,10 +280,8 @@ libxlDomainObjPrivateFree(void *data) { libxlDomainObjPrivatePtr priv = data;

- if (priv->deathW) { + if (priv->deathW) libxl_evdisable_domain_death(priv->ctx, priv->deathW); - VIR_FREE(priv->deathW); - }

libxl_ctx_free(priv->ctx); VIR_FREE(priv); @@ -604,9 +601,10 @@ libxlCreateDomEvents(virDomainObjPtr vm) return 0;

error: - if (priv->deathW) + if (priv->deathW) { libxl_evdisable_domain_death(priv->ctx, priv->deathW); - VIR_FREE(priv->deathW); + priv->deathW = NULL; + } return -1; }

ACK.

On 2013年01月16日 07:15, Jim Fehlig wrote:

--- src/libxl/libxl_driver.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 61ceae1..baa05e8 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -269,7 +269,12 @@ libxlDomainObjPrivateAlloc(void) if (VIR_ALLOC(priv)< 0) return NULL;

- libxl_ctx_alloc(&priv->ctx, LIBXL_VERSION, 0, libxl_driver->logger); + if (libxl_ctx_alloc(&priv->ctx, LIBXL_VERSION, 0, libxl_driver->logger)) { + VIR_ERROR(_("Failed libxl context initialization")); + VIR_FREE(priv); + return NULL; + }

Checked the codes of libxl_ctx_alloc, which returns greater than 0 on failure. So ACK.

+ libxl_osevent_register_hooks(priv->ctx,&libxl_event_callbacks, priv);

return priv;

On 2013年01月16日 07:15, Jim Fehlig wrote:

If building the libxl domain config fails, cleanup before returning failure. --- src/libxl/libxl_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index baa05e8..6da0272 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -769,7 +769,7 @@ libxlVmStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm, libxl_domain_config_init(&d_config);

if (libxlBuildDomainConfig(driver, vm->def,&d_config)< 0) - return -1; + goto error;

if (libxlFreeMem(priv,&d_config)< 0) { virReportError(VIR_ERR_INTERNAL_ERROR,

vm->hasManagedSave = false; } VIR_FREE(managed_save_path); This can be removed. But I'm fine if you keep it there when pushing } ACK.

Osier Yang wrote:

On 2013年01月16日 07:15, Jim Fehlig wrote:

...

If building the libxl domain config fails, cleanup before returning failure. --- src/libxl/libxl_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index baa05e8..6da0272 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -769,7 +769,7 @@ libxlVmStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm, libxl_domain_config_init(&d_config);

if (libxlBuildDomainConfig(driver, vm->def,&d_config)< 0) - return -1; + goto error;

if (libxlFreeMem(priv,&d_config)< 0) { virReportError(VIR_ERR_INTERNAL_ERROR,

vm->hasManagedSave = false; } VIR_FREE(managed_save_path);

This can be removed. It's freed in "error". But I'm fine if you keep it there when pushing. ACK

But managed_save_path would be leaked on success then. I pushed this one as is, and the other patches too. Thanks for the review, Jim