Add input checks to functions

[moogle19]

Because missing arguments could led to undefined behaviour, input checks were added for all methods which require input parameters.
I looked at the 'zfs' output when parameters are missing for the error message format.

[EpiJunkie]

Might it be better to use a function that gets ran in __parse_cmd to validate? That way the validation code can be kept out of the function doing the work. You could use $# to return the number of parameters used from the command line.

I was thinking something like this:

# Verifies the number of parameters and exits if met or exceeded
__verify_number_of_arguments() {
    local _number_of_minimum_parameters=$1        # Minimum number of parameters
    local _number_of_supplied_parameters=$2       # As counted by "$#", has to be ran from __parse_cmd() and called by __parse_cmd $@
    local _number_of_maximum_parameters=$3        # (Optional) Maximum number of parameters

    # While $3 is optional, it does need a value if not set.
    [ -z "${_number_of_maximum_parameters}" ] && local _number_of_maximum_parameters=$_number_of_supplied_parameters

    if [ "${_number_of_supplied_parameters}" -lt "${_number_of_minimum_parameters}" ] || [ "${_number_of_supplied_parameters}" -gt "${_number_of_maximum_parameters}" ]; then
        __help
        echo "Incorrect number of arguments used. Please see above for correct syntax."
                exit 1
    fi
}

And then this gets utilized in __parse_cmd this way:

    list)     __verify_number_of_arguments "1" "$#" "2"
              __list "$2"
              exit
    ;;
    create)   __verify_number_of_arguments "3" "$#" "4"
              __create "$@"
              exit
    ;;
    set)   __verify_number_of_arguments "3" "$#"
              __set "$@"
              exit
    ;;

Thoughts?

[pr1ntf]

I agree with @EpiJunkie on this. This seems easier in the long run, and helps "future proof" the idea.

[EpiJunkie]

@moogle19 I can submit this as a PR if you do not wish to. If you do though, I request you leave my name and handle in the commit message. I know that is pretty vain.

Either way, thank you for the effort.

[pr1ntf]

"You're so vain you probably think this PR is about you"

reference

[EpiJunkie]

lawlz, that should probably be an edition name at some point.

[moogle19]

Yeah, that is possible, but it could be difficult for all functions with flags (like delete), because you first have to check for the flag and than for the number of arguments. That could be a bit too much stuff in the __parse_cmd function.

Also the count of arguments is tightly connected to the function. For example if a flag is added or removed and the input check happens in the __parse_cmd function, you would have to change the implementation in two different places/files, which is not ideal.

[EpiJunkie]

Sorry I was unclear, the "$#" has a special meaning in Bourne. "$#" supplies the number of parameters and "$#" is the literal variable you would use. See here.

For delete the call to verify the number of inputs from __parse_cmd would be:

      delete)   __verify_number_of_arguments "2" "$#" "3"
                __delete "$@"
                exit
      ;;

Where "2" is the minimum number of parameters accepted (without the "-f" flag used) and the "3" is the maximum number of parameters accepted for this command (when the "-f" is used.)

Each case in __parse_cmd would have it's own call to the __verify_number_of_arguments function with it's appropriate parameters to check.

[moogle19]

But wouldn't that allow something like iohyve delete -f to be verified successfully, even if it is not allowed?

[EpiJunkie]

Agreed, I realize my suggestion is not a complete solution but it is a step in the right direction, as is your suggestion. With this PR we are suggesting parameter count verification and not input validity.

[moogle19]

Just thought about it and realized that my comment was stupid, because something like iohyve delete -f -f would be verified successfully in my implementation and is not allowed.

[EpiJunkie]

It is a valid point.

[pr1ntf]

@moogle19 makes a very good point on the flags. I think in order to future-proof this idea, we should add a Contributing document to the Wiki to remind future devs to add this type of check if they intend to add a function to iohyve.

I'll get started on that document if you would like to fix the conflicts, @moogle19.

[pr1ntf]

AYYYYYYYY only two weeks wait time this time. My bad.

[moogle19]