[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] block-iscsi with Xen 4.5 / 4.6
On 2016-05-04 20:50, Roger Pau Monné wrote:
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.
I'll look at this - I've been steered away from this style of code my
entire life, so its a little unfamiliar to me.
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.
I agree. The reason for not sending things through as a patch right now
is:
a) It doesn't work; and
b) It's currently in a horrible state.
I do intend to share this for inclusion once I get the problems worked
out. I do agree it should be structured like the multipath option here.
--
Steven Haigh
Email: netwiz@xxxxxxxxx
Web: https://www.crc.id.au
Phone: (03) 9001 6090 - 0412 935 897
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
