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