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