[merged] Rework libarchive import support #275

jlebon · 2016-04-22T20:04:17Z

From the first commit message:

ostree-repo-libarchive.c: major refactor

- Make hardlink handling more generic. The previous strategy worked for
  tar archives, but not for cpio. It now works for both.
- Add support for SEL labeling (through the OstreeRepoCommitModifier)
- Add support for xattr_callback (through the OstreeRepoCommitModifier)
- Add support for filter (through the OstreeRepoCommitModifier)
- Add a use_ostree_convention option

The remaining commits add tests for the new features.

The main motivation for this work is coreos/rpm-ostree#107. With these patches, rpm-ostree will be able to much more easily import RPMs into repos, with full support for filecaps and SELinux labeling.

cgwalters · 2016-04-22T20:11:23Z

src/libostree/ostree-repo-libarchive.c

+path_relative (const char *src)
+{
+  if (src[0] == '.' && src[1] == '/')
+    src += 2;


Doesn't this also need to be a loop so we canonicalize paths like ././foo ?

Good point.

jlebon · 2016-04-22T20:29:12Z

⬆️ Fixup added!

cgwalters · 2016-04-22T20:37:52Z

Only took a quick scan, but the high level looks really good. Will look more next week.

cgwalters · 2016-04-25T12:57:36Z

src/libostree/ostree-repo-libarchive.c

    src += 2;
-  else if (src[0] == '.' && src[1] == '\0')
-    src++;
  while (src[0] == '/')
    src++;


This still won't handle /./foo right? I think the code needs to be more like:

while (TRUE) { if (src[0] == '.' && src[1] == '/') { src += 2; continue; } if (src[0] == '/') { src++; continue } break; }

Or something? There's got to be some LGPL-or-below code we can look at as a reference...okay, some quick git grep relative in libarchive led me to strip_absolute_path which is rather terrifying...we could likely drop the Windows code at least.

Ahhh OK, I will tweak this so we at least handle the non-expensive cases.

jlebon · 2016-04-25T17:14:11Z

Added fixups! ⬆️

cgwalters · 2016-04-27T15:55:23Z

src/ostree/ot-builtin-commit.c

-        }
-    }
+  lines = g_strsplit (contents, "\n", -1);
+  for (char **iter = lines; iter && *iter; iter++)


I'd be a bit more comfortable if we had braces { for nested loops/conditionals...otherwise it's too easy down the line to add an additional statement and have it go in the wrong section.

(Although https://gnu.wildebeest.org/blog/mjw/2016/01/09/looking-forward-to-gcc6-nice-new-warnings/ will help with that)

cgwalters-bot · 2016-05-02T19:04:29Z

☔ The latest upstream changes (presumably 7021c4f) made this pull request unmergeable. Please resolve the merge conflicts.

- Make hardlink handling more generic. The previous strategy worked for tar archives, but not for cpio. It now works for both. - Add support for SEL labeling (through the OstreeRepoCommitModifier) - Add support for xattr_callback (through the OstreeRepoCommitModifier) - Add support for filter (through the OstreeRepoCommitModifier) - Add a use_ostree_convention option

This was already supported by the commit modifier API, just needed to expose it. This will also be used to test the libarchive API in a future test.

- Test both tar and cpio archives - Test more hardlink corner cases - Test symlinks more rigorously - Test stat override - Test skip list

- Do a bit of refactoring - Add test for use_ostree_convention - Add test for xattr_callback - Add test for SELinux labeling

jlebon · 2016-05-05T21:04:57Z

I rebased and added fixups to address the last comments. I squashed all the previous fixups and only kept the new ones.

Since for now we're punting on the name:id issues some clients will face, I went a different way than we had originally talked about re. passing hashtables through the OstreeRepoImportArchiveOptions. I simply added a callback_with_entry_pathname flag which causes the path sent to the filter & xattr callbacks to match the original entry from the archive. This will be useful for e.g. rpm-ostree so that the unpacker can match the entry with rpmfi overrides.

For the name:id issues, we do not impose any restrictions in the ImportArchive API. Instead, client apps like rpm-ostree should do the checking themselves (e.g. in the filter callback) and report an error to the user if it does not yet support uid/gid != 0 (see also coreos/rpm-ostree#49).

cgwalters · 2016-05-05T21:21:56Z

src/libostree/ostree-repo-libarchive.c

+   * archive. For us however, this will be important. So we do our best to deal
+   * with non-conventional paths (but note that we don't try to be sophisticated
+   * enough to handle e.g. ../ & ./ in the middle of normal dir comps, which
+   * would probably require a stack). Also important, we relativize the path. */


Note ot_util_path_split_validate() does intentionally explicitly error out if we find .. as a path component. See also ot_util_filename_validate().

I think with the current logic it'd be possible to have foo/../../../../bar...and we wouldn't trip any assertions =/

We really need unit tests for this function.

I submitted #283

jlebon · 2016-05-06T01:43:41Z

⬆️ Added a fixup to validate all paths using ot_util_path_split_validate().

cgwalters · 2016-05-06T14:44:18Z

src/libostree/ostree-repo-libarchive.c

@@ -81,13 +82,23 @@ path_relative (const char *src)
  if (src[0] == '.' && src[1] == '\0')
    src += 1;

+  /* make sure that the final path is valid (no . or ..) */
+  if (!ot_util_path_split_validate (src, NULL, error))


A bit expensive to do it this way, we end up mallocing all the path components just to throw them away. But eh...we can revisit this later.

cgwalters · 2016-05-06T14:44:51Z

@cgwalters-bot r+ 37c09a3

cgwalters-bot · 2016-05-06T14:44:57Z

⌛ Testing commit 37c09a3 with merge dfbb612...

This was already supported by the commit modifier API, just needed to expose it. This will also be used to test the libarchive API in a future test. Closes: #275 Approved by: cgwalters

- Test both tar and cpio archives - Test more hardlink corner cases - Test symlinks more rigorously - Test stat override - Test skip list Closes: #275 Approved by: cgwalters

- Do a bit of refactoring - Add test for use_ostree_convention - Add test for xattr_callback - Add test for SELinux labeling Closes: #275 Approved by: cgwalters

cgwalters-bot · 2016-05-06T14:55:38Z

☀️ Test successful - status-atomicjenkins
Approved by: cgwalters
Pushing dfbb612 to master...

cgwalters reviewed Apr 22, 2016
View reviewed changes

cgwalters reviewed Apr 25, 2016
View reviewed changes

cgwalters reviewed Apr 27, 2016
View reviewed changes

jlebon added 7 commits May 5, 2016 15:58

ot-builtin-commit.c: add --skip-list option

bcd2396

This was already supported by the commit modifier API, just needed to expose it. This will also be used to test the libarchive API in a future test.

tests/test-libarchive.sh: add more test

bd1e440

- Test both tar and cpio archives - Test more hardlink corner cases - Test symlinks more rigorously - Test stat override - Test skip list

tests/test-libarchive-import.c: add tests

51dd3db

- Do a bit of refactoring - Add test for use_ostree_convention - Add test for xattr_callback - Add test for SELinux labeling

fixup! ostree-repo-libarchive.c: major refactor

123de15

fixup! ot-builtin-commit.c: add --skip-list option

8b7d8ec

fixup! tests/test-libarchive-import.c: add tests

0f23fe5

jlebon force-pushed the pr/libarchive-rework branch from 74213a0 to 0f23fe5 Compare May 5, 2016 21:03

cgwalters reviewed May 5, 2016
View reviewed changes

jlebon added 2 commits May 5, 2016 21:40

fixup! ostree-repo-libarchive.c: major refactor

7c2116d

fixup! ostree-repo-libarchive.c: major refactor

37c09a3

cgwalters reviewed May 6, 2016
View reviewed changes

cgwalters-bot pushed a commit that referenced this pull request May 6, 2016

tests/test-libarchive-import.c: add tests

dfbb612

- Do a bit of refactoring - Add test for use_ostree_convention - Add test for xattr_callback - Add test for SELinux labeling Closes: #275 Approved by: cgwalters

cgwalters-bot changed the title ~~Rework libarchive import support~~ [merged] Rework libarchive import support May 6, 2016

cgwalters-bot closed this in b1d3dd1 May 6, 2016

jlebon deleted the pr/libarchive-rework branch May 6, 2016 15:26

jlebon mentioned this pull request May 20, 2016

[merged] Package layering coreos/rpm-ostree#289

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[merged] Rework libarchive import support #275

[merged] Rework libarchive import support #275

jlebon commented Apr 22, 2016

cgwalters Apr 22, 2016

jlebon Apr 22, 2016

jlebon commented Apr 22, 2016

cgwalters commented Apr 22, 2016

cgwalters Apr 25, 2016

jlebon Apr 25, 2016

jlebon commented Apr 25, 2016

cgwalters Apr 27, 2016

cgwalters Apr 27, 2016

cgwalters-bot commented May 2, 2016

jlebon commented May 5, 2016

cgwalters May 5, 2016

jlebon commented May 6, 2016

cgwalters May 6, 2016

cgwalters commented May 6, 2016

cgwalters-bot commented May 6, 2016

cgwalters-bot commented May 6, 2016

[merged] Rework libarchive import support #275

[merged] Rework libarchive import support #275

Conversation

jlebon commented Apr 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlebon commented Apr 22, 2016

cgwalters commented Apr 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlebon commented Apr 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgwalters-bot commented May 2, 2016

jlebon commented May 5, 2016

Choose a reason for hiding this comment

jlebon commented May 6, 2016

Choose a reason for hiding this comment

cgwalters commented May 6, 2016

cgwalters-bot commented May 6, 2016

cgwalters-bot commented May 6, 2016