Re: [PATCH 9/9] udev: Move udev_block() and udev_unblock() into udev.c

[Coly Li <colyli@suse.de>]

On 9/7/22 8:56 PM, Mateusz Grzonka wrote:
> Add kernel style comments and better error handling.
>
> Change-Id: Ib6b8e7725d6739a2d7f7b8801e3403137c9cc402

What does the above line mean?


BTW, for the code, it looks fine to me. I want to ack this patch after 
the Change-Id in the commit log explained.

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

Thanks.

Coly Li


> ---
>   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 c84c1ac8..e68894ed 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 10a6ae40..5a1afd49 100644
> --- a/lib.c
> +++ b/lib.c
> @@ -174,35 +174,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 3c08f826..3f81cce6 100644
> --- a/mdadm.h
> +++ b/mdadm.h
> @@ -1662,8 +1662,6 @@ extern char *stat2kname(struct stat *st);
>   extern char *fd2kname(int fd);
>   extern char *stat2devnm(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 53da4d96..93193739 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 18055bfc..d6e3d7b5 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