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