Re: [libvirt] [PATCH] systemd: Escape only needed characters for machined

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(a)redhat.com&gt; > --- > 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 :|