Ethan Quach wrote: > > > Evan Layton wrote: >> HI Ethan, >> >> This looks really good. I do have a question though. >> >> On line 876 it's setting be_created if we get a failure back from >> be_clone_fs_callback. If this fails then we haven't created the BE >> but we're still setting be_created, doing the goto done and >> attempting to clean-up the BE. > > That's an incorrect assumption; if the call to be_clone_fs_callback() > fails, > that doesn't mean no datasets for the new BE have been created yet. For > example it could have succeeded in creating the new root dataset, but > failed > in cloning one of the subordinates. The places where be_created is set > are the places where datasets have potentially been created.
Yes I realized after I sent the question that it was at best incorrect... > > Though I just realized there could be a race condition. If another process > created a BE name "foobar" just as this code was running, > be_clone_fs_callback() > would fail and this code would end up destroying the "foobar" BE just > created > by the other process. I think this is going to need some rework. Good catch! > >> I can understand if we've at least >> created one of a set of needed snapshots (or clones) we need to clean >> up but what happens in the case where no snapshots (or clones) were >> created? > > For cases where we're sure no datasets have been created, be_created > doesn't > get set. Those are 893, 921, 957. Ah ok I see what you're getting at there and it makes sense to me now. Thanks for the explanation! > > > Anyway, as per my comment about, I'm going to separate 3844 and 3889 (the > latter being a much simpler fix), so that 3889 can get in earlier than > later. > I'll resend 3889 in just a few... Sounds good! -evan > > > thanks, > -ethan > > >> I guess that gets covered in _be_destroy in that the first >> zfs_open will fail? >> Anyway the changes look fine, I just wanted to make sure this scenario >> was covered. >> >> Thanks, >> -evan >> >> Ethan Quach wrote: >> >>> Two more fixes in this webrev. The first defect fixes the case where >>> creating a BE will silently ignore failures in creating corresponding >>> zones in the new BE. It will now stop the creation process and report >>> error. be_copy() will now also cleanup a partially created boot >>> environment upon any failure that happens during the creation >>> process instead of leaving it lying around. >>> >>> The second defect is a regression due to 1197. The additional test >>> case that caught this was doing a pkg image-update from a system >>> where the pool is running less then the required pool version that >>> supports snapshot user properties. >>> >>> >>> >>> Webrev: >>> ---------- >>> http://cr.opensolaris.org/~equach/webrev.3844.3889 >>> >>> Defects: >>> ---------- >>> 3884 be_copy_zones() silently ignores zones >>> http://defect.opensolaris.org/bz/show_bug.cgi?id=3844 >>> >>> 3889 be_create_snapshot fails when running pkg image-update >>> http://defect.opensolaris.org/bz/show_bug.cgi?id=3889 >>> >>> >>> thanks, >>> -ethan >>> >>> _______________________________________________ >>> caiman-discuss mailing list >>> caiman-discuss at opensolaris.org >>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >>> >> >> _______________________________________________ >> caiman-discuss mailing list >> caiman-discuss at opensolaris.org >> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >>
