6:41 a.m.
v2 of:
https://www.redhat.com/archives/libvir-list/2017-October/msg01063.html
Patches are to be found here too:
https://github.com/zippy2/libvirt/tree/qemu_mem_path_v3
diff to v1:
-Dropped qemu.conf config knob
-s/rmdir/virFileDeleteTree/ in qemuProcessBuildDestroyMemoryPathsImpl() because
qemu leaves files behind and thus we need to unlink them too.
Michal Privoznik (4):
conf: s/virDomainObjGetShortName/virDomainDefGetShortName/
qemu: Move memPath generation from memoryBackingDir to a separate
function
qemu: Use predictable file names for memory-backend-file
news: Document predictable file names for memory-backend-file
docs/news.xml | 11 ++
src/conf/domain_conf.c | 4 +-
src/conf/domain_conf.h | 2 +-
src/libvirt_private.syms | 2 +-
src/qemu/qemu_command.c | 9 +-
src/qemu/qemu_conf.c | 71 ++++++++++-
src/qemu/qemu_conf.h | 10 ++
src/qemu/qemu_domain.c | 4 +-
src/qemu/qemu_driver.c | 19 ++-
src/qemu/qemu_hotplug.c | 2 +-
src/qemu/qemu_process.c | 137 +++++++++++++++------
src/qemu/qemu_process.h | 8 +-
.../qemuxml2argv-cpu-numa-memshared.args | 6 +-
.../qemuxml2argv-fd-memory-numa-topology.args | 3 +-
.../qemuxml2argv-fd-memory-numa-topology2.args | 6 +-
.../qemuxml2argv-fd-memory-numa-topology3.args | 9 +-
.../qemuxml2argv-hugepages-memaccess2.args | 9 +-
17 files changed, 244 insertions(+), 68 deletions(-)
--
2.13.6
6:41 a.m.
New subject: [libvirt] [PATCH v2 1/4] conf: s/virDomainObjGetShortName/virDomainDefGetShortName/
This function works over domain definition and not domain object.
Its name is thus misleading.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/domain_conf.c | 4 ++--
src/conf/domain_conf.h | 2 +-
src/libvirt_private.syms | 2 +-
src/qemu/qemu_conf.c | 2 +-
src/qemu/qemu_domain.c | 4 ++--
src/qemu/qemu_driver.c | 2 +-
6 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 77c20c697..58b75c672 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -27871,14 +27871,14 @@ virDomainDefHasMemballoon(const virDomainDef *def)
#define VIR_DOMAIN_SHORT_NAME_MAX 20
/**
- * virDomainObjGetShortName:
+ * virDomainDefGetShortName:
* @vm: Machine for which to get a name
* @unique: Make sure the name is unique (use id as well)
*
* Shorten domain name to avoid possible path length limitations.
*/
char *
-virDomainObjGetShortName(const virDomainDef *def)
+virDomainDefGetShortName(const virDomainDef *def)
{
wchar_t wshortname[VIR_DOMAIN_SHORT_NAME_MAX + 1] = {0};
size_t len = 0;
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 38de70b15..bd541a9c5 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -3366,7 +3366,7 @@ int virDomainDefGetVcpuPinInfoHelper(virDomainDefPtr def,
bool virDomainDefHasMemballoon(const virDomainDef *def) ATTRIBUTE_NONNULL(1);
-char *virDomainObjGetShortName(const virDomainDef *def) ATTRIBUTE_NONNULL(1);
+char *virDomainDefGetShortName(const virDomainDef *def) ATTRIBUTE_NONNULL(1);
int
virDomainGetBlkioParametersAssignFromDef(virDomainDefPtr def,
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 448d962b2..3ebff18aa 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -262,6 +262,7 @@ virDomainDefGetMemoryInitial;
virDomainDefGetMemoryTotal;
virDomainDefGetOnlineVcpumap;
virDomainDefGetSecurityLabelDef;
+virDomainDefGetShortName;
virDomainDefGetVcpu;
virDomainDefGetVcpuPinInfoHelper;
virDomainDefGetVcpus;
@@ -457,7 +458,6 @@ virDomainObjGetMetadata;
virDomainObjGetOneDef;
virDomainObjGetOneDefState;
virDomainObjGetPersistentDef;
-virDomainObjGetShortName;
virDomainObjGetState;
virDomainObjNew;
virDomainObjParseNode;
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index ec61c9c52..bf6b334f4 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -1659,7 +1659,7 @@ qemuGetDomainHugepagePath(const virDomainDef *def,
virHugeTLBFSPtr hugepage)
{
char *base = qemuGetBaseHugepagePath(hugepage);
- char *domPath = virDomainObjGetShortName(def);
+ char *domPath = virDomainDefGetShortName(def);
char *ret = NULL;
if (base && domPath)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index c7c9e94da..e828fa2f6 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -1705,7 +1705,7 @@ qemuDomainSetPrivatePaths(virQEMUDriverPtr driver,
{
virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
qemuDomainObjPrivatePtr priv = vm->privateData;
- char *domname = virDomainObjGetShortName(vm->def);
+ char *domname = virDomainDefGetShortName(vm->def);
int ret = -1;
if (!domname)
@@ -8203,7 +8203,7 @@ qemuDomainGetPreservedMountPath(virQEMUDriverConfigPtr cfg,
char *path = NULL;
char *tmp;
const char *suffix = mountpoint + strlen(DEVPREFIX);
- char *domname = virDomainObjGetShortName(vm->def);
+ char *domname = virDomainDefGetShortName(vm->def);
size_t off;
if (!domname)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 74fdfdb0f..135c20749 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4079,7 +4079,7 @@ getAutoDumpPath(virQEMUDriverPtr driver,
virDomainObjPtr vm)
{
char *dumpfile = NULL;
- char *domname = virDomainObjGetShortName(vm->def);
+ char *domname = virDomainDefGetShortName(vm->def);
char timestr[100];
struct tm time_info;
time_t curtime = time(NULL);
--
2.13.6
6:11 p.m.
New subject: [libvirt] [PATCH v2 1/4] conf: s/virDomainObjGetShortName/virDomainDefGetShortName/
On 10/24/2017 07:41 AM, Michal Privoznik wrote:
This function works over domain definition and not domain object.
Its name is thus misleading.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/domain_conf.c | 4 ++--
src/conf/domain_conf.h | 2 +-
src/libvirt_private.syms | 2 +-
src/qemu/qemu_conf.c | 2 +-
src/qemu/qemu_domain.c | 4 ++--
src/qemu/qemu_driver.c | 2 +-
6 files changed, 8 insertions(+), 8 deletions(-)
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
BTW: I wonder if this function is somewhat broken - it's using def->id
in order to generate the output, right? Which means for paths generated
before the guest starts, they'll start with "-1"... IOW: Directory and
file names that start with "-".
This wasn't a problem originally (commit id a042275a) since the
resulting path had "domain-" prepended to it - resulting in
"domain--1-QEMUGuest1, for example).
Personally, this looks REALLY odd when (re)using for example with a
hugepage test where the path is "-mem-path
/dev/hugepages2M/libvirt/qemu/-1-QEMUGuest1".
OK - maybe it's just me, but seeing a filename start with "-" is really
painful because it's not just a simple task of "rm $file", since
"-"
needs to escaped so as to not be considered an argument to rm <sigh>.
I know, send a patch or just grumble and let someone else do it ;-)
3:04 a.m.
New subject: [libvirt] [PATCH v2 1/4] conf: s/virDomainObjGetShortName/virDomainDefGetShortName/
On 11/02/2017 12:11 AM, John Ferlan wrote:
On 10/24/2017 07:41 AM, Michal Privoznik wrote:
> This function works over domain definition and not domain object.
> Its name is thus misleading.
>
> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> ---
> src/conf/domain_conf.c | 4 ++--
> src/conf/domain_conf.h | 2 +-
> src/libvirt_private.syms | 2 +-
> src/qemu/qemu_conf.c | 2 +-
> src/qemu/qemu_domain.c | 4 ++--
> src/qemu/qemu_driver.c | 2 +-
> 6 files changed, 8 insertions(+), 8 deletions(-)
>
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
BTW: I wonder if this function is somewhat broken - it's using def->id
in order to generate the output, right? Which means for paths generated
before the guest starts, they'll start with "-1"... IOW: Directory and
file names that start with "-".
This wasn't a problem originally (commit id a042275a) since the
resulting path had "domain-" prepended to it - resulting in
"domain--1-QEMUGuest1, for example).
Personally, this looks REALLY odd when (re)using for example with a
hugepage test where the path is "-mem-path
/dev/hugepages2M/libvirt/qemu/-1-QEMUGuest1".
This is because we don't set an ID for domains when testing. However, in
real scenarios this function is called over running domains. Therefore,
the path will always start with a natural number. I don't think there's
anything to fix really. We could go with "domain-" prefix but that'd be
no better in fact. Yes, it would help with 'rm' case but other than that
it has no added value.
Michal
6:41 a.m.
New subject: [libvirt] [PATCH v2 2/4] qemu: Move memPath generation from memoryBackingDir to a separate function
In near future we will need more than just a plain VIR_STRDUP().
Better implement that in a separate function and in
qemuBuildMemoryBackendStr() which is complicated enough already.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_command.c | 2 +-
src/qemu/qemu_conf.c | 18 ++++++++++++++++++
src/qemu/qemu_conf.h | 3 +++
3 files changed, 22 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index b1cfafa79..82d2fb2a3 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3439,7 +3439,7 @@ qemuBuildMemoryBackendStr(virJSONValuePtr *backendProps,
} else {
/* We can have both pagesize and mem source. If that's the case,
* prefer hugepages as those are more specific. */
- if (VIR_STRDUP(memPath, cfg->memoryBackingDir) < 0)
+ if (qemuGetMemoryBackingPath(cfg, &memPath) < 0)
goto cleanup;
}
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index bf6b334f4..23dcc25e1 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -1748,3 +1748,21 @@ qemuGetDomainHupageMemPath(const virDomainDef *def,
return 0;
}
+
+
+/**
+ * qemuGetMemoryBackingPath:
+ * @cfg: the driver config
+ * @memPath: constructed path
+ *
+ * Constructs path to memory backing dir and stores it at @memPath.
+ *
+ * Returns: 0 on success,
+ * -1 otherwise (with error reported).
+ */
+int
+qemuGetMemoryBackingPath(virQEMUDriverConfigPtr cfg,
+ char **memPath)
+{
+ return VIR_STRDUP(*memPath, cfg->memoryBackingDir);
+}
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index 13b6f818a..9d6866816 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -363,4 +363,7 @@ int qemuGetDomainHupageMemPath(const virDomainDef *def,
virQEMUDriverConfigPtr cfg,
unsigned long long pagesize,
char **memPath);
+
+int qemuGetMemoryBackingPath(virQEMUDriverConfigPtr cfg,
+ char **memPath);
#endif /* __QEMUD_CONF_H */
--
2.13.6
6:11 p.m.
New subject: [libvirt] [PATCH v2 2/4] qemu: Move memPath generation from memoryBackingDir to a separate function
On 10/24/2017 07:41 AM, Michal Privoznik wrote:
In near future we will need more than just a plain VIR_STRDUP().
Better implement that in a separate function and in
qemuBuildMemoryBackendStr() which is complicated enough already.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_command.c | 2 +-
src/qemu/qemu_conf.c | 18 ++++++++++++++++++
src/qemu/qemu_conf.h | 3 +++
3 files changed, 22 insertions(+), 1 deletion(-)
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
6:41 a.m.
New subject: [libvirt] [PATCH v2 3/4] qemu: Use predictable file names for memory-backend-file
In some cases management application needs to allocate memory for
qemu upfront and then just let qemu use that. Since we don't want
to expose path for memory-backend-file anywhere in the domain
XML, we can generate predictable paths. In this case:
$memoryBackingDir/libvirt/qemu/$shortName/$alias
where $shortName is result of virDomainObjGetShortName().
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_command.c | 9 +-
src/qemu/qemu_conf.c | 55 ++++++++-
src/qemu/qemu_conf.h | 9 +-
src/qemu/qemu_driver.c | 17 +++
src/qemu/qemu_hotplug.c | 2 +-
src/qemu/qemu_process.c | 137 +++++++++++++++------
src/qemu/qemu_process.h | 8 +-
.../qemuxml2argv-cpu-numa-memshared.args | 6 +-
.../qemuxml2argv-fd-memory-numa-topology.args | 3 +-
.../qemuxml2argv-fd-memory-numa-topology2.args | 6 +-
.../qemuxml2argv-fd-memory-numa-topology3.args | 9 +-
.../qemuxml2argv-hugepages-memaccess2.args | 9 +-
12 files changed, 207 insertions(+), 63 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 82d2fb2a3..d6d2654cd 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3439,7 +3439,7 @@ qemuBuildMemoryBackendStr(virJSONValuePtr *backendProps,
} else {
/* We can have both pagesize and mem source. If that's the case,
* prefer hugepages as those are more specific. */
- if (qemuGetMemoryBackingPath(cfg, &memPath) < 0)
+ if (qemuGetMemoryBackingPath(def, cfg, mem->info.alias, &memPath) <
0)
goto cleanup;
}
@@ -3542,12 +3542,13 @@ qemuBuildMemoryCellBackendStr(virDomainDefPtr def,
unsigned long long memsize = virDomainNumaGetNodeMemorySize(def->numa,
cell);
+ if (virAsprintf(&alias, "ram-node%zu", cell) < 0)
+ goto cleanup;
+
*backendStr = NULL;
mem.size = memsize;
mem.targetNode = cell;
-
- if (virAsprintf(&alias, "ram-node%zu", cell) < 0)
- goto cleanup;
+ mem.info.alias = alias;
if ((rc = qemuBuildMemoryBackendStr(&props, &backendType, cfg,
priv->qemuCaps,
def, &mem, priv->autoNodeset, false)) <
0)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 23dcc25e1..22c023255 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -1750,9 +1750,41 @@ qemuGetDomainHupageMemPath(const virDomainDef *def,
}
+int
+qemuGetMemoryBackingBasePath(virQEMUDriverConfigPtr cfg,
+ char **path)
+{
+ return virAsprintf(path, "%s/libvirt/qemu", cfg->memoryBackingDir);
+}
+
+
+int
+qemuGetMemoryBackingDomainPath(const virDomainDef *def,
+ virQEMUDriverConfigPtr cfg,
+ char **path)
+{
+ char *shortName = NULL;
+ char *base = NULL;
+ int ret = -1;
+
+ if (!(shortName = virDomainDefGetShortName(def)) ||
+ qemuGetMemoryBackingBasePath(cfg, &base) < 0 ||
+ virAsprintf(path, "%s/%s", base, shortName) < 0)
+ goto cleanup;
+
+ ret = 0;
+ cleanup:
+ VIR_FREE(base);
+ VIR_FREE(shortName);
+ return ret;
+}
+
+
/**
* qemuGetMemoryBackingPath:
+ * @def: domain definition
* @cfg: the driver config
+ * @alias: memory object alias
* @memPath: constructed path
*
* Constructs path to memory backing dir and stores it at @memPath.
@@ -1761,8 +1793,27 @@ qemuGetDomainHupageMemPath(const virDomainDef *def,
* -1 otherwise (with error reported).
*/
int
-qemuGetMemoryBackingPath(virQEMUDriverConfigPtr cfg,
+qemuGetMemoryBackingPath(const virDomainDef *def,
+ virQEMUDriverConfigPtr cfg,
+ const char *alias,
char **memPath)
{
- return VIR_STRDUP(*memPath, cfg->memoryBackingDir);
+ char *domainPath = NULL;
+ int ret = -1;
+
+ if (!alias) {
+ /* This should never happen (TM) */
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("memory device alias is not assigned"));
+ goto cleanup;
+ }
+
+ if (qemuGetMemoryBackingDomainPath(def, cfg, &domainPath) < 0 ||
+ virAsprintf(memPath, "%s/%s", domainPath, alias) < 0)
+ goto cleanup;
+
+ ret = 0;
+ cleanup:
+ VIR_FREE(domainPath);
+ return ret;
}
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index 9d6866816..a553e30e2 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -364,6 +364,13 @@ int qemuGetDomainHupageMemPath(const virDomainDef *def,
unsigned long long pagesize,
char **memPath);
-int qemuGetMemoryBackingPath(virQEMUDriverConfigPtr cfg,
+int qemuGetMemoryBackingBasePath(virQEMUDriverConfigPtr cfg,
+ char **path);
+int qemuGetMemoryBackingDomainPath(const virDomainDef *def,
+ virQEMUDriverConfigPtr cfg,
+ char **path);
+int qemuGetMemoryBackingPath(const virDomainDef *def,
+ virQEMUDriverConfigPtr cfg,
+ const char *alias,
char **memPath);
#endif /* __QEMUD_CONF_H */
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 135c20749..b22fb7518 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -630,6 +630,7 @@ qemuStateInitialize(bool privileged,
uid_t run_uid = -1;
gid_t run_gid = -1;
char *hugepagePath = NULL;
+ char *memoryBackingPath = NULL;
size_t i;
if (VIR_ALLOC(qemu_driver) < 0)
@@ -888,6 +889,21 @@ qemuStateInitialize(bool privileged,
VIR_FREE(hugepagePath);
}
+ if (qemuGetMemoryBackingBasePath(cfg, &memoryBackingPath) < 0)
+ goto error;
+
+ if (virFileMakePath(memoryBackingPath) < 0) {
+ virReportSystemError(errno,
+ _("unable to create memory backing path %s"),
+ memoryBackingPath);
+ goto error;
+ }
+
+ if (privileged &&
+ virFileUpdatePerm(memoryBackingPath,
+ 0, S_IXGRP | S_IXOTH) < 0)
+ goto error;
+
if (!(qemu_driver->closeCallbacks = virCloseCallbacksNew()))
goto error;
@@ -945,6 +961,7 @@ qemuStateInitialize(bool privileged,
virObjectUnref(conn);
VIR_FREE(driverConf);
VIR_FREE(hugepagePath);
+ VIR_FREE(memoryBackingPath);
qemuStateCleanup();
return -1;
}
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 91f7f9ed6..5701c033b 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -2077,7 +2077,7 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver,
priv->qemuCaps, vm->def, mem, NULL, true) <
0)
goto cleanup;
- if (qemuProcessBuildDestroyHugepagesPath(driver, vm, mem, true) < 0)
+ if (qemuProcessBuildDestroyMemoryPaths(driver, vm, mem, true) < 0)
goto cleanup;
if (qemuDomainNamespaceSetupMemory(driver, vm, mem) < 0)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index fdc868912..4fe973c59 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3324,60 +3324,117 @@ qemuProcessNeedHugepagesPath(virDomainDefPtr def,
}
+static bool
+qemuProcessNeedMemoryBackingPath(virDomainDefPtr def,
+ virDomainMemoryDefPtr mem)
+{
+ size_t i;
+ size_t numaNodes;
+
+ if (def->mem.source == VIR_DOMAIN_MEMORY_SOURCE_FILE ||
+ def->mem.access != VIR_DOMAIN_MEMORY_ACCESS_DEFAULT)
+ return true;
+
+ numaNodes = virDomainNumaGetNodeCount(def->numa);
+ for (i = 0; i < numaNodes; i++) {
+ if (virDomainNumaGetNodeMemoryAccessMode(def->numa, i)
+ != VIR_DOMAIN_MEMORY_ACCESS_DEFAULT)
+ return true;
+ }
+
+ if (mem &&
+ mem->model == VIR_DOMAIN_MEMORY_MODEL_DIMM &&
+ (mem->access != VIR_DOMAIN_MEMORY_ACCESS_DEFAULT ||
+ (mem->targetNode >= 0 &&
+ virDomainNumaGetNodeMemoryAccessMode(def->numa, mem->targetNode)
+ != VIR_DOMAIN_MEMORY_ACCESS_DEFAULT)))
+ return true;
+
+ return false;
+}
+
+
+static int
+qemuProcessBuildDestroyMemoryPathsImpl(virQEMUDriverPtr driver,
+ virDomainDefPtr def,
+ const char *path,
+ bool build)
+{
+ if (build) {
+ if (virFileExists(path))
+ return 0;
+
+ if (virFileMakePathWithMode(path, 0700) < 0) {
+ virReportSystemError(errno,
+ _("Unable to create %s"),
+ path);
+ return -1;
+ }
+
+ if (qemuSecurityDomainSetPathLabel(driver->securityManager,
+ def, path) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Unable to label %s"), path);
+ return -1;
+ }
+ } else {
+ if (virFileDeleteTree(path) < 0 &&
+ errno != ENOENT)
+ VIR_WARN("Unable to remove hugepage path: %s (errno=%d)",
+ path, errno);
+ }
+
+ return 0;
+}
+
+
int
-qemuProcessBuildDestroyHugepagesPath(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- virDomainMemoryDefPtr mem,
- bool build)
+qemuProcessBuildDestroyMemoryPaths(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ virDomainMemoryDefPtr mem,
+ bool build)
{
virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
- char *hugepagePath = NULL;
+ char *path = NULL;
size_t i;
- bool shouldBuild = false;
+ bool shouldBuildHP = false;
+ bool shouldBuildMB = false;
int ret = -1;
- if (build)
- shouldBuild = qemuProcessNeedHugepagesPath(vm->def, mem);
+ if (build) {
+ shouldBuildHP = qemuProcessNeedHugepagesPath(vm->def, mem);
+ shouldBuildMB = qemuProcessNeedMemoryBackingPath(vm->def, mem);
+ }
- if (!build || shouldBuild) {
+ if (!build || shouldBuildHP) {
for (i = 0; i < cfg->nhugetlbfs; i++) {
- VIR_FREE(hugepagePath);
- hugepagePath = qemuGetDomainHugepagePath(vm->def,
&cfg->hugetlbfs[i]);
+ path = qemuGetDomainHugepagePath(vm->def, &cfg->hugetlbfs[i]);
- if (!hugepagePath)
+ if (!path)
goto cleanup;
- if (build) {
- if (virFileExists(hugepagePath)) {
- ret = 0;
- goto cleanup;
- }
-
- if (virFileMakePathWithMode(hugepagePath, 0700) < 0) {
- virReportSystemError(errno,
- _("Unable to create %s"),
- hugepagePath);
- goto cleanup;
- }
+ if (qemuProcessBuildDestroyMemoryPathsImpl(driver, vm->def,
+ path, build) < 0)
+ goto cleanup;
- if (qemuSecurityDomainSetPathLabel(driver->securityManager,
- vm->def, hugepagePath) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- "%s", _("Unable to set huge path in
security driver"));
- goto cleanup;
- }
- } else {
- if (rmdir(hugepagePath) < 0 &&
- errno != ENOENT)
- VIR_WARN("Unable to remove hugepage path: %s (errno=%d)",
- hugepagePath, errno);
- }
+ VIR_FREE(path);
}
}
+ if (!build || shouldBuildMB) {
+ if (qemuGetMemoryBackingDomainPath(vm->def, cfg, &path) < 0)
+ goto cleanup;
+
+ if (qemuProcessBuildDestroyMemoryPathsImpl(driver, vm->def,
+ path, build) < 0)
+ goto cleanup;
+
+ VIR_FREE(path);
+ }
+
ret = 0;
cleanup:
- VIR_FREE(hugepagePath);
+ VIR_FREE(path);
virObjectUnref(cfg);
return ret;
}
@@ -5550,7 +5607,7 @@ qemuProcessPrepareHost(virQEMUDriverPtr driver,
NULL) < 0)
goto cleanup;
- if (qemuProcessBuildDestroyHugepagesPath(driver, vm, NULL, true) < 0)
+ if (qemuProcessBuildDestroyMemoryPaths(driver, vm, NULL, true) < 0)
goto cleanup;
/* Ensure no historical cgroup for this VM is lying around bogus
@@ -6254,7 +6311,7 @@ void qemuProcessStop(virQEMUDriverPtr driver,
goto endjob;
}
- qemuProcessBuildDestroyHugepagesPath(driver, vm, NULL, false);
+ qemuProcessBuildDestroyMemoryPaths(driver, vm, NULL, false);
vm->def->id = -1;
@@ -7112,7 +7169,7 @@ qemuProcessReconnect(void *opaque)
goto cleanup;
}
- if (qemuProcessBuildDestroyHugepagesPath(driver, obj, NULL, true) < 0)
+ if (qemuProcessBuildDestroyMemoryPaths(driver, obj, NULL, true) < 0)
goto error;
if ((qemuDomainAssignAddresses(obj->def, priv->qemuCaps,
diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h
index 814b86d8a..cd9a72031 100644
--- a/src/qemu/qemu_process.h
+++ b/src/qemu/qemu_process.h
@@ -38,10 +38,10 @@ int qemuProcessStopCPUs(virQEMUDriverPtr driver,
virDomainPausedReason reason,
qemuDomainAsyncJob asyncJob);
-int qemuProcessBuildDestroyHugepagesPath(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- virDomainMemoryDefPtr mem,
- bool build);
+int qemuProcessBuildDestroyMemoryPaths(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ virDomainMemoryDefPtr mem,
+ bool build);
void qemuProcessAutostartAll(virQEMUDriverPtr driver);
void qemuProcessReconnectAll(virConnectPtr conn, virQEMUDriverPtr driver);
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-cpu-numa-memshared.args
b/tests/qemuxml2argvdata/qemuxml2argv-cpu-numa-memshared.args
index 5700c3413..352819429 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-cpu-numa-memshared.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-cpu-numa-memshared.args
@@ -10,10 +10,12 @@ QEMU_AUDIO_DRV=none \
-M pc \
-m 214 \
-smp 16,sockets=2,cores=4,threads=2 \
--object memory-backend-file,id=ram-node0,mem-path=/var/lib/libvirt/qemu/ram,\
+-object memory-backend-file,id=ram-node0,\
+mem-path=/var/lib/libvirt/qemu/ram/libvirt/qemu/-1-QEMUGuest1/ram-node0,\
share=yes,size=112197632 \
-numa node,nodeid=0,cpus=0-7,memdev=ram-node0 \
--object memory-backend-file,id=ram-node1,mem-path=/var/lib/libvirt/qemu/ram,\
+-object memory-backend-file,id=ram-node1,\
+mem-path=/var/lib/libvirt/qemu/ram/libvirt/qemu/-1-QEMUGuest1/ram-node1,\
share=no,size=112197632 \
-numa node,nodeid=1,cpus=8-15,memdev=ram-node1 \
-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-fd-memory-numa-topology.args
b/tests/qemuxml2argvdata/qemuxml2argv-fd-memory-numa-topology.args
index 12f3d8ab8..fa1353259 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-fd-memory-numa-topology.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-fd-memory-numa-topology.args
@@ -11,7 +11,8 @@ QEMU_AUDIO_DRV=none \
-m 14336 \
-mem-prealloc \
-smp 8,sockets=1,cores=8,threads=1 \
--object memory-backend-file,id=ram-node0,mem-path=/var/lib/libvirt/qemu/ram,\
+-object memory-backend-file,id=ram-node0,\
+mem-path=/var/lib/libvirt/qemu/ram/libvirt/qemu/-1-instance-00000092/ram-node0,\
share=yes,size=15032385536 \
-numa node,nodeid=0,cpus=0-7,memdev=ram-node0 \
-uuid 126f2720-6f8e-45ab-a886-ec9277079a67 \
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-fd-memory-numa-topology2.args
b/tests/qemuxml2argvdata/qemuxml2argv-fd-memory-numa-topology2.args
index 585e4d506..6f73a1b99 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-fd-memory-numa-topology2.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-fd-memory-numa-topology2.args
@@ -11,10 +11,12 @@ QEMU_AUDIO_DRV=none \
-m 28672 \
-mem-prealloc \
-smp 20,sockets=1,cores=8,threads=1 \
--object memory-backend-file,id=ram-node0,mem-path=/var/lib/libvirt/qemu/ram,\
+-object memory-backend-file,id=ram-node0,\
+mem-path=/var/lib/libvirt/qemu/ram/libvirt/qemu/-1-instance-00000092/ram-node0,\
share=no,size=15032385536 \
-numa node,nodeid=0,cpus=0-7,memdev=ram-node0 \
--object memory-backend-file,id=ram-node1,mem-path=/var/lib/libvirt/qemu/ram,\
+-object memory-backend-file,id=ram-node1,\
+mem-path=/var/lib/libvirt/qemu/ram/libvirt/qemu/-1-instance-00000092/ram-node1,\
share=yes,size=15032385536 \
-numa node,nodeid=1,cpus=8-15,memdev=ram-node1 \
-uuid 126f2720-6f8e-45ab-a886-ec9277079a67 \
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-fd-memory-numa-topology3.args
b/tests/qemuxml2argvdata/qemuxml2argv-fd-memory-numa-topology3.args
index e9a57a69e..3c352fe03 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-fd-memory-numa-topology3.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-fd-memory-numa-topology3.args
@@ -11,13 +11,16 @@ QEMU_AUDIO_DRV=none \
-m 43008 \
-mem-prealloc \
-smp 32,sockets=1,cores=24,threads=1 \
--object memory-backend-file,id=ram-node0,mem-path=/var/lib/libvirt/qemu/ram,\
+-object memory-backend-file,id=ram-node0,\
+mem-path=/var/lib/libvirt/qemu/ram/libvirt/qemu/-1-instance-00000092/ram-node0,\
share=yes,size=15032385536 \
-numa node,nodeid=0,cpus=0-1,memdev=ram-node0 \
--object memory-backend-file,id=ram-node1,mem-path=/var/lib/libvirt/qemu/ram,\
+-object memory-backend-file,id=ram-node1,\
+mem-path=/var/lib/libvirt/qemu/ram/libvirt/qemu/-1-instance-00000092/ram-node1,\
share=yes,size=15032385536 \
-numa node,nodeid=1,cpus=2-3,memdev=ram-node1 \
--object memory-backend-file,id=ram-node2,mem-path=/var/lib/libvirt/qemu/ram,\
+-object memory-backend-file,id=ram-node2,\
+mem-path=/var/lib/libvirt/qemu/ram/libvirt/qemu/-1-instance-00000092/ram-node2,\
share=no,size=15032385536 \
-numa node,nodeid=2,cpus=4-5,memdev=ram-node2 \
-uuid 126f2720-6f8e-45ab-a886-ec9277079a67 \
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-memaccess2.args
b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-memaccess2.args
index 55db49171..d8e506c19 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-memaccess2.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-memaccess2.args
@@ -10,17 +10,20 @@ QEMU_AUDIO_DRV=none \
-M pc \
-m size=4194304k,slots=16,maxmem=8388608k \
-smp 4,sockets=4,cores=1,threads=1 \
--object memory-backend-file,id=ram-node0,mem-path=/var/lib/libvirt/qemu/ram,\
+-object memory-backend-file,id=ram-node0,\
+mem-path=/var/lib/libvirt/qemu/ram/libvirt/qemu/-1-QEMUGuest1/ram-node0,\
share=no,size=1073741824,host-nodes=0-3,policy=bind \
-numa node,nodeid=0,cpus=0,memdev=ram-node0 \
-object memory-backend-file,id=ram-node1,prealloc=yes,\
mem-path=/dev/hugepages2M/libvirt/qemu/-1-QEMUGuest1,share=yes,size=1073741824,\
host-nodes=0-3,policy=bind \
-numa node,nodeid=1,cpus=1,memdev=ram-node1 \
--object memory-backend-file,id=ram-node2,mem-path=/var/lib/libvirt/qemu/ram,\
+-object memory-backend-file,id=ram-node2,\
+mem-path=/var/lib/libvirt/qemu/ram/libvirt/qemu/-1-QEMUGuest1/ram-node2,\
share=no,size=1073741824,host-nodes=0-3,policy=bind \
-numa node,nodeid=2,cpus=2,memdev=ram-node2 \
--object memory-backend-file,id=ram-node3,mem-path=/var/lib/libvirt/qemu/ram,\
+-object memory-backend-file,id=ram-node3,\
+mem-path=/var/lib/libvirt/qemu/ram/libvirt/qemu/-1-QEMUGuest1/ram-node3,\
share=no,size=1073741824,host-nodes=3,policy=bind \
-numa node,nodeid=3,cpus=3,memdev=ram-node3 \
-object memory-backend-file,id=memdimm0,prealloc=yes,\
--
2.13.6
6:15 p.m.
New subject: [libvirt] [PATCH v2 3/4] qemu: Use predictable file names for memory-backend-file
On 10/24/2017 07:41 AM, Michal Privoznik wrote:
In some cases management application needs to allocate memory for
qemu upfront and then just let qemu use that. Since we don't want
to expose path for memory-backend-file anywhere in the domain
XML, we can generate predictable paths. In this case:
$memoryBackingDir/libvirt/qemu/$shortName/$alias
where $shortName is result of virDomainObjGetShortName().
s/Obj/Def
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_command.c | 9 +-
src/qemu/qemu_conf.c | 55 ++++++++-
src/qemu/qemu_conf.h | 9 +-
src/qemu/qemu_driver.c | 17 +++
src/qemu/qemu_hotplug.c | 2 +-
src/qemu/qemu_process.c | 137 +++++++++++++++------
src/qemu/qemu_process.h | 8 +-
.../qemuxml2argv-cpu-numa-memshared.args | 6 +-
.../qemuxml2argv-fd-memory-numa-topology.args | 3 +-
.../qemuxml2argv-fd-memory-numa-topology2.args | 6 +-
.../qemuxml2argv-fd-memory-numa-topology3.args | 9 +-
.../qemuxml2argv-hugepages-memaccess2.args | 9 +-
12 files changed, 207 insertions(+), 63 deletions(-)
There's parts of this that would seemingly be able to be separated out
so that it's easier to review...
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 82d2fb2a3..d6d2654cd 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3439,7 +3439,7 @@ qemuBuildMemoryBackendStr(virJSONValuePtr *backendProps,
} else {
/* We can have both pagesize and mem source. If that's the case,
* prefer hugepages as those are more specific. */
- if (qemuGetMemoryBackingPath(cfg, &memPath) < 0)
+ if (qemuGetMemoryBackingPath(def, cfg, mem->info.alias, &memPath)
< 0)
goto cleanup;
}
@@ -3542,12 +3542,13 @@ qemuBuildMemoryCellBackendStr(virDomainDefPtr def,
unsigned long long memsize = virDomainNumaGetNodeMemorySize(def->numa,
cell);
+ if (virAsprintf(&alias, "ram-node%zu", cell) < 0)
+ goto cleanup;
+
This is just a simple trivial move and not related...
*backendStr = NULL;
mem.size = memsize;
mem.targetNode = cell;
-
- if (virAsprintf(&alias, "ram-node%zu", cell) < 0)
- goto cleanup;
+ mem.info.alias = alias;
if ((rc = qemuBuildMemoryBackendStr(&props, &backendType, cfg,
priv->qemuCaps,
def, &mem, priv->autoNodeset, false))
< 0)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 23dcc25e1..22c023255 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -1750,9 +1750,41 @@ qemuGetDomainHupageMemPath(const virDomainDef *def,
}
+int
+qemuGetMemoryBackingBasePath(virQEMUDriverConfigPtr cfg,
+ char **path)
+{
+ return virAsprintf(path, "%s/libvirt/qemu", cfg->memoryBackingDir);
Not of a fan of redundancy, but I'm also not sure what better way there
is...
For privileged domains it's "/var/lib/libvirt/qemu/ram/libvirt/qemu/"
and unpriv'd it's $XDG_CONFIG_HOME+"/qemu/ram/libvirt/qemu".
The former repeats "libvirt/qemu" while the latter only repeats
"qemu".
I would think we could drop the "qemu" at the very least, but it doesn't
matter that much to me.
Random thought - do memory file paths even work for non priv'd domains?
+}
+
+
+int
+qemuGetMemoryBackingDomainPath(const virDomainDef *def,
+ virQEMUDriverConfigPtr cfg,
+ char **path)
+{
+ char *shortName = NULL;
+ char *base = NULL;
+ int ret = -1;
+
+ if (!(shortName = virDomainDefGetShortName(def)) ||
+ qemuGetMemoryBackingBasePath(cfg, &base) < 0 ||
+ virAsprintf(path, "%s/%s", base, shortName) < 0)
+ goto cleanup;
+
As I noted in review of patch 1... This continues the logic of creating
paths that start with "-", which to me is, well, not right. Maybe others
aren't offended by it.
Is there anyway we can prepend something here to "shortName" - I'd say
"ram", but that's redundant with the path, but that to me is at least
better than a directory name starting with "-".
+ ret = 0;
+ cleanup:
+ VIR_FREE(base);
+ VIR_FREE(shortName);
+ return ret;
+}
+
+
/**
* qemuGetMemoryBackingPath:
+ * @def: domain definition
* @cfg: the driver config
+ * @alias: memory object alias
* @memPath: constructed path
*
* Constructs path to memory backing dir and stores it at @memPath.
@@ -1761,8 +1793,27 @@ qemuGetDomainHupageMemPath(const virDomainDef *def,
* -1 otherwise (with error reported).
*/
int
-qemuGetMemoryBackingPath(virQEMUDriverConfigPtr cfg,
+qemuGetMemoryBackingPath(const virDomainDef *def,
+ virQEMUDriverConfigPtr cfg,
+ const char *alias,
char **memPath)
{
- return VIR_STRDUP(*memPath, cfg->memoryBackingDir);
+ char *domainPath = NULL;
+ int ret = -1;
+
+ if (!alias) {
+ /* This should never happen (TM) */
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("memory device alias is not assigned"));
+ goto cleanup;
+ }
+
+ if (qemuGetMemoryBackingDomainPath(def, cfg, &domainPath) < 0 ||
+ virAsprintf(memPath, "%s/%s", domainPath, alias) < 0)
+ goto cleanup;
+
+ ret = 0;
+ cleanup:
+ VIR_FREE(domainPath);
+ return ret;
}
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index 9d6866816..a553e30e2 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -364,6 +364,13 @@ int qemuGetDomainHupageMemPath(const virDomainDef *def,
unsigned long long pagesize,
char **memPath);
-int qemuGetMemoryBackingPath(virQEMUDriverConfigPtr cfg,
+int qemuGetMemoryBackingBasePath(virQEMUDriverConfigPtr cfg,
+ char **path);
+int qemuGetMemoryBackingDomainPath(const virDomainDef *def,
+ virQEMUDriverConfigPtr cfg,
+ char **path);
+int qemuGetMemoryBackingPath(const virDomainDef *def,
+ virQEMUDriverConfigPtr cfg,
+ const char *alias,
char **memPath);
#endif /* __QEMUD_CONF_H */
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 135c20749..b22fb7518 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -630,6 +630,7 @@ qemuStateInitialize(bool privileged,
uid_t run_uid = -1;
gid_t run_gid = -1;
char *hugepagePath = NULL;
+ char *memoryBackingPath = NULL;
size_t i;
if (VIR_ALLOC(qemu_driver) < 0)
@@ -888,6 +889,21 @@ qemuStateInitialize(bool privileged,
VIR_FREE(hugepagePath);
}
+ if (qemuGetMemoryBackingBasePath(cfg, &memoryBackingPath) < 0)
+ goto error;
+
+ if (virFileMakePath(memoryBackingPath) < 0) {
+ virReportSystemError(errno,
+ _("unable to create memory backing path %s"),
+ memoryBackingPath);
+ goto error;
+ }
+
+ if (privileged &&
+ virFileUpdatePerm(memoryBackingPath,
+ 0, S_IXGRP | S_IXOTH) < 0)
+ goto error;
+
if (!(qemu_driver->closeCallbacks = virCloseCallbacksNew()))
goto error;
@@ -945,6 +961,7 @@ qemuStateInitialize(bool privileged,
virObjectUnref(conn);
VIR_FREE(driverConf);
VIR_FREE(hugepagePath);
+ VIR_FREE(memoryBackingPath);
qemuStateCleanup();
return -1;
}
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 91f7f9ed6..5701c033b 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -2077,7 +2077,7 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver,
priv->qemuCaps, vm->def, mem, NULL, true) <
0)
goto cleanup;
- if (qemuProcessBuildDestroyHugepagesPath(driver, vm, mem, true) < 0)
+ if (qemuProcessBuildDestroyMemoryPaths(driver, vm, mem, true) < 0)
goto cleanup;
if (qemuDomainNamespaceSetupMemory(driver, vm, mem) < 0)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index fdc868912..4fe973c59 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3324,60 +3324,117 @@ qemuProcessNeedHugepagesPath(virDomainDefPtr def,
}
+static bool
+qemuProcessNeedMemoryBackingPath(virDomainDefPtr def,
+ virDomainMemoryDefPtr mem)
+{
+ size_t i;
+ size_t numaNodes;
+
+ if (def->mem.source == VIR_DOMAIN_MEMORY_SOURCE_FILE ||
+ def->mem.access != VIR_DOMAIN_MEMORY_ACCESS_DEFAULT)
+ return true;
+
+ numaNodes = virDomainNumaGetNodeCount(def->numa);
+ for (i = 0; i < numaNodes; i++) {
+ if (virDomainNumaGetNodeMemoryAccessMode(def->numa, i)
+ != VIR_DOMAIN_MEMORY_ACCESS_DEFAULT)
+ return true;
+ }
+
+ if (mem &&
+ mem->model == VIR_DOMAIN_MEMORY_MODEL_DIMM &&
+ (mem->access != VIR_DOMAIN_MEMORY_ACCESS_DEFAULT ||
+ (mem->targetNode >= 0 &&
+ virDomainNumaGetNodeMemoryAccessMode(def->numa, mem->targetNode)
+ != VIR_DOMAIN_MEMORY_ACCESS_DEFAULT)))
+ return true;
+
+ return false;
+}
+
+
Starting here...
+static int
+qemuProcessBuildDestroyMemoryPathsImpl(virQEMUDriverPtr driver,
+ virDomainDefPtr def,
+ const char *path,
+ bool build)
Could just be qemuProcessBuildDestroyPath as it's one of many possible
calls within (at least) the hugepage loop.
+{
+ if (build) {
+ if (virFileExists(path))
+ return 0;
+
+ if (virFileMakePathWithMode(path, 0700) < 0) {
+ virReportSystemError(errno,
+ _("Unable to create %s"),
+ path);
+ return -1;
+ }
+
+ if (qemuSecurityDomainSetPathLabel(driver->securityManager,
+ def, path) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Unable to label %s"), path);
+ return -1;
+ }
+ } else {
+ if (virFileDeleteTree(path) < 0 &&
+ errno != ENOENT)
+ VIR_WARN("Unable to remove hugepage path: %s (errno=%d)",
This error message would need to change to remove "hugepage"
+ path, errno);
+ }
+
+ return 0;
+}
+
+
... this seems separable too, modulo the change to use virFileDeleteTree
which then would be a follow-up patch to be accepted on its own
merits... The other option being a check to add some sort of isDir type
logic or flag is input considering that hugepages wouldn't need to
delete the path, right? Or should they?
As Dan points out in v1 - we do need to keep the $dir/libvirt/qemu
though for other domains and we wouldn't want those subdirectories to
get deleted from/for Hugepages before we do so for memory backing,
right? I think the way you have this we'd delete those before getting
to them in the logic, but my eyes are really tired right now.
int
-qemuProcessBuildDestroyHugepagesPath(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- virDomainMemoryDefPtr mem,
- bool build)
+qemuProcessBuildDestroyMemoryPaths(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ virDomainMemoryDefPtr mem,
+ bool build)
You could seemingly make the API name change separately too, IDC, but
some do generally speaking of course...
{
virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
- char *hugepagePath = NULL;
+ char *path = NULL;
size_t i;
- bool shouldBuild = false;
+ bool shouldBuildHP = false;
+ bool shouldBuildMB = false;
int ret = -1;
- if (build)
- shouldBuild = qemuProcessNeedHugepagesPath(vm->def, mem);
+ if (build) {
+ shouldBuildHP = qemuProcessNeedHugepagesPath(vm->def, mem);
+ shouldBuildMB = qemuProcessNeedMemoryBackingPath(vm->def, mem);
+ }
- if (!build || shouldBuild) {
+ if (!build || shouldBuildHP) {
for (i = 0; i < cfg->nhugetlbfs; i++) {
- VIR_FREE(hugepagePath);
- hugepagePath = qemuGetDomainHugepagePath(vm->def,
&cfg->hugetlbfs[i]);
+ path = qemuGetDomainHugepagePath(vm->def, &cfg->hugetlbfs[i]);
- if (!hugepagePath)
+ if (!path)
goto cleanup;
- if (build) {
- if (virFileExists(hugepagePath)) {
- ret = 0;
- goto cleanup;
- }
-
- if (virFileMakePathWithMode(hugepagePath, 0700) < 0) {
- virReportSystemError(errno,
- _("Unable to create %s"),
- hugepagePath);
- goto cleanup;
- }
+ if (qemuProcessBuildDestroyMemoryPathsImpl(driver, vm->def,
+ path, build) < 0)
+ goto cleanup;
- if (qemuSecurityDomainSetPathLabel(driver->securityManager,
- vm->def, hugepagePath) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- "%s", _("Unable to set huge path in
security driver"));
- goto cleanup;
- }
- } else {
- if (rmdir(hugepagePath) < 0 &&
- errno != ENOENT)
- VIR_WARN("Unable to remove hugepage path: %s (errno=%d)",
- hugepagePath, errno);
- }
+ VIR_FREE(path);
}
}
+ if (!build || shouldBuildMB) {
+ if (qemuGetMemoryBackingDomainPath(vm->def, cfg, &path) < 0)
+ goto cleanup;
+
+ if (qemuProcessBuildDestroyMemoryPathsImpl(driver, vm->def,
+ path, build) < 0)
+ goto cleanup;
+
+ VIR_FREE(path);
+ }
+
ret = 0;
cleanup:
- VIR_FREE(hugepagePath);
+ VIR_FREE(path);
virObjectUnref(cfg);
return ret;
}
@@ -5550,7 +5607,7 @@ qemuProcessPrepareHost(virQEMUDriverPtr driver,
NULL) < 0)
goto cleanup;
- if (qemuProcessBuildDestroyHugepagesPath(driver, vm, NULL, true) < 0)
+ if (qemuProcessBuildDestroyMemoryPaths(driver, vm, NULL, true) < 0)
goto cleanup;
/* Ensure no historical cgroup for this VM is lying around bogus
@@ -6254,7 +6311,7 @@ void qemuProcessStop(virQEMUDriverPtr driver,
goto endjob;
}
- qemuProcessBuildDestroyHugepagesPath(driver, vm, NULL, false);
+ qemuProcessBuildDestroyMemoryPaths(driver, vm, NULL, false);
vm->def->id = -1;
@@ -7112,7 +7169,7 @@ qemuProcessReconnect(void *opaque)
goto cleanup;
}
- if (qemuProcessBuildDestroyHugepagesPath(driver, obj, NULL, true) < 0)
+ if (qemuProcessBuildDestroyMemoryPaths(driver, obj, NULL, true) < 0)
goto error;
if ((qemuDomainAssignAddresses(obj->def, priv->qemuCaps,
diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h
index 814b86d8a..cd9a72031 100644
--- a/src/qemu/qemu_process.h
+++ b/src/qemu/qemu_process.h
@@ -38,10 +38,10 @@ int qemuProcessStopCPUs(virQEMUDriverPtr driver,
virDomainPausedReason reason,
qemuDomainAsyncJob asyncJob);
-int qemuProcessBuildDestroyHugepagesPath(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- virDomainMemoryDefPtr mem,
- bool build);
+int qemuProcessBuildDestroyMemoryPaths(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ virDomainMemoryDefPtr mem,
+ bool build);
void qemuProcessAutostartAll(virQEMUDriverPtr driver);
void qemuProcessReconnectAll(virConnectPtr conn, virQEMUDriverPtr driver);
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-cpu-numa-memshared.args
b/tests/qemuxml2argvdata/qemuxml2argv-cpu-numa-memshared.args
index 5700c3413..352819429 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-cpu-numa-memshared.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-cpu-numa-memshared.args
@@ -10,10 +10,12 @@ QEMU_AUDIO_DRV=none \
-M pc \
-m 214 \
-smp 16,sockets=2,cores=4,threads=2 \
--object memory-backend-file,id=ram-node0,mem-path=/var/lib/libvirt/qemu/ram,\
+-object memory-backend-file,id=ram-node0,\
+mem-path=/var/lib/libvirt/qemu/ram/libvirt/qemu/-1-QEMUGuest1/ram-node0,\
And this is just where it really obvious what's happening - although it
took me a bit to figure it all out, especially in my post KVM Forum
travel haze.
John
share=yes,size=112197632 \
-numa node,nodeid=0,cpus=0-7,memdev=ram-node0 \
--object memory-backend-file,id=ram-node1,mem-path=/var/lib/libvirt/qemu/ram,\
+-object memory-backend-file,id=ram-node1,\
+mem-path=/var/lib/libvirt/qemu/ram/libvirt/qemu/-1-QEMUGuest1/ram-node1,\
share=no,size=112197632 \
-numa node,nodeid=1,cpus=8-15,memdev=ram-node1 \
-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-fd-memory-numa-topology.args
b/tests/qemuxml2argvdata/qemuxml2argv-fd-memory-numa-topology.args
index 12f3d8ab8..fa1353259 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-fd-memory-numa-topology.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-fd-memory-numa-topology.args
@@ -11,7 +11,8 @@ QEMU_AUDIO_DRV=none \
-m 14336 \
-mem-prealloc \
-smp 8,sockets=1,cores=8,threads=1 \
--object memory-backend-file,id=ram-node0,mem-path=/var/lib/libvirt/qemu/ram,\
+-object memory-backend-file,id=ram-node0,\
+mem-path=/var/lib/libvirt/qemu/ram/libvirt/qemu/-1-instance-00000092/ram-node0,\
share=yes,size=15032385536 \
-numa node,nodeid=0,cpus=0-7,memdev=ram-node0 \
-uuid 126f2720-6f8e-45ab-a886-ec9277079a67 \
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-fd-memory-numa-topology2.args
b/tests/qemuxml2argvdata/qemuxml2argv-fd-memory-numa-topology2.args
index 585e4d506..6f73a1b99 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-fd-memory-numa-topology2.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-fd-memory-numa-topology2.args
@@ -11,10 +11,12 @@ QEMU_AUDIO_DRV=none \
-m 28672 \
-mem-prealloc \
-smp 20,sockets=1,cores=8,threads=1 \
--object memory-backend-file,id=ram-node0,mem-path=/var/lib/libvirt/qemu/ram,\
+-object memory-backend-file,id=ram-node0,\
+mem-path=/var/lib/libvirt/qemu/ram/libvirt/qemu/-1-instance-00000092/ram-node0,\
share=no,size=15032385536 \
-numa node,nodeid=0,cpus=0-7,memdev=ram-node0 \
--object memory-backend-file,id=ram-node1,mem-path=/var/lib/libvirt/qemu/ram,\
+-object memory-backend-file,id=ram-node1,\
+mem-path=/var/lib/libvirt/qemu/ram/libvirt/qemu/-1-instance-00000092/ram-node1,\
share=yes,size=15032385536 \
-numa node,nodeid=1,cpus=8-15,memdev=ram-node1 \
-uuid 126f2720-6f8e-45ab-a886-ec9277079a67 \
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-fd-memory-numa-topology3.args
b/tests/qemuxml2argvdata/qemuxml2argv-fd-memory-numa-topology3.args
index e9a57a69e..3c352fe03 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-fd-memory-numa-topology3.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-fd-memory-numa-topology3.args
@@ -11,13 +11,16 @@ QEMU_AUDIO_DRV=none \
-m 43008 \
-mem-prealloc \
-smp 32,sockets=1,cores=24,threads=1 \
--object memory-backend-file,id=ram-node0,mem-path=/var/lib/libvirt/qemu/ram,\
+-object memory-backend-file,id=ram-node0,\
+mem-path=/var/lib/libvirt/qemu/ram/libvirt/qemu/-1-instance-00000092/ram-node0,\
share=yes,size=15032385536 \
-numa node,nodeid=0,cpus=0-1,memdev=ram-node0 \
--object memory-backend-file,id=ram-node1,mem-path=/var/lib/libvirt/qemu/ram,\
+-object memory-backend-file,id=ram-node1,\
+mem-path=/var/lib/libvirt/qemu/ram/libvirt/qemu/-1-instance-00000092/ram-node1,\
share=yes,size=15032385536 \
-numa node,nodeid=1,cpus=2-3,memdev=ram-node1 \
--object memory-backend-file,id=ram-node2,mem-path=/var/lib/libvirt/qemu/ram,\
+-object memory-backend-file,id=ram-node2,\
+mem-path=/var/lib/libvirt/qemu/ram/libvirt/qemu/-1-instance-00000092/ram-node2,\
share=no,size=15032385536 \
-numa node,nodeid=2,cpus=4-5,memdev=ram-node2 \
-uuid 126f2720-6f8e-45ab-a886-ec9277079a67 \
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-memaccess2.args
b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-memaccess2.args
index 55db49171..d8e506c19 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-memaccess2.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-memaccess2.args
@@ -10,17 +10,20 @@ QEMU_AUDIO_DRV=none \
-M pc \
-m size=4194304k,slots=16,maxmem=8388608k \
-smp 4,sockets=4,cores=1,threads=1 \
--object memory-backend-file,id=ram-node0,mem-path=/var/lib/libvirt/qemu/ram,\
+-object memory-backend-file,id=ram-node0,\
+mem-path=/var/lib/libvirt/qemu/ram/libvirt/qemu/-1-QEMUGuest1/ram-node0,\
share=no,size=1073741824,host-nodes=0-3,policy=bind \
-numa node,nodeid=0,cpus=0,memdev=ram-node0 \
-object memory-backend-file,id=ram-node1,prealloc=yes,\
mem-path=/dev/hugepages2M/libvirt/qemu/-1-QEMUGuest1,share=yes,size=1073741824,\
host-nodes=0-3,policy=bind \
-numa node,nodeid=1,cpus=1,memdev=ram-node1 \
--object memory-backend-file,id=ram-node2,mem-path=/var/lib/libvirt/qemu/ram,\
+-object memory-backend-file,id=ram-node2,\
+mem-path=/var/lib/libvirt/qemu/ram/libvirt/qemu/-1-QEMUGuest1/ram-node2,\
share=no,size=1073741824,host-nodes=0-3,policy=bind \
-numa node,nodeid=2,cpus=2,memdev=ram-node2 \
--object memory-backend-file,id=ram-node3,mem-path=/var/lib/libvirt/qemu/ram,\
+-object memory-backend-file,id=ram-node3,\
+mem-path=/var/lib/libvirt/qemu/ram/libvirt/qemu/-1-QEMUGuest1/ram-node3,\
share=no,size=1073741824,host-nodes=3,policy=bind \
-numa node,nodeid=3,cpus=3,memdev=ram-node3 \
-object memory-backend-file,id=memdimm0,prealloc=yes,\
3:04 a.m.
New subject: [libvirt] [PATCH v2 3/4] qemu: Use predictable file names for memory-backend-file
On 11/02/2017 12:15 AM, John Ferlan wrote:
On 10/24/2017 07:41 AM, Michal Privoznik wrote:
> In some cases management application needs to allocate memory for
> qemu upfront and then just let qemu use that. Since we don't want
> to expose path for memory-backend-file anywhere in the domain
> XML, we can generate predictable paths. In this case:
>
> $memoryBackingDir/libvirt/qemu/$shortName/$alias
>
> where $shortName is result of virDomainObjGetShortName().
s/Obj/Def
>
> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> ---
> src/qemu/qemu_command.c | 9 +-
> src/qemu/qemu_conf.c | 55 ++++++++-
> src/qemu/qemu_conf.h | 9 +-
> src/qemu/qemu_driver.c | 17 +++
> src/qemu/qemu_hotplug.c | 2 +-
> src/qemu/qemu_process.c | 137 +++++++++++++++------
> src/qemu/qemu_process.h | 8 +-
> .../qemuxml2argv-cpu-numa-memshared.args | 6 +-
> .../qemuxml2argv-fd-memory-numa-topology.args | 3 +-
> .../qemuxml2argv-fd-memory-numa-topology2.args | 6 +-
> .../qemuxml2argv-fd-memory-numa-topology3.args | 9 +-
> .../qemuxml2argv-hugepages-memaccess2.args | 9 +-
> 12 files changed, 207 insertions(+), 63 deletions(-)
>
There's parts of this that would seemingly be able to be separated out
so that it's easier to review...
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 82d2fb2a3..d6d2654cd 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -3439,7 +3439,7 @@ qemuBuildMemoryBackendStr(virJSONValuePtr *backendProps,
> } else {
> /* We can have both pagesize and mem source. If that's the case,
> * prefer hugepages as those are more specific. */
> - if (qemuGetMemoryBackingPath(cfg, &memPath) < 0)
> + if (qemuGetMemoryBackingPath(def, cfg, mem->info.alias, &memPath)
< 0)
> goto cleanup;
> }
>
> @@ -3542,12 +3542,13 @@ qemuBuildMemoryCellBackendStr(virDomainDefPtr def,
> unsigned long long memsize = virDomainNumaGetNodeMemorySize(def->numa,
> cell);
>
> + if (virAsprintf(&alias, "ram-node%zu", cell) < 0)
> + goto cleanup;
> +
This is just a simple trivial move and not related...
> *backendStr = NULL;
> mem.size = memsize;
> mem.targetNode = cell;
> -
> - if (virAsprintf(&alias, "ram-node%zu", cell) < 0)
> - goto cleanup;
> + mem.info.alias = alias;
Nope. It is related. So the idea I'm implementing here is that the file
name we put onto qemu cmd line ends with the alias of the device. And
the alias is taken from mem.info.alias. Therefore I need to set it.
>
> if ((rc = qemuBuildMemoryBackendStr(&props, &backendType, cfg,
priv->qemuCaps,
> def, &mem, priv->autoNodeset, false))
< 0)
> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index 23dcc25e1..22c023255 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c
> @@ -1750,9 +1750,41 @@ qemuGetDomainHupageMemPath(const virDomainDef *def,
> }
>
>
> +int
> +qemuGetMemoryBackingBasePath(virQEMUDriverConfigPtr cfg,
> + char **path)
> +{
> + return virAsprintf(path, "%s/libvirt/qemu",
cfg->memoryBackingDir);
Not of a fan of redundancy, but I'm also not sure what better way there
is...
For privileged domains it's "/var/lib/libvirt/qemu/ram/libvirt/qemu/"
and unpriv'd it's $XDG_CONFIG_HOME+"/qemu/ram/libvirt/qemu".
The former repeats "libvirt/qemu" while the latter only repeats
"qemu".
I would think we could drop the "qemu" at the very least, but it doesn't
matter that much to me.
Yeah. I hear you. But trying to hack around this would cause more
problems than it would solve IMO.
Random thought - do memory file paths even work for non priv'd domains?
They should. I don't see a reason why they shouldn't.
> +}
> +
> +
> +int
> +qemuGetMemoryBackingDomainPath(const virDomainDef *def,
> + virQEMUDriverConfigPtr cfg,
> + char **path)
> +{
> + char *shortName = NULL;
> + char *base = NULL;
> + int ret = -1;
> +
> + if (!(shortName = virDomainDefGetShortName(def)) ||
> + qemuGetMemoryBackingBasePath(cfg, &base) < 0 ||
> + virAsprintf(path, "%s/%s", base, shortName) < 0)
> + goto cleanup;
> +
As I noted in review of patch 1... This continues the logic of creating
paths that start with "-", which to me is, well, not right. Maybe others
aren't offended by it.
Is there anyway we can prepend something here to "shortName" - I'd say
"ram", but that's redundant with the path, but that to me is at least
better than a directory name starting with "-".
This won't ever happen. This code runs when ID has already been
allocated for the domain.
> + ret = 0;
> + cleanup:
> + VIR_FREE(base);
> + VIR_FREE(shortName);
> + return ret;
> +}
> +
> +
> /**
> * qemuGetMemoryBackingPath:
> + * @def: domain definition
> * @cfg: the driver config
> + * @alias: memory object alias
> * @memPath: constructed path
> *
> * Constructs path to memory backing dir and stores it at @memPath.
> @@ -1761,8 +1793,27 @@ qemuGetDomainHupageMemPath(const virDomainDef *def,
> * -1 otherwise (with error reported).
> */
> int
> -qemuGetMemoryBackingPath(virQEMUDriverConfigPtr cfg,
> +qemuGetMemoryBackingPath(const virDomainDef *def,
> + virQEMUDriverConfigPtr cfg,
> + const char *alias,
> char **memPath)
> {
> - return VIR_STRDUP(*memPath, cfg->memoryBackingDir);
> + char *domainPath = NULL;
> + int ret = -1;
> +
> + if (!alias) {
> + /* This should never happen (TM) */
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("memory device alias is not assigned"));
> + goto cleanup;
> + }
> +
> + if (qemuGetMemoryBackingDomainPath(def, cfg, &domainPath) < 0 ||
> + virAsprintf(memPath, "%s/%s", domainPath, alias) < 0)
> + goto cleanup;
> +
> + ret = 0;
> + cleanup:
> + VIR_FREE(domainPath);
> + return ret;
> }
> diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
> index 9d6866816..a553e30e2 100644
> --- a/src/qemu/qemu_conf.h
> +++ b/src/qemu/qemu_conf.h
> @@ -364,6 +364,13 @@ int qemuGetDomainHupageMemPath(const virDomainDef *def,
> unsigned long long pagesize,
> char **memPath);
>
> -int qemuGetMemoryBackingPath(virQEMUDriverConfigPtr cfg,
> +int qemuGetMemoryBackingBasePath(virQEMUDriverConfigPtr cfg,
> + char **path);
> +int qemuGetMemoryBackingDomainPath(const virDomainDef *def,
> + virQEMUDriverConfigPtr cfg,
> + char **path);
> +int qemuGetMemoryBackingPath(const virDomainDef *def,
> + virQEMUDriverConfigPtr cfg,
> + const char *alias,
> char **memPath);
> #endif /* __QEMUD_CONF_H */
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 135c20749..b22fb7518 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -630,6 +630,7 @@ qemuStateInitialize(bool privileged,
> uid_t run_uid = -1;
> gid_t run_gid = -1;
> char *hugepagePath = NULL;
> + char *memoryBackingPath = NULL;
> size_t i;
>
> if (VIR_ALLOC(qemu_driver) < 0)
> @@ -888,6 +889,21 @@ qemuStateInitialize(bool privileged,
> VIR_FREE(hugepagePath);
> }
>
> + if (qemuGetMemoryBackingBasePath(cfg, &memoryBackingPath) < 0)
> + goto error;
> +
> + if (virFileMakePath(memoryBackingPath) < 0) {
> + virReportSystemError(errno,
> + _("unable to create memory backing path
%s"),
> + memoryBackingPath);
> + goto error;
> + }
> +
> + if (privileged &&
> + virFileUpdatePerm(memoryBackingPath,
> + 0, S_IXGRP | S_IXOTH) < 0)
> + goto error;
> +
> if (!(qemu_driver->closeCallbacks = virCloseCallbacksNew()))
> goto error;
>
> @@ -945,6 +961,7 @@ qemuStateInitialize(bool privileged,
> virObjectUnref(conn);
> VIR_FREE(driverConf);
> VIR_FREE(hugepagePath);
> + VIR_FREE(memoryBackingPath);
> qemuStateCleanup();
> return -1;
> }
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 91f7f9ed6..5701c033b 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -2077,7 +2077,7 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver,
> priv->qemuCaps, vm->def, mem, NULL, true)
< 0)
> goto cleanup;
>
> - if (qemuProcessBuildDestroyHugepagesPath(driver, vm, mem, true) < 0)
> + if (qemuProcessBuildDestroyMemoryPaths(driver, vm, mem, true) < 0)
> goto cleanup;
>
> if (qemuDomainNamespaceSetupMemory(driver, vm, mem) < 0)
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index fdc868912..4fe973c59 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -3324,60 +3324,117 @@ qemuProcessNeedHugepagesPath(virDomainDefPtr def,
> }
>
>
> +static bool
> +qemuProcessNeedMemoryBackingPath(virDomainDefPtr def,
> + virDomainMemoryDefPtr mem)
> +{
> + size_t i;
> + size_t numaNodes;
> +
> + if (def->mem.source == VIR_DOMAIN_MEMORY_SOURCE_FILE ||
> + def->mem.access != VIR_DOMAIN_MEMORY_ACCESS_DEFAULT)
> + return true;
> +
> + numaNodes = virDomainNumaGetNodeCount(def->numa);
> + for (i = 0; i < numaNodes; i++) {
> + if (virDomainNumaGetNodeMemoryAccessMode(def->numa, i)
> + != VIR_DOMAIN_MEMORY_ACCESS_DEFAULT)
> + return true;
> + }
> +
> + if (mem &&
> + mem->model == VIR_DOMAIN_MEMORY_MODEL_DIMM &&
> + (mem->access != VIR_DOMAIN_MEMORY_ACCESS_DEFAULT ||
> + (mem->targetNode >= 0 &&
> + virDomainNumaGetNodeMemoryAccessMode(def->numa, mem->targetNode)
> + != VIR_DOMAIN_MEMORY_ACCESS_DEFAULT)))
> + return true;
> +
> + return false;
> +}
> +
> +
Starting here...
> +static int
> +qemuProcessBuildDestroyMemoryPathsImpl(virQEMUDriverPtr driver,
> + virDomainDefPtr def,
> + const char *path,
> + bool build)
Could just be qemuProcessBuildDestroyPath as it's one of many possible
calls within (at least) the hugepage loop.
> +{
> + if (build) {
> + if (virFileExists(path))
> + return 0;
> +
> + if (virFileMakePathWithMode(path, 0700) < 0) {
> + virReportSystemError(errno,
> + _("Unable to create %s"),
> + path);
> + return -1;
> + }
> +
> + if (qemuSecurityDomainSetPathLabel(driver->securityManager,
> + def, path) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Unable to label %s"), path);
> + return -1;
> + }
> + } else {
> + if (virFileDeleteTree(path) < 0 &&
> + errno != ENOENT)
> + VIR_WARN("Unable to remove hugepage path: %s (errno=%d)",
This error message would need to change to remove "hugepage"
> + path, errno);
> + }
> +
> + return 0;
> +}
> +
> +
... this seems separable too, modulo the change to use virFileDeleteTree
which then would be a follow-up patch to be accepted on its own
merits... The other option being a check to add some sort of isDir type
logic or flag is input considering that hugepages wouldn't need to
delete the path, right? Or should they?
As Dan points out in v1 - we do need to keep the $dir/libvirt/qemu
though for other domains and we wouldn't want those subdirectories to
get deleted from/for Hugepages before we do so for memory backing,
right? I think the way you have this we'd delete those before getting
to them in the logic, but my eyes are really tired right now.
> int
> -qemuProcessBuildDestroyHugepagesPath(virQEMUDriverPtr driver,
> - virDomainObjPtr vm,
> - virDomainMemoryDefPtr mem,
> - bool build)
> +qemuProcessBuildDestroyMemoryPaths(virQEMUDriverPtr driver,
> + virDomainObjPtr vm,
> + virDomainMemoryDefPtr mem,
> + bool build)
You could seemingly make the API name change separately too, IDC, but
some do generally speaking of course...
> {
> virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
> - char *hugepagePath = NULL;
> + char *path = NULL;
> size_t i;
> - bool shouldBuild = false;
> + bool shouldBuildHP = false;
> + bool shouldBuildMB = false;
> int ret = -1;
>
> - if (build)
> - shouldBuild = qemuProcessNeedHugepagesPath(vm->def, mem);
> + if (build) {
> + shouldBuildHP = qemuProcessNeedHugepagesPath(vm->def, mem);
> + shouldBuildMB = qemuProcessNeedMemoryBackingPath(vm->def, mem);
> + }
>
> - if (!build || shouldBuild) {
> + if (!build || shouldBuildHP) {
> for (i = 0; i < cfg->nhugetlbfs; i++) {
> - VIR_FREE(hugepagePath);
> - hugepagePath = qemuGetDomainHugepagePath(vm->def,
&cfg->hugetlbfs[i]);
> + path = qemuGetDomainHugepagePath(vm->def,
&cfg->hugetlbfs[i]);
>
> - if (!hugepagePath)
> + if (!path)
> goto cleanup;
>
> - if (build) {
> - if (virFileExists(hugepagePath)) {
> - ret = 0;
> - goto cleanup;
> - }
> -
> - if (virFileMakePathWithMode(hugepagePath, 0700) < 0) {
> - virReportSystemError(errno,
> - _("Unable to create %s"),
> - hugepagePath);
> - goto cleanup;
> - }
> + if (qemuProcessBuildDestroyMemoryPathsImpl(driver, vm->def,
> + path, build) < 0)
> + goto cleanup;
>
> - if (qemuSecurityDomainSetPathLabel(driver->securityManager,
> - vm->def, hugepagePath) < 0)
{
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - "%s", _("Unable to set huge path
in security driver"));
> - goto cleanup;
> - }
> - } else {
> - if (rmdir(hugepagePath) < 0 &&
> - errno != ENOENT)
> - VIR_WARN("Unable to remove hugepage path: %s
(errno=%d)",
> - hugepagePath, errno);
> - }
> + VIR_FREE(path);
> }
> }
>
> + if (!build || shouldBuildMB) {
> + if (qemuGetMemoryBackingDomainPath(vm->def, cfg, &path) < 0)
> + goto cleanup;
> +
> + if (qemuProcessBuildDestroyMemoryPathsImpl(driver, vm->def,
> + path, build) < 0)
> + goto cleanup;
> +
> + VIR_FREE(path);
> + }
> +
> ret = 0;
> cleanup:
> - VIR_FREE(hugepagePath);
> + VIR_FREE(path);
> virObjectUnref(cfg);
> return ret;
> }
> @@ -5550,7 +5607,7 @@ qemuProcessPrepareHost(virQEMUDriverPtr driver,
> NULL) < 0)
> goto cleanup;
>
> - if (qemuProcessBuildDestroyHugepagesPath(driver, vm, NULL, true) < 0)
> + if (qemuProcessBuildDestroyMemoryPaths(driver, vm, NULL, true) < 0)
> goto cleanup;
>
> /* Ensure no historical cgroup for this VM is lying around bogus
> @@ -6254,7 +6311,7 @@ void qemuProcessStop(virQEMUDriverPtr driver,
> goto endjob;
> }
>
> - qemuProcessBuildDestroyHugepagesPath(driver, vm, NULL, false);
> + qemuProcessBuildDestroyMemoryPaths(driver, vm, NULL, false);
>
> vm->def->id = -1;
>
> @@ -7112,7 +7169,7 @@ qemuProcessReconnect(void *opaque)
> goto cleanup;
> }
>
> - if (qemuProcessBuildDestroyHugepagesPath(driver, obj, NULL, true) < 0)
> + if (qemuProcessBuildDestroyMemoryPaths(driver, obj, NULL, true) < 0)
> goto error;
>
> if ((qemuDomainAssignAddresses(obj->def, priv->qemuCaps,
> diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h
> index 814b86d8a..cd9a72031 100644
> --- a/src/qemu/qemu_process.h
> +++ b/src/qemu/qemu_process.h
> @@ -38,10 +38,10 @@ int qemuProcessStopCPUs(virQEMUDriverPtr driver,
> virDomainPausedReason reason,
> qemuDomainAsyncJob asyncJob);
>
> -int qemuProcessBuildDestroyHugepagesPath(virQEMUDriverPtr driver,
> - virDomainObjPtr vm,
> - virDomainMemoryDefPtr mem,
> - bool build);
> +int qemuProcessBuildDestroyMemoryPaths(virQEMUDriverPtr driver,
> + virDomainObjPtr vm,
> + virDomainMemoryDefPtr mem,
> + bool build);
>
> void qemuProcessAutostartAll(virQEMUDriverPtr driver);
> void qemuProcessReconnectAll(virConnectPtr conn, virQEMUDriverPtr driver);
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-cpu-numa-memshared.args
b/tests/qemuxml2argvdata/qemuxml2argv-cpu-numa-memshared.args
> index 5700c3413..352819429 100644
> --- a/tests/qemuxml2argvdata/qemuxml2argv-cpu-numa-memshared.args
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-cpu-numa-memshared.args
> @@ -10,10 +10,12 @@ QEMU_AUDIO_DRV=none \
> -M pc \
> -m 214 \
> -smp 16,sockets=2,cores=4,threads=2 \
> --object memory-backend-file,id=ram-node0,mem-path=/var/lib/libvirt/qemu/ram,\
> +-object memory-backend-file,id=ram-node0,\
> +mem-path=/var/lib/libvirt/qemu/ram/libvirt/qemu/-1-QEMUGuest1/ram-node0,\
And this is just where it really obvious what's happening - although it
took me a bit to figure it all out, especially in my post KVM Forum
travel haze.
Yes. This is the expected result and the reason why I wrote this patch.
Okay, I'll try to separate out some changes and repost.
Thanks.
Michal
6:41 a.m.
New subject: [libvirt] [PATCH v2 4/4] news: Document predictable file names for memory-backend-file
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
docs/news.xml | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/docs/news.xml b/docs/news.xml
index 989b82aa5..29eaacb9a 100644
--- a/docs/news.xml
+++ b/docs/news.xml
@@ -51,6 +51,17 @@
</change>
</section>
<section title="Improvements">
+ <change>
+ <summary>
+ qemu: Generate predictable paths for qemu memory backends
+ </summary>
+ <description>
+ In some cases management applications need to know
+ paths passed to memory-backend-file objects upfront.
+ Libvirt now generates predictable paths so applications
+ can prepare the files if they need to do so.
+ </description>
+ </change>
</section>
<section title="Bug fixes">
<change>
--
2.13.6
6:15 p.m.
New subject: [libvirt] [PATCH v2 4/4] news: Document predictable file names for memory-backend-file
On 10/24/2017 07:41 AM, Michal Privoznik wrote:
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
docs/news.xml | 11 +++++++++++
1 file changed, 11 insertions(+)
+1 for news.xml, but you'll need to be sure to update it when you get
around to pushing this.
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
3:04 a.m.
New subject: [libvirt] [PATCH v2 4/4] news: Document predictable file names for memory-backend-file
On 11/02/2017 12:15 AM, John Ferlan wrote:
On 10/24/2017 07:41 AM, Michal Privoznik wrote:
> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> ---
> docs/news.xml | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
+1 for news.xml, but you'll need to be sure to update it when you get
around to pushing this.
Yup. It'll be after the release so I will need to create whole new
structure for the new release.
Thanks.
Michal
2577
days inactive
2586
days old
11 comments
2 participants
participants (2)
-
John Ferlan -
Michal Privoznik