GNOME Bugzilla – Bug 740911
ostree remote delete doesn't work if /etc/ostree/remotes.d/ has more than two conf.
Last modified: 2014-12-08 17:51:42 UTC
Issue description ==== Please see following reproduce steps. step-1. add remote repositories. (here I added abc and xyz) [root@localhost ostree]# ostree remote add abc abc.com [root@localhost ostree]# ostree remote add xyz xyz.com [root@localhost ostree]# ls /etc/ostree/remotes.d/ abc.conf xyz.conf step-2. Run ostree remote delete. [root@localhost ostree]# ostree remote delete xyz step-3. Check the result. [root@localhost ostree]# ls /etc/ostree/remotes.d/ abc.conf xyz.conf [root@localhost ostree]# cat /ostree/repo/config [core] repo_version=1 mode=bare [remote "abc"] url=abc.com As you can see on the above results, if there are two or more remote repostiroes in /etc/ostree/remotes.d, "$ ostree remote delete" will not work well.
This root cause is g_key_file_has_group() doesn't support space. So ostree will be into the wrong procedure here[1]. As remove the white space in [remote "NAME"] is not big issue, I have sent the patch[2] to replace to "_". Please comment or review about this. [1] https://github.com/GNOME/ostree/blob/master/src/libostree/ostree-repo.c#L511-L514 [2] https://github.com/GNOME/ostree/pull/21/
The problem isn't the space in the group name, the problem is the way it's trying to distinguish system from non-system remotes. The self->config key file in memory contains all the remotes as key file groups, whether it was read directly from the repo/config file or from remotes.d. So as long as the remote name is valid, the group membership test is always true. One solution is for OstreeRepo to embed some kind of special key in the memory-only GKeyFile to distinguish the source of each remote. Something like: # In-memory GKeyFile representation. # The "is-system" keys are added by OstreeRepo and NOT saved to disk. [core] repo_version=1 mode=bare # This remote was read from repo/config. [remote "abc"] url=abc.com is-system=false # This remote was read from remotes.d. [remote "xyz"] url=xyz.com is-system=true Then perhaps introduce a new OstreeRepo function to check for the key: gboolean ostree_repo_remote_is_system (OstreeRepo *self, const char *name, gboolean *out_is_system, GError **error) (Invalid remote name sets the GError and returns FALSE.)
You're right that we need to maintain this mapping. I think your special key idea would work, but it'd be a little annoying to filter it out when writing. How about an internal GHashTable<char * remotename, gboolean is-system> ?
That's indeed simpler, although booleans in hash tables are also annoying. How about just a set of system-wide remotes: GHashTable<char *remotename, char *remotename> *system_remotes Works nicely with g_hash_table_contains().
Yep, set is even better.
So I might be over-thinking this, but it seems like merging system-wide remote files into OstreeRepo's internal config is problematic in other ways. For example, ostree_repo_write_config() will write that merged data out to the repo's config file, which is wrong. Now libostree itself is careful not to do that, but ostree_repo_write_config() is public API. Plus "ostree config set" will trigger it as well. I'm leaning toward defining a private OstreeRemote struct (not sure it needs to be a full GObject class), and tracking them in a hash table. typedef struct { volatile int ref_count; char *name; GFile *file; GHashTable<char *key, char *value> *options; } OstreeRemote; GHashTable<char *remotename, OstreeRemote *remote> *remotes For system-wide remotes, the GFile would point back to the remotes.d file. For repo-specific remotes defined in repo/config, the GFile would be NULL. This way the OstreeRepo's internal config would remain pristine. However it would change the libostree API semantics slightly in that ostree_repo_copy_config() would no longer include all the remote config data. But I would argue that's an unwanted internal implementation leak anyway.
(In reply to comment #6) > However it would change the libostree API semantics slightly in that > ostree_repo_copy_config() would no longer include all the remote config data. > But I would argue that's an unwanted internal implementation leak anyway. I can't think of what would break from that change, I doubt anyone outside of the ostree commandline is using _copy_config().
I'm diggin pull requests over static patches, so: https://github.com/GNOME/ostree/pull/24
The full fix spanned 4 commits starting here: https://git.gnome.org/browse/ostree/commit/?id=f3dcb7a052ecc94fb25a9e4598dea911e71e6221