Re: [OE-core][dunfell 3/5] systemd-systemctl: fix instance template WantedBy symlink construction

[Richard Purdie <richard.purdie@linuxfoundation.org>]

On Thu, 2023-06-15 at 20:40 +0000, Peter Kjellerstedt wrote:
> > -----Original Message-----
> > From: openembedded-core@lists.openembedded.org <openembedded-
> > core@lists.openembedded.org> On Behalf Of Steve Sakoman
> > Sent: den 14 juni 2023 16:05
> > To: openembedded-core@lists.openembedded.org
> > Subject: [OE-core][dunfell 3/5] systemd-systemctl: fix instance
> > template
> > WantedBy symlink construction
> > 
> > From: Martin Siegumfeldt <mns@gomspace.com>
> > 
> > Fix issue of the below instance template systemd service dependency
> > 
> > [Install]
> > WantedBy=svc-wants@%i.service
> > 
> > creating the symlink (instance "a" example)
> > 
> > /etc/systemd/system/svc-
> > wants@%i.service.wants/svc-wanted-by@a.service
> > 
> > which should be
> > 
> > /etc/systemd/system/svc-wants@a.service.wants
> > /svc-wanted-by@a.service
> > 
> > as implemented by this change.
> > 
> > The functionality appears regressed just after "thud" baseline when
> > the
> > logic was refactored from shell script into python (commit
> > 925e30cb104ece7bfa48b78144e758a46dc9ec3f)
> > 
> > (From OE-Core rev: 308397f0bb3d6f3d4e9ec2c6a10823184049c9b5)
> > 
> > Signed-off-by: Martin Siegumfeldt <mns@gomspace.com>
> > Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> > Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
> > Signed-off-by: Steve Sakoman <steve@sakoman.com>
> > (cherry picked from commit
> > 372b29c8ad270d4d430c26a4e614976c7029afaf)
> > Signed-off-by: Steve Sakoman <steve@sakoman.com>
> > ---
> >  .../systemd/systemd-systemctl/systemctl             | 13
> > ++++++++++---
> >  1 file changed, 10 insertions(+), 3 deletions(-)
> > 
> > diff --git a/meta/recipes-core/systemd/systemd-systemctl/systemctl
> > b/meta/recipes-core/systemd/systemd-systemctl/systemctl
> > index 6aa2e20465..577c373181 100755
> > --- a/meta/recipes-core/systemd/systemd-systemctl/systemctl
> > +++ b/meta/recipes-core/systemd/systemd-systemctl/systemctl
> > @@ -182,12 +182,19 @@ class SystemdUnit():
> > 
> >          raise SystemdUnitNotFoundError(self.root, unit)
> > 
> > -    def _process_deps(self, config, service, location, prop,
> > dirstem):
> > +    def _process_deps(self, config, service, location, prop,
> > dirstem, instance):
> >          systemdir = self.root / SYSCONFDIR / "systemd" / "system"
> > 
> >          target = ROOT / location.relative_to(self.root)
> >          try:
> >              for dependent in config.get('Install', prop):
> > +                # determine whether or not dependent is a template
> > with an actual
> > +                # instance (i.e. a '@%i')
> > +                dependent_is_template =
> > re.match(r"[^@]+@(?P<instance>[^\.]*)\.", dependent)
> > +                if dependent_is_template:
> > +                    # if so, replace with the actual instance to
> > achieve
> > +                    #
> > svc-wants@a.service.wants/svc-wanted-by@a.service
> > +                    dependent =
> > re.sub(dependent_is_template.group('instance'), instance,
> > dependent, 1)
> >                  wants = systemdir / "{}.{}".format(dependent,
> > dirstem) / service
> >                  add_link(wants, target)
> > 
> > @@ -227,8 +234,8 @@ class SystemdUnit():
> >          else:
> >              service = self.unit
> > 
> > -        self._process_deps(config, service, path, 'WantedBy',
> > 'wants')
> > -        self._process_deps(config, service, path, 'RequiredBy',
> > 'requires')
> > +        self._process_deps(config, service, path, 'WantedBy',
> > 'wants', instance)
> > +        self._process_deps(config, service, path, 'RequiredBy',
> > 'requires', instance)
> > 
> >          try:
> >              for also in config.get('Install', 'Also'):
> > --
> > 2.34.1
> 
> You might want to be careful with this one. It broke systemctl for us
> when it 
> hit Mickledore and we have had to backport the version from Langdale
> for now.
> 
> The problem is that it does not handle non-instanced units depending
> on 
> an instanced unit. In our case we have a unit foo.service that
> contains 
> a RequiredBy=bar@0.service, which causes an error like this:
> 
> Traceback (most recent call last):
>   File ".../recipe-sysroot-native/usr/bin/systemctl", line 366, in
> <module>
>     main()
>   File ".../recipe-sysroot-native/usr/bin/systemctl", line 352, in
> main
>     SystemdUnit(root, service).enable()
>   File ".../recipe-sysroot-native/usr/bin/systemctl", line 250, in
> enable
>     self._process_deps(config, service, path, 'WantedBy', 'wants',
> instance)
>   File ".../recipe-sysroot-native/usr/bin/systemctl", line 210, in
> _process_deps
>     dependent = re.sub(dependent_is_template.group('instance'),
> instance, dependent, 1)
>                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> ^^^^^^^^^^^^^^^^^^
>   File ".../recipe-sysroot-native/usr/lib/python3.11/re/__init__.py",
> line 185, in sub
>     return _compile(pattern, flags).sub(repl, string, count)
>            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>   File ".../recipe-sysroot-native/usr/lib/python3.11/re/__init__.py",
> line 317, in _subx
>     template = _compile_repl(template, pattern)
>                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>   File ".../recipe-sysroot-native/usr/lib/python3.11/re/__init__.py",
> line 308, in _compile_repl
>     return _parser.parse_template(repl, pattern)
>            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>   File ".../recipe-sysroot-native/usr/lib/python3.11/re/_parser.py",
> line 1000, in parse_template
>     s = Tokenizer(source)
>         ^^^^^^^^^^^^^^^^^
>   File ".../recipe-sysroot-native/usr/lib/python3.11/re/_parser.py",
> line 226, in __init__
>     string = str(string, 'latin1')
>              ^^^^^^^^^^^^^^^^^^^^^
> TypeError: decoding to str: need a bytes-like object, NoneType found
> 
> There are probably other cases too when different kinds of units are 
> involved (at least as far as I could gather from the systemd manual).
> 
> We have an internal issue to fix this, but I am going on vacation on 
> Monday, so I will unfortunately not have any time to look at it until
> after summer.

There is a patch in master recently which may help?

Cheers,

Richard