9 a.m.
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(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
> 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