lib/repo-finder-mount: Change the schema for finding repos on volumes #1179

pwithnall · 2017-09-15T15:58:22Z

See issue #1174 for the rationale behind this. In summary:
• It required two lists of collection–refs to be maintained: one in the
repository, and one pointing to the repository.
• It didn’t automatically work for live USBs of OSs based on OSTree
(where there’s always a repository at /ostree/repo).
• It was unnecessarily complex.

The new scheme allows a list of repositories to be searched, but without
needing a layer of indirection through their collection–refs. It adds
/ostree/repo and /.ostree/repo as well-known repository locations which
are always checked on a mounted volume (if they exist).

Update the unit tests accordingly.

Signed-off-by: Philip Withnall withnall@endlessm.com

Plus a few other trivial fixes.

#1174

cgwalters · 2017-09-15T21:17:51Z

src/libostree/ostree-repo.c

+ * ostree_repo_open() not being called on them yet), %FALSE will be returned.
+ *
+ * Returns: %TRUE if @a and @b are the same repository on disk, %FALSE otherwise
+ * Since: 2017.11


Oops, fixup pushed.

cgwalters · 2017-09-18T13:43:19Z

src/libostree/ostree-repo-finder-mount.c

+              GHashTable *repo_refs = repo_and_refs->refs;
+              g_autofree char *repo_path = g_file_get_path (ostree_repo_get_path (repo));
+
+              const gchar *checksum = g_hash_table_lookup (repo_refs, refs[i]);


Now that we have a nested loop, probably best to extract refs[i] to a local variable?

Sensible. Fixup pushed.

cgwalters · 2017-09-18T14:08:20Z

src/libostree/ostree-repo-finder-mount.c

+               * $mount_root/.ostree/repos.d/$something.
+               * Add it to the results, keyed by the canonicalised repository URI
+               * to deduplicate the results. */
+              g_autofree char *canonical_repo_path = realpath (repo_path, NULL);


Not new in this patch but I don't quite get why we need to canonicalize; feels like a leftover from the previous symlinks schema?

There are still symlinks in this schema (although fewer of them); canonicalisation allows us to eliminate the duplicate arising from (for example):

$usb_stick/ostree/repo being a repository

$usb_stick/.ostree/repos.d/alias → $usb_stick/ostree/repo being a symlink
or the duplicate arising from two symlinks in $usb_stick/.ostree/repos.d pointing to the same repository elsewhere on the stick.

Is there any downside to canonicalising? I guess it’s not dirfd-relative, which is a a bit of a pain, but I don’t think realpathat() exists. I can write a libglnx version of realpathat() if you’re concerned about that.

We could canonicalize by making a hashset - maybe add ostree_repo_hash too since you just added _equals()? Anyways, doesn't have to be done in this patch.

I’ve opened issue #1191 as a follow-up.

rh-atomic-bot · 2017-09-18T17:21:29Z

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

cgwalters · 2017-09-18T19:20:47Z

Ah sorry, I hadn't realized #1172 would conflict - can you rebase?

This will compare their root directory inodes to see if they are the same repository on disk. A convenience method for the users of the public API who can’t access OstreeRepo.inode. Signed-off-by: Philip Withnall <withnall@endlessm.com>

See issue ostreedev#1174 for the rationale behind this. In summary: • It required two lists of collection–refs to be maintained: one in the repository, and one pointing to the repository. • It didn’t automatically work for live USBs of OSs based on OSTree (where there’s always a repository at /ostree/repo). • It was unnecessarily complex. The new scheme allows a list of repositories to be searched, but without needing a layer of indirection through their collection–refs. It adds /ostree/repo and /.ostree/repo as well-known repository locations which are always checked on a mounted volume (if they exist). Update the unit tests accordingly. Signed-off-by: Philip Withnall <withnall@endlessm.com> ostreedev#1174

This was some incomplete planning from while the find_remotes() API was being designed; now totally outdated. Signed-off-by: Philip Withnall <withnall@endlessm.com>

…volumes

pwithnall · 2017-09-19T09:51:31Z

Ah sorry, I hadn't realized #1172 would conflict - can you rebase?

Rebased.

cgwalters · 2017-09-19T12:59:48Z

@rh-atomic-bot r+ 4842bec

rh-atomic-bot · 2017-09-19T12:59:52Z

⌛ Testing commit 4842bec with merge 9e0100c...

This will compare their root directory inodes to see if they are the same repository on disk. A convenience method for the users of the public API who can’t access OstreeRepo.inode. Signed-off-by: Philip Withnall <withnall@endlessm.com> Closes: #1179 Approved by: cgwalters

See issue #1174 for the rationale behind this. In summary: • It required two lists of collection–refs to be maintained: one in the repository, and one pointing to the repository. • It didn’t automatically work for live USBs of OSs based on OSTree (where there’s always a repository at /ostree/repo). • It was unnecessarily complex. The new scheme allows a list of repositories to be searched, but without needing a layer of indirection through their collection–refs. It adds /ostree/repo and /.ostree/repo as well-known repository locations which are always checked on a mounted volume (if they exist). Update the unit tests accordingly. Signed-off-by: Philip Withnall <withnall@endlessm.com> #1174 Closes: #1179 Approved by: cgwalters

This was some incomplete planning from while the find_remotes() API was being designed; now totally outdated. Signed-off-by: Philip Withnall <withnall@endlessm.com> Closes: #1179 Approved by: cgwalters

cgwalters · 2017-09-19T13:00:21Z

(PR context failure is due to mirror desync with the base image, glibc gets downgraded since we found an older glibc-devel)

rh-atomic-bot · 2017-09-19T13:11:16Z

💔 Test failed - status-atomicjenkins

cgwalters · 2017-09-19T14:51:06Z

@rh-atomic-bot retry

rh-atomic-bot · 2017-09-19T14:51:11Z

⌛ Testing commit 4842bec with merge c62b360...

See issue #1174 for the rationale behind this. In summary: • It required two lists of collection–refs to be maintained: one in the repository, and one pointing to the repository. • It didn’t automatically work for live USBs of OSs based on OSTree (where there’s always a repository at /ostree/repo). • It was unnecessarily complex. The new scheme allows a list of repositories to be searched, but without needing a layer of indirection through their collection–refs. It adds /ostree/repo and /.ostree/repo as well-known repository locations which are always checked on a mounted volume (if they exist). Update the unit tests accordingly. Signed-off-by: Philip Withnall <withnall@endlessm.com> #1174 Closes: #1179 Approved by: cgwalters

This was some incomplete planning from while the find_remotes() API was being designed; now totally outdated. Signed-off-by: Philip Withnall <withnall@endlessm.com> Closes: #1179 Approved by: cgwalters

rh-atomic-bot · 2017-09-19T15:03:01Z

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

pwithnall · 2017-09-22T16:05:33Z

@cgwalters and I talked about this today and decided that it probably also makes sense to hard-code searches for /var/lib/flatpak and /var/lib/containers/atomic in OstreeRepoFinderMount. I’ll do that on Monday; I’ve filed #1210 for it.

The use case for this is for finding remotes from a live USB which contains an OS which uses separate repos for the OS and system flatpaks; or which contains a non-OSTree OS and a system flatpak repo.

pwithnall self-assigned this Sep 15, 2017

pwithnall requested a review from cgwalters September 15, 2017 15:58

pwithnall force-pushed the repo-finder-mount-schema branch from 823b647 to a72264b Compare September 18, 2017 13:05

pwithnall mentioned this pull request Sep 18, 2017

create-usb: Add a create-usb command to complement OstreeRepoFinderMount #1182

Closed

cgwalters reviewed Sep 18, 2017

View reviewed changes

pwithnall added 5 commits September 19, 2017 10:50

lib/repo: Drop outdated FIXME comment from ostree-repo.h

b64ab67

This was some incomplete planning from while the find_remotes() API was being designed; now totally outdated. Signed-off-by: Philip Withnall <withnall@endlessm.com>

fixup! lib/repo: Add ostree_repo_equal() for comparing repos

8cb6bde

fixup! lib/repo-finder-mount: Change the schema for finding repos on …

4842bec

…volumes

pwithnall force-pushed the repo-finder-mount-schema branch from 79b0558 to 4842bec Compare September 19, 2017 09:51

cgwalters added the homu/approved label Sep 19, 2017

rh-atomic-bot closed this in 981eb6c Sep 19, 2017

pwithnall mentioned this pull request Sep 19, 2017

Add ostree_repo_hash() and use it to canonicalise sets of repos #1191

Open

pwithnall deleted the repo-finder-mount-schema branch September 22, 2017 11:02

pwithnall mentioned this pull request Sep 22, 2017

Change schema for finding repositories on USB sticks in OstreeRepoFinderMount #1174

Closed

pwithnall mentioned this pull request Sep 22, 2017

lib/repo-finder-mount: Support /var/lib/flatpak #1210

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib/repo-finder-mount: Change the schema for finding repos on volumes #1179

lib/repo-finder-mount: Change the schema for finding repos on volumes #1179

pwithnall commented Sep 15, 2017

cgwalters Sep 15, 2017

pwithnall Sep 18, 2017

cgwalters Sep 18, 2017

pwithnall Sep 18, 2017

cgwalters Sep 18, 2017

pwithnall Sep 18, 2017

cgwalters Sep 19, 2017

pwithnall Sep 19, 2017

rh-atomic-bot commented Sep 18, 2017

cgwalters commented Sep 18, 2017

pwithnall commented Sep 19, 2017

cgwalters commented Sep 19, 2017

rh-atomic-bot commented Sep 19, 2017

cgwalters commented Sep 19, 2017

rh-atomic-bot commented Sep 19, 2017

cgwalters commented Sep 19, 2017

rh-atomic-bot commented Sep 19, 2017

rh-atomic-bot commented Sep 19, 2017

pwithnall commented Sep 22, 2017 •

edited

Loading

lib/repo-finder-mount: Change the schema for finding repos on volumes #1179

lib/repo-finder-mount: Change the schema for finding repos on volumes #1179

Conversation

pwithnall commented Sep 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rh-atomic-bot commented Sep 18, 2017

cgwalters commented Sep 18, 2017

pwithnall commented Sep 19, 2017

cgwalters commented Sep 19, 2017

rh-atomic-bot commented Sep 19, 2017

cgwalters commented Sep 19, 2017

rh-atomic-bot commented Sep 19, 2017

cgwalters commented Sep 19, 2017

rh-atomic-bot commented Sep 19, 2017

rh-atomic-bot commented Sep 19, 2017

pwithnall commented Sep 22, 2017 • edited Loading

pwithnall commented Sep 22, 2017 •

edited

Loading