Re: [dm-devel] [PATCH] 11-dm-mpath.rules: fix warnings reported by udevadm verify
On Mon, May 22, 2023 at 06:11:14PM +0200, Martin Wilck wrote: [...] > I see. I'll apply this patch then (and the other one with the missing > comma, too), but unless you object, I'll add a note to the > commit message explaining that this for improving readability > and coding style compliance. I want to avoid the impression that the > existing code is technically wrong, which it isn't. That's fine with me, thanks. -- ldv -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH] 11-dm-mpath.rules: fix warnings reported by udevadm verify
On Mon, May 22, 2023 at 12:19:04PM +0200, Martin Wilck wrote: > On Sat, 2023-05-20 at 01:33 +0300, Dmitry V. Levin wrote: > > Fix the following warnings reported by udevadm verify: > > > > multipath/11-dm-mpath.rules:18 Whitespace after comma is expected. > > multipath/11-dm-mpath.rules:73 Whitespace after comma is expected. > > multipath/11-dm-mpath.rules:73 Whitespace after comma is expected. > > multipath/11-dm-mpath.rules:78 Whitespace after comma is expected. > > multipath/11-dm-mpath.rules:78 Whitespace after comma is expected. > > multipath/11-dm-mpath.rules:78 Whitespace after comma is expected. > > multipath/11-dm-mpath.rules: udev rules check failed > > > > Signed-off-by: Dmitry V. Levin > > Maybe you should have mentioned that you've just invented this syntax > rule yourself (https://github.com/systemd/systemd/pull/26980). > I see no requirement for adding whitespace after a comma in the udev > man page. > > Is this an attempt to change the udev rule syntax retroactively? 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 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH] 11-dm-mpath.rules: fix warnings reported by udevadm verify
On Mon, 2023-05-22 at 13:49 +0300, Dmitry V. Levin wrote: > On Mon, May 22, 2023 at 12:19:04PM +0200, Martin Wilck wrote: > > On Sat, 2023-05-20 at 01:33 +0300, Dmitry V. Levin wrote: > > > Fix the following warnings reported by udevadm verify: > > > > > > multipath/11-dm-mpath.rules:18 Whitespace after comma is > > > expected. > > > multipath/11-dm-mpath.rules:73 Whitespace after comma is > > > expected. > > > multipath/11-dm-mpath.rules:73 Whitespace after comma is > > > expected. > > > multipath/11-dm-mpath.rules:78 Whitespace after comma is > > > expected. > > > multipath/11-dm-mpath.rules:78 Whitespace after comma is > > > expected. > > > multipath/11-dm-mpath.rules:78 Whitespace after comma is > > > expected. > > > multipath/11-dm-mpath.rules: udev rules check failed > > > > > > Signed-off-by: Dmitry V. Levin > > > > Maybe you should have mentioned that you've just invented this > > syntax > > rule yourself (https://github.com/systemd/systemd/pull/26980). > > I see no requirement for adding whitespace after a comma in the > > udev > > man page. > > > > Is this an attempt to change the udev rule syntax retroactively? > > 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. Ok, that answers what I was asking. Thanks. Btw I did not know that commas could be left out, I found out while I looked into this issue. > In contrast, `udevadm verify` is a tool that checks syntactic, > semantic, > and style correctness of udev rules files. So this is about style and readability. Fair enough. It would have been nice to mention that in the commit message. > It indeed expects whitespace > after a comma in udev rules - a style most of existing udev rules > follow. The multipath-tools rules do, too; they just don't add whitespace between the comma and the line continuation marker '\'. > but most of existing udev rules add whitespace > between a comma and a backslash. I see. I'll apply this patch then (and the other one with the missing comma, too), but unless you object, I'll add a note to the commit message explaining that this for improving readability and coding style compliance. I want to avoid the impression that the existing code is technically wrong, which it isn't. Reviewed-by: Martin Wilck -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH] 11-dm-mpath.rules: fix warnings reported by udevadm verify
On Sat, 2023-05-20 at 01:33 +0300, Dmitry V. Levin wrote: > Fix the following warnings reported by udevadm verify: > > multipath/11-dm-mpath.rules:18 Whitespace after comma is expected. > multipath/11-dm-mpath.rules:73 Whitespace after comma is expected. > multipath/11-dm-mpath.rules:73 Whitespace after comma is expected. > multipath/11-dm-mpath.rules:78 Whitespace after comma is expected. > multipath/11-dm-mpath.rules:78 Whitespace after comma is expected. > multipath/11-dm-mpath.rules:78 Whitespace after comma is expected. > multipath/11-dm-mpath.rules: udev rules check failed > > Signed-off-by: Dmitry V. Levin Maybe you should have mentioned that you've just invented this syntax rule yourself (https://github.com/systemd/systemd/pull/26980). I see no requirement for adding whitespace after a comma in the udev man page. Is this an attempt to change the udev rule syntax retroactively? 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. Regards, Martin -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH] 11-dm-mpath.rules: fix warnings reported by udevadm verify
Fix the following warnings reported by udevadm verify:
multipath/11-dm-mpath.rules:18 Whitespace after comma is expected.
multipath/11-dm-mpath.rules:73 Whitespace after comma is expected.
multipath/11-dm-mpath.rules:73 Whitespace after comma is expected.
multipath/11-dm-mpath.rules:78 Whitespace after comma is expected.
multipath/11-dm-mpath.rules:78 Whitespace after comma is expected.
multipath/11-dm-mpath.rules:78 Whitespace after comma is expected.
multipath/11-dm-mpath.rules: udev rules check failed
Signed-off-by: Dmitry V. Levin
---
multipath/11-dm-mpath.rules | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/multipath/11-dm-mpath.rules b/multipath/11-dm-mpath.rules
index d191ae8d..c339f521 100644
--- a/multipath/11-dm-mpath.rules
+++ b/multipath/11-dm-mpath.rules
@@ -14,7 +14,7 @@ ENV{.MPATH_DEVICE_READY_OLD}="$env{MPATH_DEVICE_READY}"
# multipath sets DM_SUBSYSTEM_UDEV_FLAG2 when it reloads a
# table with no active devices. If this happens, mark the
# device not ready
-ENV{DM_SUBSYSTEM_UDEV_FLAG2}=="1", ENV{MPATH_DEVICE_READY}="0",\
+ENV{DM_SUBSYSTEM_UDEV_FLAG2}=="1", ENV{MPATH_DEVICE_READY}="0", \
GOTO="mpath_action"
# If the last path has failed mark the device not ready
@@ -68,13 +68,13 @@ ENV{MPATH_DEVICE_READY}=="0", ENV{DM_NOSCAN}="1"
# Also skip all foreign rules if no path is available.
# Remember the original value of DM_DISABLE_OTHER_RULES_FLAG
# and restore it back once we have at least one path available.
-ENV{MPATH_DEVICE_READY}=="0", ENV{.MPATH_DEVICE_READY_OLD}=="1",\
- ENV{DM_DISABLE_OTHER_RULES_FLAG_OLD}=="",\
+ENV{MPATH_DEVICE_READY}=="0", ENV{.MPATH_DEVICE_READY_OLD}=="1", \
+ ENV{DM_DISABLE_OTHER_RULES_FLAG_OLD}=="", \
ENV{DM_DISABLE_OTHER_RULES_FLAG_OLD}="$env{DM_UDEV_DISABLE_OTHER_RULES_FLAG}"
ENV{MPATH_DEVICE_READY}=="0", ENV{DM_UDEV_DISABLE_OTHER_RULES_FLAG}="1"
-ENV{MPATH_DEVICE_READY}!="0", ENV{.MPATH_DEVICE_READY_OLD}=="0",\
-
ENV{DM_UDEV_DISABLE_OTHER_RULES_FLAG}="$env{DM_DISABLE_OTHER_RULES_FLAG_OLD}",\
- ENV{DM_DISABLE_OTHER_RULES_FLAG_OLD}="",\
+ENV{MPATH_DEVICE_READY}!="0", ENV{.MPATH_DEVICE_READY_OLD}=="0", \
+
ENV{DM_UDEV_DISABLE_OTHER_RULES_FLAG}="$env{DM_DISABLE_OTHER_RULES_FLAG_OLD}", \
+ ENV{DM_DISABLE_OTHER_RULES_FLAG_OLD}="", \
ENV{DM_ACTIVATION}="1", ENV{MPATH_UNCHANGED}="0"
# The code to check multipath state ends here. We need to set
--
ldv
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
