Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

C utilities for summarizing and committing changes #158

Merged
merged 47 commits into from
Jun 13, 2024
Merged

C utilities for summarizing and committing changes #158

merged 47 commits into from
Jun 13, 2024

Conversation

mgree
Copy link
Contributor

@mgree mgree commented Jun 6, 2024

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

TODO

@mgree mgree requested a review from SleepyMug June 6, 2024 15:42
@mgree mgree marked this pull request as ready for review June 11, 2024 17:02
@SleepyMug
Copy link
Collaborator

SleepyMug commented Jun 12, 2024

TODO requested:

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

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.
  • Regarding "trusted.overlay.opaque", due to the fact that we set userattr in try's overlay options, we don't have to deal with them.

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
Copy link
Contributor Author

mgree commented Jun 12, 2024

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

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

@SleepyMug
Copy link
Collaborator

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
Copy link
Contributor Author

mgree commented Jun 12, 2024

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

@mgree mgree merged commit c79af21 into main Jun 13, 2024
19 checks passed
@mgree mgree deleted the commit-utils branch June 13, 2024 00:42
@mgree mgree mentioned this pull request Jun 13, 2024
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

try summary is noisy and slow
2 participants