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