ahrens commented on this pull request.
looks good aside from the comment updates I noted
> + * See if the snapshot is a snapshot of the filesystem
+ * or the snapshot is an origin of the filesystem.
+ */
+ if (snapds->ds_dir == ds->ds_dir ||
+ (dsl_dir_is_clone(ds->ds_dir) &&
+ dsl_dir_phys(ds->ds_dir)->dd_origin_obj ==
+ snapds->ds_object)) {
+ error = SET_ERROR(EEXIST);
+ } else {
+ error = SET_ERROR(ESRCH);
+ }
+ dsl_dataset_rele(snapds, FTAG);
+ dsl_dataset_rele(ds, FTAG);
+ return (error);
+ }
+ dsl_dataset_rele(snapds, FTAG);
}
/* must not have any bookmarks after the most recent snapshot */
while you're here, can you fix line 2545, it needs to release `ds` as well when
returning the error.
> + /* Check if the target snapshot exists at all. */
+ error = dsl_dataset_hold(dp, ddra->ddra_tosnap, FTAG, &snapds);
+ if (error != 0) {
+ /* Distinguish between missing dataset and snapshot. */
+ if (error == ENOENT || error == EXDEV)
+ error = SET_ERROR(ESRCH);
+ dsl_dataset_rele(ds, FTAG);
+ return (error);
+ }
+ ASSERT(snapds->ds_is_snapshot);
+
+ /* Check if the snapshot is the latest snapshot indeed. */
+ if (snapds != ds->ds_prev) {
+ /*
+ * See if the snapshot is a snapshot of the filesystem
+ * or the snapshot is an origin of the filesystem.
nit: `is *the* origin`
you might say something like, `distinguish between the case where the only
problem is intervening snapshots (EEXIST) vs the snapshot can't possibly be a
valid target for rollback (or doesn't exist) (ESRCH).`
- dsl_dataset_name(ds->ds_prev, namebuf);
- if (strcmp(namebuf, ddra->ddra_tosnap) != 0)
- return (SET_ERROR(EXDEV));
+ /* Check if the target snapshot exists at all. */
+ error = dsl_dataset_hold(dp, ddra->ddra_tosnap, FTAG, &snapds);
+ if (error != 0) {
+ /* Distinguish between missing dataset and snapshot. */
I think this is squashing `missing dataset and missing snapshot` to the same
error, not distinguishing them.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/434#pullrequestreview-61313581
------------------------------------------
openzfs-developer
Archives:
https://openzfs.topicbox.com/groups/developer/discussions/T5032507ba3c52b88-M9986a7c719f13ac1bcf683b9
Powered by Topicbox: https://topicbox.com
