Control: tag -1 + confirmed upstream
On Tue, Sep 12, 2023 at 10:11:58PM +0200, Alexandre Detiste wrote:
> In v1.x the trailing "> /dev/null" was purposely chopped of the end of the
> line
> because there wasn't an OnSucces= handler anyway and the goal
> was to further parse the bash-one liner.
>
> Some of the brittle bash-parsing has already been dropped of from 2.x.
Concretely, we already killed stuff like this:
if (len(self.command) == 6 and
self.command[0] == '[' and
self.command[1] in ['-x','-f','-e'] and
self.command[2] == self.command[5] and
self.command[3] == ']' and
self.command[4] == '&&' ):
self.testremoved = self.command[2]
self.command = self.command[5:]
if (len(self.command) == 5 and
self.command[0] == 'test' and
self.command[1] in ['-x','-f','-e'] and
self.command[2] == self.command[4] and
self.command[3] == '&&' ):
self.testremoved = self.command[2]
self.command = self.command[4:]
in the C++ rewrite as "brittle damage" ‒
https://github.com/systemd-cron/systemd-cron/pull/101#issuecomment-1673266966.
The only substitutions left in decode_command() that do explicit damage
to the line are
if(auto pgm = this->which(this->command[0]); pgm && *pgm !=
this->command[0]) {
if(!this->command.command0)
++this->command.command.b;
this->command.command0 = std::move(pgm);
}
which dereferences the first word so we can avoid going through
/bin/sh copied-line.sh for single-word lines
(which, I've just realised, breaks when stuff lands in/is removed from,
say /usr/local/bin, and systemd isn't reloaded ‒ imagine
cp /bin/id /usr/local/bin
systemd daemon-reload
rm /usr/local/bin/id
‒ and now a "@daily id" job no longer works!
since it tries executing /usr/local/bin/id!;
IMO we should trim the whole function before
if(!std::binary_search(std::begin(KSH_SHELLS), std::end(KSH_SHELLS),
vore::basename(this->shell)))
and axe KSH_SHELLS, and always generate a scriptlet)
and
if(this->command.size() > 2 && this->command.command.e[-2] ==
">"sv && this->command.command.e[-1] == "/dev/null"sv)
this->command.command.e -= 2;
if(this->command.size() > 1 && this->command.command.e[-1] ==
">/dev/null"sv)
this->command.command.e -= 1;
which you're seeing.
> We can either [1] remove the code that chop of the /dev/null
I agree. This had, maybe, potentially, some minute degree of merit
before we did normal cron-style mail.
Now that we behave like cron, it's just knowing-better-than-the-user.
> or [2] isolate it to only run when CRON_MAIL_SUCCESS=never
> (a setting that allow users to revert to the previous behaviour).
The user has all the controls to configure this precisely as they need now,
this is clearly confusing, since it goes against the explicit and overt
user configuration.
> I personally prefer "1" because of my own past habits
Agree, just kill it.
> even if "2" will look cleaner from a new point of view..
I don't think it is, really. Respect what the user wrote,
don't do needless magic (like this), and users will be happy.
Maybe add a changelog note that
"Fix: sd-cron-g no longer damages the line by stripping >[ ]/dev/null".
signature.asc
Description: PGP signature
