[libvirt] [PATCH 3/4] util: introduce new helpers to manage shmem device
Luyao Huang
lhuang at redhat.com
Sun Aug 2 14:00:22 UTC 2015
Hi Marc-André
On 07/27/2015 11:43 PM, Marc-André Lureau wrote:
> Hi
>
> On Thu, Jul 23, 2015 at 12:13 PM, Luyao Huang <lhuang at redhat.com> wrote:
>> Signed-off-by: Luyao Huang <lhuang at 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
>
Oh, my fault, i forgot this, thanks for pointing out.
>> 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
Yes, i will fix them in next version
>> +
>> # 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
Okay
>> + 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.
Hmm...indeed, maybe we could remove the lock here.
>> + 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.
In my opinion, shmem device could be used in different driver (although
only qemu use it right now). The shmem device set up part will be
different in different driver, it will cause some strange issue during
the set up and clean up part.
Thanks a lot for your review and help.
Luyao
>
More information about the libvir-list
mailing list