Evan, Looks okay now.
thanks, -ethan Evan Layton wrote: > Ethan Quach wrote: >> >> Evan Layton wrote: >>> Ethan Quach wrote: >>>> >>>> >>>> Evan Layton wrote: >>>>> Ethan Quach wrote: >>>>>> Evan, >>>>>> >>>>>> Why not just move lines 272-274 to 301, instead of adding 266-271 ? >>>>> >>>>> We need to check for a valid name before calling >>>>> getActiveBEAndActiveOnBootBE(be.trgtBeNameOrSnapshot[0]) don't we? >>>>> >>>>> If I moved lines 272-274 down to 301 we're could be calling into >>>>> getActiveBEAndActiveOnBootBE with an invalid name in >>>>> be.trgtBeNameOrSnapshot[0]. >>>> >>>> Interesting.. I looked at getActiveBEAndActiveOnBootBE(), and there's >>>> no reason why that function should even be taking an argument. It >>>> uses the argument in an error message, and it doesn't even make sense >>>> to use the passed in argument there. >>> >>> Hmm you're right that doesn't make any sense to have that there. This >>> argument could be removed and the error message changed so it doesn't >>> contain the argument. >> >> Changing the error to be BEADM_ERR_NO_BES_EXIST seems appropriate. > > Done > >> >>> >>>> >>>> ...anyway, the main issue I have is that I don't want to see the >>>> .split() being called twice at 267 and 298. Could you set an >>>> is_snapshot flag at 267, and then use that flag at 282 and 294 >>>> instead? You wouldn't need to resplit at 298 either. >>> >>> Good point, it really doesn't make sense to be doing the split twice... >>> >>> I've added the is_snapshot flag and removed the split at 298. The >>> webrev is updated. >> >> Just one more nit. 284, no need to compare the string if its >> a snapshot, so could you switch the order of these two conditions? > > and done. > > Thanks for looking at it! > > -evan > >> >> >> thanks, >> -ethan >> >> >>> >>> -evan >>> >>>> >>>> >>>> thanks, >>>> -ethan >>>> >>>>> >>>>> -evan >>>>> >>>>>> >>>>>> >>>>>> -ethan >>>>>> >>>>>> >>>>>> Evan Layton wrote: >>>>>>> Hi, >>>>>>> >>>>>>> I need to get a review the following simple fix. >>>>>>> >>>>>>> Some background: This bug was introduced with the fix for 5749. >>>>>>> In the >>>>>>> case of beadm destroy we validate the name of the BE however if >>>>>>> we're >>>>>>> destroying a snapshot of a BE we were not splitting out the name >>>>>>> of the >>>>>>> BE from the snapshot name before doing the name validation. I >>>>>>> checked >>>>>>> through the rest of beadm and didn't find any other areas where >>>>>>> we deal >>>>>>> with both BE names and snapshots that we were not checking for a >>>>>>> snapshot >>>>>>> before validating the BE name. >>>>>>> >>>>>>> 7071 beadm can fail to destroy snapshot >>>>>>> http://defect.opensolaris.org/bz/show_bug.cgi?id=7071 >>>>>>> >>>>>>> Webrev: >>>>>>> http://cr.opensolaris.org/~evanl/7071/ >>>>>>> >>>>>>> Thanks! >>>>>>> -evan >>>>>>> _______________________________________________ >>>>>>> caiman-discuss mailing list >>>>>>> caiman-discuss at opensolaris.org >>>>>>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >>>>> >>> >
