[dm-devel] [PATCH] 11-dm-mpath.rules: fix warnings reported by udevadm verify

[dm-devel] [PATCH] 11-dm-mpath.rules: fix warnings reported by udevadm verify

[Dmitry V. Levin]

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 <ldv@strace.io>
---
 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

Re: [dm-devel] [PATCH] 11-dm-mpath.rules: fix warnings reported by udevadm verify

[Martin Wilck]

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 <ldv@strace.io>

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

Re: [dm-devel] [PATCH] 11-dm-mpath.rules: fix warnings reported by udevadm verify

[Dmitry V. Levin]

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 <ldv@strace.io>
> 
> 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

[Martin Wilck]

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 <ldv@strace.io>
> > 
> > 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 <mwilck@suse.com>

--
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

[Dmitry V. Levin]

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