[libvirt] [PATCH 3/4] util: introduce new helpers to manage shmem device
Luyao Huang
lhuang at redhat.com
Sun Aug 2 14:00:36 UTC 2015
On 07/30/2015 06:12 PM, Daniel P. Berrange wrote:
> On Thu, Jul 23, 2015 at 06:13:48PM +0800, Luyao Huang 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
>> 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 = \
>> 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 \
> Indentation is messed up for these
Okay, got it
>> 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;
>> +
> Did you run 'make check' because they should fail on the
> sort ordering test.
Sorry, my fault, i forgot it, i will fix them in next version
>> 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
>> +#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);
>> +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;
>> + }
>> + } else {
>> + if (VIR_STRDUP(shmobj->drvname, drvname) < 0)
>> + goto error;
>> + }
>> +
>> + if (VIR_STRDUP(tmpdomain, domname) < 0)
>> + goto error;
>> +
>> + if (VIR_APPEND_ELEMENT(shmobj->domains, shmobj->ndomains, tmpdomain) < 0)
>> + goto error;
>> +
>> + return 0;
>> +
>> + error:
>> + VIR_FREE(tmpdomain);
> In this error codepath you have possibly already set
> shmobj->drvname, so you need to be able to undo that.
oh, good catch, i will fix this issue in next version.
>> + return -1;
>> +}
>> +#ifdef HAVE_SHM_OPEN
>> +int
>> +virShmOpen(const char *name,
>> + unsigned long long size,
>> + mode_t mode)
>> +{
>> + int fd = -1;
>> + struct stat sb;
>> +
>> + if ((fd = shm_open(name, O_RDWR, mode)) < 0) {
>> + virReportSystemError(errno,
>> + _("Unable to open shared memory"
>> + " objects '%s'"),
>> + name);
>> + return -1;
>> + }
>> +
>> + if (fstat(fd, &sb) < 0) {
>> + virReportSystemError(errno,
>> + _("cannot stat file '%s'"), name);
>> + goto error;
>> + }
>> +
>> + if (sb.st_size < size) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("already exist shared memory object"
>> + " size %ju is smaller than required size %llu"),
>> + (uintmax_t)sb.st_size, size);
>> + goto error;
> I think this needs to be != instead of <
>
> Consider Domain A with shmsize=1024 and Domain B with shmsize=2048
>
> The check will raise an error if domain A is started before domain B
> preventing both A & B running at same time. If B is started before
> A though, it will happily let both run.
Indeed, i will modify the code to forbid use a shm if the size is not
equal,
>> +int
>> +virShmCreate(const char *name,
>> + unsigned long long size,
>> + bool outerr,
>> + bool *othercreate,
>> + mode_t mode)
>> +{
>> + int fd = -1;
>> +
>> + fd = shm_open(name, O_RDWR|O_CREAT|O_EXCL, mode);
>> +
>> + if (fd > 0) {
>> + if (ftruncate(fd, size) != 0) {
>> + virReportSystemError(errno,
>> + _("Unable to truncate"
>> + " file descriptor to %llu"),
>> + size);
>> + ignore_value(shm_unlink(name));
>> + VIR_FORCE_CLOSE(fd);
>> + return -1;
>> + }
>> + } else if (errno == EEXIST) {
>> + if (outerr) {
>> + virReportSystemError(errno,
>> + _("shared memory objects"
>> + " '%s' is already exist"),
>> + name);
>> + return -1;
>> + }
>> +
>> + VIR_WARN("shared memory objects '%s' is already exist", name);
>> + *othercreate = true;
>> +
> You're leaking 'fd' in this case
Hmm...i couldn't find the leaking place, i guess your mean i will
leaking 'fd' when "errno == EEXIST", but at that time the fd <= 0.
>
>> + return virShmOpen(name, size, mode);
>> + } else {
>> + virReportSystemError(errno,
>> + _("Unable to create shared memory"
>> + " objects '%s'"),
>> + name);
>> + return -1;
>> + }
>> +
>> + return fd;
>> +}
>> +
>> +int
>> +virShmUnlink(const char *name)
>> +{
>> + int ret;
>> +
>> + if ((ret = shm_unlink(name)) < 0) {
>> + virReportSystemError(errno,
>> + _("Unable to delete shared memory"
>> + " objects '%s'"),
>> + name);
>> + }
>> + return ret;
>> +}
>> +
>> +# if defined(__linux__)
>> +# define SHM_DEFAULT_PATH "/dev/shm"
>> +
>> +int
>> +virShmBuildPath(const char *name, char **path)
>> +{
>> +
>> + if (virAsprintf(path, "%s/%s", SHM_DEFAULT_PATH, name) < 0)
>> + return -1;
>> +
>> + if (!virFileExists(*path)) {
>> + virReportSystemError(errno,
>> + _("could not access %s"),
>> + *path);
>> + VIR_FREE(*path);
>> + return -1;
>> + }
>> + return 0;
>> +}
>> +
>> +# else
>> +
>> +int
>> +virShmBuildPath(const char *name ATTRIBUTE_UNUSED,
>> + char **path ATTRIBUTE_UNUSED)
>> +{
>> + virReportSystemError(ENOSYS, "%s",
>> + _("Cannot get share memory object path on this platform"));
>> + return -2;
>> +}
> Why -2 instead of -1 ?
I want to make caller function know if we not support this function. But
i am not sure it is a good idea, and i will give a explanation about why
we do this in another mail.
>> diff --git a/src/util/virshm.h b/src/util/virshm.h
>> new file mode 100644
>> index 0000000..945a541
>> --- /dev/null
>> +++ b/src/util/virshm.h
>> +# include "internal.h"
>> +# include "virutil.h"
>> +# include "virobject.h"
>> +
>> +typedef enum {
>> + VIR_SHM_TYPE_SHM = 0,
>> + VIR_SHM_TYPE_SERVER,
>> +
>> + VIR_SHM_TYPE_LAST
>> +} virShmObjectType;
> I'm unclear why we need the different type constants
> here ?
I want to avoid the wrong region from the different type shmem device,
here is a example:
There is a shmem device which won't use ivshmem-server and we create a
new shmobject to save it information, and then a shmem device which have
the same name with the first shmem device but its a server enabled shmem
device need region to list too, then we will try to add the second shmem
device to the first shmobject if the size is equal.
So i want to introduce a new type constants to split these different
shmem device.
>> +
>> +typedef struct _virShmObject virShmObject;
>> +typedef virShmObject *virShmObjectPtr;
>> +struct _virShmObject {
>> + char *name; /* shmem object name */
>> + int type; /* shmem object type */
>> + unsigned long long size; /* size of shmem object */
>> + char *path; /* shmem path */
>> + bool othercreate; /* a bool parameter record if the shm is created by libvirt */
>> +
>> + char *drvname; /* which driver */
>> + char **domains; /* domain(s) using this shm */
>> + size_t ndomains; /* number of useds */
>> +};
>> +
>> +VIR_ENUM_DECL(virShmObject);
>> +
>> +typedef struct _virShmObjectList virShmObjectList;
>> +typedef virShmObjectList *virShmObjectListPtr;
>> +struct _virShmObjectList {
>> + virObjectLockable parent;
>> + char *stateDir;
>> + size_t nshmobjs;
>> + virShmObjectPtr *shmobjs;
>> +};
> I think of the struct definitions should be private. If
> callers need access to the contents we can provide APIs
>
Okay, i will move them in virshm.c in next version
>> +# ifdef HAVE_SHM_OPEN
>> +int virShmCreate(const char *name, unsigned long long size,
>> + bool outerr, bool *othercreate, mode_t mode)
>> + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(4);
>> +int virShmOpen(const char *name,
>> + unsigned long long size,
>> + mode_t mode)
>> + ATTRIBUTE_NONNULL(1);
>> +int virShmUnlink(const char *name)
>> + ATTRIBUTE_NONNULL(1);
>> +int virShmBuildPath(const char *name, char **path)
>> + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
>> +# else /* HAVE_SHM_OPEN */
>> +# define virShmCreate(name, size, outerr, othercreate, mode) -2
>> +# define virShmOpen(name, size, mode) -2
>> +# define virShmUnlink(name) -2
>> +# define virShmBuildPath(name, path) -2
>> +
>> +# endif /* HAVE_SHM_OPEN */
> You can't just #define away the APIs - this will lead to undefined
> symbols being referenced in the linker script which breaks on
> Win32. You need to provide stub implementations. Also you need to
> use virReportError() any time you return an error status, as you
> already did for virShmBuildPath.
Oh, i see, i will fix them.
Thanks a lot for your review and help.
> Regards,
> Daniel
Thanks,
Luyao
More information about the libvir-list
mailing list