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

[Martin Kletzander]

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 at 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 :|
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20151127/b9ee448e/attachment-0001.sig>