11:59 a.m.
Inspired by the talk here:
https://www.redhat.com/archives/libvir-list/2020-March/msg00232.html
Instead of using md5() and the first 6 letters of it, I've decided to go
with SHA256 and 8 letters as this offers 16^8 = 4294967296 = 2^32
possibilities, which is the limit of inodes for ext4. If we ever hit a
conflict, we can just use 9 letters of the hash.
Michal Prívozník (2):
qemu_conf: Track embed root dir
conf: Don't generate clashing machine names for embed driver
src/conf/domain_conf.c | 18 +++++++++++++++---
src/conf/domain_conf.h | 1 +
src/lxc/lxc_domain.c | 2 +-
src/qemu/qemu_conf.c | 2 ++
src/qemu/qemu_conf.h | 2 ++
src/qemu/qemu_domain.c | 4 +++-
tests/virsystemdtest.c | 35 ++++++++++++++++++++---------------
7 files changed, 44 insertions(+), 20 deletions(-)
--
2.24.1
11:59 a.m.
New subject: [PATCH 1/2] qemu_conf: Track embed root dir
When initializing virQEMUDriverConfig structure we are given the
root directory for possible embed connection. Save it for future
use. While we could get it later from @uri member, it's not as
easy as dereferencing a pointer (virURIParse() +
virURIGetParam() + error reporting).
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_conf.c | 2 ++
src/qemu/qemu_conf.h | 2 ++
2 files changed, 4 insertions(+)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 17a6eb3422..d57ffdaeb5 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -115,6 +115,7 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged,
if (root) {
cfg->uri = g_strdup_printf("qemu:///embed?root=%s", root);
+ cfg->root = g_strdup(root);
} else {
cfg->uri = g_strdup(privileged ? "qemu:///system" :
"qemu:///session");
}
@@ -299,6 +300,7 @@ static void virQEMUDriverConfigDispose(void *obj)
virStringListFree(cfg->cgroupDeviceACL);
VIR_FREE(cfg->uri);
+ VIR_FREE(cfg->root);
VIR_FREE(cfg->configBaseDir);
VIR_FREE(cfg->configDir);
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index 3ce9566b71..82fea63fbc 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -76,6 +76,8 @@ struct _virQEMUDriverConfig {
virObject parent;
char *uri;
+ char *root; /* The root directory for embed driver,
+ NULL for system/session connections */
uid_t user;
gid_t group;
--
2.24.1
8:50 a.m.
New subject: [PATCH 1/2] qemu_conf: Track embed root dir
On Fri, 2020-03-13 at 17:59 +0100, Michal Privoznik wrote:
When initializing virQEMUDriverConfig structure we are given the
root directory for possible embed connection. Save it for future
use. While we could get it later from @uri member, it's not as
easy as dereferencing a pointer (virURIParse() +
virURIGetParam() + error reporting).
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_conf.c | 2 ++
src/qemu/qemu_conf.h | 2 ++
2 files changed, 4 insertions(+)
Reviewed-by: Andrea Bolognani <abologna(a)redhat.com>
--
Andrea Bolognani / Red Hat / Virtualization
11:59 a.m.
New subject: [PATCH 2/2] conf: Don't generate clashing machine names for embed driver
So far, when using the qemu:///embed driver, management
applications can't chose whether they want to register their
domains in machined or not. While having that option is certainly
desired, it will require more work. What we can do meanwhile is
to generate names that include part of hash of the root
directory. This is to ensure that if two applications using
different roots but the same domain name (and ID) start the
domain no clashing name for machined is generated.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/domain_conf.c | 18 +++++++++++++++---
src/conf/domain_conf.h | 1 +
src/lxc/lxc_domain.c | 2 +-
src/qemu/qemu_domain.c | 4 +++-
tests/virsystemdtest.c | 35 ++++++++++++++++++++---------------
5 files changed, 40 insertions(+), 20 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index f8a8d133ba..c4894e1b26 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -62,6 +62,7 @@
#include "virdomainsnapshotobjlist.h"
#include "virdomaincheckpointobjlist.h"
#include "virutil.h"
+#include "vircrypto.h"
#define VIR_FROM_THIS VIR_FROM_DOMAIN
@@ -30956,22 +30957,33 @@ virDomainMachineNameAppendValid(virBufferPtr buf,
char *
virDomainGenerateMachineName(const char *drivername,
+ const char *root,
int id,
const char *name,
bool privileged)
{
- char *username = NULL;
virBuffer buf = VIR_BUFFER_INITIALIZER;
- if (privileged) {
+ if (root) {
+ g_autofree char * hash = NULL;
+
+ /* When two embed drivers start two domains with the same @name and @id
+ * we would generate a non-unique name. Include parts of hashed @root
+ * which guarantees uniqueness. The first 8 characters of SHA256 ought
+ * to be enough for anybody. */
+ if (virCryptoHashString(VIR_CRYPTO_HASH_SHA256, root, &hash) < 0)
+ return NULL;
+
+ virBufferAsprintf(&buf, "%s-embed-%.8s-", drivername, hash);
+ } else if (privileged) {
virBufferAsprintf(&buf, "%s-", drivername);
} else {
+ g_autofree char *username = NULL;
if (!(username = virGetUserName(geteuid()))) {
virBufferFreeAndReset(&buf);
return NULL;
}
virBufferAsprintf(&buf, "%s-%s-", username, drivername);
- VIR_FREE(username);
}
virBufferAsprintf(&buf, "%d-", id);
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 91b776c28a..73bd097cf8 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -3649,6 +3649,7 @@ int virDomainDiskSetBlockIOTune(virDomainDiskDefPtr disk,
char *
virDomainGenerateMachineName(const char *drivername,
+ const char *root,
int id,
const char *name,
bool privileged);
diff --git a/src/lxc/lxc_domain.c b/src/lxc/lxc_domain.c
index 03d0f46b24..ebd2c2b56e 100644
--- a/src/lxc/lxc_domain.c
+++ b/src/lxc/lxc_domain.c
@@ -406,7 +406,7 @@ virLXCDomainGetMachineName(virDomainDefPtr def, pid_t pid)
}
if (!ret)
- ret = virDomainGenerateMachineName("lxc", def->id, def->name,
true);
+ ret = virDomainGenerateMachineName("lxc", NULL, def->id,
def->name, true);
return ret;
}
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 4b467afa81..dcee42369f 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -16256,6 +16256,7 @@ qemuDomainGetMachineName(virDomainObjPtr vm)
{
qemuDomainObjPrivatePtr priv = vm->privateData;
virQEMUDriverPtr driver = priv->driver;
+ g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
char *ret = NULL;
if (vm->pid > 0) {
@@ -16265,7 +16266,8 @@ qemuDomainGetMachineName(virDomainObjPtr vm)
}
if (!ret)
- ret = virDomainGenerateMachineName("qemu", vm->def->id,
vm->def->name,
+ ret = virDomainGenerateMachineName("qemu", cfg->root,
+ vm->def->id, vm->def->name,
virQEMUDriverIsPrivileged(driver));
return ret;
diff --git a/tests/virsystemdtest.c b/tests/virsystemdtest.c
index fa0980c845..050941dce8 100644
--- a/tests/virsystemdtest.c
+++ b/tests/virsystemdtest.c
@@ -379,6 +379,7 @@ testGetMachineName(const void *opaque G_GNUC_UNUSED)
struct testNameData {
const char *name;
const char *expected;
+ const char *root;
int id;
bool legacy;
};
@@ -413,8 +414,8 @@ testMachineName(const void *opaque)
int ret = -1;
char *actual = NULL;
- if (!(actual = virDomainGenerateMachineName("qemu", data->id,
- data->name, true)))
+ if (!(actual = virDomainGenerateMachineName("qemu", data->root,
+ data->id, data->name, true)))
goto cleanup;
if (STRNEQ(actual, data->expected)) {
@@ -724,30 +725,34 @@ mymain(void)
TEST_SCOPE_NEW("qemu-3-demo",
"machine-qemu\\x2d3\\x2ddemo.scope");
-# define TEST_MACHINE(_name, _id, machinename) \
+# define TEST_MACHINE(_name, _root, _id, machinename) \
do { \
struct testNameData data = { \
- .name = _name, .expected = machinename, .id = _id, \
+ .name = _name, .expected = machinename, .root = _root, .id = _id, \
}; \
if (virTestRun("Test scopename", testMachineName, &data) < 0) \
ret = -1; \
} while (0)
- TEST_MACHINE("demo", 1, "qemu-1-demo");
- TEST_MACHINE("demo-name", 2, "qemu-2-demo-name");
- TEST_MACHINE("demo!name", 3, "qemu-3-demoname");
- TEST_MACHINE(".demo", 4, "qemu-4-demo");
- TEST_MACHINE("bull\U0001f4a9", 5, "qemu-5-bull");
- TEST_MACHINE("demo..name", 6, "qemu-6-demo.name");
- TEST_MACHINE("12345678901234567890123456789012345678901234567890123456789",
7,
+ TEST_MACHINE("demo", NULL, 1, "qemu-1-demo");
+ TEST_MACHINE("demo-name", NULL, 2, "qemu-2-demo-name");
+ TEST_MACHINE("demo!name", NULL, 3, "qemu-3-demoname");
+ TEST_MACHINE(".demo", NULL, 4, "qemu-4-demo");
+ TEST_MACHINE("bull\U0001f4a9", NULL, 5, "qemu-5-bull");
+ TEST_MACHINE("demo..name", NULL, 6, "qemu-6-demo.name");
+ TEST_MACHINE("12345678901234567890123456789012345678901234567890123456789",
NULL, 7,
"qemu-7-123456789012345678901234567890123456789012345678901234567");
-
TEST_MACHINE("123456789012345678901234567890123456789012345678901234567890", 8,
+
TEST_MACHINE("123456789012345678901234567890123456789012345678901234567890",
NULL, 8,
"qemu-8-123456789012345678901234567890123456789012345678901234567");
-
TEST_MACHINE("kstest-network-device-default-httpks_(c9eed63e-981e-48ec-acdc-56b3f8c5f678)",
100,
+
TEST_MACHINE("kstest-network-device-default-httpks_(c9eed63e-981e-48ec-acdc-56b3f8c5f678)",
+ NULL, 100,
"qemu-100-kstest-network-device-default-httpksc9eed63e-981e-48ec");
-
TEST_MACHINE("kstest-network-device-default-httpks_(c9eed63e-981e-48ec--cdc-56b3f8c5f678)",
10,
+
TEST_MACHINE("kstest-network-device-default-httpks_(c9eed63e-981e-48ec--cdc-56b3f8c5f678)",
+ NULL, 10,
"qemu-10-kstest-network-device-default-httpksc9eed63e-981e-48ec-c");
- TEST_MACHINE("demo.-.test.", 11, "qemu-11-demo.test");
+ TEST_MACHINE("demo.-.test.", NULL, 11, "qemu-11-demo.test");
+ TEST_MACHINE("demo", "/tmp/root1", 1,
"qemu-embed-0991f456-1-demo");
+ TEST_MACHINE("demo", "/tmp/root2", 1,
"qemu-embed-95d47ff5-1-demo");
# define TESTS_PM_SUPPORT_HELPER(name, function) \
do { \
--
2.24.1
8:55 a.m.
New subject: [PATCH 2/2] conf: Don't generate clashing machine names for embed driver
On Fri, 2020-03-13 at 17:59 +0100, Michal Privoznik wrote:
[...]
- if (privileged) {
+ if (root) {
+ g_autofree char * hash = NULL;
+
+ /* When two embed drivers start two domains with the same @name and @id
+ * we would generate a non-unique name. Include parts of hashed @root
+ * which guarantees uniqueness. The first 8 characters of SHA256 ought
+ * to be enough for anybody. */
+ if (virCryptoHashString(VIR_CRYPTO_HASH_SHA256, root, &hash) < 0)
+ return NULL;
+
+ virBufferAsprintf(&buf, "%s-embed-%.8s-", drivername, hash);
When libvirt is non-privileged we use $username-$drivername, so there
would be a precedent for something like $hash-$drivername-embed; that
said, having $drivername first makes more sense to me, so if anything
I suggest we change the existing one to $drivername-$username.
+ } else if (privileged) {
virBufferAsprintf(&buf, "%s-", drivername);
} else {
+ g_autofree char *username = NULL;
if (!(username = virGetUserName(geteuid()))) {
virBufferFreeAndReset(&buf);
return NULL;
}
virBufferAsprintf(&buf, "%s-%s-", username, drivername);
- VIR_FREE(username);
This hunk is unrelated, so please drop it. It can be a standalone
trivial patch instead.
With that done,
Reviewed-by: Andrea Bolognani <abologna(a)redhat.com>
Thank you so much for taking care of this!
--
Andrea Bolognani / Red Hat / Virtualization
1710
days inactive
1715
days old
4 comments
2 participants
participants (2)
-
Andrea Bolognani -
Michal Privoznik