[OpenWrt-Devel] [PATCH] mdadm: add new init features; documentation; bug fixes

[Jo-Philipp Wich]

Hi,

comments inline.

> [...]
> +	# partition	- stateless, mdadm --assemble --scan --config=partition; see mdadm(8)
> +	# uci		- stateful, dynamically generated mdadm.conf via uci array values (below),
> +	#		  stored in /var/etc/mdadm.conf
> +	# </file>	- stateful, manually generated mdadm.conf file(s),
> +	#		  <file> must be preceded by a / and may be a readable filename

These file tags look wrong.

> [...]
> +#
> +# The mdadm 'array' section(s) are for stateful, manual configurations. Experts only.  Use with caution.
> +#
> +#
> +# The use of multiple 'array' sections is supported by /etc/init.d/mdadm.
> +# They must all be named 'array'.
> +#
> +# As of this writing unnamed 'mdadm' sections are still allowed, but deprecated. Do not use.
> +#

Config "array" sections were used before as well? I do not understand
the deprecation remark.

> +
> +#config array
> +	#
> +	# example 'array' options may be a valid mix of:
> +	#
> +	# bitmap
> +	# container
> +	# device
> +	# devices
> +	# member
> +	# name
> +	# spare_group
> +	# spares
> +	# super_minor
> +	# uuid
> +	#
>  	# option bitmap /bitmap.md
>  	# option container 00000000:00000000:00000000:00000000
> +	# option device /dev/md0
> +	# -and/or a devices list-
> +	# list devices /dev/hd* # mdadm allows glob, see glob(7)
> +	# list devices /dev/sd*
> +	# list devices /dev/sda1
> +	# list devices /dev/sdb1
> +	# list devices containers
> +	# list devices partitions
>  	# option member 1
> +	# option name raid:0
> +	# option spare_group spares
> +	# option spares 0
> +	# option super_minor 0
> +	# use uuid from block info (preferred), or mdadm --misc --detail /dev/md0
> +	# option uuid 2084de11-70c4-4521-8f95-6113e75f1fe9
> +	#
> +	# These options directly translate to mdadm -- options, see man mdadm(8)

Consider linking to https://linux.die.net/man/8/mdadm, "man" is rarely
available on OpenWrt.

> diff --git a/package/utils/mdadm/files/mdadm.init b/package/utils/mdadm/files/mdadm.init
> index 64a50b3..5453f7d 100644
> --- a/package/utils/mdadm/files/mdadm.init
> +++ b/package/utils/mdadm/files/mdadm.init
> @@ -1,13 +1,21 @@
>  #!/bin/sh /etc/rc.common
>  
> -START=13
> -STOP=98
> +# 20190106, joseph.tingiris at gmail.com, significant revision; new features,
> +#                                      safer error handling, code formatting,
> +#                                      & multiple bug fixes

Please drop this comment, changes are recorded in Git & commit message.

> +
> +START=12
> +STOP=X99 # X99? seems to work; passed service enable/disable tests & boot tests

Please stick to plain 99, will change the umount prio in a separate commit.

>  
>  USE_PROCD=1
>  PROG=/sbin/mdadm
>  NAME=mdadm
>  
> -CONF="/var/etc/mdadm.conf"
> +VERBOSE=0 # off
> +
> +TMP_FILE="/var/etc/mdadm.conf" # /var/etc is on /tmp; used for temporary state, to enable stateful only mdadm features

Please remove this comment.

> +
> +[ ! -x "$PROG" ] && exit 1
>  
>  append_list_item() {
>  	append "$2" "$1" "$3"
> @@ -30,30 +38,165 @@ append_option() {
>  	[ -n "$str" ] && append "$var" $(printf "%s=%s" "${name:-${opt//_/-}}" "$str")
>  }
>  
> -mdadm_common() {
> -	local cfg="$1"
> -	local email devices
> +verbose() {
> +	local msg="$1"
> +	local level="$2"
>  
> -	if [ -x /usr/sbin/sendmail ]; then
> -		config_get email "$cfg" email
> -		[ -n "$email" ] && printf "MAILADDR %s\n" "$email" >> $CONF
> +	[ -z "$level" ] && level="INFO"
> +
> +	if [ "$VERBOSE" == "1" ]; then
> +		if [ ${#SSH_TTY} -gt 0 ]; then
> +			printf "$NAME: init %7s - %b\n" "$level" "$msg"
> +		else
> +			# no SSH_TTY goes to logger
> +			printf "$NAME: init %7s - %b\n" "$level" "$msg" | logger -t mdadm
> +		fi
>  	fi

User sessions may not always happen via SSH, could be telnet, uart,
screen. Stick to either variant, independently of $SSH_TTY.

> +}
>  
> -	config_list_foreach "$cfg" devices append_list_item devices " "
> -	[ -n "$devices" ] && printf "DEVICE %s\n" "$devices" >> $CONF
> +mdadm_conf_auto() {
> +	local mdadm_conf="$1"
> +
> +	if [ ! -w "$mdadm_conf" ]; then
> +		if [ -z "$mdadm_conf" ]; then
> +			verbose "mdadm_conf value is empty" ERROR
> +		else
> +			verbose "'$mdadm_conf' file not found writable" ERROR
> +		fi
> +		return 1
> +	fi
> +
> +	local block_md block_uuid mdadm_md mdadm_md_rc mdadm_uuid
> +
> +	# Check block info for active linux_raid_members, if necessary then compare with mdadm, & dynamically update $mdadm_conf
> +
> +	block_md=0 # counter
> +	for block_uuid in $(block info 2> /dev/null | grep linux_raid_member | awk '{print $2}' | awk -F\" '{print $(NF-1)}'); do

Consider `block info 2> /dev/null | sed -ne 's#^.* UUID="\([^"]*\)".*
TYPE="linux_raid_member"#\1#p'` instead, saves two forks & execs.

> +		mdadm_md=""
> +		mdadm_md_rc=0
> +
> +		while [ -z "$mdadm_md" ]; do
> +			if [ -b "/dev/md$block_md" ]; then
> +				# handle mdadm restart, service reload, multiple starts without
stops, physical unplug, etc.
> +
> +				verbose "/dev/md$block_md block device already exists" NOTICE
> +
> +				# active arrays will promptly respond; first check
> +				mdadm_uuid=$($PROG --detail --test --brief "/dev/md$block_md" 2>
/dev/null | grep -E "((.*)(UUID=(.){36})$" | head -1 | awk -F= '{print
$NF}')

In general I like to transform `command | grep | awk` sequences into
`command | sed -ne s/.../.../p` ones. Can you give an example of the
output you want to extract the uuid from?

> [...]
> +
> +reload_service() {
> +	# fix. stopping mdadm every time a reload is requested is bad.
> +	# procd needs this or it'll call restart, which will call stop_service and then start_service.
> +	# running start_service() should be OK, it already rescans for hotplugged devices, add new arrays, etc.
> +	# todo: better test calling start_service() on reload, for now, verbosely ignore & exit 0 ...
> +	verbose "reload_service called, ignoring" NOTICE
> +	exit 0

Please replace this with a "return".

The `rc.common` will call start() upon reload() when no
`reload_service()` is implemented. This is fine for most init scripts
since `start()` is free of side effects there.

Mdadm is special in this regard since its start procedure does all the
work and the daemon process started later is just the health monitor...

One could argue though that calling start() which will then rescan
arrays etc. is appropriate for a reload but its a matter of preference I
guess.

>  }
>  
>  start_service() {
> -	local email
> +	local config config_detail config_file config_level config_mode config_rc config_state config_verbose mdadm_conf
> +
> +	# mdadm.global specific locals
> +	local alert_program email email_from mail_program mdadm_args monitor_frequency
> +
> +	config_verbose=$(uci_get mdadm.global.verbose 2> /dev/null | awk '{print tolower($1)}')

Drop the awk call.

> +	if [ "$config_verbose" == "1" ] || [ "$config_verbose" == "on" ] || [ "$config_verbose" == "true" ]; then
> +		VERBOSE=1 # turn verbose on globally
> +		config_verbose=1
> +		mdadm_args="--verbose"
> +	else
> +		unset -v config_verbose
> +		mdadm_args="--quiet"
> +	fi
> +
> +	verbose "start_service $@" INFO
> +
> +	config_rc=0
> [...]
> +
> +	# mdadm (or postfix?) bug; workaround.  mdadm 4.0 for 18.06.1 has a compiled in default of '/usr/lib/sendmail -t'.
> +	# There's no configurable way to change it and mdadm must be recompiled differently for Openwrt, or posfix should
> +	# add the link. In 18.06.1, postfix 3.3.0-1 installs sendmail in /usr/sbin; mdadm complains & no mail is delivered.
> +	# Other distro's postfix pkg typically installs this link ... or one in /etc/alternatives.
> +	# Since mdadm needs it, I'm adding it here to be sure mdadm can send email if there's a /usr/sbin/sendmail.

We should simply fix the package build to have working defaults.

> +
> +	# There's really no point in making mdadm's compiled in sendmail configurable via uci.
> +	mail_program="/usr/lib/sendmail"
> +	if [ -x /usr/sbin/sendmail ] && [ ! -e "$mail_program" ]; then
> +		# a sym link will suffice
> +		ln -s /usr/sbin/sendmail "$mail_program"

Please do not install arbitrary symlinks in the fs.

> +		if [ $? -ne 0 ]; then
> +			verbose "ln -s /usr/bin/sendmail /usr/lib/sendmail failed" WARNING
> +		fi
> +	fi
> +
> +	if [ ! -x "$mail_program" ]; then
> +		verbose "disabling email; mail program '/usr/lib/sendmail' not found executable (install postfix)" WARNING
> +		email=""
> +	fi
> +
> +	if [ "$config_state" == "stateful" ] && [ -n "$mdadm_conf" ]; then
> +		if mkdir -p "${mdadm_conf%/*}" &> /dev/null; then
> +			printf "# Autogenerated from /etc/init.d/mdadm, do not edit!\n" > $mdadm_conf
> +
> +			# Use mdadm.global.email_from only if there's a valid mta; only one is allowed per mdadm.conf(5)
> +			# todo: see what other mtas Openwrt has in their opkg repos & maybe support others
> +			if [ -x "$mail_program" ]; then
> +				email_from=$(uci_get mdadm.global.email_from 2> /dev/null)
> +			fi
> +
> +			if [ "$config" == "auto" ]; then
> +				# stateful mdadm.conf auto configuration
> +				if ! mdadm_conf_auto "$mdadm_conf"; then
> +					#there's quite a bit of logic & error handling in mdadm_conf_auto; if it doesn't return 0 then it's a fatality
> +					config_detail="$config_detail (couldn't find any meta devices; check connections, or try stateless autoconfig)"
> +					config_level="FATAL"
> +					config_rc=1
> +				fi
> +			else
> +				# stateful mdadm.conf uci configuration
> +
> +				# load uci config from /etc/config/mdadm
> +				config_load mdadm
> +
> +				# This is how mdadm uci mdadm config sections should be named, like fstab does with 'mount'.
> +				# The included uci /etc/config/mdadm provides more documentation & direction.
> +				config_foreach mdadm_conf_uci array "$mdadm_conf"
> +
> +				# The unlabled mdadm. at mdadm[0] section should be (is now) deprecated.
> +				# It's more difficult to document how to use, redundant, and over complicated this init configuration.

I am still unsure what you mean with unlabelled here. Both `config
mdadm` and `config array` are unlabelled sections. The former is an
anonymous section of type `mdadm`, the latter an anonymous section of
type `array`. Neither has a name or label.

In the existing init script configuration, `config mdadm` was meant for
global settings while one or more `config array` sections were supposed
to describe the arrays. The config_foreach() iterator has been used for
convenience. There was no explicit guard to disallow multiple
definitions, maybe this is what confused things.

> +
> +				# Confused; originally config_foreach?
> +				# It's possible to specify multiple mdadm sections with array options in all sections.
> +				# Thus multiple option emails which could result in multiple MAILADDR being appended to mdadm.conf.
> +				# That confuses mdadm.
> +
> +				# The following code is here to prevent regressions.
> +
> +				config_foreach mdadm_conf_uci mdadm "$mdadm_conf"
> +
> +				# For backward compatibility; this will allow an mdadm. at mdadm[0] section's option email.
> +				# (only if mdadm.global.email is not set; again, prefer mdadm.global.email)
> +				#
> +				# bug fixed. The first legacy mdadm section option email will be used.
> +				# a better fix would be to *only* support array sections.
> +				#
> +				[ -z "$email" ] && [ -x "$mail_program" ] && email=$(uci_get mdadm. at mdadm[0].email 2> /dev/null)
> +				# email_from is a new feature; was not previously handled; no need to and please don't backport
> +
> +			fi
> +
> [...]
> +
> +		else
> +			config_detail="$config_detail (mkdir failed; check permissions)"
> +			config_level="FATAL"
> +			config_rc=1
> +		fi
> +	fi
> +
> +	if [ $config_rc -ne 0 ]; then
> +		# FATAL
> +		verbose "$config_state,$config_mode config='$config', $config_detail" "$config_level" INFO
> +		[ -n "$mdadm_conf" ] && [ -w "$mdadm_conf" ] && rm -f "$mdadm_conf"
> +		exit $config_rc
> +	fi
> +
> +	# Good to go, no more fatal exits, finish getting global & setting local values ... they're all optional.
> +
> +	mdadm_args="$mdadm_args --scan"

Consider using append()

> +
> +	if [ -n "$mdadm_conf" ]; then
> +		# mdadm.global.config </file>

That file tag looks superfluos.

> +		mdadm_args="$mdadm_args --config=$mdadm_conf"

Consider using append()

> +	else
> +		# mdadm.global.config all (containers, partitions, etc), except none
> +		[ -n "$config" ] && [ "$config" != 'none' ] && mdadm_args="$mdadm_args --config=$config"

Consider using append()

> +	fi
> +
> +	local assemble_count assemble_rc
> +
> +	assemble_count=$($PROG --detail --brief --scan 2>/dev/null | wc -l)
> +	verbose "$assemble_count arrays are currently assembled" INFO
> +
> +	# setup assembly mode
> +
> +	verbose "(assemble) '$PROG --assemble $mdadm_args'" INFO
> +	$PROG --assemble $mdadm_args &> /dev/null
> +	assemble_rc=$?
> +
> +	if [ $assemble_rc -eq 0 ]; then

Why not just shorten into this?

  if ! $PROG --assemble $mdadm_args &> /dev/null; then

> +		verbose "all arrays assembled successfully" OK
> +		if [ -n "$config_verbose" ]; then
> +			>/tmp/mdadm.detail

Please consider piping this to logger or write it into a fixed place
into /var/log/. Imho it does not belong into /tmp/.

> +			local assemble_dev assemble_devs=$($PROG --detail --brief --scan | awk '{print $2}')
> +			for assemble_dev in $assemble_devs; do
> +				printf "\n" >> /tmp/mdadm.detail
> +				$PROG --verbose --detail $assemble_dev >> /tmp/mdadm.detail
> +				printf "\n" >> /tmp/mdadm.detail
> +			done
> +			unset -v assemble_dev
> +			while read line; do
> +				verbose "$line" INFO
> +			done < "/tmp/mdadm.detail"

Is this the entire purpose of "/tmp/mdadm.detail" ? Maybe simply make a
subshell and pipe its output to logger/verbose instead?

> +			unset -v line
> +		fi
> +	else
> +		if [ $assemble_count -eq 0 ]; then
> +			verbose "no arrays assembled successfully" ERROR
> +		else
> +			verbose "no new arrays need assembly" NOTICE
> +		fi
> +	fi
> +
> +	# setup monitor mode
> +
> +	alert_program=$(uci_get mdadm.global.alert_program 2> /dev/null)
> +	if [ -n "$alert_program" ]; then
> +		if [ -x "$alert_program" ]; then
> +			mdadm_args="$mdadm_args --alert=$alert_program"

Consider using append()

> +		else
> +			verbose "disabling alerts; alert_progam '$alert_program' not found executable" WARNING
> +		fi
> +	fi
> +
> +	[ -n "$email" ] && mdadm_args="$mdadm_args --mail=$email"

Consider using append()

> +
> +	monitor_frequency=$(uci_get mdadm.global.monitor_frequency 2> /dev/null | sed -e 's/[^0-9]*//g')
> +	if [ -n "$monitor_frequency" ]; then
> +		mdadm_args="$mdadm_args --delay=$monitor_frequency"

Consider using append()

> +		verbose "setting monitor frequency to '$monitor_frequency' seconds"
> +	fi
> +
> +	verbose "(monitor) '$PROG --monitor --syslog $mdadm_args'" INFO
> +
> +	# /etc/rc.common doesn't like valid sh constructs like while read line; do echo $line; done <<< "$(echo hello; echo world)" # 8|

(echo hello; echo world) | while read line; do echo $line; done   works
though...

> +	# Taking the time machine back to sh is rough enough as it is ... I need to better understand its purpose. I spent too much time
> +	# in /lib/functions.  It's all circa 2006-2013.  Yikes.  Where's the man pages?  Or even code comments?  It would've been way
> +	# easier just to do *everything* the good ol' sysvinit fashioned way, or use systemd.  Just a gripe, I really do like OpenWrt.

Does this comment relate to the procd_set_param invocation below? That
value is tokenized into an array, passed to procd and eventually given
to exec(), so no shell constructs on the service command line, unless
you do  procd_set_param command 'sh' '-c' '(echo weird; echo stuff)'

>  	procd_open_instance
> -	procd_set_param command "$PROG" --monitor --syslog --scan --config="$CONF"
> +	procd_set_param command "$PROG" --monitor --syslog $mdadm_args
>  	procd_close_instance
> +
> +	verbose "$config_state,$config_mode config=$config, $config_detail complete" "$config_level"
>  }
>  
>  stop_service() {
>  	$PROG --stop --scan
> +	if [ $? -eq 0 ]; then
> +		verbose "'$PROG --stop --scan' stop_service succeeded" OK
> +	else
> +		verbose "'$PROG --stop --scan' stop_service failed" ERROR
> +	fi
>  }
> -
> 



~ Jo

_______________________________________________
openwrt-devel mailing list
openwrt-devel at lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel