Xen devel - Apr 2013 - [PATCH v2] hotplug/Linux: add iscsi block hotplug script

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

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

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