9:22 p.m.
Well, try starting such domain. More info in patches.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1282846
Martin Kletzander (3):
Revert "systemd: Escape only needed characters for machined"
systemd: Add virSystemdGetMachineNameByPID
systemd: Modernize machine naming
src/libvirt_private.syms | 1 +
src/lxc/lxc_cgroup.c | 13 +++-
src/lxc/lxc_domain.c | 1 +
src/lxc/lxc_domain.h | 1 +
src/lxc/lxc_process.c | 19 ++++--
src/qemu/qemu_cgroup.c | 27 +++++---
src/qemu/qemu_cgroup.h | 2 +-
src/qemu/qemu_domain.c | 1 +
src/qemu/qemu_domain.h | 1 +
src/qemu/qemu_process.c | 4 +-
src/util/vircgroup.c | 25 ++++----
src/util/vircgroup.h | 12 ++--
src/util/virsystemd.c | 156 ++++++++++++++++++++++++++++++++++++-----------
src/util/virsystemd.h | 13 ++--
tests/virsystemdtest.c | 81 ++++++++++++++++++------
15 files changed, 262 insertions(+), 95 deletions(-)
--
2.7.0
9:22 p.m.
New subject: [libvirt] [PATCH 1/3] Revert "systemd: Escape only needed characters for machined"
This reverts commit 0e0149ce91d84f40b98acf4c4bb0da6e29b9c15c.
That commit was added to comply with systemd rules that were changed in
the meantime, so this patch is pointless.
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
src/util/virsystemd.c | 23 +++++++----------------
tests/virsystemdtest.c | 4 ++--
2 files changed, 9 insertions(+), 18 deletions(-)
diff --git a/src/util/virsystemd.c b/src/util/virsystemd.c
index 337d6a208ab6..abd883c73844 100644
--- a/src/util/virsystemd.c
+++ b/src/util/virsystemd.c
@@ -38,17 +38,8 @@
VIR_LOG_INIT("util.systemd");
-/**
- * virSystemdEscapeName:
- *
- * This function escapes various characters in @name and appends that
- * escaped string to @buf, in order to comply with the requirements
- * from systemd/machined. Parameter @full_escape decides whether to
- * also escape dot as a first character and '-'.
- */
static void virSystemdEscapeName(virBufferPtr buf,
- const char *name,
- bool full_escape)
+ const char *name)
{
static const char hextable[16] = "0123456789abcdef";
@@ -66,7 +57,7 @@ static void virSystemdEscapeName(virBufferPtr buf,
"ABCDEFGHIJKLMNOPQRSTUVWXYZ" \
":-_.\\"
- if (full_escape && *name == '.') {
+ if (*name == '.') {
ESCAPE(*name);
name++;
}
@@ -74,7 +65,7 @@ static void virSystemdEscapeName(virBufferPtr buf,
while (*name) {
if (*name == '/')
virBufferAddChar(buf, '-');
- else if ((full_escape && *name == '-') ||
+ else if (*name == '-' ||
*name == '\\' ||
!strchr(VALID_CHARS, *name))
ESCAPE(*name);
@@ -94,9 +85,9 @@ char *virSystemdMakeScopeName(const char *name,
virBuffer buf = VIR_BUFFER_INITIALIZER;
virBufferAddLit(&buf, "machine-");
- virSystemdEscapeName(&buf, drivername, true);
+ virSystemdEscapeName(&buf, drivername);
virBufferAddLit(&buf, "\\x2d");
- virSystemdEscapeName(&buf, name, true);
+ virSystemdEscapeName(&buf, name);
virBufferAddLit(&buf, ".scope");
if (virBufferCheckError(&buf) < 0)
@@ -113,7 +104,7 @@ char *virSystemdMakeSliceName(const char *partition)
if (*partition == '/')
partition++;
- virSystemdEscapeName(&buf, partition, true);
+ virSystemdEscapeName(&buf, partition);
virBufferAddLit(&buf, ".slice");
if (virBufferCheckError(&buf) < 0)
@@ -139,7 +130,7 @@ char *virSystemdMakeMachineName(const char *name,
virBufferAsprintf(&buf, "%s-%s-", username, drivername);
}
- virSystemdEscapeName(&buf, name, false);
+ virSystemdEscapeName(&buf, name);
machinename = virBufferContentAndReset(&buf);
cleanup:
diff --git a/tests/virsystemdtest.c b/tests/virsystemdtest.c
index 49d37c2032ec..06fec5495bc2 100644
--- a/tests/virsystemdtest.c
+++ b/tests/virsystemdtest.c
@@ -517,9 +517,9 @@ mymain(void)
} while (0)
TEST_MACHINE("demo", "qemu-demo");
- TEST_MACHINE("demo-name", "qemu-demo-name");
+ TEST_MACHINE("demo-name", "qemu-demo\\x2dname");
TEST_MACHINE("demo!name", "qemu-demo\\x21name");
- TEST_MACHINE(".demo", "qemu-.demo");
+ TEST_MACHINE(".demo", "qemu-\\x2edemo");
TEST_MACHINE("bull\U0001f4a9", "qemu-bull\\xf0\\x9f\\x92\\xa9");
# define TESTS_PM_SUPPORT_HELPER(name, function) \
--
2.7.0
2:07 p.m.
New subject: [libvirt] [PATCH 1/3] Revert "systemd: Escape only needed characters for machined"
On Tue, Feb 02, 2016 at 09:22:47PM +0100, Martin Kletzander wrote:
This reverts commit 0e0149ce91d84f40b98acf4c4bb0da6e29b9c15c.
That commit was added to comply with systemd rules that were changed in
the meantime, so this patch is pointless.
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
src/util/virsystemd.c | 23 +++++++----------------
tests/virsystemdtest.c | 4 ++--
2 files changed, 9 insertions(+), 18 deletions(-)
ACK
9:31 p.m.
New subject: [libvirt] [PATCH 1/3] Revert "systemd: Escape only needed characters for machined"
On Wed, 2016-02-03 at 14:07 +0100, Pavel Hrdina wrote:
On Tue, Feb 02, 2016 at 09:22:47PM +0100, Martin Kletzander wrote:
> This reverts commit 0e0149ce91d84f40b98acf4c4bb0da6e29b9c15c.
>
> That commit was added to comply with systemd rules that were changed in
> the meantime, so this patch is pointless.
>
> Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
> ---
> src/util/virsystemd.c | 23 +++++++----------------
> tests/virsystemdtest.c | 4 ++--
> 2 files changed, 9 insertions(+), 18 deletions(-)
ACK
I have a guest named debian-q35, and after this commit I can no
longer start it:
error: Failed to start domain debian-q35
error: Invalid machine name
Looks like the name gets escaped as
qemu-debian\x2dq35
and systemd doesn't like it. After reverting your revert I can
start the guest again. I'm running
systemd-222-13.fc23.x86_64
which I assume is pretty recent.
Cheers.
--
Andrea Bolognani
Software Engineer - Virtualization Team
10:28 a.m.
New subject: [libvirt] [PATCH 1/3] Revert "systemd: Escape only needed characters for machined"
On Wed, Feb 03, 2016 at 09:31:50PM +0100, Andrea Bolognani wrote:
On Wed, 2016-02-03 at 14:07 +0100, Pavel Hrdina wrote:
> On Tue, Feb 02, 2016 at 09:22:47PM +0100, Martin Kletzander wrote:
> > This reverts commit 0e0149ce91d84f40b98acf4c4bb0da6e29b9c15c.
> >
> > That commit was added to comply with systemd rules that were changed in
> > the meantime, so this patch is pointless.
> >
> > Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
> > ---
> > src/util/virsystemd.c | 23 +++++++----------------
> > tests/virsystemdtest.c | 4 ++--
> > 2 files changed, 9 insertions(+), 18 deletions(-)
>
> ACK
I have a guest named debian-q35, and after this commit I can no
longer start it:
error: Failed to start domain debian-q35
error: Invalid machine name
Looks like the name gets escaped as
qemu-debian\x2dq35
and systemd doesn't like it. After reverting your revert I can
start the guest again. I'm running
systemd-222-13.fc23.x86_64
which I assume is pretty recent.
Sorry for that, I completely forgot that in my latest series the first
two patches are dependent on the last one. I'm trying never to do that,
but this time I wanted to split it so that it's nice to review, polus
the revert was there...
Anyway, really sorry for that, I'm just now testing the (hopefully) last
version of the third patch, will post it really soon.
Cheers.
--
Andrea Bolognani
Software Engineer - Virtualization Team
9:22 p.m.
New subject: [libvirt] [PATCH 2/3] systemd: Add virSystemdGetMachineNameByPID
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
src/libvirt_private.syms | 1 +
src/util/virsystemd.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++
src/util/virsystemd.h | 2 ++
tests/virsystemdtest.c | 44 ++++++++++++++++++++++++++++++++++++++++
4 files changed, 100 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 3d0ec9d786b6..e2d552e0193d 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2273,6 +2273,7 @@ virSystemdCanHibernate;
virSystemdCanHybridSleep;
virSystemdCanSuspend;
virSystemdCreateMachine;
+virSystemdGetMachineNameByPID;
virSystemdMakeMachineName;
virSystemdMakeScopeName;
virSystemdMakeSliceName;
diff --git a/src/util/virsystemd.c b/src/util/virsystemd.c
index abd883c73844..3349d95a4597 100644
--- a/src/util/virsystemd.c
+++ b/src/util/virsystemd.c
@@ -139,6 +139,59 @@ char *virSystemdMakeMachineName(const char *name,
return machinename;
}
+char *
+virSystemdGetMachineNameByPID(pid_t pid)
+{
+ DBusConnection *conn;
+ DBusMessage *reply;
+ char *name = NULL, *object = NULL;
+
+ if (virDBusIsServiceEnabled("org.freedesktop.machine1") < 0)
+ goto cleanup;
+
+ if (virDBusIsServiceRegistered("org.freedesktop.systemd1") < 0)
+ goto cleanup;
+
+ if (!(conn = virDBusGetSystemBus()))
+ goto cleanup;
+
+ if (virDBusCallMethod(conn, &reply, NULL,
+ "org.freedesktop.machine1",
+ "/org/freedesktop/machine1",
+ "org.freedesktop.machine1.Manager",
+ "GetMachineByPID",
+ "u", pid) < 0)
+ goto cleanup;
+
+ if (virDBusMessageRead(reply, "o", &object) < 0)
+ goto cleanup;
+
+ VIR_DEBUG("Domain with pid %llu has object path '%s'",
+ (unsigned long long)pid, object);
+
+ if (virDBusCallMethod(conn, &reply, NULL,
+ "org.freedesktop.machine1",
+ object,
+ "org.freedesktop.DBus.Properties",
+ "Get",
+ "ss",
+ "org.freedesktop.machine1.Machine",
+ "Name") < 0)
+ goto cleanup;
+
+ if (virDBusMessageRead(reply, "v", "s", &name) < 0)
+ goto cleanup;
+
+ VIR_DEBUG("Domain with pid %llu has machine name '%s'",
+ (unsigned long long)pid, name);
+
+ cleanup:
+ VIR_FREE(object);
+ dbus_message_unref(reply);
+
+ return name;
+}
+
/**
* virSystemdCreateMachine:
* @name: driver unique name of the machine
diff --git a/src/util/virsystemd.h b/src/util/virsystemd.h
index 8af216910188..a13a4c0c48be 100644
--- a/src/util/virsystemd.h
+++ b/src/util/virsystemd.h
@@ -55,4 +55,6 @@ int virSystemdCanHibernate(bool *result);
int virSystemdCanHybridSleep(bool *result);
+char *virSystemdGetMachineNameByPID(pid_t pid);
+
#endif /* __VIR_SYSTEMD_H__ */
diff --git a/tests/virsystemdtest.c b/tests/virsystemdtest.c
index 06fec5495bc2..1746a02b131d 100644
--- a/tests/virsystemdtest.c
+++ b/tests/virsystemdtest.c
@@ -54,6 +54,31 @@ VIR_MOCK_WRAP_RET_ARGS(dbus_connection_send_with_reply_and_block,
"Something went wrong creating the machine");
} else {
reply = dbus_message_new(DBUS_MESSAGE_TYPE_METHOD_RETURN);
+
+ if (STREQ(member, "GetMachineByPID")) {
+ const char *object_path =
"/org/freedesktop/machine1/machine/qemu_2ddemo";
+ DBusMessageIter iter;
+
+ dbus_message_iter_init_append(reply, &iter);
+ if (!dbus_message_iter_append_basic(&iter,
+ DBUS_TYPE_OBJECT_PATH,
+ &object_path))
+ goto error;
+ } else if (STREQ(member, "Get")) {
+ const char *name = "qemu-demo";
+ DBusMessageIter iter;
+ DBusMessageIter sub;
+
+ dbus_message_iter_init_append(reply, &iter);
+ dbus_message_iter_open_container(&iter, DBUS_TYPE_VARIANT,
+ "s", &sub);
+
+ if (!dbus_message_iter_append_basic(&sub,
+ DBUS_TYPE_STRING,
+ &name))
+ goto error;
+ dbus_message_iter_close_container(&iter, &sub);
+ }
}
} else if (STREQ(service, "org.freedesktop.login1")) {
char *supported = getenv("RESULT_SUPPORT");
@@ -337,6 +362,23 @@ static int testCreateNetwork(const void *opaque ATTRIBUTE_UNUSED)
return 0;
}
+static int testGetMachineName(const void *opaque ATTRIBUTE_UNUSED)
+{
+ char *tmp = virSystemdGetMachineNameByPID(1234);
+ int ret = -1;
+
+ if (!tmp) {
+ fprintf(stderr, "%s", "Failed to create LXC machine\n");
+ return ret;
+ }
+
+ if (STREQ(tmp, "qemu-demo"))
+ ret = 0;
+
+ VIR_FREE(tmp);
+ return ret;
+}
+
struct testNameData {
const char *name;
@@ -491,6 +533,8 @@ mymain(void)
ret = -1;
if (virtTestRun("Test create with network ", testCreateNetwork, NULL) <
0)
ret = -1;
+ if (virtTestRun("Test getting machine name ", testGetMachineName, NULL)
< 0)
+ ret = -1;
# define TEST_SCOPE(name, unitname) \
do { \
--
2.7.0
2:28 p.m.
New subject: [libvirt] [PATCH 2/3] systemd: Add virSystemdGetMachineNameByPID
On Tue, Feb 02, 2016 at 09:22:48PM +0100, Martin Kletzander wrote:
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
src/libvirt_private.syms | 1 +
src/util/virsystemd.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++
src/util/virsystemd.h | 2 ++
tests/virsystemdtest.c | 44 ++++++++++++++++++++++++++++++++++++++++
4 files changed, 100 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 3d0ec9d786b6..e2d552e0193d 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2273,6 +2273,7 @@ virSystemdCanHibernate;
virSystemdCanHybridSleep;
virSystemdCanSuspend;
virSystemdCreateMachine;
+virSystemdGetMachineNameByPID;
virSystemdMakeMachineName;
virSystemdMakeScopeName;
virSystemdMakeSliceName;
diff --git a/src/util/virsystemd.c b/src/util/virsystemd.c
index abd883c73844..3349d95a4597 100644
--- a/src/util/virsystemd.c
+++ b/src/util/virsystemd.c
@@ -139,6 +139,59 @@ char *virSystemdMakeMachineName(const char *name,
return machinename;
}
I know that this file doesn't follow this unwritten rule, but we tend to place
two empty lines between functions.
+char *
+virSystemdGetMachineNameByPID(pid_t pid)
+{
+ DBusConnection *conn;
+ DBusMessage *reply;
+ char *name = NULL, *object = NULL;
[...]
+ cleanup:
+ VIR_FREE(object);
+ dbus_message_unref(reply);
+
+ return name;
+}
+
Same here.
[...]
@@ -337,6 +362,23 @@ static int testCreateNetwork(const void *opaque
ATTRIBUTE_UNUSED)
return 0;
}
Here too, and also place the return type on separate line.
+static int testGetMachineName(const void *opaque ATTRIBUTE_UNUSED)
+{
+ char *tmp = virSystemdGetMachineNameByPID(1234);
+ int ret = -1;
+
+ if (!tmp) {
+ fprintf(stderr, "%s", "Failed to create LXC machine\n");
+ return ret;
+ }
+
+ if (STREQ(tmp, "qemu-demo"))
+ ret = 0;
+
+ VIR_FREE(tmp);
+ return ret;
+}
+
And here.
ACK with the formatting fixed.
3 p.m.
New subject: [libvirt] [PATCH 2/3] systemd: Add virSystemdGetMachineNameByPID
On Wed, Feb 03, 2016 at 02:28:03PM +0100, Pavel Hrdina wrote:
On Tue, Feb 02, 2016 at 09:22:48PM +0100, Martin Kletzander wrote:
> Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
> ---
> src/libvirt_private.syms | 1 +
> src/util/virsystemd.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++
> src/util/virsystemd.h | 2 ++
> tests/virsystemdtest.c | 44 ++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 100 insertions(+)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 3d0ec9d786b6..e2d552e0193d 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2273,6 +2273,7 @@ virSystemdCanHibernate;
> virSystemdCanHybridSleep;
> virSystemdCanSuspend;
> virSystemdCreateMachine;
> +virSystemdGetMachineNameByPID;
> virSystemdMakeMachineName;
> virSystemdMakeScopeName;
> virSystemdMakeSliceName;
> diff --git a/src/util/virsystemd.c b/src/util/virsystemd.c
> index abd883c73844..3349d95a4597 100644
> --- a/src/util/virsystemd.c
> +++ b/src/util/virsystemd.c
> @@ -139,6 +139,59 @@ char *virSystemdMakeMachineName(const char *name,
> return machinename;
> }
>
I know that this file doesn't follow this unwritten rule, but we tend to place
two empty lines between functions.
The rule is more to be consistent than using two lines. So I'm usually
picking whatever is used the most in that file. In this one I'd say
it's about 50/50
> +char *
> +virSystemdGetMachineNameByPID(pid_t pid)
> +{
> + DBusConnection *conn;
> + DBusMessage *reply;
> + char *name = NULL, *object = NULL;
[...]
> + cleanup:
> + VIR_FREE(object);
> + dbus_message_unref(reply);
> +
> + return name;
> +}
> +
Same here.
[...]
> @@ -337,6 +362,23 @@ static int testCreateNetwork(const void *opaque
ATTRIBUTE_UNUSED)
> return 0;
> }
>
Here too, and also place the return type on separate line.
That makes it the second function in that file to be formatted like
that, yay! =)
> +static int testGetMachineName(const void *opaque
ATTRIBUTE_UNUSED)
> +{
> + char *tmp = virSystemdGetMachineNameByPID(1234);
> + int ret = -1;
> +
> + if (!tmp) {
> + fprintf(stderr, "%s", "Failed to create LXC
machine\n");
> + return ret;
> + }
> +
> + if (STREQ(tmp, "qemu-demo"))
> + ret = 0;
> +
> + VIR_FREE(tmp);
> + return ret;
> +}
> +
And here.
They are here already, you just removed the line in the reply.
ACK with the formatting fixed.
I didn't necessarily agree, but I want this pushed, so I "fixed" what
you said so I don't need to engage in a discussion about that. And
pushed the first two patches, I'll send a v2 for the third one.
9:22 p.m.
New subject: [libvirt] [PATCH 3/3] systemd: Modernize machine naming
So, systemd-machined has this philosophy that machine names are like
hostnames and hence should follow the same rules. But we always allowed
international characters in domain names. Thus we need to modify the
machine name we are passing to systemd.
In order to change some machinenames that we will be passing to systemd,
we also need to call TerminateMachine at the end of a lifetime of a
domain. Even for domains that were started with older libvirt. That
can be achieved thanks to virSystemdGetMachineNameByPID(). And because
we can change machine names, we can get rid of the inconsistent and
pointless escaping of domain names when creating machine names.
So this patch modifies the naming in the following way. It tries using
the name we were using earlier (without escaping the name, see above)
and if that is not possible, because it doesn't follow the rules given
by systemd, it then fallbacks to using <drivername>-<uuid> instead.
That way we can start domains we couldn't start before. Well, at least
on systemd.
To make it work all together, the machineName (which is needed only for
systemd) is saved in domain's private data. That way the generation is
moved to the driver and we don't need to pass various unnecessary
arguments to cgroup functions.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1282846
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
src/lxc/lxc_cgroup.c | 13 ++++++--
src/lxc/lxc_domain.c | 1 +
src/lxc/lxc_domain.h | 1 +
src/lxc/lxc_process.c | 19 ++++++++----
src/qemu/qemu_cgroup.c | 27 +++++++++++-----
src/qemu/qemu_cgroup.h | 2 +-
src/qemu/qemu_domain.c | 1 +
src/qemu/qemu_domain.h | 1 +
src/qemu/qemu_process.c | 4 +--
src/util/vircgroup.c | 25 ++++++++-------
src/util/vircgroup.h | 12 ++++----
src/util/virsystemd.c | 82 +++++++++++++++++++++++++++++++++++++------------
src/util/virsystemd.h | 11 +++----
tests/virsystemdtest.c | 41 +++++++++++++------------
14 files changed, 158 insertions(+), 82 deletions(-)
diff --git a/src/lxc/lxc_cgroup.c b/src/lxc/lxc_cgroup.c
index ad254e4934fc..aba0cb430b5b 100644
--- a/src/lxc/lxc_cgroup.c
+++ b/src/lxc/lxc_cgroup.c
@@ -29,6 +29,7 @@
#include "viralloc.h"
#include "vircgroup.h"
#include "virstring.h"
+#include "virsystemd.h"
#define VIR_FROM_THIS VIR_FROM_LXC
@@ -483,6 +484,13 @@ virCgroupPtr virLXCCgroupCreate(virDomainDefPtr def,
int *nicindexes)
{
virCgroupPtr cgroup = NULL;
+ char *machineName = virSystemdMakeMachineName("lxc",
+ def->name,
+ def->uuid,
+ true);
+
+ if (!machineName)
+ goto cleanup;
if (def->resource->partition[0] != '/') {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
@@ -491,9 +499,8 @@ virCgroupPtr virLXCCgroupCreate(virDomainDefPtr def,
goto cleanup;
}
- if (virCgroupNewMachine(def->name,
+ if (virCgroupNewMachine(machineName,
"lxc",
- true,
def->uuid,
NULL,
initpid,
@@ -517,6 +524,8 @@ virCgroupPtr virLXCCgroupCreate(virDomainDefPtr def,
}
cleanup:
+ VIR_FREE(machineName);
+
return cgroup;
}
diff --git a/src/lxc/lxc_domain.c b/src/lxc/lxc_domain.c
index c3f7a564b36b..6732e934453f 100644
--- a/src/lxc/lxc_domain.c
+++ b/src/lxc/lxc_domain.c
@@ -28,6 +28,7 @@
#include "virerror.h"
#include <libxml/xpathInternals.h>
#include "virstring.h"
+#include "vircgroup.h"
#include "virutil.h"
#include "virfile.h"
diff --git a/src/lxc/lxc_domain.h b/src/lxc/lxc_domain.h
index 2119c7899007..39c6e7def9ce 100644
--- a/src/lxc/lxc_domain.h
+++ b/src/lxc/lxc_domain.h
@@ -64,6 +64,7 @@ struct _virLXCDomainObjPrivate {
pid_t initpid;
virCgroupPtr cgroup;
+ char *machineName;
};
extern virDomainXMLNamespace virLXCDriverDomainXMLNamespace;
diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
index f7e2b810b74b..b14b69da4a16 100644
--- a/src/lxc/lxc_process.c
+++ b/src/lxc/lxc_process.c
@@ -233,8 +233,7 @@ static void virLXCProcessCleanup(virLXCDriverPtr driver,
* properly. See https://bugs.freedesktop.org/show_bug.cgi?id=68370 for
* the bug we are working around here.
*/
- virSystemdTerminateMachine(vm->def->name, "lxc", true);
-
+ virCgroupTerminateMachine(priv->machineName);
/* The "release" hook cleans up additional resources */
if (virHookPresent(VIR_HOOK_DRIVER_LXC)) {
@@ -1494,8 +1493,8 @@ int virLXCProcessStart(virConnectPtr conn,
* point so lets detect that first, since it gives us a
* more reliable way to kill everything off if something
* goes wrong from here onwards ... */
- if (virCgroupNewDetectMachine(vm->def->name, "lxc", vm->pid,
- -1, &priv->cgroup) < 0)
+ if (virCgroupNewDetectMachine(vm->def->name, vm->def->uuid,
"lxc",
+ vm->pid, -1, &priv->cgroup) < 0)
goto cleanup;
if (!priv->cgroup) {
@@ -1505,6 +1504,11 @@ int virLXCProcessStart(virConnectPtr conn,
goto cleanup;
}
+ /* Get the machine name so we can properly delete it through
+ * systemd later */
+ if (!(priv->machineName = virSystemdGetMachineNameByPID(vm->pid)))
+ virResetLastError();
+
/* And we can get the first monitor connection now too */
if (!(priv->monitor = virLXCProcessConnectMonitor(driver, vm))) {
/* Intentionally overwrite the real monitor error message,
@@ -1677,8 +1681,8 @@ virLXCProcessReconnectDomain(virDomainObjPtr vm,
if (!(priv->monitor = virLXCProcessConnectMonitor(driver, vm)))
goto error;
- if (virCgroupNewDetectMachine(vm->def->name, "lxc", vm->pid,
- -1, &priv->cgroup) < 0)
+ if (virCgroupNewDetectMachine(vm->def->name, vm->def->uuid,
"lxc",
+ vm->pid, -1, &priv->cgroup) < 0)
goto error;
if (!priv->cgroup) {
@@ -1688,6 +1692,9 @@ virLXCProcessReconnectDomain(virDomainObjPtr vm,
goto error;
}
+ if (!(priv->machineName = virSystemdGetMachineNameByPID(vm->pid)))
+ virResetLastError();
+
if (virLXCUpdateActiveUSBHostdevs(driver, vm->def) < 0)
goto error;
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index e41f4617c455..aa7641cc4bbd 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -36,6 +36,7 @@
#include "virfile.h"
#include "virtypedparam.h"
#include "virnuma.h"
+#include "virsystemd.h"
#define VIR_FROM_THIS VIR_FROM_QEMU
@@ -772,9 +773,19 @@ qemuInitCgroup(virQEMUDriverPtr driver,
goto cleanup;
}
- if (virCgroupNewMachine(vm->def->name,
+ /*
+ * We need to do this because of systemd-machined, because
+ * CreateMachine requires the name to be a valid hostname.
+ */
+ priv->machineName = virSystemdMakeMachineName("qemu",
+ vm->def->name,
+ vm->def->uuid,
+ virQEMUDriverIsPrivileged(driver));
+ if (!priv->machineName)
+ goto cleanup;
+
+ if (virCgroupNewMachine(priv->machineName,
"qemu",
- true,
vm->def->uuid,
NULL,
vm->pid,
@@ -887,12 +898,17 @@ qemuConnectCgroup(virQEMUDriverPtr driver,
virCgroupFree(&priv->cgroup);
if (virCgroupNewDetectMachine(vm->def->name,
+ vm->def->uuid,
"qemu",
vm->pid,
cfg->cgroupControllers,
&priv->cgroup) < 0)
goto cleanup;
+ priv->machineName = virSystemdGetMachineNameByPID(vm->pid);
+ if (!priv->machineName)
+ virResetLastError();
+
qemuRestoreCgroupState(vm);
done:
@@ -1264,17 +1280,14 @@ qemuSetupCgroupForIOThreads(virDomainObjPtr vm)
}
int
-qemuRemoveCgroup(virQEMUDriverPtr driver,
- virDomainObjPtr vm)
+qemuRemoveCgroup(virDomainObjPtr vm)
{
qemuDomainObjPrivatePtr priv = vm->privateData;
if (priv->cgroup == NULL)
return 0; /* Not supported, so claim success */
- if (virCgroupTerminateMachine(vm->def->name,
- "qemu",
- virQEMUDriverIsPrivileged(driver)) < 0) {
+ if (virCgroupTerminateMachine(priv->machineName) < 0) {
if (!virCgroupNewIgnoreError())
VIR_DEBUG("Failed to terminate cgroup for %s",
vm->def->name);
}
diff --git a/src/qemu/qemu_cgroup.h b/src/qemu/qemu_cgroup.h
index 2bcf071d3792..347d126f7394 100644
--- a/src/qemu/qemu_cgroup.h
+++ b/src/qemu/qemu_cgroup.h
@@ -56,7 +56,7 @@ int qemuSetupCgroupCpusetCpus(virCgroupPtr cgroup, virBitmapPtr
cpumask);
int qemuSetupCgroupForVcpu(virDomainObjPtr vm);
int qemuSetupCgroupForIOThreads(virDomainObjPtr vm);
int qemuSetupCgroupForEmulator(virDomainObjPtr vm);
-int qemuRemoveCgroup(virQEMUDriverPtr driver, virDomainObjPtr vm);
+int qemuRemoveCgroup(virDomainObjPtr vm);
int qemuAddToCgroup(virDomainObjPtr vm);
#endif /* __QEMU_CGROUP_H__ */
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 1df1b74ab806..80a6e730c9ab 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -39,6 +39,7 @@
#include "virtime.h"
#include "virstoragefile.h"
#include "virstring.h"
+#include "virsystemd.h"
#include "virthreadjob.h"
#include "viratomic.h"
#include "virprocess.h"
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 7fc4fffacecc..70707f2c57a1 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -203,6 +203,7 @@ struct _qemuDomainObjPrivate {
bool signalIOError; /* true if the domain condition should be signalled on
I/O error */
+ char *machineName;
};
# define QEMU_DOMAIN_DISK_PRIVATE(disk) \
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index ee94d3f7542b..70cf8bc34d45 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -4682,7 +4682,7 @@ qemuProcessLaunch(virConnectPtr conn,
/* Ensure no historical cgroup for this VM is lying around bogus
* settings */
VIR_DEBUG("Ensuring no historical cgroup is lying around");
- qemuRemoveCgroup(driver, vm);
+ qemuRemoveCgroup(vm);
VIR_DEBUG("Setting up ports for graphics");
if (qemuProcessSetupGraphics(driver, vm) < 0)
@@ -5477,7 +5477,7 @@ void qemuProcessStop(virQEMUDriverPtr driver,
}
retry:
- if ((ret = qemuRemoveCgroup(driver, vm)) < 0) {
+ if ((ret = qemuRemoveCgroup(vm)) < 0) {
if (ret == -EBUSY && (retries++ < 5)) {
usleep(200*1000);
goto retry;
diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
index 9b56e271ea23..dfc81f44242f 100644
--- a/src/util/vircgroup.c
+++ b/src/util/vircgroup.c
@@ -43,6 +43,7 @@
#include "vircgrouppriv.h"
#include "virutil.h"
+#include "viruuid.h"
#include "viralloc.h"
#include "virerror.h"
#include "virlog.h"
@@ -243,12 +244,14 @@ static bool
virCgroupValidateMachineGroup(virCgroupPtr group,
const char *name,
const char *drivername,
+ const unsigned char *uuid,
bool stripEmulatorSuffix)
{
size_t i;
bool valid = false;
char *partname;
char *scopename;
+ char uuidstr[VIR_UUID_STRING_BUFLEN];
if (virAsprintf(&partname, "%s.libvirt-%s",
name, drivername) < 0)
@@ -263,6 +266,8 @@ virCgroupValidateMachineGroup(virCgroupPtr group,
if (virCgroupPartitionEscape(&scopename) < 0)
goto cleanup;
+ virUUIDFormat(uuid, uuidstr);
+
for (i = 0; i < VIR_CGROUP_CONTROLLER_LAST; i++) {
char *tmp;
@@ -290,6 +295,7 @@ virCgroupValidateMachineGroup(virCgroupPtr group,
tmp++;
if (STRNEQ(tmp, name) &&
+ STRNEQ(tmp, uuidstr) &&
STRNEQ(tmp, partname) &&
STRNEQ(tmp, scopename)) {
VIR_DEBUG("Name '%s' for controller '%s' does not match
"
@@ -1554,6 +1560,7 @@ virCgroupNewDetect(pid_t pid,
*/
int
virCgroupNewDetectMachine(const char *name,
+ const unsigned char *uuid,
const char *drivername,
pid_t pid,
int controllers,
@@ -1565,7 +1572,7 @@ virCgroupNewDetectMachine(const char *name,
return -1;
}
- if (!virCgroupValidateMachineGroup(*group, name, drivername, true)) {
+ if (!virCgroupValidateMachineGroup(*group, name, drivername, uuid, true)) {
VIR_DEBUG("Failed to validate machine name for '%s' driver
'%s'",
name, drivername);
virCgroupFree(group);
@@ -1582,7 +1589,6 @@ virCgroupNewDetectMachine(const char *name,
static int
virCgroupNewMachineSystemd(const char *name,
const char *drivername,
- bool privileged,
const unsigned char *uuid,
const char *rootdir,
pid_t pidleader,
@@ -1602,7 +1608,6 @@ virCgroupNewMachineSystemd(const char *name,
VIR_DEBUG("Trying to setup machine '%s' via systemd", name);
if ((rv = virSystemdCreateMachine(name,
drivername,
- privileged,
uuid,
rootdir,
pidleader,
@@ -1690,11 +1695,9 @@ virCgroupNewMachineSystemd(const char *name,
/*
* Returns 0 on success, -1 on fatal error
*/
-int virCgroupTerminateMachine(const char *name,
- const char *drivername,
- bool privileged)
+int virCgroupTerminateMachine(const char *name)
{
- return virSystemdTerminateMachine(name, drivername, privileged);
+ return virSystemdTerminateMachine(name);
}
@@ -1749,7 +1752,6 @@ virCgroupNewMachineManual(const char *name,
int
virCgroupNewMachine(const char *name,
const char *drivername,
- bool privileged,
const unsigned char *uuid,
const char *rootdir,
pid_t pidleader,
@@ -1766,7 +1768,6 @@ virCgroupNewMachine(const char *name,
if ((rv = virCgroupNewMachineSystemd(name,
drivername,
- privileged,
uuid,
rootdir,
pidleader,
@@ -4218,6 +4219,7 @@ virCgroupNewDetect(pid_t pid ATTRIBUTE_UNUSED,
int
virCgroupNewDetectMachine(const char *name ATTRIBUTE_UNUSED,
+ const unsigned char *uuid ATTRIBUTE_UNUSED,
const char *drivername ATTRIBUTE_UNUSED,
pid_t pid ATTRIBUTE_UNUSED,
int controllers ATTRIBUTE_UNUSED,
@@ -4229,9 +4231,7 @@ virCgroupNewDetectMachine(const char *name ATTRIBUTE_UNUSED,
}
-int virCgroupTerminateMachine(const char *name ATTRIBUTE_UNUSED,
- const char *drivername ATTRIBUTE_UNUSED,
- bool privileged ATTRIBUTE_UNUSED)
+int virCgroupTerminateMachine(const char *name ATTRIBUTE_UNUSED)
{
virReportSystemError(ENXIO, "%s",
_("Control groups not supported on this platform"));
@@ -4242,7 +4242,6 @@ int virCgroupTerminateMachine(const char *name ATTRIBUTE_UNUSED,
int
virCgroupNewMachine(const char *name ATTRIBUTE_UNUSED,
const char *drivername ATTRIBUTE_UNUSED,
- bool privileged ATTRIBUTE_UNUSED,
const unsigned char *uuid ATTRIBUTE_UNUSED,
const char *rootdir ATTRIBUTE_UNUSED,
pid_t pidleader ATTRIBUTE_UNUSED,
diff --git a/src/util/vircgroup.h b/src/util/vircgroup.h
index d754b1f3bd7a..dd4cb719f1f1 100644
--- a/src/util/vircgroup.h
+++ b/src/util/vircgroup.h
@@ -90,14 +90,16 @@ int virCgroupNewDetect(pid_t pid,
virCgroupPtr *group);
int virCgroupNewDetectMachine(const char *name,
+ const unsigned char *uuid,
const char *drivername,
pid_t pid,
int controllers,
- virCgroupPtr *group);
+ virCgroupPtr *group)
+ ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2)
+ ATTRIBUTE_NONNULL(3);
int virCgroupNewMachine(const char *name,
const char *drivername,
- bool privileged,
const unsigned char *uuid,
const char *rootdir,
pid_t pidleader,
@@ -110,10 +112,8 @@ int virCgroupNewMachine(const char *name,
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2)
ATTRIBUTE_NONNULL(4);
-int virCgroupTerminateMachine(const char *name,
- const char *drivername,
- bool privileged)
- ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
+int virCgroupTerminateMachine(const char *name)
+ ATTRIBUTE_NONNULL(1);
bool virCgroupNewIgnoreError(void);
diff --git a/src/util/virsystemd.c b/src/util/virsystemd.c
index 3349d95a4597..62934096d70c 100644
--- a/src/util/virsystemd.c
+++ b/src/util/virsystemd.c
@@ -25,6 +25,8 @@
# include <systemd/sd-daemon.h>
#endif
+#include "internal.h"
+
#include "virsystemd.h"
#include "viratomic.h"
#include "virdbus.h"
@@ -33,6 +35,7 @@
#include "virutil.h"
#include "virlog.h"
#include "virerror.h"
+#include "viruuid.h"
#define VIR_FROM_THIS VIR_FROM_SYSTEMD
@@ -113,9 +116,10 @@ char *virSystemdMakeSliceName(const char *partition)
return virBufferContentAndReset(&buf);
}
-char *virSystemdMakeMachineName(const char *name,
- const char *drivername,
- bool privileged)
+static char *
+virSystemdMakeMachineNameNice(const char *drivername,
+ const char *name,
+ bool privileged)
{
char *machinename = NULL;
char *username = NULL;
@@ -130,7 +134,7 @@ char *virSystemdMakeMachineName(const char *name,
virBufferAsprintf(&buf, "%s-%s-", username, drivername);
}
- virSystemdEscapeName(&buf, name);
+ virBufferAddStr(&buf, name);
machinename = virBufferContentAndReset(&buf);
cleanup:
@@ -209,7 +213,6 @@ virSystemdGetMachineNameByPID(pid_t pid)
*/
int virSystemdCreateMachine(const char *name,
const char *drivername,
- bool privileged,
const unsigned char *uuid,
const char *rootdir,
pid_t pidleader,
@@ -220,7 +223,6 @@ int virSystemdCreateMachine(const char *name,
{
int ret;
DBusConnection *conn;
- char *machinename = NULL;
char *creatorname = NULL;
char *slicename = NULL;
static int hasCreateWithNetwork = 1;
@@ -236,8 +238,6 @@ int virSystemdCreateMachine(const char *name,
return -1;
ret = -1;
- if (!(machinename = virSystemdMakeMachineName(name, drivername, privileged)))
- goto cleanup;
if (virAsprintf(&creatorname, "libvirt-%s", drivername) < 0)
goto cleanup;
@@ -315,7 +315,7 @@ int virSystemdCreateMachine(const char *name,
"org.freedesktop.machine1.Manager",
"CreateMachineWithNetwork",
"sayssusa&ia(sv)",
- machinename,
+ name,
16,
uuid[0], uuid[1], uuid[2], uuid[3],
uuid[4], uuid[5], uuid[6], uuid[7],
@@ -357,7 +357,7 @@ int virSystemdCreateMachine(const char *name,
"org.freedesktop.machine1.Manager",
"CreateMachine",
"sayssusa(sv)",
- machinename,
+ name,
16,
uuid[0], uuid[1], uuid[2], uuid[3],
uuid[4], uuid[5], uuid[6], uuid[7],
@@ -378,20 +378,19 @@ int virSystemdCreateMachine(const char *name,
cleanup:
VIR_FREE(creatorname);
- VIR_FREE(machinename);
VIR_FREE(slicename);
return ret;
}
-int virSystemdTerminateMachine(const char *name,
- const char *drivername,
- bool privileged)
+int virSystemdTerminateMachine(const char *name)
{
int ret;
DBusConnection *conn;
- char *machinename = NULL;
virError error;
+ if (!name)
+ return 0;
+
memset(&error, 0, sizeof(error));
ret = virDBusIsServiceEnabled("org.freedesktop.machine1");
@@ -406,9 +405,6 @@ int virSystemdTerminateMachine(const char *name,
if (!(conn = virDBusGetSystemBus()))
goto cleanup;
- if (!(machinename = virSystemdMakeMachineName(name, drivername, privileged)))
- goto cleanup;
-
/*
* The systemd DBus API we're invoking has the
* following signature
@@ -428,7 +424,7 @@ int virSystemdTerminateMachine(const char *name,
"org.freedesktop.machine1.Manager",
"TerminateMachine",
"s",
- machinename) < 0)
+ name) < 0)
goto cleanup;
if (error.code == VIR_ERR_ERROR &&
@@ -443,7 +439,6 @@ int virSystemdTerminateMachine(const char *name,
cleanup:
virResetError(&error);
- VIR_FREE(machinename);
return ret;
}
@@ -513,3 +508,50 @@ int virSystemdCanHybridSleep(bool *result)
{
return virSystemdPMSupportTarget("CanHybridSleep", result);
}
+
+#define HOSTNAME_CHARS \
+ "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789-."
+
+static bool
+virSystemdIsValidMachineName(const char *name)
+{
+ size_t len = strlen(name);
+
+ if (len > 64)
+ return false;
+
+ if (name[0] == '.')
+ return false;
+
+ if (strstr(name, ".."))
+ return false;
+
+ if (strspn(name, HOSTNAME_CHARS) != len)
+ return false;
+
+ return true;
+}
+
+#undef HOSTNAME_CHARS
+
+char *
+virSystemdMakeMachineName(const char *drivername,
+ const char *name,
+ const unsigned char *uuid,
+ bool privileged)
+{
+ char *machinename = virSystemdMakeMachineNameNice(drivername,
+ name,
+ privileged);
+ char uuidstr[VIR_UUID_STRING_BUFLEN] = {0};
+
+ if (virSystemdIsValidMachineName(machinename))
+ return machinename;
+
+ VIR_FREE(machinename);
+
+ virUUIDFormat(uuid, uuidstr);
+ ignore_value(virAsprintf(&machinename, "%s-%s", drivername, uuidstr));
+
+ return machinename;
+}
diff --git a/src/util/virsystemd.h b/src/util/virsystemd.h
index a13a4c0c48be..c8fb88c13590 100644
--- a/src/util/virsystemd.h
+++ b/src/util/virsystemd.h
@@ -23,18 +23,19 @@
# define __VIR_SYSTEMD_H__
# include "internal.h"
+# include "virbuffer.h"
char *virSystemdMakeScopeName(const char *name,
const char *drivername);
char *virSystemdMakeSliceName(const char *partition);
-char *virSystemdMakeMachineName(const char *name,
- const char *drivername,
+char *virSystemdMakeMachineName(const char *drivername,
+ const char *name,
+ const unsigned char *uuid,
bool privileged);
int virSystemdCreateMachine(const char *name,
const char *drivername,
- bool privileged,
const unsigned char *uuid,
const char *rootdir,
pid_t pidleader,
@@ -43,9 +44,7 @@ int virSystemdCreateMachine(const char *name,
int *nicindexes,
const char *partition);
-int virSystemdTerminateMachine(const char *name,
- const char *drivername,
- bool privileged);
+int virSystemdTerminateMachine(const char *name);
void virSystemdNotifyStartup(void);
diff --git a/tests/virsystemdtest.c b/tests/virsystemdtest.c
index 1746a02b131d..b9dc42202d94 100644
--- a/tests/virsystemdtest.c
+++ b/tests/virsystemdtest.c
@@ -166,7 +166,6 @@ static int testCreateContainer(const void *opaque ATTRIBUTE_UNUSED)
};
if (virSystemdCreateMachine("demo",
"lxc",
- true,
uuid,
"/proc/123/root",
123,
@@ -182,9 +181,7 @@ static int testCreateContainer(const void *opaque ATTRIBUTE_UNUSED)
static int testTerminateContainer(const void *opaque ATTRIBUTE_UNUSED)
{
- if (virSystemdTerminateMachine("demo",
- "lxc",
- true) < 0) {
+ if (virSystemdTerminateMachine("lxc-demo") < 0) {
fprintf(stderr, "%s", "Failed to terminate LXC machine\n");
return -1;
}
@@ -202,7 +199,6 @@ static int testCreateMachine(const void *opaque ATTRIBUTE_UNUSED)
};
if (virSystemdCreateMachine("demo",
"qemu",
- false,
uuid,
NULL,
123,
@@ -218,9 +214,7 @@ static int testCreateMachine(const void *opaque ATTRIBUTE_UNUSED)
static int testTerminateMachine(const void *opaque ATTRIBUTE_UNUSED)
{
- if (virSystemdTerminateMachine("demo",
- "qemu",
- false) < 0) {
+ if (virSystemdTerminateMachine("test-qemu-demo") < 0) {
fprintf(stderr, "%s", "Failed to terminate KVM machine\n");
return -1;
}
@@ -242,7 +236,6 @@ static int testCreateNoSystemd(const void *opaque ATTRIBUTE_UNUSED)
if ((rv = virSystemdCreateMachine("demo",
"qemu",
- true,
uuid,
NULL,
123,
@@ -277,7 +270,6 @@ static int testCreateSystemdNotRunning(const void *opaque
ATTRIBUTE_UNUSED)
if ((rv = virSystemdCreateMachine("demo",
"qemu",
- true,
uuid,
NULL,
123,
@@ -312,7 +304,6 @@ static int testCreateBadSystemd(const void *opaque ATTRIBUTE_UNUSED)
if ((rv = virSystemdCreateMachine("demo",
"qemu",
- true,
uuid,
NULL,
123,
@@ -348,7 +339,6 @@ static int testCreateNetwork(const void *opaque ATTRIBUTE_UNUSED)
size_t nnicindexes = ARRAY_CARDINALITY(nicindexes);
if (virSystemdCreateMachine("demo",
"lxc",
- true,
uuid,
"/proc/123/root",
123,
@@ -383,6 +373,7 @@ static int testGetMachineName(const void *opaque ATTRIBUTE_UNUSED)
struct testNameData {
const char *name;
const char *expected;
+ const unsigned char *uuid;
};
static int
@@ -415,7 +406,8 @@ testMachineName(const void *opaque)
int ret = -1;
char *actual = NULL;
- if (!(actual = virSystemdMakeMachineName(data->name, "qemu", true)))
+ if (!(actual = virSystemdMakeMachineName("qemu", data->name,
+ data->uuid, true)))
goto cleanup;
if (STRNEQ(actual, data->expected)) {
@@ -516,6 +508,12 @@ mymain(void)
{
int ret = 0;
+ unsigned char uuid[VIR_UUID_BUFLEN];
+
+ /* The one we use in tests quite often */
+ if (virUUIDParse("c7a5fdbd-edaf-9455-926a-d65c16db1809", uuid) < 0)
+ return EXIT_FAILURE;
+
if (virtTestRun("Test create container ", testCreateContainer, NULL) <
0)
ret = -1;
if (virtTestRun("Test terminate container ", testTerminateContainer, NULL)
< 0)
@@ -539,7 +537,7 @@ mymain(void)
# define TEST_SCOPE(name, unitname) \
do { \
struct testNameData data = { \
- name, unitname \
+ name, unitname, uuid \
}; \
if (virtTestRun("Test scopename", testScopeName, &data) < 0)
\
ret = -1; \
@@ -554,17 +552,22 @@ mymain(void)
# define TEST_MACHINE(name, machinename) \
do { \
struct testNameData data = { \
- name, machinename \
+ name, machinename, uuid \
}; \
if (virtTestRun("Test scopename", testMachineName, &data) < 0)
\
ret = -1; \
} while (0)
TEST_MACHINE("demo", "qemu-demo");
- TEST_MACHINE("demo-name", "qemu-demo\\x2dname");
- TEST_MACHINE("demo!name", "qemu-demo\\x21name");
- TEST_MACHINE(".demo", "qemu-\\x2edemo");
- TEST_MACHINE("bull\U0001f4a9", "qemu-bull\\xf0\\x9f\\x92\\xa9");
+ TEST_MACHINE("demo-name", "qemu-demo-name");
+ TEST_MACHINE("demo!name",
"qemu-c7a5fdbd-edaf-9455-926a-d65c16db1809");
+ TEST_MACHINE(".demo", "qemu-.demo");
+ TEST_MACHINE("bull\U0001f4a9",
"qemu-c7a5fdbd-edaf-9455-926a-d65c16db1809");
+ TEST_MACHINE("demo..name",
"qemu-c7a5fdbd-edaf-9455-926a-d65c16db1809");
+
TEST_MACHINE("12345678901234567890123456789012345678901234567890123456789",
+
"qemu-12345678901234567890123456789012345678901234567890123456789");
+
TEST_MACHINE("123456789012345678901234567890123456789012345678901234567890",
+ "qemu-c7a5fdbd-edaf-9455-926a-d65c16db1809");
# define TESTS_PM_SUPPORT_HELPER(name, function) \
do { \
--
2.7.0
2:16 p.m.
New subject: [libvirt] [PATCH 3/3] systemd: Modernize machine naming
On Tue, Feb 02, 2016 at 09:22:49PM +0100, Martin Kletzander wrote:
So, systemd-machined has this philosophy that machine names are like
hostnames and hence should follow the same rules. But we always allowed
international characters in domain names. Thus we need to modify the
machine name we are passing to systemd.
In order to change some machinenames that we will be passing to systemd,
we also need to call TerminateMachine at the end of a lifetime of a
domain. Even for domains that were started with older libvirt. That
can be achieved thanks to virSystemdGetMachineNameByPID(). And because
we can change machine names, we can get rid of the inconsistent and
pointless escaping of domain names when creating machine names.
So this patch modifies the naming in the following way. It tries using
the name we were using earlier (without escaping the name, see above)
and if that is not possible, because it doesn't follow the rules given
by systemd, it then fallbacks to using <drivername>-<uuid> instead.
That way we can start domains we couldn't start before. Well, at least
on systemd.
Ewww, please not uuids. They are really horrible things that should not
be shown to users by default. IIUC we have 2 problems you're trying to
address - some characters are invalid, and we have a name length limit.
Invalid characters can be dealt with by escaping or stripping them.
The length limit is harder, since if we merely truncate we'll not have
guaranteed unique names anymore. A nicer way to get around that would
be to append the domain ID to the it, since that is a short unqiue
token. ie <drivername>-<truncated name>-<id>
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
2:39 p.m.
New subject: [libvirt] [PATCH 3/3] systemd: Modernize machine naming
On Wed, Feb 03, 2016 at 01:16:02PM +0000, Daniel P. Berrange wrote:
On Tue, Feb 02, 2016 at 09:22:49PM +0100, Martin Kletzander wrote:
> So, systemd-machined has this philosophy that machine names are like
> hostnames and hence should follow the same rules. But we always allowed
> international characters in domain names. Thus we need to modify the
> machine name we are passing to systemd.
>
> In order to change some machinenames that we will be passing to systemd,
s/machinenames/machine names/
> we also need to call TerminateMachine at the end of a lifetime
of a
> domain. Even for domains that were started with older libvirt. That
> can be achieved thanks to virSystemdGetMachineNameByPID(). And because
> we can change machine names, we can get rid of the inconsistent and
> pointless escaping of domain names when creating machine names.
>
> So this patch modifies the naming in the following way. It tries using
> the name we were using earlier (without escaping the name, see above)
> and if that is not possible, because it doesn't follow the rules given
> by systemd, it then fallbacks to using <drivername>-<uuid> instead.
> That way we can start domains we couldn't start before. Well, at least
> on systemd.
Ewww, please not uuids. They are really horrible things that should not
be shown to users by default. IIUC we have 2 problems you're trying to
address - some characters are invalid, and we have a name length limit.
Invalid characters can be dealt with by escaping or stripping them.
The length limit is harder, since if we merely truncate we'll not have
guaranteed unique names anymore. A nicer way to get around that would
be to append the domain ID to the it, since that is a short unqiue
token. ie <drivername>-<truncated name>-<id>
We've discussed locally with some other guys and using only UUID it's not good.
I agree with Dan and I proposed similar solution (didn't know about the driver
name included in the machine name). I would suggest something like this:
domain name machine name
-----------------------------------------------------
valid chars and short <drivername>-<domainname>
valid chars but long <drivername>-<truncated domainname>-<id>
invalid chars and short <drivername>-<stripped domainname>-<id>
invalid chars and long <drivername>-<stripped&truncated
domainname>-<id>
But there could be cases, where stripping the invalid domain name could leads to
empty string, in that case there would be probably only drivername and ID.
I'm not generally against using UUID instead of ID, for me personally UUID seems
to be better choice.
Pavel
2:42 p.m.
New subject: [libvirt] [PATCH 3/3] systemd: Modernize machine naming
On Wed, Feb 03, 2016 at 01:16:02PM +0000, Daniel P. Berrange wrote:
On Tue, Feb 02, 2016 at 09:22:49PM +0100, Martin Kletzander wrote:
> So, systemd-machined has this philosophy that machine names are like
> hostnames and hence should follow the same rules. But we always allowed
> international characters in domain names. Thus we need to modify the
> machine name we are passing to systemd.
>
> In order to change some machinenames that we will be passing to systemd,
> we also need to call TerminateMachine at the end of a lifetime of a
> domain. Even for domains that were started with older libvirt. That
> can be achieved thanks to virSystemdGetMachineNameByPID(). And because
> we can change machine names, we can get rid of the inconsistent and
> pointless escaping of domain names when creating machine names.
>
> So this patch modifies the naming in the following way. It tries using
> the name we were using earlier (without escaping the name, see above)
> and if that is not possible, because it doesn't follow the rules given
> by systemd, it then fallbacks to using <drivername>-<uuid> instead.
> That way we can start domains we couldn't start before. Well, at least
> on systemd.
Ewww, please not uuids. They are really horrible things that should not
be shown to users by default. IIUC we have 2 problems you're trying to
address - some characters are invalid, and we have a name length limit.
I hate UUIDs, but the only other thing that I was offered was punnycode
encoding using libidn. And nobody seemed to have an opinion on that.
Invalid characters can be dealt with by escaping or stripping them.
The length limit is harder, since if we merely truncate we'll not have
guaranteed unique names anymore. A nicer way to get around that would
be to append the domain ID to the it, since that is a short unqiue
token. ie <drivername>-<truncated name>-<id>
TL;DR Woud you agree on <drivername>-<id>-<name> where non-ascii
characters are stripped fromt the name and then the whole string is
truncated to 64 characters if it is longer than that?
More blabber follows...
I like the idea of using ID. That would help a lot. However, it would
only make sense for long enough names. Let's say there won't be any
user who would have a two machines, one with too long of a name and
other with a name that is a prefix of the first one with -<number>
appended. Also we could use the ID for every machine name, I think
<drivername>-<id>-<name> still looks nice.
Anyway, that doesn't deal with international characters in the name.
And that can be split into two different problems as well. You can have
a name that has one non-ASCII character in it. If we strip it, we are
not going to have the uniqueness, but you can at least know which name
represents which machine. However, there are then names that are with
no ASCII characters at all, e.g. different alphabet. Stripping those
means you have an empty string. Escaping on the other hand leads to
longer names and in the second case is not readable at all.
So as a conclusion, would you agree if we did the driver-id-name where
non-ascii characters are stripped and then the whole string is truncated
to 64 characters if it is longer?
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
2:44 p.m.
New subject: [libvirt] [PATCH 3/3] systemd: Modernize machine naming
On Wed, Feb 03, 2016 at 02:42:52PM +0100, Martin Kletzander wrote:
On Wed, Feb 03, 2016 at 01:16:02PM +0000, Daniel P. Berrange wrote:
>On Tue, Feb 02, 2016 at 09:22:49PM +0100, Martin Kletzander wrote:
>>So, systemd-machined has this philosophy that machine names are like
>>hostnames and hence should follow the same rules. But we always allowed
>>international characters in domain names. Thus we need to modify the
>>machine name we are passing to systemd.
>>
>>In order to change some machinenames that we will be passing to systemd,
>>we also need to call TerminateMachine at the end of a lifetime of a
>>domain. Even for domains that were started with older libvirt. That
>>can be achieved thanks to virSystemdGetMachineNameByPID(). And because
>>we can change machine names, we can get rid of the inconsistent and
>>pointless escaping of domain names when creating machine names.
>>
>>So this patch modifies the naming in the following way. It tries using
>>the name we were using earlier (without escaping the name, see above)
>>and if that is not possible, because it doesn't follow the rules given
>>by systemd, it then fallbacks to using <drivername>-<uuid> instead.
>>That way we can start domains we couldn't start before. Well, at least
>>on systemd.
>
>Ewww, please not uuids. They are really horrible things that should not
>be shown to users by default. IIUC we have 2 problems you're trying to
>address - some characters are invalid, and we have a name length limit.
>
I hate UUIDs, but the only other thing that I was offered was punnycode
encoding using libidn. And nobody seemed to have an opinion on that.
>Invalid characters can be dealt with by escaping or stripping them.
>The length limit is harder, since if we merely truncate we'll not have
>guaranteed unique names anymore. A nicer way to get around that would
>be to append the domain ID to the it, since that is a short unqiue
>token. ie <drivername>-<truncated name>-<id>
>
TL;DR Woud you agree on <drivername>-<id>-<name> where non-ascii
characters are stripped fromt the name and then the whole string is
truncated to 64 characters if it is longer than that?
More blabber follows...
I like the idea of using ID. That would help a lot. However, it would
only make sense for long enough names. Let's say there won't be any
user who would have a two machines, one with too long of a name and
other with a name that is a prefix of the first one with -<number>
appended. Also we could use the ID for every machine name, I think
<drivername>-<id>-<name> still looks nice.
Anyway, that doesn't deal with international characters in the name.
And that can be split into two different problems as well. You can have
a name that has one non-ASCII character in it. If we strip it, we are
not going to have the uniqueness, but you can at least know which name
represents which machine. However, there are then names that are with
no ASCII characters at all, e.g. different alphabet. Stripping those
means you have an empty string. Escaping on the other hand leads to
longer names and in the second case is not readable at all.
So as a conclusion, would you agree if we did the driver-id-name where
non-ascii characters are stripped and then the whole string is truncated
to 64 characters if it is longer?
Yeah, using id as a prefix unconditionally sounds fine to me - it should
make the code simpler too.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
3147
days inactive
3149
days old
12 comments
4 participants
participants (4)
-
Andrea Bolognani -
Daniel P. Berrange -
Martin Kletzander -
Pavel Hrdina