Re: [Xen-devel] [PATCH] hotplug/Linux: add iscsi block hotplug script

[Roger Pau Monnà <roger.pau@xxxxxxxxxx>]

On 25/04/13 15:02, Ian Campbell wrote:
> On Thu, 2013-04-25 at 13:32 +0100, Roger Pau Monne wrote:
>> On 24/04/13 18:48, Ian Campbell wrote:
>>> On Wed, 2013-04-24 at 17:31 +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.
>>>
>>> Thanks. Does this need any docs somewhere, e.g. how to format the target
>>> spec?
>>>
>>> Is this derived from any of the other block-iscsi scripts floating
>>> around on the net?
>>>
>>>> Signed-off-by: Roger Pau Monnà <roger.pau@xxxxxxxxxx>
>>>> Cc: Ian Campbell <ian.campbell@xxxxxxxxxx>
>>>> Cc: Ian Jackson <ian.jackson@xxxxxxxxxx>
>>>> ---
>>>> 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 |  198 
>>>> +++++++++++++++++++++++++++++++++++++++
>>>>  2 files changed, 199 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..3cd6e34
>>>> --- /dev/null
>>>> +++ b/tools/hotplug/Linux/block-iscsi
>>>> @@ -0,0 +1,198 @@
>>>> +#!/bin/sh -e
>>>
>>> Does this rely on any bashisms? Most of the other scripts use bash here,
>>> and I'd be a bit worried about bashisms in the common code.
>>
>> This script originally didn't use any bashisms, but if I include
>> block-common.sh I have to use bash for sure.
> 
> I think I would prefer using the library functions over avoiding bash.
> 
> If someone hates bash that much they should port the library over
> too ;-)
> 
>>>
>>>> +        echo "Unable to find iscsiadm tool"
>>>> +        return 1
>>>
>>> Error paths should use fatal() from xen-hotplug-common.sh so the error
>>> is propagated to xenstore and libxl can print it.
>>
>> Yes, if we call the script directly from libxl the output form the
>> script will be logged by libxl,
> 
> Actually logged or just sent to stdout which libxl hasn't interfered
> with? or does libxl suck in the stdout of the script? It looks to me
> like libxl does the former.
> 
>> but since this script can also be called from udev I have to use fatal.
> 
> I think even for libxl, since it writes the error node which libxl does
> then log.

Yes, libxl checks the node, so it's better to use fatal/log and all this
functions instead of printing to stdout/stderr.

> 
>>>
>>>> +
>>>> +# Sets $dev to point to the device associated with the value in iqn
>>>> +find_device()
>>>> +{
>>>> +    while [ ! -e /dev/disk/by-path/*"$iqn"-lun-0 ]; do
>>>> +        sleep 0.1
>>>> +    done
>>>
>>> This loop is potentially infinite? (i.e. if something is broken)
>>
>> Yes, I was trusting the timeout in libxl to prevent that, but again
>> since this is not using the new calling convention it should work
>> properly with udev and I have no idea if udev has a timeout, but we
>> shouldn't rely on that.
> 
> Good.
> 
>>>> +    if [ ! -b "$sddev" ]; then
>>>> +        echo "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
>>>> +            echo "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 
>>>> 2>&1
>>>
>>> Might the error message be useful here? if you don't redirect
>>> to /dev/null then you can use exec >>/tmp/hotplug.log tricks to debug
>>> things...
>>
>> iscsiadm always prints output, even when the target is successfully
>> attached, and I haven't found any option to prevent it, so for now I
>> guess we will have to redirect it to /dev/null
> 
> Does it produce output on stderr even on success too? i.e. can we just
> redirect stdout? In a sane world the useful error messages would be on
> stderr ;-)

Mmmm, good point, yes, I can redirect stdout only.

> 
> Ian.
> 


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