Re: [Xen-devel] block-iscsi with Xen 4.5 / 4.6

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

On Wed, May 04, 2016 at 08:18:18PM +1000, Steven Haigh wrote:
> On 4/05/2016 7:37 PM, Roger Pau Monné wrote:
> > On Wed, May 04, 2016 at 06:41:26PM +1000, Steven Haigh wrote:
> >> On 4/05/2016 5:34 PM, Roger Pau Monné wrote:
> >>> On Wed, May 04, 2016 at 03:06:23PM +1000, Steven Haigh wrote:
> >>> It is important for us to use the '-e' in order to make sure all the 
> >>> failure 
> >>> points are correctly handled, without the '-e' some command might fail 
> >>> and 
> >>> the script wouldn't realize.
> >>
> >> I honestly think this is pretty nasty. While it may not be true of all
> >> scripts, the block-iscsi script can only really fail in a couple of
> >> places - yet we have this set of procedures called:
> >>
> >> parse_target -> check_tools -> prepare -> add -> attach -> find_device
> >> -> write_dev.
> >>
> >> At least check_tools, prepare, add, attach, find_device could all be
> >> rolled into a single function - as the majority of the rest is 1-4 lines
> >> of code.
> > 
> > No, check_tools is used by both the attach and the detach path, so it 
> > cannot 
> > be rolled into a single function together with the other ones, and the same 
> > applies to mostly all other functions (find_device is also shared between 
> > the add and remove functions).
> > 
> > IMHO, I think the current code is fine because each function has a small 
> > logical task to accomplish, so it's easy to make sure each function does 
> > what it's supposed to do, nothing more and nothing less. Batching 
> > everything 
> > into one big function would make this harder.
> > 
> > That doesn't mean that I'm not open to improving it, so if you think it 
> > would be better/easier using some other logical organization patches are 
> > welcome :).
> 
> Right now, my changes are here:
>       http://paste.fedoraproject.org/362462/62356799/

It seems like you can achieve what you are trying to do by using:

if ! sg_persist -d ${dev} -o -G -S ${key}; then
        cleanup code (possibly just calling the remove function)
fi

This should work fine with '-e' set.

Having that said, you should post the code as a patch with a proper SoB [0]. 
Also, look at how the "multipath" argument is passed, IMHO you should do the 
same and add a "locked" parameter that you can also pass in inside of the 
"target" field.

Roger.

[0] http://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches

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