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 >>>> >>
