On Tue, Jul 24, 2012 at 14:22:49 +0100, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange@xxxxxxxxxx>
>
> Move the code that handles the LXC monitor out of the
> lxc_process.c file and into lxc_monitor.{c,h}
>
> Signed-off-by: Daniel P. Berrange <berrange@xxxxxxxxxx>
> ---
> po/POTFILES.in | 1 +
> src/Makefile.am | 1 +
> src/lxc/lxc_domain.h | 4 +-
> src/lxc/lxc_monitor.c | 163 +++++++++++++++++++++++++++++++++++++++++++++++++
> src/lxc/lxc_monitor.h | 54 ++++++++++++++++
> src/lxc/lxc_process.c | 70 ++++++++++++---------
> 6 files changed, 262 insertions(+), 31 deletions(-)
> create mode 100644 src/lxc/lxc_monitor.c
> create mode 100644 src/lxc/lxc_monitor.h
>
> diff --git a/po/POTFILES.in b/po/POTFILES.in
> index 7587c61..025d7fa 100644
> --- a/po/POTFILES.in
> +++ b/po/POTFILES.in
> @@ -47,6 +47,7 @@ src/lxc/lxc_container.c
> src/lxc/lxc_conf.c
> src/lxc/lxc_controller.c
> src/lxc/lxc_driver.c
> +src/lxc/lxc_monitor.c
> src/lxc/lxc_process.c
> src/libxl/libxl_driver.c
> src/libxl/libxl_conf.c
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 93fcf3b..a910aa1 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -352,6 +352,7 @@ LXC_DRIVER_SOURCES = \
> lxc/lxc_container.c lxc/lxc_container.h \
> lxc/lxc_cgroup.c lxc/lxc_cgroup.h \
> lxc/lxc_domain.c lxc/lxc_domain.h \
> + lxc/lxc_monitor.c lxc/lxc_monitor.h \
> lxc/lxc_process.c lxc/lxc_process.h \
> lxc/lxc_driver.c lxc/lxc_driver.h
>
> diff --git a/src/lxc/lxc_domain.h b/src/lxc/lxc_domain.h
> index 799248d..ff7c96f 100644
> --- a/src/lxc/lxc_domain.h
> +++ b/src/lxc/lxc_domain.h
> @@ -24,13 +24,13 @@
> # define __LXC_DOMAIN_H__
>
> # include "lxc_conf.h"
> -# include "rpc/virnetclient.h"
> +# include "lxc_monitor.h"
>
>
> typedef struct _virLXCDomainObjPrivate virLXCDomainObjPrivate;
> typedef virLXCDomainObjPrivate *virLXCDomainObjPrivatePtr;
> struct _virLXCDomainObjPrivate {
> - virNetClientPtr monitor;
> + virLXCMonitorPtr monitor;
> };
>
> void virLXCDomainSetPrivateDataHooks(virCapsPtr caps);
> diff --git a/src/lxc/lxc_monitor.c b/src/lxc/lxc_monitor.c
> new file mode 100644
> index 0000000..6740aa7
> --- /dev/null
> +++ b/src/lxc/lxc_monitor.c
> @@ -0,0 +1,163 @@
> +/*
> + * Copyright (C) 2010-2012 Red Hat, Inc.
> + *
> + * lxc_monitor.c: client for LXC controller monitor
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> + */
The last paragraph should rather be
* You should have received a copy of the GNU Lesser General Public
* License along with this library; If not, see
* <http://www.gnu.org/licenses/>.
> +
> +#include <config.h>
> +
> +#include "lxc_monitor.h"
> +#include "lxc_conf.h"
> +
> +#include "memory.h"
> +
> +#include "virterror_internal.h"
> +#include "logging.h"
> +#include "threads.h"
> +#include "rpc/virnetclient.h"
> +
> +#define VIR_FROM_THIS VIR_FROM_LXC
> +
> +struct _virLXCMonitor {
> + int refs;
> +
> + virMutex lock; /* also used to protect fd */
Which fd are you referring to?
> +
> + virDomainObjPtr vm;
> + virLXCMonitorCallbacksPtr cb;
> +
> + virNetClientPtr client;
> +};
> +
> +static void virLXCMonitorFree(virLXCMonitorPtr mon);
> +
> +
> +static void virLXCMonitorEOFNotify(virNetClientPtr client ATTRIBUTE_UNUSED,
> + int reason ATTRIBUTE_UNUSED,
> + void *opaque)
> +{
> + virLXCMonitorPtr mon = opaque;
> + virLXCMonitorCallbackEOFNotify eofNotify;
> + virDomainObjPtr vm;
> +
> + virLXCMonitorLock(mon);
> + eofNotify = mon->cb->eofNotify;
> + vm = mon->vm;
> + virLXCMonitorUnlock(mon);
> +
> + eofNotify(mon, vm);
> +}
> +
> +
> +virLXCMonitorPtr virLXCMonitorNew(virDomainObjPtr vm,
> + const char *socketdir,
> + virLXCMonitorCallbacksPtr cb)
> +{
> + virLXCMonitorPtr mon;
> + char *sockpath = NULL;
> +
> + if (VIR_ALLOC(mon) < 0)
virReportOOMError() is missing here
> + return NULL;
> +
> + if (virMutexInit(&mon->lock) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("cannot initialize monitor mutex"));
> + VIR_FREE(mon);
> + return NULL;
> + }
> +
> + if (virAsprintf(&sockpath, "%s/%s.sock",
> + socketdir, vm->def->name) < 0)
> + goto no_memory;
> +
> + if (!(mon->client = virNetClientNewUNIX(sockpath, false, NULL)))
> + goto error;
> +
> +
> + mon->vm = vm;
> + mon->cb = cb;
> +
> + virNetClientSetCloseCallback(mon->client, virLXCMonitorEOFNotify, mon, NULL);
> +
> +cleanup:
> + VIR_FREE(sockpath);
> + return mon;
> +
> +no_memory:
> + virReportOOMError();
> +error:
> + virLXCMonitorClose(mon);
> + virLXCMonitorFree(mon);
No need to explicitly call virLXCMonitorClose here, virLXCMonitorFree already
calls virLXCMonitorClose if necessary.
> + mon = NULL;
> + goto cleanup;
> +}
> +
> +
> +static void virLXCMonitorFree(virLXCMonitorPtr mon)
> +{
> + VIR_DEBUG("mon=%p", mon);
> + if (mon->client)
> + virLXCMonitorClose(mon);
> +
> + if (mon->cb && mon->cb->destroy)
> + (mon->cb->destroy)(mon, mon->vm);
> + virMutexDestroy(&mon->lock);
> + virNetClientFree(mon->client);
No need to call virNetClientFree here since mon->client will always be NULL
at this point (virLXCMonitorClose already freed it).
> + VIR_FREE(mon);
> +}
> +
> +
> +int virLXCMonitorRef(virLXCMonitorPtr mon)
> +{
> + mon->refs++;
> + return mon->refs;
> +}
> +
> +int virLXCMonitorUnref(virLXCMonitorPtr mon)
> +{
> + mon->refs--;
> +
> + if (mon->refs == 0) {
> + virLXCMonitorUnlock(mon);
> + virLXCMonitorFree(mon);
> + return 0;
> + }
> +
> + return mon->refs;
> +}
> +
> +
> +void virLXCMonitorClose(virLXCMonitorPtr mon)
> +{
> + if (mon->client) {
> + virNetClientClose(mon->client);
> + virNetClientFree(mon->client);
> + mon->client = NULL;
> + }
> +}
> +
> +
> +void virLXCMonitorLock(virLXCMonitorPtr mon)
> +{
> + virMutexLock(&mon->lock);
> +}
> +
> +
> +void virLXCMonitorUnlock(virLXCMonitorPtr mon)
> +{
> + virMutexUnlock(&mon->lock);
> +}
> diff --git a/src/lxc/lxc_monitor.h b/src/lxc/lxc_monitor.h
> new file mode 100644
> index 0000000..32b8d36
> --- /dev/null
> +++ b/src/lxc/lxc_monitor.h
> @@ -0,0 +1,54 @@
> +/*
> + * Copyright (C) 2010-2012 Red Hat, Inc.
> + *
> + * lxc_monitor.h: client for LXC controller monitor
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
Obsolete usage of FSF address again.
> + */
> +
> +#ifndef __LXC_MONITOR_H__
> +# define __LXC_MONITOR_H__
> +
> +# include "domain_conf.h"
> +
> +typedef struct _virLXCMonitor virLXCMonitor;
> +typedef virLXCMonitor *virLXCMonitorPtr;
> +
> +typedef struct _virLXCMonitorCallbacks virLXCMonitorCallbacks;
> +typedef virLXCMonitorCallbacks *virLXCMonitorCallbacksPtr;
> +
> +typedef void (*virLXCMonitorCallbackDestroy)(virLXCMonitorPtr mon,
> + virDomainObjPtr vm);
> +typedef void (*virLXCMonitorCallbackEOFNotify)(virLXCMonitorPtr mon,
> + virDomainObjPtr vm);
> +
> +struct _virLXCMonitorCallbacks {
> + virLXCMonitorCallbackDestroy destroy;
> + virLXCMonitorCallbackEOFNotify eofNotify;
> +};
> +
> +virLXCMonitorPtr virLXCMonitorNew(virDomainObjPtr vm,
> + const char *socketdir,
> + virLXCMonitorCallbacksPtr cb);
> +
> +void virLXCMonitorClose(virLXCMonitorPtr mon);
> +
> +void virLXCMonitorLock(virLXCMonitorPtr mon);
> +void virLXCMonitorUnlock(virLXCMonitorPtr mon);
> +
> +int virLXCMonitorRef(virLXCMonitorPtr mon);
> +int virLXCMonitorUnref(virLXCMonitorPtr mon);
> +
> +#endif /* __LXC_MONITOR_H__ */
> diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
> index 3a1959f..1bfd032 100644
> --- a/src/lxc/lxc_process.c
> +++ b/src/lxc/lxc_process.c
> @@ -178,9 +178,11 @@ static void virLXCProcessCleanup(virLXCDriverPtr driver,
> /* Stop autodestroy in case guest is restarted */
> virLXCProcessAutoDestroyRemove(driver, vm);
>
> - virNetClientClose(priv->monitor);
> - virNetClientFree(priv->monitor);
> - priv->monitor = NULL;
> + if (priv->monitor) {
> + virLXCMonitorClose(priv->monitor);
> + virLXCMonitorUnref(priv->monitor);
Shouldn't we unlock the monitor if virLXCMonitorUnref returns positive number?
If not because we know priv->monitor is the last reference, we don't need to
explicitly call virLXCMonitorClose. In any case, virLXCMonitorUnref requires
monitor to be locked.
> + priv->monitor = NULL;
> + }
>
> virPidFileDelete(driver->stateDir, vm->def->name);
> virDomainDeleteConfig(driver->stateDir, NULL, vm);
> @@ -471,13 +473,25 @@ cleanup:
> }
>
>
> +static void virLXCProcessMonitorDestroy(virLXCMonitorPtr mon,
> + virDomainObjPtr vm)
> +{
> + virLXCDomainObjPrivatePtr priv;
> +
> + virDomainObjLock(vm);
> + priv = vm->privateData;
> + if (priv->monitor == mon)
> + priv->monitor = NULL;
> + if (virDomainObjUnref(vm) > 0)
> + virDomainObjUnlock(vm);
> +}
> +
> +
> extern virLXCDriverPtr lxc_driver;
> -static void virLXCProcessMonitorEOFNotify(virNetClientPtr client ATTRIBUTE_UNUSED,
> - int reason ATTRIBUTE_UNUSED,
> - void *opaque)
> +static void virLXCProcessMonitorEOFNotify(virLXCMonitorPtr mon ATTRIBUTE_UNUSED,
> + virDomainObjPtr vm)
> {
> virLXCDriverPtr driver = lxc_driver;
> - virDomainObjPtr vm = opaque;
> virDomainEventPtr event = NULL;
>
> lxcDriverLock(driver);
> @@ -504,36 +518,32 @@ static void virLXCProcessMonitorEOFNotify(virNetClientPtr client ATTRIBUTE_UNUSE
> }
>
>
> -static virNetClientPtr virLXCProcessConnectMonitor(virLXCDriverPtr driver,
> - virDomainObjPtr vm)
> -{
> - char *sockpath = NULL;
> - virNetClientPtr monitor = NULL;
> +static virLXCMonitorCallbacks monitorCallbacks = {
> + .eofNotify = virLXCProcessMonitorEOFNotify,
> + .destroy = virLXCProcessMonitorDestroy,
> +};
>
> - if (virAsprintf(&sockpath, "%s/%s.sock",
> - driver->stateDir, vm->def->name) < 0) {
> - virReportOOMError();
> - return NULL;
> - }
> +
> +static virLXCMonitorPtr virLXCProcessConnectMonitor(virLXCDriverPtr driver,
> + virDomainObjPtr vm)
> +{
> + virLXCMonitorPtr monitor = NULL;
>
> if (virSecurityManagerSetSocketLabel(driver->securityManager, vm->def) < 0)
> goto cleanup;
>
> - monitor = virNetClientNewUNIX(sockpath, false, NULL);
> + monitor = virLXCMonitorNew(vm, driver->stateDir, &monitorCallbacks);
>
> if (virSecurityManagerClearSocketLabel(driver->securityManager, vm->def) < 0) {
> - virNetClientFree(monitor);
> - monitor = NULL;
> + if (monitor) {
> + virLXCMonitorClose(monitor);
> + virLXCMonitorUnref(monitor);
No need to explicitly call virLXCMonitorClose here. Not to mention that
virLXCMonitorUnref requires monitor to be locked and virLXCMonitorNew doesn't
return locked monitor nor does it initialize refs to 1. This code would just
result in leaked memory and monitor->refs == -1.
> + monitor = NULL;
> + }
> goto cleanup;
> }
>
> - if (!monitor)
> - goto cleanup;
> -
> - virNetClientSetCloseCallback(monitor, virLXCProcessMonitorEOFNotify, vm, NULL);
> -
> cleanup:
> - VIR_FREE(sockpath);
> return monitor;
> }
>
> @@ -1041,9 +1051,11 @@ cleanup:
> VIR_FREE(veths[i]);
> }
> if (rc != 0) {
> - virNetClientClose(priv->monitor);
> - virNetClientFree(priv->monitor);
> - priv->monitor = NULL;
> + if (priv->monitor) {
> + virLXCMonitorClose(priv->monitor);
> + virLXCMonitorUnref(priv->monitor);
> + priv->monitor = NULL;
> + }
Another weired usage of virLXCMonitorUnref.
> virDomainConfVMNWFilterTeardown(vm);
>
> virSecurityManagerRestoreAllLabel(driver->securityManager,
The reference counting and locking is not ideal throughout this patch.
Jirka
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list