Re: [PATCH 3/4] hotplug: Update block-tap

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Thu, Feb 01, 2024 at 01:30:23PM -0500, Jason Andryuk wrote:
> Implement a sharing check like the regular block script.
> 
> Checking tapback inside block-tap is too late since it needs to be
> running to transition the backend to InitWait before block-tap is run.
> 
> tap-ctl check will be removed when the requirement for the blktap kernel
> driver is removed.  Remove it now as it is of limited use.
> 
> find_device() needs to be non-fatal allow a sharing check.
> 
> Only write physical-device-path because that is all that tapback needs.
> Also write_dev doesn't handled files and would incorrectly store
> physical-device as 0:0 which would confuse the minor inside tapback

Missing SOB.


Is `block-tap` still going to work with the example given in the script
header? That is:
    "script=block-tap,vdev=xvda,target=<type>:<file>"
Or maybe, that example is already broken?

> ---
> diff --git a/tools/hotplug/Linux/block-tap b/tools/hotplug/Linux/block-tap
> index 89247921b9..5eca09f0f6 100755
> --- a/tools/hotplug/Linux/block-tap
> +++ b/tools/hotplug/Linux/block-tap
> +count_using()
> +{
> +  local file="$1"
> +  local f
> +
> +  local i=0
> +  local base_path="$XENBUS_BASE_PATH/$XENBUS_TYPE"
> +  for dom in $(xenstore-list "$base_path")
> +  do
> +    for dev in $(xenstore-list "$base_path/$dom")

This function is probably missing "local dom dev".

> +    do
> +      f=$(xenstore_read_default "$base_path/$dom/$dev/params" "")
> +      f=$(echo "$f" | cut -d ":" -f 2)
> +
> +      if [ -n "$f" ] && [ "$file" = $f ] ; then
> +          i=$(( i + 1 ))
> +      fi
> +    done
> +  done
> +
> +  echo "$i"
> +}
> +

> +check_tap_sharing()
> +{
> +  local file="$1"
> +  local mode="$2"
> +  local dev
> +
> +  local base_path="$XENBUS_BASE_PATH/$XENBUS_TYPE"
> +  for dom in $(xenstore-list "$base_path") ; do

Should we add "local dom" to the function?

> +    for dev in $(xenstore-list "$base_path/$dom") ; do
> +      f=$(xenstore_read_default "$base_path/$dom/$dev/params" "")

Same here, maybe "local f" would be good to have too.

> @@ -89,15 +183,57 @@ find_device()
>  # the device
>  add()
>  {
> -    dev=$(tap-ctl create -a $target)
> -    write_dev $dev
> +    local minor
> +    local pid
> +    local res
> +
> +    claim_lock "block"
> +
> +    if find_device; then
> +        result=$( check_tap_sharing "$file" "$mode" )
> +        if [ "$result" != "ok" ] ; then
> +            do_ebusy "tap $type file $file in use " "$mode" "${result%% *}"
> +        fi
> +    else
> +        tap_create

The new function tap_create() is doing something similar to the replace
`tap-ctl create` call, right?


>  # Disconnects the device
>  remove()
>  {
> -    find_device
> -    do_or_die tap-ctl destroy -p ${pid} -m ${minor} > /dev/null
> +    local minor
> +    local pid
> +
> +    claim_lock "block"
> +
> +    if tap_shared ; then
> +        return
> +    fi
> +
> +    minor=$( xenstore_read "$XENBUS_PATH/minor" )
> +    pid=$( xenstore_read "$XENBUS_PATH/pid" )
> +
> +    [ -n "$minor" ] || fatal "minor missing"
> +    [ -n "$pid" ] || fatal "pid missing"
> +    do_or_die tap-ctl close -p "$pid" -m "$minor" > /dev/null
> +    do_or_die tap-ctl detach -p "$pid" -m "$minor" > /dev/null

Should we also call `tap-ctl free`, like `tap-ctl destroy` seems to do?

> +
> +    release_lock "block"
>  }
>  
>  command=$1

Thanks,

-- 
Anthony PERARD