[libvirt] [PATCH] systemd: Escape only needed characters for machined
My previous fix in commit e24eda48cfae84a9003456b68eaf753a26123639 was incomplete, or rather more complete than it needed to be. The problem is that even though machined requires non-ASCII characters to be escaped does not mean that it needs to follow the exact same rules as unit files and services, therefore rendering our escape function, namely virSystemdEscapeName(), as overkill. Well, that should not be a problem, because if we escape more than needed, it will not break anything. However, that is not the case with current release of systemd because even though it requires some characters to be escaped, *any* escaped character will cause the function to fail. Even though that will be fixed, we need to make sure that machines, which were able to start before the commit mentioned above, are still able to start now. So this patch changes virSystemdEscapeName() to operate as the aforementioned overkill merely based on its new parameter. If that parameter is false, which only occurs in the latest call to this function from virSystemdMakeMachineName(), it will not escape characters that would cause the machine being unable to be started. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1282846 Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/util/virsystemd.c | 15 ++++++++------- tests/virsystemdtest.c | 4 ++-- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/src/util/virsystemd.c b/src/util/virsystemd.c index abd883c73844..257efaab7829 100644 --- a/src/util/virsystemd.c +++ b/src/util/virsystemd.c @@ -39,7 +39,8 @@ VIR_LOG_INIT("util.systemd"); static void virSystemdEscapeName(virBufferPtr buf, - const char *name) + const char *name, + bool full_escape) { static const char hextable[16] = "0123456789abcdef"; @@ -57,7 +58,7 @@ static void virSystemdEscapeName(virBufferPtr buf, "ABCDEFGHIJKLMNOPQRSTUVWXYZ" \ ":-_.\\" - if (*name == '.') { + if (full_escape && *name == '.') { ESCAPE(*name); name++; } @@ -65,7 +66,7 @@ static void virSystemdEscapeName(virBufferPtr buf, while (*name) { if (*name == '/') virBufferAddChar(buf, '-'); - else if (*name == '-' || + else if ((full_escape && *name == '-') || *name == '\\' || !strchr(VALID_CHARS, *name)) ESCAPE(*name); @@ -85,9 +86,9 @@ char *virSystemdMakeScopeName(const char *name, virBuffer buf = VIR_BUFFER_INITIALIZER; virBufferAddLit(&buf, "machine-"); - virSystemdEscapeName(&buf, drivername); + virSystemdEscapeName(&buf, drivername, true); virBufferAddLit(&buf, "\\x2d"); - virSystemdEscapeName(&buf, name); + virSystemdEscapeName(&buf, name, true); virBufferAddLit(&buf, ".scope"); if (virBufferCheckError(&buf) < 0) @@ -104,7 +105,7 @@ char *virSystemdMakeSliceName(const char *partition) if (*partition == '/') partition++; - virSystemdEscapeName(&buf, partition); + virSystemdEscapeName(&buf, partition, true); virBufferAddLit(&buf, ".slice"); if (virBufferCheckError(&buf) < 0) @@ -130,7 +131,7 @@ char *virSystemdMakeMachineName(const char *name, virBufferAsprintf(&buf, "%s-%s-", username, drivername); } - virSystemdEscapeName(&buf, name); + virSystemdEscapeName(&buf, name, false); machinename = virBufferContentAndReset(&buf); cleanup: diff --git a/tests/virsystemdtest.c b/tests/virsystemdtest.c index 06fec5495bc2..49d37c2032ec 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\\x2dname"); + TEST_MACHINE("demo-name", "qemu-demo-name"); TEST_MACHINE("demo!name", "qemu-demo\\x21name"); - TEST_MACHINE(".demo", "qemu-\\x2edemo"); + TEST_MACHINE(".demo", "qemu-.demo"); TEST_MACHINE("bull\U0001f4a9", "qemu-bull\\xf0\\x9f\\x92\\xa9"); # define TESTS_PM_SUPPORT_HELPER(name, function) \ -- 2.6.3
On Fri, Nov 27, 2015 at 02:59:51PM +0100, Martin Kletzander wrote:
My previous fix in commit e24eda48cfae84a9003456b68eaf753a26123639 was incomplete, or rather more complete than it needed to be. The problem is that even though machined requires non-ASCII characters to be escaped does not mean that it needs to follow the exact same rules as unit files and services, therefore rendering our escape function, namely virSystemdEscapeName(), as overkill. Well, that should not be a problem, because if we escape more than needed, it will not break anything. However, that is not the case with current release of systemd because even though it requires some characters to be escaped, *any* escaped character will cause the function to fail. Even though that will be fixed, we need to make sure that machines, which were able to start before the commit mentioned above, are still able to start now. So this patch changes virSystemdEscapeName() to operate as the aforementioned overkill merely based on its new parameter. If that parameter is false, which only occurs in the latest call to this function from virSystemdMakeMachineName(), it will not escape characters that would cause the machine being unable to be started.
Or to put it simpler: Machine name escaping follows the same rules as serice name escape, except that '.' and '-' must not be escaped in machine names, due to a bug in systemd-machined.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1282846
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/util/virsystemd.c | 15 ++++++++------- tests/virsystemdtest.c | 4 ++-- 2 files changed, 10 insertions(+), 9 deletions(-)
diff --git a/src/util/virsystemd.c b/src/util/virsystemd.c index abd883c73844..257efaab7829 100644 --- a/src/util/virsystemd.c +++ b/src/util/virsystemd.c @@ -39,7 +39,8 @@ VIR_LOG_INIT("util.systemd");
Can we put a comment in about why we have different rules.
static void virSystemdEscapeName(virBufferPtr buf, - const char *name) + const char *name, + bool full_escape) { static const char hextable[16] = "0123456789abcdef";
@@ -57,7 +58,7 @@ static void virSystemdEscapeName(virBufferPtr buf, "ABCDEFGHIJKLMNOPQRSTUVWXYZ" \ ":-_.\\"
- if (*name == '.') { + if (full_escape && *name == '.') { ESCAPE(*name); name++; } @@ -65,7 +66,7 @@ static void virSystemdEscapeName(virBufferPtr buf, while (*name) { if (*name == '/') virBufferAddChar(buf, '-'); - else if (*name == '-' || + else if ((full_escape && *name == '-') || *name == '\\' || !strchr(VALID_CHARS, *name)) ESCAPE(*name); @@ -85,9 +86,9 @@ char *virSystemdMakeScopeName(const char *name, virBuffer buf = VIR_BUFFER_INITIALIZER;
virBufferAddLit(&buf, "machine-"); - virSystemdEscapeName(&buf, drivername); + virSystemdEscapeName(&buf, drivername, true); virBufferAddLit(&buf, "\\x2d"); - virSystemdEscapeName(&buf, name); + virSystemdEscapeName(&buf, name, true); virBufferAddLit(&buf, ".scope");
if (virBufferCheckError(&buf) < 0) @@ -104,7 +105,7 @@ char *virSystemdMakeSliceName(const char *partition) if (*partition == '/') partition++;
- virSystemdEscapeName(&buf, partition); + virSystemdEscapeName(&buf, partition, true); virBufferAddLit(&buf, ".slice");
if (virBufferCheckError(&buf) < 0) @@ -130,7 +131,7 @@ char *virSystemdMakeMachineName(const char *name, virBufferAsprintf(&buf, "%s-%s-", username, drivername); }
- virSystemdEscapeName(&buf, name); + virSystemdEscapeName(&buf, name, false);
machinename = virBufferContentAndReset(&buf); cleanup: diff --git a/tests/virsystemdtest.c b/tests/virsystemdtest.c index 06fec5495bc2..49d37c2032ec 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\\x2dname"); + TEST_MACHINE("demo-name", "qemu-demo-name"); TEST_MACHINE("demo!name", "qemu-demo\\x21name"); - TEST_MACHINE(".demo", "qemu-\\x2edemo"); + TEST_MACHINE(".demo", "qemu-.demo"); TEST_MACHINE("bull\U0001f4a9", "qemu-bull\\xf0\\x9f\\x92\\xa9");
# define TESTS_PM_SUPPORT_HELPER(name, function) \
ACK 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 :|
On Fri, Nov 27, 2015 at 02:05:06PM +0000, Daniel P. Berrange wrote:
On Fri, Nov 27, 2015 at 02:59:51PM +0100, Martin Kletzander wrote:
My previous fix in commit e24eda48cfae84a9003456b68eaf753a26123639 was incomplete, or rather more complete than it needed to be. The problem is that even though machined requires non-ASCII characters to be escaped does not mean that it needs to follow the exact same rules as unit files and services, therefore rendering our escape function, namely virSystemdEscapeName(), as overkill. Well, that should not be a problem, because if we escape more than needed, it will not break anything. However, that is not the case with current release of systemd because even though it requires some characters to be escaped, *any* escaped character will cause the function to fail. Even though that will be fixed, we need to make sure that machines, which were able to start before the commit mentioned above, are still able to start now. So this patch changes virSystemdEscapeName() to operate as the aforementioned overkill merely based on its new parameter. If that parameter is false, which only occurs in the latest call to this function from virSystemdMakeMachineName(), it will not escape characters that would cause the machine being unable to be started.
Or to put it simpler:
Machine name escaping follows the same rules as serice name escape, except that '.' and '-' must not be escaped in machine names, due to a bug in systemd-machined.
I used your version ...
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1282846
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/util/virsystemd.c | 15 ++++++++------- tests/virsystemdtest.c | 4 ++-- 2 files changed, 10 insertions(+), 9 deletions(-)
diff --git a/src/util/virsystemd.c b/src/util/virsystemd.c index abd883c73844..257efaab7829 100644 --- a/src/util/virsystemd.c +++ b/src/util/virsystemd.c @@ -39,7 +39,8 @@ VIR_LOG_INIT("util.systemd");
Can we put a comment in about why we have different rules.
and commented this function and pushed. Thanks.
static void virSystemdEscapeName(virBufferPtr buf, - const char *name) + const char *name, + bool full_escape) { static const char hextable[16] = "0123456789abcdef";
@@ -57,7 +58,7 @@ static void virSystemdEscapeName(virBufferPtr buf, "ABCDEFGHIJKLMNOPQRSTUVWXYZ" \ ":-_.\\"
- if (*name == '.') { + if (full_escape && *name == '.') { ESCAPE(*name); name++; } @@ -65,7 +66,7 @@ static void virSystemdEscapeName(virBufferPtr buf, while (*name) { if (*name == '/') virBufferAddChar(buf, '-'); - else if (*name == '-' || + else if ((full_escape && *name == '-') || *name == '\\' || !strchr(VALID_CHARS, *name)) ESCAPE(*name); @@ -85,9 +86,9 @@ char *virSystemdMakeScopeName(const char *name, virBuffer buf = VIR_BUFFER_INITIALIZER;
virBufferAddLit(&buf, "machine-"); - virSystemdEscapeName(&buf, drivername); + virSystemdEscapeName(&buf, drivername, true); virBufferAddLit(&buf, "\\x2d"); - virSystemdEscapeName(&buf, name); + virSystemdEscapeName(&buf, name, true); virBufferAddLit(&buf, ".scope");
if (virBufferCheckError(&buf) < 0) @@ -104,7 +105,7 @@ char *virSystemdMakeSliceName(const char *partition) if (*partition == '/') partition++;
- virSystemdEscapeName(&buf, partition); + virSystemdEscapeName(&buf, partition, true); virBufferAddLit(&buf, ".slice");
if (virBufferCheckError(&buf) < 0) @@ -130,7 +131,7 @@ char *virSystemdMakeMachineName(const char *name, virBufferAsprintf(&buf, "%s-%s-", username, drivername); }
- virSystemdEscapeName(&buf, name); + virSystemdEscapeName(&buf, name, false);
machinename = virBufferContentAndReset(&buf); cleanup: diff --git a/tests/virsystemdtest.c b/tests/virsystemdtest.c index 06fec5495bc2..49d37c2032ec 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\\x2dname"); + TEST_MACHINE("demo-name", "qemu-demo-name"); TEST_MACHINE("demo!name", "qemu-demo\\x21name"); - TEST_MACHINE(".demo", "qemu-\\x2edemo"); + TEST_MACHINE(".demo", "qemu-.demo"); TEST_MACHINE("bull\U0001f4a9", "qemu-bull\\xf0\\x9f\\x92\\xa9");
# define TESTS_PM_SUPPORT_HELPER(name, function) \
ACK
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 :|
participants (2)
-
Daniel P. Berrange -
Martin Kletzander