[PATCH v2] hotplug/Linux: add iscsi block hotplug script

[PATCH v2] hotplug/Linux: add iscsi block hotplug script

[Roger Pau Monne]

This hotplug script has been tested with IET and NetBSD iSCSI targets,
without authentication.

This hotplug script will only work with PV guests not using pygrub.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@citrix.com>
---
Changes since v1:
 * Added an example in the script header to show the usage.
 * Changed shebang to use bash.
 * Replace type with command, for portability reasons.
 * Replace echo with fatal/log functions.
 * Include block-common.sh.
 * Prevent infinite loop when waitign for device to settle.
 * Only redirect stdout to /dev/null.
Changes due to 4.3 release freeze:
 * We can no longer provide a user/password, because they will be
   stored in xenstore "params" backend node, which can be read by the
   guest.
 * Only works with PV domains because we don't have a hook in libxl to
   pass the block device created by the script to Qemu.
 * Doesn't work with pygrub because we don't call the script until
   pygrub has already been executed, and even if we called it earlier
   we still need a hook in order to provide the right block device to
   pygrub.
Changes since v1:
 * Add -e to script shebang, and use 'set +e' if we know hotplug
   execution might fail.
---
 tools/hotplug/Linux/Makefile    |    1 +
 tools/hotplug/Linux/block-iscsi |  209 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 210 insertions(+), 0 deletions(-)
 create mode 100644 tools/hotplug/Linux/block-iscsi

diff --git a/tools/hotplug/Linux/Makefile b/tools/hotplug/Linux/Makefile
index 0605559..98c7738 100644
--- a/tools/hotplug/Linux/Makefile
+++ b/tools/hotplug/Linux/Makefile
@@ -21,6 +21,7 @@ XEN_SCRIPTS += blktap
 XEN_SCRIPTS += xen-hotplug-cleanup
 XEN_SCRIPTS += external-device-migrate
 XEN_SCRIPTS += vscsi
+XEN_SCRIPTS += block-iscsi
 XEN_SCRIPT_DATA = xen-script-common.sh locking.sh logging.sh
 XEN_SCRIPT_DATA += xen-hotplug-common.sh xen-network-common.sh vif-common.sh
 XEN_SCRIPT_DATA += block-common.sh
diff --git a/tools/hotplug/Linux/block-iscsi b/tools/hotplug/Linux/block-iscsi
new file mode 100644
index 0000000..9f74c20
--- /dev/null
+++ b/tools/hotplug/Linux/block-iscsi
@@ -0,0 +1,209 @@
+#!/bin/bash -e
+#
+# Open-iSCSI Xen block device hotplug script
+#
+# Author Roger Pau Monné <roger.pau@citrix.com>
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU Lesser General Public License as published
+# by the Free Software Foundation; version 2.1 only. with the special
+# exception on linking described in file LICENSE.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU Lesser General Public License for more details.
+#
+# Usage:
+#
+# Target should be specified using the following syntax:
+#
+# script=block-iscsi,vdev=xvda,target=iqn=<iqn>,portal=<portal IP>
+#
+# Portal address must be in IP format.
+#
+
+dir=$(dirname "$0")
+. "$dir/block-common.sh"
+
+remove_label()
+{
+    echo $1 | sed "s/^\("$2"\)//"
+}
+
+check_tools()
+{
+    if ! command -v iscsiadm > /dev/null 2>&1; then
+        fatal "Unable to find iscsiadm tool"
+        return 1
+    fi
+    if [ "$multipath" = "y" ] && ! command -v multipath > /dev/null 2>&1; then
+        fatal "Unable to find multipath"
+        return 1
+    fi
+}
+
+# Sets the following global variables based on the params field passed in as
+# a parameter: iqn, portal, auth_method, user, multipath, password
+parse_target()
+{
+    # set multipath default value
+    multipath="n"
+    for param in $(echo "$1" | tr "," "\n")
+    do
+        case $param in
+        iqn=*)
+            iqn=$(remove_label $param "iqn=")
+            ;;
+        portal=*)
+            portal=$(remove_label $param "portal=")
+            ;;
+        multipath=*)
+            multipath=$(remove_label $param "multipath=")
+            ;;
+        esac
+    done
+    if [ -z "$iqn" ] || [ -z "$portal" ]; then
+        fatal "iqn= and portal= are required parameters"
+        return 1
+    fi
+    if [ "$multipath" != "y" ] && [ "$multipath" != "n" ]; then
+        fatal "multipath valid values are y and n, $multipath not a valid value"
+        return 1
+    fi
+    return 0
+}
+
+# Sets $dev to point to the device associated with the value in iqn
+find_device()
+{
+    count=0
+    while [ ! -e /dev/disk/by-path/*"$iqn"-lun-0 ]; do
+        sleep 0.1
+        count=`expr $count + 1`
+        if [ count = 100 ]; then
+            # 10s timeout while waiting for iSCSI disk to settle
+            fatal "timeout waiting for iSCSI disk to settle"
+            return 1
+        fi
+    done
+    sddev=$(readlink -f /dev/disk/by-path/*"$iqn"-lun-0 || true)
+    if [ ! -b "$sddev" ]; then
+        fatal "Unable to find attached device path"
+        return 1
+    fi
+    if [ "$multipath" = "y" ]; then
+        mdev=$(multipath -ll "$sddev" | head -1 | awk '{ print $1}')
+        if [ ! -b /dev/mapper/"$mdev" ]; then
+            fatal "Unable to find attached device multipath"
+            return 1
+        fi
+        dev="/dev/mapper/$mdev"
+    else
+        dev="$sddev"
+    fi
+    return 0
+}
+
+# Attaches the target $iqn in $portal and sets $dev to point to the
+# multipath device
+attach()
+{
+    set +e
+    iscsiadm -m node --targetname "$iqn" -p "$portal" --login > /dev/null
+    rc=$?
+    set -e
+    if [ $rc -ne 0 ]; then
+        fatal "Unable to connect to target"
+        return 1
+    fi
+    find_device
+    if [ $? -ne 0 ]; then
+        fatal "Unable to find iSCSI device"
+        return 1
+    fi
+    return 0
+}
+
+# Discovers targets in $portal and checks that $iqn is one of those targets
+# Also sets the auth parameters to attach the device
+prepare()
+{
+    # Check if target is already opened
+    set +e
+    iscsiadm -m session 2>&1 | grep -q "$iqn"
+    rc=$?
+    set -e
+    if [ $rc -eq 0 ]; then
+        fatal "Device already opened"
+        return 1
+    fi
+    # Discover portal targets
+    set +e
+    iscsiadm -m discovery -t st -p $portal 2>&1 | grep -q "$iqn"
+    rc=$?
+    set -e
+    if [ $rc -ne 0 ]; then
+        fatal "No matching target iqn found"
+        return 1
+    fi
+    return 0
+}
+
+# Attaches the device and writes xenstore backend entries to connect
+# the device
+add()
+{
+    attach
+    if [ $? -ne 0 ]; then
+        fatal "Failed to attach device"
+        return 1
+    fi
+    write_dev $dev
+    return 0
+}
+
+# Disconnects the device
+remove()
+{
+    find_device
+    if [ $? -ne 0 ]; then
+        fatal "Unable to find device"
+        return 1
+    fi
+    set +e
+    iscsiadm -m node --targetname "$iqn" -p "$portal" --logout > /dev/null
+    rc=$?
+    set -e
+    if [ $rc -ne 0 ]; then
+        fatal "Unable to disconnect target"
+        return 1
+    fi
+    return 0
+}
+
+command=$1
+target=$(xenstore-read $XENBUS_PATH/params || true)
+if [ -z "$target" ]; then
+    fatal "No information about the target"
+    exit 1
+fi
+
+parse_target "$target"
+
+check_tools || exit 1
+
+case $command in
+add)
+    prepare
+    add
+    exit $?
+    ;;
+remove)
+    remove
+    exit $?
+    ;;
+*)
+    exit 1
+    ;;
+esac
-- 
1.7.7.5 (Apple Git-26)


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v2] hotplug/Linux: add iscsi block hotplug script

[Ian Campbell]

On Fri, 2013-04-26 at 14:30 +0100, Roger Pau Monne wrote:
> This hotplug script has been tested with IET and NetBSD iSCSI targets,
> without authentication.
> 
> This hotplug script will only work with PV guests not using pygrub.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Ian Jackson <ian.jackson@citrix.com>
> ---
> Changes since v1:
>  * Added an example in the script header to show the usage.
>  * Changed shebang to use bash.
>  * Replace type with command, for portability reasons.
>  * Replace echo with fatal/log functions.
>  * Include block-common.sh.
>  * Prevent infinite loop when waitign for device to settle.
>  * Only redirect stdout to /dev/null.
> Changes due to 4.3 release freeze:
>  * We can no longer provide a user/password, because they will be
>    stored in xenstore "params" backend node, which can be read by the
>    guest.
>  * Only works with PV domains because we don't have a hook in libxl to
>    pass the block device created by the script to Qemu.
>  * Doesn't work with pygrub because we don't call the script until
>    pygrub has already been executed, and even if we called it earlier
>    we still need a hook in order to provide the right block device to
>    pygrub.
> Changes since v1:
>  * Add -e to script shebang, and use 'set +e' if we know hotplug
>    execution might fail.
> ---
>  tools/hotplug/Linux/Makefile    |    1 +
>  tools/hotplug/Linux/block-iscsi |  209 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 210 insertions(+), 0 deletions(-)
>  create mode 100644 tools/hotplug/Linux/block-iscsi
> 
> diff --git a/tools/hotplug/Linux/Makefile b/tools/hotplug/Linux/Makefile
> index 0605559..98c7738 100644
> --- a/tools/hotplug/Linux/Makefile
> +++ b/tools/hotplug/Linux/Makefile
> @@ -21,6 +21,7 @@ XEN_SCRIPTS += blktap
>  XEN_SCRIPTS += xen-hotplug-cleanup
>  XEN_SCRIPTS += external-device-migrate
>  XEN_SCRIPTS += vscsi
> +XEN_SCRIPTS += block-iscsi
>  XEN_SCRIPT_DATA = xen-script-common.sh locking.sh logging.sh
>  XEN_SCRIPT_DATA += xen-hotplug-common.sh xen-network-common.sh vif-common.sh
>  XEN_SCRIPT_DATA += block-common.sh
> diff --git a/tools/hotplug/Linux/block-iscsi b/tools/hotplug/Linux/block-iscsi
> new file mode 100644
> index 0000000..9f74c20
> --- /dev/null
> +++ b/tools/hotplug/Linux/block-iscsi
> @@ -0,0 +1,209 @@
> +#!/bin/bash -e
> +#
> +# Open-iSCSI Xen block device hotplug script
> +#
> +# Author Roger Pau Monné <roger.pau@citrix.com>
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU Lesser General Public License as published
> +# by the Free Software Foundation; version 2.1 only. with the special
> +# exception on linking described in file LICENSE.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU Lesser General Public License for more details.
> +#
> +# Usage:
> +#
> +# Target should be specified using the following syntax:
> +#
> +# script=block-iscsi,vdev=xvda,target=iqn=<iqn>,portal=<portal IP>
> +#
> +# Portal address must be in IP format.
> +#
> +
> +dir=$(dirname "$0")
> +. "$dir/block-common.sh"
> +
> +remove_label()
> +{
> +    echo $1 | sed "s/^\("$2"\)//"
> +}
> +
> +check_tools()
> +{
> +    if ! command -v iscsiadm > /dev/null 2>&1; then
> +        fatal "Unable to find iscsiadm tool"
> +        return 1
> +    fi
> +    if [ "$multipath" = "y" ] && ! command -v multipath > /dev/null 2>&1; then
> +        fatal "Unable to find multipath"

this isn't critical unless the user asked for multipath I think?

> +        return 1
> +    fi
> +}
> +
> +# Sets the following global variables based on the params field passed in as
> +# a parameter: iqn, portal, auth_method, user, multipath, password
> +parse_target()
> +{
> +    # set multipath default value
> +    multipath="n"
> +    for param in $(echo "$1" | tr "," "\n")
> +    do
> +        case $param in
> +        iqn=*)
> +            iqn=$(remove_label $param "iqn=")
> +            ;;
> +        portal=*)
> +            portal=$(remove_label $param "portal=")
> +            ;;
> +        multipath=*)
> +            multipath=$(remove_label $param "multipath=")
> +            ;;
> +        esac
> +    done
> +    if [ -z "$iqn" ] || [ -z "$portal" ]; then
> +        fatal "iqn= and portal= are required parameters"
> +        return 1
> +    fi
> +    if [ "$multipath" != "y" ] && [ "$multipath" != "n" ]; then
> +        fatal "multipath valid values are y and n, $multipath not a valid value"
> +        return 1

fatal already exits.

> +    fi
> +    return 0
> +}
> +
> +# Sets $dev to point to the device associated with the value in iqn
> +find_device()
> +{
> +    count=0
> +    while [ ! -e /dev/disk/by-path/*"$iqn"-lun-0 ]; do
> +        sleep 0.1
> +        count=`expr $count + 1`
> +        if [ count = 100 ]; then
> +            # 10s timeout while waiting for iSCSI disk to settle
> +            fatal "timeout waiting for iSCSI disk to settle"
> +            return 1
> +        fi
> +    done
> +    sddev=$(readlink -f /dev/disk/by-path/*"$iqn"-lun-0 || true)
> +    if [ ! -b "$sddev" ]; then
> +        fatal "Unable to find attached device path"
> +        return 1
> +    fi
> +    if [ "$multipath" = "y" ]; then
> +        mdev=$(multipath -ll "$sddev" | head -1 | awk '{ print $1}')
> +        if [ ! -b /dev/mapper/"$mdev" ]; then
> +            fatal "Unable to find attached device multipath"
> +            return 1
> +        fi
> +        dev="/dev/mapper/$mdev"
> +    else
> +        dev="$sddev"
> +    fi
> +    return 0
> +}
> +
> +# Attaches the target $iqn in $portal and sets $dev to point to the
> +# multipath device
> +attach()
> +{
> +    set +e
> +    iscsiadm -m node --targetname "$iqn" -p "$portal" --login > /dev/null
> +    rc=$?
> +    set -e
> +    if [ $rc -ne 0 ]; then
> +        fatal "Unable to connect to target"
> +        return 1
> +    fi

This is roughly the same as do_or_die iscsiadm -m node etc , apart from
the specifics of the log message.

If you want to keep the message as is then is it the same as 
	iscsiadm -m node ... || fatal "Unable,..."

> +    find_device

set -e is back in force here, so won't this barf before you have a
chance to check $? 

In fact now I look at it find device either calls fatal (and exits) or
returns 0...

> +    if [ $? -ne 0 ]; then
> +        fatal "Unable to find iSCSI device"
> +        return 1
> +    fi
> +    return 0
> +}
> +
> +# Discovers targets in $portal and checks that $iqn is one of those targets
> +# Also sets the auth parameters to attach the device
> +prepare()
> +{
> +    # Check if target is already opened
> +    set +e
> +    iscsiadm -m session 2>&1 | grep -q "$iqn"

Does the use of a pipe here make this incompatible with do_or_die? or
the use of || fatal "mmm"?

> +    rc=$?
> +    set -e
> +    if [ $rc -eq 0 ]; then
> +        fatal "Device already opened"
> +        return 1
> +    fi
> +    # Discover portal targets
> +    set +e
> +    iscsiadm -m discovery -t st -p $portal 2>&1 | grep -q "$iqn"
> +    rc=$?
> +    set -e
> +    if [ $rc -ne 0 ]; then
> +        fatal "No matching target iqn found"
> +        return 1
> +    fi
> +    return 0
> +}
> +
> +# Attaches the device and writes xenstore backend entries to connect
> +# the device
> +add()
> +{
> +    attach
> +    if [ $? -ne 0 ]; then
> +        fatal "Failed to attach device"
> +        return 1
> +    fi

attach either succeeds or exits.

> +    write_dev $dev
> +    return 0
> +}
> +
> +# Disconnects the device
> +remove()
> +{
> +    find_device
> +    if [ $? -ne 0 ]; then
> +        fatal "Unable to find device"
> +        return 1
> +    fi
> +    set +e
> +    iscsiadm -m node --targetname "$iqn" -p "$portal" --logout > /dev/null
> +    rc=$?
> +    set -e
> +    if [ $rc -ne 0 ]; then
> +        fatal "Unable to disconnect target"
> +        return 1
> +    fi
> +    return 0
> +}
> +
> +command=$1
> +target=$(xenstore-read $XENBUS_PATH/params || true)
> +if [ -z "$target" ]; then
> +    fatal "No information about the target"
> +    exit 1
> +fi
> +
> +parse_target "$target"
> +
> +check_tools || exit 1
> +
> +case $command in
> +add)
> +    prepare
> +    add
> +    exit $?
> +    ;;
> +remove)
> +    remove
> +    exit $?
> +    ;;
> +*)
> +    exit 1
> +    ;;
> +esac



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v2] hotplug/Linux: add iscsi block hotplug script

[Roger Pau Monné]

On 26/04/13 17:17, Ian Campbell wrote:
> On Fri, 2013-04-26 at 14:30 +0100, Roger Pau Monne wrote:
>> This hotplug script has been tested with IET and NetBSD iSCSI targets,
>> without authentication.
>>
>> This hotplug script will only work with PV guests not using pygrub.
>>
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> Cc: Ian Campbell <ian.campbell@citrix.com>
>> Cc: Ian Jackson <ian.jackson@citrix.com>
>> ---
>> Changes since v1:
>>  * Added an example in the script header to show the usage.
>>  * Changed shebang to use bash.
>>  * Replace type with command, for portability reasons.
>>  * Replace echo with fatal/log functions.
>>  * Include block-common.sh.
>>  * Prevent infinite loop when waitign for device to settle.
>>  * Only redirect stdout to /dev/null.
>> Changes due to 4.3 release freeze:
>>  * We can no longer provide a user/password, because they will be
>>    stored in xenstore "params" backend node, which can be read by the
>>    guest.
>>  * Only works with PV domains because we don't have a hook in libxl to
>>    pass the block device created by the script to Qemu.
>>  * Doesn't work with pygrub because we don't call the script until
>>    pygrub has already been executed, and even if we called it earlier
>>    we still need a hook in order to provide the right block device to
>>    pygrub.
>> Changes since v1:
>>  * Add -e to script shebang, and use 'set +e' if we know hotplug
>>    execution might fail.
>> ---
>>  tools/hotplug/Linux/Makefile    |    1 +
>>  tools/hotplug/Linux/block-iscsi |  209 +++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 210 insertions(+), 0 deletions(-)
>>  create mode 100644 tools/hotplug/Linux/block-iscsi
>>
>> diff --git a/tools/hotplug/Linux/Makefile b/tools/hotplug/Linux/Makefile
>> index 0605559..98c7738 100644
>> --- a/tools/hotplug/Linux/Makefile
>> +++ b/tools/hotplug/Linux/Makefile
>> @@ -21,6 +21,7 @@ XEN_SCRIPTS += blktap
>>  XEN_SCRIPTS += xen-hotplug-cleanup
>>  XEN_SCRIPTS += external-device-migrate
>>  XEN_SCRIPTS += vscsi
>> +XEN_SCRIPTS += block-iscsi
>>  XEN_SCRIPT_DATA = xen-script-common.sh locking.sh logging.sh
>>  XEN_SCRIPT_DATA += xen-hotplug-common.sh xen-network-common.sh vif-common.sh
>>  XEN_SCRIPT_DATA += block-common.sh
>> diff --git a/tools/hotplug/Linux/block-iscsi b/tools/hotplug/Linux/block-iscsi
>> new file mode 100644
>> index 0000000..9f74c20
>> --- /dev/null
>> +++ b/tools/hotplug/Linux/block-iscsi
>> @@ -0,0 +1,209 @@
>> +#!/bin/bash -e
>> +#
>> +# Open-iSCSI Xen block device hotplug script
>> +#
>> +# Author Roger Pau Monné <roger.pau@citrix.com>
>> +#
>> +# This program is free software; you can redistribute it and/or modify
>> +# it under the terms of the GNU Lesser General Public License as published
>> +# by the Free Software Foundation; version 2.1 only. with the special
>> +# exception on linking described in file LICENSE.
>> +#
>> +# This program is distributed in the hope that it will be useful,
>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +# GNU Lesser General Public License for more details.
>> +#
>> +# Usage:
>> +#
>> +# Target should be specified using the following syntax:
>> +#
>> +# script=block-iscsi,vdev=xvda,target=iqn=<iqn>,portal=<portal IP>
>> +#
>> +# Portal address must be in IP format.
>> +#
>> +
>> +dir=$(dirname "$0")
>> +. "$dir/block-common.sh"
>> +
>> +remove_label()
>> +{
>> +    echo $1 | sed "s/^\("$2"\)//"
>> +}
>> +
>> +check_tools()
>> +{
>> +    if ! command -v iscsiadm > /dev/null 2>&1; then
>> +        fatal "Unable to find iscsiadm tool"
>> +        return 1
>> +    fi
>> +    if [ "$multipath" = "y" ] && ! command -v multipath > /dev/null 2>&1; then
>> +        fatal "Unable to find multipath"
> 
> this isn't critical unless the user asked for multipath I think?

That's why we check for $multipath = "y", if the user requested
multipath and the hotplug script cannot find it, crash.

> 
>> +        return 1
>> +    fi
>> +}
>> +
>> +# Sets the following global variables based on the params field passed in as
>> +# a parameter: iqn, portal, auth_method, user, multipath, password
>> +parse_target()
>> +{
>> +    # set multipath default value
>> +    multipath="n"
>> +    for param in $(echo "$1" | tr "," "\n")
>> +    do
>> +        case $param in
>> +        iqn=*)
>> +            iqn=$(remove_label $param "iqn=")
>> +            ;;
>> +        portal=*)
>> +            portal=$(remove_label $param "portal=")
>> +            ;;
>> +        multipath=*)
>> +            multipath=$(remove_label $param "multipath=")
>> +            ;;
>> +        esac
>> +    done
>> +    if [ -z "$iqn" ] || [ -z "$portal" ]; then
>> +        fatal "iqn= and portal= are required parameters"
>> +        return 1
>> +    fi
>> +    if [ "$multipath" != "y" ] && [ "$multipath" != "n" ]; then
>> +        fatal "multipath valid values are y and n, $multipath not a valid value"
>> +        return 1
> 
> fatal already exits.

Ack, will change all occurrences.

> 
>> +    fi
>> +    return 0
>> +}
>> +
>> +# Sets $dev to point to the device associated with the value in iqn
>> +find_device()
>> +{
>> +    count=0
>> +    while [ ! -e /dev/disk/by-path/*"$iqn"-lun-0 ]; do
>> +        sleep 0.1
>> +        count=`expr $count + 1`
>> +        if [ count = 100 ]; then
>> +            # 10s timeout while waiting for iSCSI disk to settle
>> +            fatal "timeout waiting for iSCSI disk to settle"
>> +            return 1
>> +        fi
>> +    done
>> +    sddev=$(readlink -f /dev/disk/by-path/*"$iqn"-lun-0 || true)
>> +    if [ ! -b "$sddev" ]; then
>> +        fatal "Unable to find attached device path"
>> +        return 1
>> +    fi
>> +    if [ "$multipath" = "y" ]; then
>> +        mdev=$(multipath -ll "$sddev" | head -1 | awk '{ print $1}')
>> +        if [ ! -b /dev/mapper/"$mdev" ]; then
>> +            fatal "Unable to find attached device multipath"
>> +            return 1
>> +        fi
>> +        dev="/dev/mapper/$mdev"
>> +    else
>> +        dev="$sddev"
>> +    fi
>> +    return 0
>> +}
>> +
>> +# Attaches the target $iqn in $portal and sets $dev to point to the
>> +# multipath device
>> +attach()
>> +{
>> +    set +e
>> +    iscsiadm -m node --targetname "$iqn" -p "$portal" --login > /dev/null
>> +    rc=$?
>> +    set -e
>> +    if [ $rc -ne 0 ]; then
>> +        fatal "Unable to connect to target"
>> +        return 1
>> +    fi
> 
> This is roughly the same as do_or_die iscsiadm -m node etc , apart from
> the specifics of the log message.
> 
> If you want to keep the message as is then is it the same as 
> 	iscsiadm -m node ... || fatal "Unable,..."

I've switched to do_or_die, at the end the message didn't provide much
more info, and it's probably better to print the command that failed.

>> +    find_device
> 
> set -e is back in force here, so won't this barf before you have a
> chance to check $? 
> 
> In fact now I look at it find device either calls fatal (and exits) or
> returns 0...

Yes, since the script calls fatal I've removed all the checks for the
return values of internal functions.

> 
>> +    if [ $? -ne 0 ]; then
>> +        fatal "Unable to find iSCSI device"
>> +        return 1
>> +    fi
>> +    return 0
>> +}
>> +
>> +# Discovers targets in $portal and checks that $iqn is one of those targets
>> +# Also sets the auth parameters to attach the device
>> +prepare()
>> +{
>> +    # Check if target is already opened
>> +    set +e
>> +    iscsiadm -m session 2>&1 | grep -q "$iqn"
> 
> Does the use of a pipe here make this incompatible with do_or_die? or
> the use of || fatal "mmm"?

In this specific case I should use &&, because we expect the command to
fail.

> 
>> +    rc=$?
>> +    set -e
>> +    if [ $rc -eq 0 ]; then
>> +        fatal "Device already opened"
>> +        return 1
>> +    fi
>> +    # Discover portal targets
>> +    set +e
>> +    iscsiadm -m discovery -t st -p $portal 2>&1 | grep -q "$iqn"

Here a || is appropriate.

>> +    rc=$?
>> +    set -e
>> +    if [ $rc -ne 0 ]; then
>> +        fatal "No matching target iqn found"
>> +        return 1
>> +    fi
>> +    return 0
>> +}
>> +
>> +# Attaches the device and writes xenstore backend entries to connect
>> +# the device
>> +add()
>> +{
>> +    attach
>> +    if [ $? -ne 0 ]; then
>> +        fatal "Failed to attach device"
>> +        return 1
>> +    fi
> 
> attach either succeeds or exits.

Done.

> 
>> +    write_dev $dev
>> +    return 0
>> +}
>> +
>> +# Disconnects the device
>> +remove()
>> +{
>> +    find_device
>> +    if [ $? -ne 0 ]; then
>> +        fatal "Unable to find device"
>> +        return 1
>> +    fi
>> +    set +e
>> +    iscsiadm -m node --targetname "$iqn" -p "$portal" --logout > /dev/null
>> +    rc=$?
>> +    set -e
>> +    if [ $rc -ne 0 ]; then
>> +        fatal "Unable to disconnect target"
>> +        return 1
>> +    fi
>> +    return 0
>> +}
>> +
>> +command=$1
>> +target=$(xenstore-read $XENBUS_PATH/params || true)
>> +if [ -z "$target" ]; then
>> +    fatal "No information about the target"
>> +    exit 1
>> +fi
>> +
>> +parse_target "$target"
>> +
>> +check_tools || exit 1
>> +
>> +case $command in
>> +add)
>> +    prepare
>> +    add
>> +    exit $?
>> +    ;;
>> +remove)
>> +    remove
>> +    exit $?
>> +    ;;
>> +*)
>> +    exit 1
>> +    ;;
>> +esac

I'm going to send v3 ASAP with all the comments addressed.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel