Hi Nils, Nils Goroll wrote: > Hi Pete, > >> I was thinking about this one and while the initial intent behind the >> purge option was to rescue the system when the SVM data became corrupt >> such that the a particular set needed to be removed as opposed to >> removing the whole of the SVM configuration. >> >> This is why it was not extended to the use case you point out. >> I have no objection to what you are doing and can see usefulness >> in it. > > Thank you for your explanation, it matches my assumptions about the > reasons for this functionality not to exist already. > >> As for getting a sponsor for the fixes for the three bugs >> that you've logged the place to ask is on request-sponsor at opensolaris.org > > Will do. Can I count your comment as a positive review? If yes, I'd need > one more positive review. >
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. 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). As for the call to the sdssc_delete_hosts - this looks okay. > > Aside from this - it would appear that your webrev's have disappeared - > > I can only find the one for this particular bug. > > Yes, I had changed the webrev URLs this morning to include the bugster ids: > > http://cr.opensolaris.org/~nigoroll/6895210_easy_way_to_purge_from_cluster/ > http://cr.opensolaris.org/~nigoroll/6895265_unlocking_wrong_handle_in_metaset_parse_purge/ > > > > > Is it possible to generate one webrev with all the changes included > > and I'll have a look at them.... > > Sure. I wasn't sure which was was best... > > http://cr.opensolaris.org/~nigoroll/6895265_6895210_together/ > > I havn't preprared a webrev for 895209 ohac _sdssc_delete_hosts should > pass non-NULL ep to metasetname/, it it OK that we just have a diff? > > 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 pete > ohac isn't on mercurial yet, so preparing a webrev needs a bit more > effort, but I could do that if it was important. > > Thank you, Nils
