Re: [libvirt] [PATCH 3/4] util: introduce new helpers to manage shmem device

Hi On Thu, Jul 23, 2015 at 12:13 PM, Luyao Huang <lhuang(a)redhat.com&gt; wrote: > Signed-off-by: Luyao Huang <lhuang(a)redhat.com> --- > configure.ac | 10 + > po/POTFILES.in | 3 +- > src/Makefile.am | 5 +- > src/libvirt_private.syms | 16 ++ > src/util/virshm.c | 623 +++++++++++++++++++++++++++++++++++++++++++++++ > src/util/virshm.h | 104 ++++++++ > 6 files changed, 759 insertions(+), 2 deletions(-) > create mode 100644 src/util/virshm.c > create mode 100644 src/util/virshm.h (would be nicer to have some tests) After applying this patch, make check fails with Symbol block at ./libvirt_private.syms:2081: symbols not sorted virShmObjectFree virShmObjectNew virShmObjectListAdd virShmObjectListDel virShmObjectFindByName virShmObjectListGetDefault Correct ordering virShmObjectFindByName virShmObjectFree virShmObjectListAdd virShmObjectListDel virShmObjectListGetDefault virShmObjectNew Makefile:10195: recipe for target 'check-symsorting' failed

> diff --git a/configure.ac b/configure.ac > index a7f38e8..940eb66 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -1176,6 +1176,16 @@ if test "$with_linux" = "yes"; then > ]]) > fi > +dnl > +dnl check for POSIX share memory functions > +dnl > +LIBRT_LIBS="" > +AC_CHECK_LIB([rt],[shm_open],[LIBRT_LIBS="-lrt"]) > +old_libs="$LIBS" > +LIBS="$old_libs $LIBRT_LIBS" > +AC_CHECK_FUNCS([shm_open]) > +LIBS="$old_libs" > +AC_SUBST([LIBRT_LIBS]) > dnl Need to test if pkg-config exists > PKG_PROG_PKG_CONFIG > diff --git a/po/POTFILES.in b/po/POTFILES.in > index a75f5ae..7687a82 100644 > --- a/po/POTFILES.in > +++ b/po/POTFILES.in > @@ -215,8 +215,9 @@ src/util/virpolkit.c > src/util/virportallocator.c > src/util/virprocess.c > src/util/virrandom.c > -src/util/virsexpr.c > src/util/virscsi.c > +src/util/virsexpr.c > +src/util/virshm.c > src/util/virsocketaddr.c > src/util/virstats.c > src/util/virstorageencryption.c > diff --git a/src/Makefile.am b/src/Makefile.am > index 7338ab9..048a096 100644 > --- a/src/Makefile.am > +++ b/src/Makefile.am > @@ -152,6 +152,7 @@ UTIL_SOURCES = \ > util/virscsi.c util/virscsi.h \ > util/virseclabel.c util/virseclabel.h \ > util/virsexpr.c util/virsexpr.h \ > + util/virshm.c util/virshm.h \ > util/virsocketaddr.h util/virsocketaddr.c \ > util/virstats.c util/virstats.h \ > util/virstorageencryption.c util/virstorageencryption.h \ > @@ -1048,7 +1049,7 @@ libvirt_util_la_LIBADD = $(CAPNG_LIBS) $(YAJL_LIBS) $(LIBNL_LIBS) \ > $(THREAD_LIBS) $(AUDIT_LIBS) $(DEVMAPPER_LIBS) \ > $(LIB_CLOCK_GETTIME) $(DBUS_LIBS) $(MSCOM_LIBS) $(LIBXML_LIBS) \ > $(SECDRIVER_LIBS) $(NUMACTL_LIBS) $(SYSTEMD_DAEMON_LIBS) \ > - $(POLKIT_LIBS) > + $(POLKIT_LIBS) $(LIBRT_LIBS) > noinst_LTLIBRARIES += libvirt_conf.la > @@ -1284,6 +1285,7 @@ libvirt_driver_qemu_impl_la_LIBADD = $(CAPNG_LIBS) \ > $(GNUTLS_LIBS) \ > $(LIBNL_LIBS) \ > $(LIBXML_LIBS) \ > + $(LIBRT_LIBS) \ > $(NULL) > libvirt_driver_qemu_impl_la_SOURCES = $(QEMU_DRIVER_SOURCES) > @@ -2264,6 +2266,7 @@ libvirt_setuid_rpc_client_la_LDFLAGS = \ > $(AM_LDFLAGS) \ > $(LIBXML_LIBS) \ > $(SECDRIVER_LIBS) \ > + $(LIBRT_LIBS) \ > $(NULL) > libvirt_setuid_rpc_client_la_CFLAGS = \ > -DLIBVIRT_SETUID_RPC_CLIENT \ > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms > index af73177..977fd34 100644 > --- a/src/libvirt_private.syms > +++ b/src/libvirt_private.syms > @@ -2078,6 +2078,22 @@ sexpr_u64; > string2sexpr; > +# util/virshm.h > +virShmBuildPath; > +virShmCreate; > +virShmObjectFree; > +virShmObjectNew; > +virShmObjectListAdd; > +virShmObjectListDel; > +virShmObjectFindByName; > +virShmObjectListGetDefault; > +virShmObjectRemoveStateFile; > +virShmObjectSaveState; > +virShmOpen; > +virShmRemoveUsedDomain; > +virShmSetUsedDomain; > +virShmUnlink; So these lines should be sorted

> + > # util/virsocketaddr.h > virSocketAddrBroadcast; > virSocketAddrBroadcastByPrefix; > diff --git a/src/util/virshm.c b/src/util/virshm.c > new file mode 100644 > index 0000000..7ab39be > --- /dev/null > +++ b/src/util/virshm.c > @@ -0,0 +1,623 @@ > +/* > + * virshm.c: helper API for POSIX share memory > + * > + * Copyright (C) 2015 Red Hat, Inc. > + * > + * 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, see > + * <http://www.gnu.org/licenses/>;. > + * > + */ > + > +#include <config.h> + > +#include <fcntl.h> +#include <sys/stat.h> +#include <sys/types.h> + > +#ifdef HAVE_SHM_OPEN > +# include <sys/mman.h> +#endif > + > +#include "virshm.h" > +#include "virxml.h" > +#include "virbuffer.h" > +#include "virerror.h" > +#include "virstring.h" > +#include "virlog.h" > +#include "virutil.h" > +#include "viralloc.h" > +#include "virfile.h" > +#include "configmake.h" > + > +#define VIR_FROM_THIS VIR_FROM_NONE > + > +VIR_LOG_INIT("util.shm"); > + > +#define SHMEM_STATE_DIR LOCALSTATEDIR "/run/libvirt/shmem" > + > +VIR_ENUM_IMPL(virShmObject, VIR_SHM_TYPE_LAST, > + "shm", > + "server"); > + > +static virClassPtr virShmObjectListClass; > + > +static virShmObjectListPtr mainlist; /* global shm object list */ > + > +static void virShmObjectListDispose(void *obj); > + > +static int > +virShmObjectListLoadState(virShmObjectPtr *shmobj, > + const char *stateDir, > + const char *name) > +{ > + char *stateFile = NULL; > + xmlXPathContextPtr ctxt = NULL; > + xmlDocPtr xml = NULL; > + virShmObjectPtr tmpshm; > + xmlNodePtr *usagenode = NULL; > + xmlNodePtr save = NULL; save could be moved in the using scope

> + int ret = -1; > + char *drivername = NULL; > + char *shmtype = NULL; > + int nusages; > + > + if (VIR_ALLOC(tmpshm) < 0) > + return -1; > + > + if (!(stateFile = virFileBuildPath(stateDir, name, ".xml"))) > + goto error; > + > + if (!(xml = virXMLParseFileCtxt(stateFile, &ctxt))) > + goto error; > + > + tmpshm->name = virXPathString("string(./name)", ctxt); > + if (!tmpshm->name) { > + virReportError(VIR_ERR_XML_ERROR, "%s", > + _("shmem missing name attribute")); > + goto error; > + } > + > + shmtype = virXPathString("string(./type)", ctxt); > + if (!shmtype) { > + virReportError(VIR_ERR_XML_ERROR, "%s", > + _("shmem missing type attribute")); > + goto error; > + } > + if ((tmpshm->type = virShmObjectTypeFromString(shmtype)) < 0) { > + virReportError(VIR_ERR_XML_ERROR, > + _("invalid shmem object type %s"), shmtype); You leak shmtype here and in other goto error > + goto error; > + } > + > + if (virXPathULongLong("string(./size)", ctxt, &tmpshm->size) < 0) { > + virReportError(VIR_ERR_XML_ERROR, "%s", > + _("shmem missing or have invalid size attribute")); > + goto error; > + } > + > + tmpshm->path = virXPathString("string(./path)", ctxt); > + if (virXPathBoolean("boolean(./othercreate)", ctxt)) > + tmpshm->othercreate = true; > + > + if (!(drivername = virXPathString("string(./@driver)", ctxt))) { > + virReportError(VIR_ERR_XML_ERROR, "%s", > + _("shmem usage element missing driver attribute")); > + goto error; > + } > + nusages = virXPathNodeSet("./domain", ctxt, &usagenode); > + if (nusages < 0) > + goto error; > + > + if (nusages > 0) { > + size_t i; > + > + for (i = 0; i < nusages; i++) { > + char *domainname; > + > + save = ctxt->node; > + ctxt->node = usagenode[i]; > + > + if (!(domainname = virXPathString("string(./@name)", ctxt))) { > + virReportError(VIR_ERR_XML_ERROR, "%s", > + _("shmem domain element missing name attribute")); > + goto error; > + } > + > + if (virShmSetUsedDomain(tmpshm, drivername, domainname) < 0) { > + VIR_FREE(domainname); > + goto error; > + } > + VIR_FREE(domainname); > + ctxt->node = save; > + } > + } > + *shmobj = tmpshm; > + ret = 0; > + cleanup: > + VIR_FREE(stateFile); > + VIR_FREE(drivername); > + VIR_FREE(shmtype); > + xmlFreeDoc(xml); > + xmlXPathFreeContext(ctxt); > + return ret; > + > + error: > + virShmObjectFree(tmpshm); > + goto cleanup; > +} > + > +static int > +virShmObjectListLoadAllState(virShmObjectListPtr list) > +{ > + DIR *dir; > + struct dirent *entry; > + > + if (!(dir = opendir(list->stateDir))) { > + if (errno == ENOENT) > + return 0; > + virReportSystemError(errno, _("Failed to open dir '%s'"), list->stateDir); > + return -1; > + } > + > + while (virDirRead(dir, &entry, list->stateDir) > 0) { > + virShmObjectPtr shmobj; > + > + if (entry->d_name[0] == '.' || > + !virFileStripSuffix(entry->d_name, ".xml")) > + continue; > + if (virShmObjectListLoadState(&shmobj, list->stateDir, entry->d_name) < 0) > + continue; > + if (virShmObjectListAdd(list, shmobj) < 0) > + continue; > + } > + closedir(dir); > + return 0; > +} > + > +static virShmObjectListPtr > +virShmObjectListNew(void) > +{ > + virShmObjectListPtr list; > + bool privileged = geteuid() == 0; > + > + if (!(list = virObjectLockableNew(virShmObjectListClass))) > + return NULL; > + > + virObjectLock(list); I am not sure the lock is necessary in a New function.

> + if (privileged) { > + if (VIR_STRDUP(list->stateDir, SHMEM_STATE_DIR) < 0) > + goto error; > + } else { > + char *rundir = NULL; > + > + if (!(rundir = virGetUserRuntimeDirectory())) > + goto error; > + > + if (virAsprintf(&list->stateDir, "%s/shmem", rundir) < 0) { > + VIR_FREE(rundir); > + goto error; > + } > + VIR_FREE(rundir); > + } > + if (virFileMakePath(list->stateDir) < 0) { > + virReportError(VIR_ERR_OPERATION_FAILED, > + _("Failed to create state dir '%s'"), > + list->stateDir); > + goto error; > + } > + if (virShmObjectListLoadAllState(list) < 0) > + goto error; > + > + virObjectUnlock(list); > + return list; > + > + error: > + virObjectUnlock(list); > + virObjectUnref(list); > + return NULL; > +} > + > +static int > +virShmOnceInit(void) > +{ > + if (!(virShmObjectListClass = virClassNew(virClassForObjectLockable(), > + "virShmObjectList", > + sizeof(virShmObjectList), > + virShmObjectListDispose))) > + return -1; > + > + if (!(mainlist = virShmObjectListNew())) > + return -1; > + > + return 0; > +} > + > +VIR_ONCE_GLOBAL_INIT(virShm) > + > +virShmObjectListPtr > +virShmObjectListGetDefault(void) > +{ > + if (virShmInitialize() < 0) > + return NULL; > + > + return virObjectRef(mainlist); > +} > + > +int > +virShmSetUsedDomain(virShmObjectPtr shmobj, > + const char *drvname, > + const char *domname) > +{ > + char *tmpdomain = NULL; > + > + if (shmobj->drvname) { > + if (STRNEQ(drvname, shmobj->drvname)) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("cannot use one shmem for different driver")); > + goto error; > + } I don't I understand the limitation there. Imho, this could be just a warning.