Nils Goroll wrote: > Hi Peter, > > thank you for having a look at my suggestion. > >> I must admit I dislike the use of the goto's within the suggested fix - >> I would rather see the missing calls to meta_unlock(local_sp...) added >> in. > > I had thought about this. The reasons why I thought the gotos were > somehow better were that > > - they remove duplicate code and > - the labels add some semantics. >
Yes; understood. > With these points in mind, do you still want the gotos removed? I think > it is important to have a solution which the primary maintainers are > happy with, even if it's not the one I'd prefer (honestly and seriously, > no bad feelings here). I just don't like goto's :-) Yes removing the duplicate code is goodness but this is trivial duplicate code. This is my personal preference. Indeed in the C-style guide we have: "While not completely avoidable, use of goto is discouraged." So I suggest have a look at how the ret from meta_set_purge is dealt with and then code accordingly. Once we have a sponsor assigned to this the formal code review can proceed. > >> With the change on line 1329 you are removing the ability to detect >> where in the meta_set_purge code a failure occurred. The meta_set_purge >> code returns 0 on success and => 1 on failure (have a look how the rval >> is used within that function). > > Oops, good catch, thank you. I'll change this after your next reply. Okay. > >>> http://cr.opensolaris.org/~nigoroll/bz12149_sdssc_host.c.patch >> >> Ah of course! It is not part of the ON base...apologies - separating >> out these is good (ie have the ON ones and then the OHA one) and not >> including it in the ON changes. >> >> Couple of comments here: >> >> o I'd called the variable mde (as per lots of the SVM code) >> o initialise the variable with mdnullerror: >> >> md_error_t mde = mdnullerror; > > Thanks, I've changed the diff. For the OHA stuff the process to integrate is at: http://hub.opensolaris.org/bin/view/Community+Group+ha-clusters/sponsor-tasks which has some specific cluster links off it. Ta pete > > Nils
