binpash / try

Inspect a command's effects before modifying your live system
MIT License
5.17k stars 64 forks source link

C utilities for summarizing and committing changes #158

Closed mgree closed 1 month ago

mgree commented 1 month ago

Attempting a drop-in replacement for the core logic of our summary() and commit() functions. Closes #126 and #159.

TODO

SleepyMug commented 1 month ago

TODO requested:

Other points come up during review/discussion:

Question:

In the code we have

      size_t tgt_len = ent->fts_statp->st_size + 1;
      if (tgt_len == 0) { // apparently fancy FS can lie?                                                               
        tgt_len = PATH_MAX;
      }

I guess the point here is that for the strange cases where st_size being -1 (which we have not observed?), this clause is there to make the "realloc with doubling sizes" later always start from a positive number?

mgree commented 1 month ago
* Although abusing a little of UB, `hs` currently relies on the fact that commiting does not remove files from the upperdir. So we need a way to have commit using `cp` instead of `mv`.

try-commit -c will use cp instead of mv. I deliberately did not surface this behavior in try.

Along the way, I changed try-summary and try-commit to take the sandbox as its argument, not the upperdir, which it finds itself. This feels like a nicer interface (and gives us flexibility to rename things later).

Other points come up during review/discussion:

* We have not encountered situations where xattr "trusted.overlay.whiteout" (or "user.overlay.whiteout") is used to mark deletion, but we have code for this.

* We are not handling [redirect_dir](https://docs.kernel.org/filesystems/overlayfs.html#redirect-dir).

* Regarding "trusted.overlay.opaque", due to the fact that we set userattr in [try's overlay options](https://github.com/binpash/try/blob/af013b518cad69389d150ff8be998f8ae2a78d56/try#L142), we don't have to deal with them.

Recorded these facts in utils/README.md.

Question:

In the code we have

      size_t tgt_len = ent->fts_statp->st_size + 1;
      if (tgt_len == 0) { // apparently fancy FS can lie?                                                               
        tgt_len = PATH_MAX;
      }

I guess the point here is that for the strange cases where st_size being -1 (which we have not observed?), this clause is there to make the "realloc with doubling sizes" later always start from a positive number?

Added a comment: procfs (and possible other weird FS types) reports st_size of 0 on lstat of symbolic links. :upside_down_face:

When everything passes CI, I'll merge. Next stop is #166!

SleepyMug commented 1 month ago

Question: In the code we have

      size_t tgt_len = ent->fts_statp->st_size + 1;
      if (tgt_len == 0) { // apparently fancy FS can lie?                                                               
        tgt_len = PATH_MAX;
      }

I guess the point here is that for the strange cases where st_size being -1 (which we have not observed?), this clause is there to make the "realloc with doubling sizes" later always start from a positive number?

Added a comment: procfs (and possible other weird FS types) reports st_size of 0 on lstat of symbolic links. 🙃

Not consequential at all but I have to ask: sincest_size == 0 => tgt_len == 1, this clause doesn't handle the case where st_size is 0 per se?

mgree commented 1 month ago

Ah, good catch. Fixed now. (Have you used GH's code review process? There's a whole thing where you "start a review" and group comments and can comment on lines.)