diff mbox series

[v3,8/8] udev: Move udev_block() and udev_unblock() into udev.c

Message ID 20230202112706.14228-9-mateusz.grzonka@intel.com (mailing list archive)
State Changes Requested, archived
Delegated to: Jes Sorensen
Headers show
Series Mdmonitor refactor and udev event handling improvements | expand

Commit Message

Mateusz Grzonka Feb. 2, 2023, 11:27 a.m. UTC 
Add kernel style comments and better error handling.

Signed-off-by: Mateusz Grzonka <mateusz.grzonka@intel.com>
---
 Create.c |  1 +
 lib.c    | 29 -----------------------------
 mdadm.h  |  2 --
 mdopen.c | 12 ++++++------
 udev.c   | 44 ++++++++++++++++++++++++++++++++++++++++++++
 udev.h   |  2 ++
 6 files changed, 53 insertions(+), 37 deletions(-)

Comments

Coly Li Feb. 23, 2023, 1:41 p.m. UTC | #1 
On Thu, Feb 02, 2023 at 12:27:06PM +0100, Mateusz Grzonka wrote:
> Add kernel style comments and better error handling.
> 
> Signed-off-by: Mateusz Grzonka <mateusz.grzonka@intel.com>

Acked-by: Coly Li <colyli@suse.de>


> ---
> Create.c |  1 +
> lib.c    | 29 -----------------------------
> mdadm.h  |  2 --
> mdopen.c | 12 ++++++------
> udev.c   | 44 ++++++++++++++++++++++++++++++++++++++++++++
> udev.h   |  2 ++
> 6 files changed, 53 insertions(+), 37 deletions(-)
> 
> diff --git a/Create.c b/Create.c
> index 953e7372..666a8c92 100644
> --- a/Create.c
> +++ b/Create.c
> @@ -23,6 +23,7 @@
>  */
> 
> #include	"mdadm.h"
> +#include	"udev.h"
> #include	"md_u.h"
> #include	"md_p.h"
> #include	<ctype.h>
> diff --git a/lib.c b/lib.c
> index 96ba6e8d..24cd10e3 100644
> --- a/lib.c
> +++ b/lib.c
> @@ -186,35 +186,6 @@ char *fd2devnm(int fd)
> 	return NULL;
> }
> 
> -/* When we create a new array, we don't want the content to
> - * be immediately examined by udev - it is probably meaningless.
> - * So create /run/mdadm/creating-mdXXX and expect that a udev
> - * rule will noticed this and act accordingly.
> - */
> -static char block_path[] = "/run/mdadm/creating-%s";
> -static char *unblock_path = NULL;
> -void udev_block(char *devnm)
> -{
> -	int fd;
> -	char *path = NULL;
> -
> -	xasprintf(&path, block_path, devnm);
> -	fd = open(path, O_CREAT|O_RDWR, 0600);
> -	if (fd >= 0) {
> -		close(fd);
> -		unblock_path = path;
> -	} else
> -		free(path);
> -}
> -
> -void udev_unblock(void)
> -{
> -	if (unblock_path)
> -		unlink(unblock_path);
> -	free(unblock_path);
> -	unblock_path = NULL;
> -}
> -
> /*
>  * convert a major/minor pair for a block device into a name in /dev, if possible.
>  * On the first call, walk /dev collecting name.
> diff --git a/mdadm.h b/mdadm.h
> index 23c10a52..5607c599 100644
> --- a/mdadm.h
> +++ b/mdadm.h
> @@ -1729,8 +1729,6 @@ extern char *fd2kname(int fd);
> extern char *stat2devnm(struct stat *st);
> bool stat_is_md_dev(struct stat *st);
> extern char *fd2devnm(int fd);
> -extern void udev_block(char *devnm);
> -extern void udev_unblock(void);
> 
> extern int in_initrd(void);
> 
> diff --git a/mdopen.c b/mdopen.c
> index afec34a4..ef34613a 100644
> --- a/mdopen.c
> +++ b/mdopen.c
> @@ -336,8 +336,8 @@ int create_mddev(char *dev, char *name, int autof, int trustworthy,
> 	devnm[0] = 0;
> 	if (num < 0 && cname && ci->names) {
> 		sprintf(devnm, "md_%s", cname);
> -		if (block_udev)
> -			udev_block(devnm);
> +		if (block_udev && udev_block(devnm) != UDEV_STATUS_SUCCESS)
> +			return -1;
> 		if (!create_named_array(devnm)) {
> 			devnm[0] = 0;
> 			udev_unblock();
> @@ -345,8 +345,8 @@ int create_mddev(char *dev, char *name, int autof, int trustworthy,
> 	}
> 	if (num >= 0) {
> 		sprintf(devnm, "md%d", num);
> -		if (block_udev)
> -			udev_block(devnm);
> +		if (block_udev && udev_block(devnm) != UDEV_STATUS_SUCCESS)
> +			return -1;
> 		if (!create_named_array(devnm)) {
> 			devnm[0] = 0;
> 			udev_unblock();
> @@ -369,8 +369,8 @@ int create_mddev(char *dev, char *name, int autof, int trustworthy,
> 				return -1;
> 			}
> 		}
> -		if (block_udev)
> -			udev_block(devnm);
> +		if (block_udev && udev_block(devnm) != UDEV_STATUS_SUCCESS)
> +			return -1;
> 	}
> 
> 	sprintf(devname, "/dev/%s", devnm);
> diff --git a/udev.c b/udev.c
> index 72a38f47..5389e5df 100644
> --- a/udev.c
> +++ b/udev.c
> @@ -30,6 +30,7 @@
> 
> static struct udev *udev;
> static struct udev_monitor *udev_monitor;
> +static char *unblock_path;
> 
> /*
>  * udev_is_available() - Checks for udev in the system.
> @@ -145,3 +146,46 @@ void udev_release(void)
> 	udev_monitor_unref(udev_monitor);
> 	udev_unref(udev);
> }
> +
> +/*
> + * udev_block() - Block udev from examining newly created arrays.
> + *
> + * When array is created, we don't want udev to examine it immediately.
> + * Function creates /run/mdadm/creating-mdXXX and expects that udev rule
> + * will notice it and act accordingly.
> + *
> + * Return:
> + * UDEV_STATUS_SUCCESS when successfully blocked udev
> + * UDEV_STATUS_ERROR on error
> + */
> +enum udev_status udev_block(char *devnm)
> +{
> +	int fd;
> +	char *path = xcalloc(1, BUFSIZ);
> +
> +	snprintf(path, BUFSIZ, "/run/mdadm/creating-%s", devnm);
> +
> +	fd = open(path, O_CREAT | O_RDWR, 0600);
> +	if (!is_fd_valid(fd)) {
> +		pr_err("Cannot block udev, error creating blocking file.\n");
> +		pr_err("%s: %s\n", strerror(errno), path);
> +		free(path);
> +		return UDEV_STATUS_ERROR;
> +	}
> +
> +	close(fd);
> +	unblock_path = path;
> +	return UDEV_STATUS_SUCCESS;
> +}
> +
> +/*
> + * udev_unblock() - Unblock udev.
> + */
> +void udev_unblock(void)
> +{
> +	if (unblock_path)
> +		unlink(unblock_path);
> +	free(unblock_path);
> +	unblock_path = NULL;
> +}
> +
> diff --git a/udev.h b/udev.h
> index 515366e7..e4da29cc 100644
> --- a/udev.h
> +++ b/udev.h
> @@ -32,5 +32,7 @@ bool udev_is_available(void);
> enum udev_status udev_initialize(void);
> enum udev_status udev_wait_for_events(int seconds);
> void udev_release(void);
> +enum udev_status udev_block(char *devnm);
> +void udev_unblock(void);
> 
> #endif
> -- 
> 2.26.2
>
Jes Sorensen March 2, 2023, 3:57 p.m. UTC | #2 
On 2/2/23 06:27, Mateusz Grzonka wrote:
> Add kernel style comments and better error handling.
> 
> Signed-off-by: Mateusz Grzonka <mateusz.grzonka@intel.com>
> ---
>  Create.c |  1 +
>  lib.c    | 29 -----------------------------
>  mdadm.h  |  2 --
>  mdopen.c | 12 ++++++------
>  udev.c   | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  udev.h   |  2 ++
>  6 files changed, 53 insertions(+), 37 deletions(-)

This breaks miserably for me on Fedora 36:

[jes@jes-t490 mdadm.git]$ make
gcc -Wall -Werror -Wstrict-prototypes -Wextra -Wno-unused-parameter
-Wimplicit-fallthrough=0 -O2 -DSendmail=\""/usr/sbin/sendmail -t"\"
-DCONFFILE=\"/etc/mdadm.conf\" -DCONFFILE2=\"/etc/mdadm/mdadm.conf\"
-DMAP_DIR=\"/run/mdadm\" -DMAP_FILE=\"map\" -DMDMON_DIR=\"/run/mdadm\"
-DFAILED_SLOTS_DIR=\"/run/mdadm/failed-slots\" -DNO_COROSYNC -DNO_DLM
-DVERSION=\"4.2-94-g47c22f0\" -DVERS_DATE="\"2023-03-02\""
-DUSE_PTHREADS  -pthread -Wl,-z,now -o mdmon mdmon.o monitor.o
managemon.o uuid.o util.o maps.o mdstat.o sysfs.o config.o mapfile.o
mdopen.o policy.o lib.o Kill.o sg_io.o dlink.o ReadMe.o super-intel.o
super-mbr.o super-gpt.o super-ddf.o sha1.o crc32.o msg.o bitmap.o
xmalloc.o platform-intel.o probe_roms.o crc32c.o -ldl -ludev
/usr/bin/ld: mdopen.o: in function `find_free_devnm':
mdopen.c:(.text+0x5f5): undefined reference to `udev_is_available'
/usr/bin/ld: mdopen.o: in function `create_mddev':
mdopen.c:(.text+0x6e6): undefined reference to `udev_is_available'
/usr/bin/ld: mdopen.c:(.text+0x8db): undefined reference to `udev_block'
/usr/bin/ld: mdopen.c:(.text+0x96f): undefined reference to
`udev_is_available'
/usr/bin/ld: mdopen.c:(.text+0xa84): undefined reference to `udev_block'
/usr/bin/ld: mdopen.c:(.text+0xef5): undefined reference to `udev_block'
/usr/bin/ld: mdopen.c:(.text+0x1106): undefined reference to `udev_unblock'
/usr/bin/ld: mdopen.c:(.text+0x11ce): undefined reference to `udev_unblock'
collect2: error: ld returned 1 exit status
make: *** [Makefile:220: mdmon] Error 1

Jes
diff mbox series

Patch

diff --git a/Create.c b/Create.c
index 953e7372..666a8c92 100644
--- a/Create.c
+++ b/Create.c
@@ -23,6 +23,7 @@ 
  */
 
 #include	"mdadm.h"
+#include	"udev.h"
 #include	"md_u.h"
 #include	"md_p.h"
 #include	<ctype.h>
diff --git a/lib.c b/lib.c
index 96ba6e8d..24cd10e3 100644
--- a/lib.c
+++ b/lib.c
@@ -186,35 +186,6 @@  char *fd2devnm(int fd)
 	return NULL;
 }
 
-/* When we create a new array, we don't want the content to
- * be immediately examined by udev - it is probably meaningless.
- * So create /run/mdadm/creating-mdXXX and expect that a udev
- * rule will noticed this and act accordingly.
- */
-static char block_path[] = "/run/mdadm/creating-%s";
-static char *unblock_path = NULL;
-void udev_block(char *devnm)
-{
-	int fd;
-	char *path = NULL;
-
-	xasprintf(&path, block_path, devnm);
-	fd = open(path, O_CREAT|O_RDWR, 0600);
-	if (fd >= 0) {
-		close(fd);
-		unblock_path = path;
-	} else
-		free(path);
-}
-
-void udev_unblock(void)
-{
-	if (unblock_path)
-		unlink(unblock_path);
-	free(unblock_path);
-	unblock_path = NULL;
-}
-
 /*
  * convert a major/minor pair for a block device into a name in /dev, if possible.
  * On the first call, walk /dev collecting name.
diff --git a/mdadm.h b/mdadm.h
index 23c10a52..5607c599 100644
--- a/mdadm.h
+++ b/mdadm.h
@@ -1729,8 +1729,6 @@  extern char *fd2kname(int fd);
 extern char *stat2devnm(struct stat *st);
 bool stat_is_md_dev(struct stat *st);
 extern char *fd2devnm(int fd);
-extern void udev_block(char *devnm);
-extern void udev_unblock(void);
 
 extern int in_initrd(void);
 
diff --git a/mdopen.c b/mdopen.c
index afec34a4..ef34613a 100644
--- a/mdopen.c
+++ b/mdopen.c
@@ -336,8 +336,8 @@  int create_mddev(char *dev, char *name, int autof, int trustworthy,
 	devnm[0] = 0;
 	if (num < 0 && cname && ci->names) {
 		sprintf(devnm, "md_%s", cname);
-		if (block_udev)
-			udev_block(devnm);
+		if (block_udev && udev_block(devnm) != UDEV_STATUS_SUCCESS)
+			return -1;
 		if (!create_named_array(devnm)) {
 			devnm[0] = 0;
 			udev_unblock();
@@ -345,8 +345,8 @@  int create_mddev(char *dev, char *name, int autof, int trustworthy,
 	}
 	if (num >= 0) {
 		sprintf(devnm, "md%d", num);
-		if (block_udev)
-			udev_block(devnm);
+		if (block_udev && udev_block(devnm) != UDEV_STATUS_SUCCESS)
+			return -1;
 		if (!create_named_array(devnm)) {
 			devnm[0] = 0;
 			udev_unblock();
@@ -369,8 +369,8 @@  int create_mddev(char *dev, char *name, int autof, int trustworthy,
 				return -1;
 			}
 		}
-		if (block_udev)
-			udev_block(devnm);
+		if (block_udev && udev_block(devnm) != UDEV_STATUS_SUCCESS)
+			return -1;
 	}
 
 	sprintf(devname, "/dev/%s", devnm);
diff --git a/udev.c b/udev.c
index 72a38f47..5389e5df 100644
--- a/udev.c
+++ b/udev.c
@@ -30,6 +30,7 @@ 
 
 static struct udev *udev;
 static struct udev_monitor *udev_monitor;
+static char *unblock_path;
 
 /*
  * udev_is_available() - Checks for udev in the system.
@@ -145,3 +146,46 @@  void udev_release(void)
 	udev_monitor_unref(udev_monitor);
 	udev_unref(udev);
 }
+
+/*
+ * udev_block() - Block udev from examining newly created arrays.
+ *
+ * When array is created, we don't want udev to examine it immediately.
+ * Function creates /run/mdadm/creating-mdXXX and expects that udev rule
+ * will notice it and act accordingly.
+ *
+ * Return:
+ * UDEV_STATUS_SUCCESS when successfully blocked udev
+ * UDEV_STATUS_ERROR on error
+ */
+enum udev_status udev_block(char *devnm)
+{
+	int fd;
+	char *path = xcalloc(1, BUFSIZ);
+
+	snprintf(path, BUFSIZ, "/run/mdadm/creating-%s", devnm);
+
+	fd = open(path, O_CREAT | O_RDWR, 0600);
+	if (!is_fd_valid(fd)) {
+		pr_err("Cannot block udev, error creating blocking file.\n");
+		pr_err("%s: %s\n", strerror(errno), path);
+		free(path);
+		return UDEV_STATUS_ERROR;
+	}
+
+	close(fd);
+	unblock_path = path;
+	return UDEV_STATUS_SUCCESS;
+}
+
+/*
+ * udev_unblock() - Unblock udev.
+ */
+void udev_unblock(void)
+{
+	if (unblock_path)
+		unlink(unblock_path);
+	free(unblock_path);
+	unblock_path = NULL;
+}
+
diff --git a/udev.h b/udev.h
index 515366e7..e4da29cc 100644
--- a/udev.h
+++ b/udev.h
@@ -32,5 +32,7 @@  bool udev_is_available(void);
 enum udev_status udev_initialize(void);
 enum udev_status udev_wait_for_events(int seconds);
 void udev_release(void);
+enum udev_status udev_block(char *devnm);
+void udev_unblock(void);
 
 #endif