On Wed, 2009-08-26 at 14:03 +0100, Daniel P. Berrange wrote:
I hope you're planning on adding all that useful info from the 0/1 mail
to the git commit log?
> * configure.in: Add check for mntent.h
> * qemud/libvirtd_qemu.aug, qemud/test_libvirtd_qemu.aug, src/qemu.conf
> Add 'hugetlbfs_mount' config parameter
> * src/qemu_conf.c, src/qemu_conf.h: Check for -mem-path flag in QEMU,
> and pass it when hugepages are requested.
> Load hugetlbfs_mount config parameter, search for mount if not given.
> * src/qemu_driver.c: Free hugetlbfs_mount/path parameter in driver shutdown.
> Create directory for QEMU hugepage usage, chowning if required.
> * docs/formatdomain.html.in: Document memoryBacking/hugepages elements
> * docs/schemas/domain.rng: Add memoryBacking/hugepages elements to schema
> * src/util.c, src/util.h, src/libvirt_private.syms: Add virFileFindMountPoint
> helper API
> * tests/qemuhelptest.c: Add -mem-path constants
> * tests/qemuxml2argvtest.c, tests/qemuxml2xmltest.c: Add tests for hugepage
> handling
> * tests/qemuxml2argvdata/qemuxml2argv-hugepages.xml,
> tests/qemuxml2argvdata/qemuxml2argv-hugepages.args: Data files for
> hugepage tests
> ---
> configure.in | 2 +-
> docs/formatdomain.html | 8 +++-
> docs/formatdomain.html.in | 8 +++
> docs/schemas/domain.rng | 9 ++++
> qemud/libvirtd_qemu.aug | 1 +
> qemud/test_libvirtd_qemu.aug | 4 ++
> src/domain_conf.c | 10 ++++-
> src/domain_conf.h | 1 +
> src/libvirt_private.syms | 1 +
> src/qemu.conf | 13 +++++
> src/qemu_conf.c | 49 ++++++++++++++++++++
> src/qemu_conf.h | 3 +
> src/qemu_driver.c | 34 ++++++++++++++
> src/util.c | 37 ++++++++++++++-
> src/util.h | 4 ++
> tests/qemuhelptest.c | 6 ++-
> tests/qemuxml2argvdata/qemuxml2argv-hugepages.args | 1 +
> tests/qemuxml2argvdata/qemuxml2argv-hugepages.xml | 25 ++++++++++
> tests/qemuxml2argvtest.c | 7 ++-
> tests/qemuxml2xmltest.c | 1 +
> 20 files changed, 217 insertions(+), 7 deletions(-)
> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hugepages.args
> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hugepages.xml
>
...
> diff --git a/src/qemu_conf.c b/src/qemu_conf.c
> index 22f5edd..4935e2f 100644
> --- a/src/qemu_conf.c
> +++ b/src/qemu_conf.c
> @@ -35,6 +35,7 @@
> #include <sys/wait.h>
> #include <arpa/inet.h>
> #include <sys/utsname.h>
> +#include <mntent.h>
>
> #include "c-ctype.h"
> #include "virterror_internal.h"
> @@ -87,6 +88,7 @@ VIR_ENUM_IMPL(qemuVideo, VIR_DOMAIN_VIDEO_TYPE_LAST,
> NULL, /* no arg needed for xen */
> NULL /* don't support vbox */);
>
> +#define PROC_MOUNT_BUF_LEN 255
>
> int qemudLoadDriverConfig(struct qemud_driver *driver,
> const char *filename) {
> @@ -106,6 +108,19 @@ int qemudLoadDriverConfig(struct qemud_driver *driver,
> return -1;
> }
>
> + /* For privilege driver, try and find hugepage mount automatically.
privileged
> + * Non-privileged driver requires admin to create a dir for the
> + * user, chown it, and then let user configure it manually */
> + if (driver->privileged &&
> + !(driver->hugetlbfs_mount = virFileFindMountPoint("hugetlbfs"))) {
> + if (errno != ENOENT) {
> + virReportSystemError(NULL, errno, "%s",
> + _("unable to find hugetlbfs mountpoint"));
> + return -1;
> + }
> + }
> +
> +
> /* Just check the file is readable before opening it, otherwise
> * libvirt emits an error.
> */
> @@ -290,10 +305,22 @@ int qemudLoadDriverConfig(struct qemud_driver *driver,
> }
> }
>
> + p = virConfGetValue (conf, "hugetlbfs_mount");
> + CHECK_TYPE ("hugetlbfs_mount", VIR_CONF_STRING);
> + if (p && p->str) {
> + VIR_FREE(driver->hugetlbfs_mount);
> + if (!(driver->hugetlbfs_mount = strdup(p->str))) {
> + virReportOOMError(NULL);
> + virConfFree(conf);
> + return -1;
> + }
> + }
> +
How come you probe for a hugetlbfs mount even when the config file
contains it?
> virConfFree (conf);
> return 0;
> }
>
> +
Spurious
...
> diff --git a/src/qemu_driver.c b/src/qemu_driver.c
> index eb22940..4140a63 100644
> --- a/src/qemu_driver.c
> +++ b/src/qemu_driver.c
> @@ -542,6 +542,38 @@ qemudStartup(int privileged) {
> goto error;
> }
>
> + /* If hugetlbfs is present, then we need to create a sub-directory within
> + * it, since we can't assume the root mount point has permissions that
> + * will let our spawned QEMU instances use it.
> + *
> + * NB the check for '/', since user may config "" to disable hugepages
> + * even when mounted
> + */
> + if (qemu_driver->hugetlbfs_mount &&
> + qemu_driver->hugetlbfs_mount[0] == '/') {
> + char *mempath = NULL;
> + int ret;
Could just use the rc variable already declared
> + if (virAsprintf(&mempath, "%s/libvirt/qemu", qemu_driver->hugetlbfs_mount) < 0)
> + goto out_of_memory;
> +
> + if ((ret = virFileMakePath(mempath)) != 0) {
> + virReportSystemError(NULL, ret,
> + _("unable to create hugepage path %s"), mempath);
> + VIR_FREE(mempath);
> + goto error;
> + }
> + if (qemu_driver->privileged &&
> + chown(mempath, qemu_driver->user, qemu_driver->group) < 0) {
> + virReportSystemError(NULL, errno,
> + _("unable to set ownership on %s to %d:%d"),
> + mempath, qemu_driver->user, qemu_driver->group);
> + VIR_FREE(mempath);
> + goto error;
> + }
> +
> + qemu_driver->hugepage_path = mempath;
> + }
> +
> /* Get all the running persistent or transient configs first */
> if (virDomainLoadAllConfigs(NULL,
> qemu_driver->caps,
> @@ -673,6 +705,8 @@ qemudShutdown(void) {
> VIR_FREE(qemu_driver->vncPassword);
> VIR_FREE(qemu_driver->vncSASLdir);
> VIR_FREE(qemu_driver->saveImageFormat);
> + VIR_FREE(qemu_driver->hugetlbfs_mount);
> + VIR_FREE(qemu_driver->hugepage_path);
>
> /* Free domain callback list */
> virDomainEventCallbackListFree(qemu_driver->domainEventCallbacks);
> diff --git a/src/util.c b/src/util.c
> index 0d4f3fa..35efee2 100644
> --- a/src/util.c
> +++ b/src/util.c
> @@ -60,7 +60,9 @@
> #if HAVE_CAPNG
> #include <cap-ng.h>
> #endif
> -
> +#ifdef HAVE_MNTENT_H
> +#include <mntent.h>
> +#endif
>
> #include "virterror_internal.h"
> #include "logging.h"
> @@ -1983,3 +1985,36 @@ int virGetGroupID(virConnectPtr conn,
> return 0;
> }
> #endif
> +
> +
> +#ifdef HAVE_MNTENT_H
Hmm, if mntent.h isn't found, the qemu driver will fail to link
Is the idea here that anywhere mntent.h isn't available, the qemu driver
won't be built so we don't need to check HAVE_MNTENT_H in qemu_conf.c?
> +/* search /proc/mounts for mount point of *type; return pointer to
> + * malloc'ed string of the path if found, otherwise return NULL
> + */
> +char *virFileFindMountPoint(const char *type)
> +{
> + FILE *f;
> + struct mntent mb;
> + char mntbuf[1024];
> + char *ret = NULL;
> +
> + f = setmntent("/proc/mounts", "r");
> + if (!f)
> + return NULL;
> +
> + while (getmntent_r(f, &mb, mntbuf, sizeof(mntbuf))) {
> + if (STREQ(mb.mnt_type, type)) {
> + ret = strdup(mb.mnt_dir);
There's no explicit OOM handling, but who cares ... it'll just appear as
an ENOENT
> + goto cleanup;
> + }
> + }
> +
> +cleanup:
> + endmntent(f);
> +
> + if (!ret)
> + errno = ENOENT;
The goto seems a bit gratuitous, I'd just break out of the loop
Looks fine, ACK
Cheers,
Mark.
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list