On 30.08.2013 23:46, Jim Fehlig wrote:
> The libxlDriverPrivate struct contains an variety of data with
> varying access needs. Similar to the QEMU and LXC drivers,
> move all the static config data into a dedicated libxlDriverConfig
> object. The only locking requirement is to hold the driver lock
> while obtaining an instance of libxlDriverConfig. Once a reference
> is held on the config object, it can be used completely lockless
> since it is immutable.
>
> Signed-off-by: Jim Fehlig <jfehlig@xxxxxxxx>
> ---
> src/libxl/libxl_conf.c | 124 ++++++++++++++++++-
> src/libxl/libxl_conf.h | 52 +++++---
> src/libxl/libxl_driver.c | 313 +++++++++++++++++++++++------------------------
> 3 files changed, 309 insertions(+), 180 deletions(-)
>
> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> index 231a53d..19fd8a6 100644
> --- a/src/libxl/libxl_conf.c
> +++ b/src/libxl/libxl_conf.c
> @@ -64,6 +64,41 @@ static const char *xen_cap_re = "(xen|hvm)-[[:digit:]]+\\.[[:digit:]]+-(x86_32|x
> static regex_t xen_cap_rec;
>
>
> +static virClassPtr libxlDriverConfigClass;
> +static void libxlDriverConfigDispose(void *obj);
> +
> +static int libxlConfigOnceInit(void)
> +{
> + if (!(libxlDriverConfigClass = virClassNew(virClassForObject(),
> + "libxlDriverConfig",
> + sizeof(libxlDriverConfig),
> + libxlDriverConfigDispose)))
> + return -1;
> +
> + return 0;
> +}
> +
> +VIR_ONCE_GLOBAL_INIT(libxlConfig)
> +
> +static void
> +libxlDriverConfigDispose(void *obj)
> +{
> + libxlDriverConfigPtr cfg = obj;
> +
> + virObjectUnref(cfg->caps);
> + libxl_ctx_free(cfg->ctx);
> + xtl_logger_destroy(cfg->logger);
> + if (cfg->logger_file)
> + VIR_FORCE_FCLOSE(cfg->logger_file);
> +
> + VIR_FREE(cfg->configDir);
> + VIR_FREE(cfg->autostartDir);
> + VIR_FREE(cfg->logDir);
> + VIR_FREE(cfg->stateDir);
> + VIR_FREE(cfg->libDir);
> + VIR_FREE(cfg->saveDir);
> +}
> +
> static int
> libxlCapsInitHost(libxl_ctx *ctx, virCapsPtr caps)
> {
> @@ -978,8 +1013,8 @@ error:
> return -1;
> }
>
> -bool
> -libxlGetAutoballoonConf(libxlDriverPrivatePtr driver)
> +static bool
> +libxlGetAutoballoonConf(libxlDriverConfigPtr cfg)
> {
> regex_t regex;
> int ret;
> @@ -990,11 +1025,94 @@ libxlGetAutoballoonConf(libxlDriverPrivatePtr driver)
> if (ret)
> return true;
>
> - ret = regexec(®ex, driver->verInfo->commandline, 0, NULL, 0);
> + ret = regexec(®ex, cfg->verInfo->commandline, 0, NULL, 0);
> regfree(®ex);
> return ret == REG_NOMATCH;
> }
>
> +libxlDriverConfigPtr
> +libxlDriverConfigNew(void)
> +{
> + libxlDriverConfigPtr cfg;
> + char *log_file = NULL;
> + char ebuf[1024];
> + unsigned int free_mem;
> +
> + if (libxlConfigInitialize() < 0)
> + return NULL;
> +
> + if (!(cfg = virObjectNew(libxlDriverConfigClass)))
> + return NULL;
> +
> + if (VIR_STRDUP(cfg->configDir, LIBXL_CONFIG_DIR) < 0)
> + goto error;
> + if (VIR_STRDUP(cfg->autostartDir, LIBXL_AUTOSTART_DIR) < 0)
> + goto error;
> + if (VIR_STRDUP(cfg->logDir, LIBXL_LOG_DIR) < 0)
> + goto error;
> + if (VIR_STRDUP(cfg->stateDir, LIBXL_STATE_DIR) < 0)
> + goto error;
> + if (VIR_STRDUP(cfg->libDir, LIBXL_LIB_DIR) < 0)
> + goto error;
> + if (VIR_STRDUP(cfg->saveDir, LIBXL_SAVE_DIR) < 0)
> + goto error;
> +
> + if (virAsprintf(&log_file, "%s/libxl-driver.log", cfg->logDir) < 0)
> + goto error;
> +
> + if ((cfg->logger_file = fopen(log_file, "a")) == NULL) {
> + VIR_ERROR(_("Failed to create log file '%s': %s"),
> + log_file, virStrerror(errno, ebuf, sizeof(ebuf)));
> + goto error;
> + }
> + VIR_FREE(log_file);
> +
> + cfg->logger =
> + (xentoollog_logger *)xtl_createlogger_stdiostream(cfg->logger_file,
> + XTL_DEBUG, 0);
> + if (!cfg->logger) {
> + VIR_ERROR(_("cannot create logger for libxenlight, disabling driver"));
> + goto error;
> + }
> +
> + if (libxl_ctx_alloc(&cfg->ctx, LIBXL_VERSION, 0, cfg->logger)) {
> + VIR_ERROR(_("cannot initialize libxenlight context, probably not "
> + "running in a Xen Dom0, disabling driver"));
> + goto error;
> + }
> +
> + if ((cfg->verInfo = libxl_get_version_info(cfg->ctx)) == NULL) {
> + VIR_ERROR(_("cannot version information from libxenlight, "
> + "disabling driver"));
> + goto error;
> + }
> + cfg->version = (cfg->verInfo->xen_version_major * 1000000) +
> + (cfg->verInfo->xen_version_minor * 1000);
> +
> + /* This will fill xenstore info about free and dom0 memory if missing,
> + * should be called before starting first domain */
> + if (libxl_get_free_memory(cfg->ctx, &free_mem)) {
> + VIR_ERROR(_("Unable to configure libxl's memory management parameters"));
> + goto error;
> + }
> +
> + /* setup autoballoon */
> + cfg->autoballoon = libxlGetAutoballoonConf(cfg);
> +
> + return cfg;
> +
> +error:
> + VIR_FREE(log_file);
> + virObjectUnref(cfg);
> + return NULL;
> +}
> +
> +libxlDriverConfigPtr
> +libxlDriverConfigGet(libxlDriverPrivatePtr driver)
> +{
> + return virObjectRef(driver->config);
> +}
> +
> virCapsPtr
> libxlMakeCapabilities(libxl_ctx *ctx)
> {
> diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h
> index be3a473..e3875ba 100644
> --- a/src/libxl/libxl_conf.h
> +++ b/src/libxl/libxl_conf.h
> @@ -51,10 +51,13 @@
>
> typedef struct _libxlDriverPrivate libxlDriverPrivate;
> typedef libxlDriverPrivate *libxlDriverPrivatePtr;
> -struct _libxlDriverPrivate {
> - virMutex lock;
> - virCapsPtr caps;
> - virDomainXMLOptionPtr xmlopt;
> +
> +typedef struct _libxlDriverConfig libxlDriverConfig;
> +typedef libxlDriverConfig *libxlDriverConfigPtr;
> +
> +struct _libxlDriverConfig {
> + virObject parent;
> +
> const libxl_version_info *verInfo;
> unsigned int version;
>
> @@ -64,27 +67,43 @@ struct _libxlDriverPrivate {
> /* libxl ctx for driver wide ops; getVersion, getNodeInfo, ... */
> libxl_ctx *ctx;
>
> - virPortAllocatorPtr reservedVNCPorts;
> -
> /* Controls automatic ballooning of domain0. If true, attempt to get
> * memory for new domains from domain0. */
> bool autoballoon;
>
> + /* Once created, caps are immutable */
> + virCapsPtr caps;
> +
> + char *configDir;
> + char *autostartDir;
> + char *logDir;
> + char *stateDir;
> + char *libDir;
> + char *saveDir;
> +};
> +
> +
> +struct _libxlDriverPrivate {
> + virMutex lock;
> +
> + /* Require lock to get reference on 'config',
> + * then lockless thereafter */
> + libxlDriverConfigPtr config;
> +
> size_t nactive;
> +
> virStateInhibitCallback inhibitCallback;
> void *inhibitOpaque;
>
> virDomainObjListPtr domains;
>
> + virDomainXMLOptionPtr xmlopt;
> +
> virDomainEventStatePtr domainEventState;
> - virSysinfoDefPtr hostsysinfo;
>
> - char *configDir;
> - char *autostartDir;
> - char *logDir;
> - char *stateDir;
> - char *libDir;
> - char *saveDir;
> + virPortAllocatorPtr reservedVNCPorts;
> +
> + virSysinfoDefPtr hostsysinfo;
> };
>
> typedef struct _libxlEventHookInfo libxlEventHookInfo;
> @@ -103,8 +122,11 @@ struct _libxlSavefileHeader {
> uint32_t unused[10];
> };
>
> -bool
> -libxlGetAutoballoonConf(libxlDriverPrivatePtr driver);
> +libxlDriverConfigPtr
> +libxlDriverConfigNew(void);
> +
> +libxlDriverConfigPtr
> +libxlDriverConfigGet(libxlDriverPrivatePtr driver);
>
> virCapsPtr
> libxlMakeCapabilities(libxl_ctx *ctx);
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index a26fbf6..e604899 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -126,35 +126,44 @@ libxlDoNodeGetInfo(libxlDriverPrivatePtr driver, virNodeInfoPtr info)
> {
> libxl_physinfo phy_info;
> virArch hostarch = virArchFromHost();
> + libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver);
> + int ret = -1;
>
> - if (libxl_get_physinfo(driver->ctx, &phy_info)) {
> + if (libxl_get_physinfo(cfg->ctx, &phy_info)) {
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> _("libxl_get_physinfo_info failed"));
> - return -1;
> + goto cleanup;
> }
>
> if (virStrcpyStatic(info->model, virArchToString(hostarch)) == NULL) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("machine type %s too big for destination"),
> virArchToString(hostarch));
> - return -1;
> + goto cleanup;
> }
>
> - info->memory = phy_info.total_pages * (driver->verInfo->pagesize / 1024);
> + info->memory = phy_info.total_pages * (cfg->verInfo->pagesize / 1024);
> info->cpus = phy_info.nr_cpus;
> info->nodes = phy_info.nr_nodes;
> info->cores = phy_info.cores_per_socket;
> info->threads = phy_info.threads_per_core;
> info->sockets = 1;
> info->mhz = phy_info.cpu_khz / 1000;
> - return 0;
> +
> + ret = 0;
> +
> +cleanup:
> + virObjectUnref(cfg);
> + return ret;
> }
>
> static char *
> libxlDomainManagedSavePath(libxlDriverPrivatePtr driver, virDomainObjPtr vm) {
> char *ret;
> + libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver);
>
> - ignore_value(virAsprintf(&ret, "%s/%s.save", driver->saveDir, vm->def->name));
> + ignore_value(virAsprintf(&ret, "%s/%s.save", cfg->saveDir, vm->def->name));
> + virObjectUnref(cfg);
> return ret;
> }
>
> @@ -168,6 +177,8 @@ libxlSaveImageOpen(libxlDriverPrivatePtr driver, const char *from,
> virDomainDefPtr def = NULL;
> libxlSavefileHeader hdr;
> char *xml = NULL;
> + libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver);
> + int ret = -1;
>
> if ((fd = virFileOpenAs(from, O_RDONLY, 0, -1, -1, 0)) < 0) {
> virReportSystemError(-fd,
> @@ -207,23 +218,25 @@ libxlSaveImageOpen(libxlDriverPrivatePtr driver, const char *from,
> goto error;
> }
>
> - if (!(def = virDomainDefParseString(xml, driver->caps, driver->xmlopt,
> + if (!(def = virDomainDefParseString(xml, cfg->caps, driver->xmlopt,
> 1 << VIR_DOMAIN_VIRT_XEN,
> VIR_DOMAIN_XML_INACTIVE)))
> goto error;
>
> - VIR_FREE(xml);
> -
> *ret_def = def;
> *ret_hdr = hdr;
>
> - return fd;
> + ret = fd;
> + goto cleanup;
>
> error:
> - VIR_FREE(xml);
> virDomainDefFree(def);
> VIR_FORCE_CLOSE(fd);
> - return -1;
> +
> +cleanup:
> + VIR_FREE(xml);
> + virObjectUnref(cfg);
> + return ret;
In libvirt we rather have the 'error' label below the 'cleanup'. It's
more common to jump from the 'error' to 'cleanup' then. So can you
please swap these two?
ACK if you fix this.
Michal
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list