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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx>
> ---
>  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 :|

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]