Re: [Ovirt-devel] [PATCH node] Usability fixes to the logging configuration.

[Bryan Kearney <bkearney redhat com>]

Some comments below

Darryl L. Pierce wrote:
> Users can now abort the configuration. At any point the user can enter
> either "abort" or an "a" depending on the input and quit the logging
> configuration.
>
> Also smoothes out the overall flow of configuration.
>
> Signed-off-by: Darryl L. Pierce <dpierce redhat com>
> ---
>  scripts/ovirt-config-logging |  144 ++++++++++++++++++++++++++----------------
>  1 files changed, 90 insertions(+), 54 deletions(-)
>
> diff --git a/scripts/ovirt-config-logging b/scripts/ovirt-config-logging
> index f79597f..8e6bc70 100755
> --- a/scripts/ovirt-config-logging
> +++ b/scripts/ovirt-config-logging
> @@ -71,62 +71,98 @@ function is_numeric {
>  }
>  
>  function prompt_user {
> +    max_log_size="10"
> +    syslog_server_ip=""
> +    syslog_server_port=""
> +
>      while true ; do
> -        max_log_size="10k"
> -        printf "\nWhat is the max size for log files on this machine [10k]? "
> -        read -e
> -        if [ -n "$REPLY" ]; then
> -            max_log_size=$REPLY
> -        fi
> -        printf "\nWhat is the IP address or server name for the syslog server? "
> -        read -e
> -        syslog_server_ip=$REPLY
> -        printf "\nWhat port does the syslog daemon run on? "
> -        read -e
> -        if is_numeric "$REPLY"; then
> -            syslog_server_port=$REPLY
> -
> -            PROTOCOLS="udp tcp"
> -            PS3="Please select the protocol to use: "
> -            select syslog_server_protocol in $PROTOCOLS;
> -            do
> -                case $syslog_server_protocol in
> -                    "tcp")
> -                        break ;;
> -                    "udp")
> -                        break;;
> -                esac
> -            done
> -
> -            printf "\n"
> -            printf "\nLogging will be configured as follows:"
> -            printf "\n======================================"
> -            printf "\n Max Logfile Size: $max_log_size"
> -            printf "\n    Remote Server: $syslog_server_ip"
> -            printf "\n      Remote Port: $syslog_server_port"
> -            printf "\n Logging Protocol: $syslog_server_protocol"
> -            printf "\n"
> -            printf "\nPlease confirm these changes (Y/N)"
> -            read -e
> -            case $REPLY in
> -                Y|y)
> -                    ovirt_rsyslog $syslog_server_ip \
> -                                  $syslog_server_port \
> -                                  $syslog_server_protocol
> -                    sed -i -e "s/^size=.*/size=$max_log_size/" \
> -                              /etc/logrotate.d/ovirt-logrotate.conf
> -                    break
> -                    ;;
> -                N|n)
> -                    printf "Discarding settings\n"
> -                    break
> -                    ;;
> -            esac
> -        else
> -            printf "Invalid port number\n"
> -        fi
> +	while true; do
> +	    printf "\n"
> +	    read -p "What is the max size for log files on this machine (def. $max_log_size)? "

I would let folks know that this is in kilibytes

> +
> +	    if [ -n "$REPLY" ]; then
> +		r=$(echo $REPLY|tr '[[:lower:]]' '[[:upper:]]')
> +		if [[ $r =~ ^[0-9]+$ ]] && [[ $r -gt 0 ]]; then
> +		    max_log_size=$r
> +		    printf "\nMaximum logging size will be ${max_log_size}k.\n"
> +		    break
> +		elif [ "ABORT" == "$r" ]; then
> +		    printf "\nAborting...\n"
> +		    return

this is undocumented. Will every input line allow me to ABORT?

> +		else
> +		    printf "\nInvalid input.\n"
> +		fi
> +	    else
> +		printf "\nLeaving log size as $max_log_size.\n"
> +		break
> +	    fi
> +	done
> +
> +	printf "\n"
> +	read -p "What is the IP address or hostname for the syslog server? "
> +	if [ -n "$REPLY" ]; then
> +	    syslog_server_ip=$REPLY
> +	    while true; do
> +		read -p "What port does the syslog daemon use? "
> +		r=$(echo $REPLY|tr '[[:lower:]]' '[[:upper:]]')
> +		if [ -n "$r" ]; then
> +		    if [[ $r =~ ^[0-9]+$ ]] && [[ $r -gt 0 ]]; then
> +			syslog_server_port=$REPLY
> +			break
> +		    elif [ "ABORT" == "$r" ]; then
> +			printf "\nAbort...\n"
> +			return
> +		    else
> +			printf "Invalid port.\n"
> +		    fi
> +		fi
> +	    done
> +
> +	    printf "\n"
> +	    while true; do
> +		read -p "Remote logging uses [t]cp or [u]dp, or [a]bort? "
> +		r=$(echo $REPLY|tr '[[:lower:]]' '[[:upper:]]')
> +		if [ "$r" == "T" ]; then syslog_server_protocol="tcp"; break; fi
> +		if [ "$r" == "U" ]; then syslog_server_protocol="udp"; break; fi
> +		if [ "$r" == "A" ]; then return; fi
> +		# else
> +		printf "Invalid input.\n"
> +	    done
> +	else
> +	    printf "\nDisabling remote logging.\n"
> +	fi

I found it odd that if I hit return I could stilll commit the logging 
change, but if I hit the abort then I lost both chanages. I would 
suggest the following:
- Keep the abort at the end of the process.
- Allow me to "skip" over the rsyslog
- Loose the ABORT Logic


> +
> +	printf "\n"
> +	printf "\nLogging will be configured as follows:"
> +	printf "\n======================================"
> +	printf "\n Max Logfile Size: $max_log_size"
> +	if [ -n "$syslog_server_ip" ]; then
> +	    printf "\n    Remote Server: $syslog_server_ip"
> +	    printf "\n      Remote Port: $syslog_server_port"
> +	    printf "\n Logging Protocol: $syslog_server_protocol"
> +	fi
> +	printf "\n"
> +	printf "\n"
> +	while true; do
> +	    read -p "Is this correct (Y/N/A)? "
> +	    r=$(echo $REPLY|tr '[[:lower:]]' '[[:upper:]]')
> +	    if [ "$r" == "Y" ]; then
> +		printf "\nSaving configuration.\n"
> +		ovirt_rsyslog $syslog_server_ip \
> +		    $syslog_server_port \
> +		    $syslog_server_protocol
> +		sed -i -e "s/^size=.*/size=${max_log_size}k/" \
> +		    /etc/logrotate.d/ovirt-logrotate.conf
> +		return
> +	    elif [ "$r" == "N" ]; then
> +		printf "\nRestarting logging configuration.\n"
> +		break
> +	    elif [ "$r" == "A" ]; then
> +		printf "\nAborting logging configuration.\n"
> +		return
> +	    fi
> +	done
>      done
> -
>  }
>  
>  # AUTO for auto-install