[systemd-devel] tmpfiles.d specifier support on "argument" when operating on files

[Zbigniew Jędrzejewski-Szmek]

On Mon, Mar 02, 2015 at 08:25:47PM +0100, Zbigniew Jędrzejewski-Szmek wrote:
> On Wed, Feb 18, 2015 at 06:17:17PM -0300, Cristian Rodríguez wrote:
> > El 18/02/15 a las 07:10, Lennart Poettering escribió:
> > >On Tue, 17.02.15 17:35, Cristian Rodríguez (crrodriguez at opensuse.org) wrote:
> > >
> > >Please fix this for all arguments, not just symlinks.
> > >
> > >>diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c
> > >>index c948d4d..1b35b8e 100644
> > >>--- a/src/tmpfiles/tmpfiles.c
> > >>+++ b/src/tmpfiles/tmpfiles.c
> > >>@@ -1590,6 +1590,12 @@ static int parse_line(const char *fname, unsigned line, const char *buffer) {
> > >>                          i.argument = strappend("/usr/share/factory/", i.path);
> > >>                          if (!i.argument)
> > >>                                  return log_oom();
> > >>+                } else {
> > >>+                    r = specifier_printf(i.argument,
> > >>specifier_table, NULL, &i.argument);
> > >
> > >Here's a memory leak, you need to free the old i.argument.
> > >
> > >Indentation! Please have a look at CODING_STYLE. You need to indent by
> > >8ch. 4ch indenting is not acceptable.
> > >
> > >>+                    if (r < 0) {
> > >>+                        log_error("[%s:%u] Failed to replace specifiers: %s", fname, line, path);
> > >>+                        return r;
> > >>+                    }
> > >
> > 
> > HI again:
> > 
> > Is the attached version cool for you ?
> Hi,
> 
> sorry for the delay. We seem to have trouble getting all patches
> reviewed recently.  Hopefully this is just
> conferences/vacations/fedora-alpha-releases, and things will
> return to normal.
> 
> > From ee8e4f440def745b6f0655b897e65d48321e46c5 Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Cristian=20Rodr=C3=ADguez?= <crrodriguez at opensuse.org>
> > Date: Wed, 18 Feb 2015 18:09:16 -0300
> > Subject: [PATCH] tmpfiles: implement specifier substitution for file
> >  "argument"
> > 
> > Only for L and C types where it makes sense.
> > ---
> >  src/tmpfiles/tmpfiles.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c
> > index 88ba7e4..6de477b 100644
> > --- a/src/tmpfiles/tmpfiles.c
> > +++ b/src/tmpfiles/tmpfiles.c
> > @@ -1568,6 +1568,18 @@ static int parse_line(const char *fname, unsigned line, const char *buffer) {
> >                  }
> >          }
> >  
> > +        if(IN_SET(i.type, CREATE_SYMLINK, COPY_FILES)) {
> Please add a space after if and for...
> 
> > +                if(i.argument) {
> > +                        _cleanup_free_ char *resolved_iarg = NULL;
> ...and empty line after variable declarations.
> 
> > +                        r = specifier_printf(i.argument, specifier_table, NULL, &resolved_iarg);
> > +                        if(r < 0)
> > +                                return log_error_errno(r, "[%s:%u] Failed to replace specifiers: %s", fname, line, path);
> > +                        r = free_and_strdup(&i.argument, resolved_iarg);
> There's no need to to use free_and_strdup here. resolved_arg was just
> newly allocated, so it can be used without any strdup.
> 
> > +                        if(r < 0)
> > +                                return log_oom();
> > +                }
> > +        }
> > +
> >          switch (i.type) {
> 
> Otherwise looks OK.

Ping?

Zbyszek