On Thu, Jul 23, 2015 at 06:13:48PM +0800, Luyao Huang wrote:
> Signed-off-by: Luyao Huang <lhuang@xxxxxxxxxx>
> ---
> 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
> 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.
> 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.
> + 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.
> +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
> + 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 ?
> 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 ?
> +
> +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
> +# 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.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list