udevadm verify: add more checks

[ldv-alt]

• extend the check for conflicting expressions
• check token delimiters

[yuwata]

2023-03-25T00:45:22.7436907Z  424/1519 systemd:dist-check / udev-rules-check                                                   FAIL             0.01s   exit status 1
2023-03-25T00:45:22.7531626Z ――――――――――――――――――――――――――――――――――――― ✀  ―――――――――――――――――――――――――――――――――――――
2023-03-25T00:45:22.7531907Z stdout:
2023-03-25T00:45:22.7532016Z 
2023-03-25T00:45:22.7532124Z 34 udev rules files have been checked.
2023-03-25T00:45:22.7532345Z   Success: 33
2023-03-25T00:45:22.7532534Z   Fail:    1
2023-03-25T00:45:22.7532707Z stderr:
2023-03-25T00:45:22.7533083Z rules.d/60-persistent-storage.rules:45 No whitespace after comma
2023-03-25T00:45:22.7533479Z rules.d/60-persistent-storage.rules:55 No whitespace after comma
2023-03-25T00:45:22.7533869Z rules.d/60-persistent-storage.rules: udev rules check failed

The failures are caused by c5ba7a2, maybe missing space before the line continuation?

[ldv-alt]

centos8 error nothing provides python3.11dist(iniconfig) needed by python3.11-pytest-7.2.0-1.el8.noarch is unrelated.

[ldv-alt]

jammy-ppc64el errors are also unrelated:

TEST-29-PORTABLE:                   FAIL     (1212 s)
TEST-53-ISSUE-16347:                FAIL     (1202 s)
TEST-62-RESTRICT-IFACES:            FAIL     (1202 s)

[yuwata]

LGTM. Several coding style comments.

[keszybz]

LGTM, two suggestions about wording.

[yuwata]

Still LGTM.

[keszybz]

LGTM too.

fix grammar

That's a bit too strong ;) It was grammatically correct, it was just a bit off semantically. But it's fine, let's merge this.

[mwilck]

I just received a multipath-tools PR for this. I have to say I'm irritated. This PR introduces new syntax restrictions to udev rules that didn't exist before, in particular the rule to add whitespace after a comma. The multipath-tools PR tries to enforce this rule for commas before a line continuation, where it's non-obvious to me that the readability of the rules is improved at all.

I have nothing against systemd improving the readability of its own rules files, and using a test tool for verifying the added syntax rules. But I find it questionable to enforce such additional restrictions on other projects without even mentioning that they are new.

Side note: I did a quick test and actually found that (contrary to what the man page says) the tokenizer doesn't even enforce a comma, as it uses the required closing quote of the "value" part of the token as token delimiter:

KERNEL=="sda"PROGRAM="/bin/echo no comma"

works (just a remark; I know that the man page demands the comma).

[keszybz]

We're trying to make the rules as readable as possible for humans. The rules on what is actually accepted haven't changed. The changes that the verifier is suggesting should make the rules more readable.

That said, I think we could relax the check to accept a line continuation without a space. There is whitespace there, we just completely strip it off when consuming the continuation symbol.

@ldv-alt could we make the second patch in opensvc/multipath-tools#66 not necessary?

[mwilck]

We're trying to make the rules as readable as possible for humans. The rules on what is actually accepted haven't changed. The changes that the verifier is suggesting should make the rules more readable.

Ok. I agree, of course, that human-readable rules are a good thing. But from the multipath-tools PR, it was neither obvious that this syntax verification was a new feature, nor that it was stricter than the parser itself, and mainly targeted at improving readability.

Also, past experience would at least make me suspect that a future change might actually change what the parser accepts. If that isn't the case, fine with me.

[ldv-alt]

For the sake of completeness, here is the message I sent earlier to the mailing list:

As you probably know, udevd silently accepts much broader syntax, for
example, it doesn't need neither comma no whitespace between KEY=VALUE
expressions, and I doubt this will ever change in the future.

In contrast, `udevadm verify` is a tool that checks syntactic, semantic,
and style correctness of udev rules files.  It indeed expects whitespace
after a comma in udev rules - a style most of existing udev rules follow.

> Furthermore, there is actually whitespace after the comma in the code
> this patch changes, it just happens to be at the beginning of the
> following line, which your syntax check ignores.

When I saw the parser of udev rules used by udevd for the first time,
I was also surprised to find out that it discards all leading whitespace
regardless of line continuations.  As result, that whitespace is not
visible to the syntax check at all.  So yes, you are literally correct,
there is whitespace there, but most of existing udev rules add whitespace
between a comma and a backslash.

[ldv-alt]

That said, I think we could relax the check to accept a line continuation without a space. There is whitespace there, we just completely strip it off when consuming the continuation symbol.

@ldv-alt could we make the second patch in opensvc/multipath-tools#66 not necessary?

@keszybz, I thought about this before. Unfortunately, this issue is not as obvious as it looks from the first glance.
For example, udevd parses

ACTION=="\
         remove", GOTO="end"

as an equivalent to

ACTION=="remove", GOTO="end"

Both line continuations and leading whitespace removal are processed by the parser before parsing the line itself,
so it cannot differentiate between the kind of line continuation that happens inside tokens and other kinds of line continuation.

I'd rather not step into these murky waters unnecessarily.