[PATCH] Do not support dmsetup udev{flags, complete, complete_all, cookies} when udev_sync is disabled and tiny fix for udevcomplete

[PATCH] Do not support dmsetup udev{flags, complete, complete_all, cookies} when udev_sync is disabled and tiny fix for udevcomplete

[Peter Rajnoha]

Just a tiny cleanup - we should be consistent here and disable dmsetup udev{flags,complete,
complete_all,cookies} commands if udev_sync is disabled, not udevcomplete_all and
udevcookies only.

This patch also includes a tiny fix for one specific situation and that is when using
automatically generated flags to control udev rules (to switch them off) while the
software does not use udev_sync interface yet - we have cookies with flags, but with
no semaphore identifier (it is zero). So dmsetup udevcomplete should detect this
and it should not try to find such semaphore. It should not show any errors that
the semaphore with identifier "0" could not be found...

Peter


diff --git a/tools/dmsetup.c b/tools/dmsetup.c
index a7f1d25..69685e9 100644
--- a/tools/dmsetup.c
+++ b/tools/dmsetup.c
@@ -758,6 +758,40 @@ static int _splitname(int argc, char **argv, void *data __attribute((unused)))
 	return r;
 }
 
+#ifndef UDEV_SYNC_SUPPORT
+
+static const char _cmd_not_supported[] = "Command not supported. Recompile with \"--enable-udev-sync\" to enable.";
+
+static int _udevflags(int args, char **argv, void *data __attribute((unused)))
+{
+	log_error(_cmd_not_supported);
+
+	return 0;
+}
+
+static int _udevcomplete(int argc, char **argv, void *data __attribute((unused)))
+{
+	log_error(_cmd_not_supported);
+
+	return 0;
+}
+
+static int _udevcomplete_all(int argc __attribute((unused)), char **argv __attribute((unused)), void *data __attribute((unused)))
+{
+	log_error(_cmd_not_supported);
+
+	return 0;
+}
+
+static int _udevcookies(int argc __attribute((unused)), char **argv __attribute((unused)), void *data __attribute((unused)))
+{
+	log_error(_cmd_not_supported);
+
+	return 0;
+}
+
+#else	/* UDEV_SYNC_SUPPORT */
+
 static uint32_t _get_cookie_value(char *str_value)
 {
 	unsigned long int value;
@@ -821,32 +855,22 @@ static int _udevcomplete(int argc, char **argv, void *data __attribute((unused))
 	if (!(cookie = _get_cookie_value(argv[1])))
 		return 0;
 
-	/* strip flags from the cookie and use cookie magic instead */
-	cookie = (cookie & ~DM_UDEV_FLAGS_MASK) |
-		  (DM_COOKIE_MAGIC << DM_UDEV_FLAGS_SHIFT);
-
-	return dm_udev_complete(cookie);
-}
-
-#ifndef UDEV_SYNC_SUPPORT
-static const char _cmd_not_supported[] = "Command not supported. Recompile with \"--enable-udev-sync\" to enable.";
-
-static int _udevcomplete_all(int argc __attribute((unused)), char **argv __attribute((unused)), void *data __attribute((unused)))
-{
-	log_error(_cmd_not_supported);
-
-	return 0;
-}
+	/*
+	 * Strip flags from the cookie and use cookie magic instead.
+	 * If the cookie has non-zero prefix and the base is zero then
+	 * this one carries flags to control udev rules only and it is
+	 * not meant to be for notification (e.g. setting the flags
+	 * automatically to disable the rules for software that does
+	 * not use udev synchronisation interface).
+	 */
+	if (!(cookie &= ~DM_UDEV_FLAGS_MASK))
+		return 1;
 
-static int _udevcookies(int argc __attribute((unused)), char **argv __attribute((unused)), void *data __attribute((unused)))
-{
-	log_error(_cmd_not_supported);
+	cookie |= DM_COOKIE_MAGIC << DM_UDEV_FLAGS_SHIFT;
 
-	return 0;
+	return dm_udev_complete(cookie);
 }
 
-#else	/* UDEV_SYNC_SUPPORT */
-
 static char _yes_no_prompt(const char *prompt, ...)
 {
 	int c = 0, ret = 0;

[PATCH] Do not support dmsetup udev{flags, complete, complete_all, cookies} when udev_sync is disabled and tiny fix for udevcomplete

[Bastian Blank]

On Wed, Nov 04, 2009 at 11:55:03AM +0100, Peter Rajnoha wrote:
> Just a tiny cleanup - we should be consistent here and disable dmsetup udev{flags,complete,
> complete_all,cookies} commands if udev_sync is disabled, not udevcomplete_all and
> udevcookies only.

What exactly are you trying to do? udevflags and udevcomplete is used in
your udev rules, which can be used without udev sync enabled at all.

Bastian

-- 
Our missions are peaceful -- not for conquest.  When we do battle, it
is only because we have no choice.
		- Kirk, "The Squire of Gothos", stardate 2124.5

[PATCH] Do not support dmsetup udev{flags, complete, complete_all, cookies} when udev_sync is disabled and tiny fix for udevcomplete

[Peter Rajnoha]

On 11/04/2009 12:25 PM, Bastian Blank wrote:
> On Wed, Nov 04, 2009 at 11:55:03AM +0100, Peter Rajnoha wrote:
>> Just a tiny cleanup - we should be consistent here and disable dmsetup udev{flags,complete,
>> complete_all,cookies} commands if udev_sync is disabled, not udevcomplete_all and
>> udevcookies only.
> 
> What exactly are you trying to do? udevflags and udevcomplete is used in
> your udev rules, which can be used without udev sync enabled at all.
> 

When udev_sync is disabled, there's no cookie set and propagated and no flags are set
as well (dm_task_set_cookie is just a bogus function in this situation). There's no
DM_COOKIE in the udev environment then. We have tests in the rules if DM_COOKIE exists
and udevcomplete and udevflags is called just in case we have this set since it depends
on it.

However, even without DM_COOKIE env var, the rules themselves try to set the flags
directly (or at least try to mimic that, but we can't do that 100% from within udev
rules only - that's why I added the flag support in libdevmapper directly...).

The same logic applies when using old kernels < 2.6.31 (simply because these kernels
don't set DM_COOKIE@all...)

So this is when libdevmapper is compiled *without* udev_sync.

---

Another situation is when libdevmapper is compiled *with* udev_sync and software
using libdevmapper does not make use of the synchronisation interface - although the
software itself does not call dm_task_set_cookie and does not set any flags, we
can detect this and we set DM_UDEV_DISABLE_DM_RULES_FLAG and DM_UDEV_DISABLE_SUBSYSTEM_RULES_FLAG
directly in libdevmapper, so we can avoid unexpected udev/libdevmapper races when
dealing with node and symlink creation (this is also the situation in which we happen
to have the base of the cookie zero and the prefix non-zero - which is why I need a tiny
cleanup for udevcomplete I propose within this patch as well :) ).

This applies until someone *directly* says "I'm OK with these races" - this is done by
calling "dm_udev_set_sync_support(0)" (the same happens when using dmsetup/lvm commands
with "--noudevsync" option - it just bypasses udev_sync).

(..sorry for being a little chatty here, but I think this is something that may
be of interest for more people.. So I think it's good to mention the context of this..)

But using the udev rules with udev_sync enabled is the recommended way.

Anyway, thanks for the question - this was a good one, really!

Peter

[PATCH] Do not support dmsetup udev{flags, complete, complete_all, cookies} when udev_sync is disabled and tiny fix for udevcomplete

[Peter Rajnoha]

On 11/04/2009 01:32 PM, Peter Rajnoha wrote:
> On 11/04/2009 12:25 PM, Bastian Blank wrote:
>> On Wed, Nov 04, 2009 at 11:55:03AM +0100, Peter Rajnoha wrote:
>>> Just a tiny cleanup - we should be consistent here and disable dmsetup udev{flags,complete,
>>> complete_all,cookies} commands if udev_sync is disabled, not udevcomplete_all and
>>> udevcookies only.
>>
>> What exactly are you trying to do? udevflags and udevcomplete is used in
>> your udev rules, which can be used without udev sync enabled at all.
>>
> 
> When udev_sync is disabled, there's no cookie set and propagated and no flags are set
> as well (dm_task_set_cookie is just a bogus function in this situation).

Well, upon further thought, maybe it would be better to still keep the flag support
even when udev_sync is disabled (either totally by not compiling it in or by using
the --noudevsync option). If there are important devices for which the rules should
be skipped, we should do this no matter what the udev_sync state is -enabled or disabled.

This would provide better handling when udev rules are installed, but udev_sync is
not compiled in (although this is not recommended). Also, the flags will work even when
--noudevsync option is selected, so we don't lose important flags then if there are any
(however, it will still require kernel >= 2.6.31).

Anyway, the cleanup should be made one way or the other. The patch for the second
alternative would be quite simple (with comments added to explain the situation):


diff --git a/libdm/libdevmapper.h b/libdm/libdevmapper.h
index ac0a758..8f9b5b5 100644
--- a/libdm/libdevmapper.h
+++ b/libdm/libdevmapper.h
@@ -1032,6 +1032,10 @@ void dm_report_field_set_value(struct dm_report_field *field, const void *value,
  * of udev rules we use by decoding the cookie prefix. When doing the
  * notification, we replace the cookie prefix with DM_COOKIE_MAGIC,
  * so we notify the right semaphore.
+ * It is still possible to use cookies for passing the flags to udev
+ * rules even when udev_sync is disabled. The base part of the cookie
+ * will be zero (there's no notification semaphore) and prefix will be
+ * set then. However, having udev_sync enabled is highly recommended.
  */
 #define DM_COOKIE_MAGIC 0x0D4D
 #define DM_UDEV_FLAGS_MASK 0xFFFF0000
@@ -1076,7 +1080,7 @@ void dm_report_field_set_value(struct dm_report_field *field, const void *value,
 int dm_cookie_supported(void);
 
 /*
- * Udev notification functions.
+ * Udev synchronisation functions.
  */
 void dm_udev_set_sync_support(int sync_with_udev);
 int dm_udev_get_sync_support(void);
diff --git a/libdm/libdm-common.c b/libdm/libdm-common.c
index 121f020..43ef63a 100644
--- a/libdm/libdm-common.c
+++ b/libdm/libdm-common.c
@@ -886,6 +886,8 @@ int dm_udev_get_sync_support(void)
 
 int dm_task_set_cookie(struct dm_task *dmt, uint32_t *cookie, uint16_t flags)
 {
+	if (dm_cookie_supported())
+		dmt->event_nr = flags << DM_UDEV_FLAGS_SHIFT;
 	*cookie = 0;
 
 	return 1;
@@ -1141,8 +1143,11 @@ int dm_task_set_cookie(struct dm_task *dmt, uint32_t *cookie, uint16_t flags)
 {
 	int semid;
 
+	if (dm_cookie_supported())
+		dmt->event_nr = flags << DM_UDEV_FLAGS_SHIFT;
+
 	if (!dm_udev_get_sync_support()) {
-		dmt->event_nr = *cookie = 0;
+		*cookie = 0;
 		return 1;
 	}
 
@@ -1159,8 +1164,7 @@ int dm_task_set_cookie(struct dm_task *dmt, uint32_t *cookie, uint16_t flags)
 		goto bad;
 	}
 
-	dmt->event_nr = (~DM_UDEV_FLAGS_MASK & *cookie) |
-			(flags << DM_UDEV_FLAGS_SHIFT);
+	dmt->event_nr |= ~DM_UDEV_FLAGS_MASK & *cookie;
 	dmt->cookie_set = 1;
 
 	log_debug("Udev cookie 0x%" PRIx32 " (semid %d) assigned to dm_task "
diff --git a/tools/dmsetup.c b/tools/dmsetup.c
index 186f8f0..888edff 100644
--- a/tools/dmsetup.c
+++ b/tools/dmsetup.c
@@ -843,9 +843,17 @@ static int _udevcomplete(int argc, char **argv, void *data __attribute((unused))
 	if (!(cookie = _get_cookie_value(argv[1])))
 		return 0;
 
-	/* strip flags from the cookie and use cookie magic instead */
-	cookie = (cookie & ~DM_UDEV_FLAGS_MASK) |
-		  (DM_COOKIE_MAGIC << DM_UDEV_FLAGS_SHIFT);
+	/*
+	 * Strip flags from the cookie and use cookie magic instead.
+	 * If the cookie has non-zero prefix and the base is zero then
+	 * this one carries flags to control udev rules only and it is
+	 * not meant to be for notification. Return with success in this
+	 * situation.
+	 */
+	if (!(cookie &= ~DM_UDEV_FLAGS_MASK))
+		return 1;
+
+	cookie |= DM_COOKIE_MAGIC << DM_UDEV_FLAGS_SHIFT;
 
 	return dm_udev_complete(cookie);
 }