On (29/07/16 12:55), Jakub Hrozek wrote:
>On Fri, Jul 29, 2016 at 09:31:32AM +0200, Lukas Slebodnik wrote:
>> On (28/07/16 19:37), Michal Židek wrote:
>> >On 07/28/2016 02:11 PM, Michal Židek wrote:
>> >> On 07/28/2016 01:57 PM, Jakub Hrozek wrote:
>> >> > On Thu, Jul 28, 2016 at 01:51:40PM +0200, Pavel Březina wrote:
>> >> > > On 07/28/2016 01:38 PM, Jakub Hrozek wrote:
>> >> > > > On Thu, Jul 28, 2016 at 12:23:24PM +0200, Michal Židek wrote:
>> >> > > > > On 07/28/2016 10:00 AM, Pavel Březina wrote:
>> >> > > > > > On 07/27/2016 03:28 PM, Michal Židek wrote:
>> >> > > > > > > On 07/27/2016 11:09 AM, Jakub Hrozek wrote:
>> >> > > > > > > > On Wed, Jul 27, 2016 at 11:03:34AM +0200, Pavel Březina
>> >> > > > > > > > wrote:
>> >> > > > > > > > > On 07/26/2016 04:19 PM, Michal Židek wrote:
>> >> > > > > > > > > > On 07/26/2016 01:19 PM, Pavel Březina wrote:
>> >> > > > > > > > > > > On 07/25/2016 02:12 PM, Michal Židek wrote:
>> >> > > > > > > > > > > > Hi,
>> >> > > > > > > > > > > >
>> >> > > > > > > > > > > > this patches makes the sssctl commands more similar
>> >> > > > > > > > > > > > to
>> >> > > > > > > > > > > > ipa tool commands. I also think this pattern makes
>> >> > > > > > > > > > > > it
>> >> > > > > > > > > > > > easier to remember the commands.
>> >> > > > > > > > > > > >
>> >> > > > > > > > > > > > Note that in the future, there will be more user-*
>> >> > > > > > > > > > > > group-* and netgroup-* commands (like seed for user,
>> >> > > > > > > > > > > > list of all etc.)
>> >> > > > > > > > > > > >
>> >> > > > > > > > > > > > Comments are welcome.
>> >> > > > > > > > > > > >
>> >> > > > > > > > > > > > Michal
>> >> > > > > > > > > > >
>> >> > > > > > > > > > > Hi,
>> >> > > > > > > > > > > ok, it looks like a good idea. When touching the
>> >> > > > > > > > > > > code, can
>> >> > > > > > > > > > > you also
>> >> > > > > > > > > > > convert sss_override command to use the macro
>> >> > > > > > > > > > > instead? And I
>> >> > > > > > > > > > > think it
>> >> > > > > > > > > > > may be nice to also add a macro for command sentinel
>> >> > > > > > > > > > > i.e. for
>> >> > > > > > > > > > > {NULL,
>> >> > > > > > > > > > > ...}.
>> >> > > > > > > > > > >
>> >> > > > > > > > > > > I'm not very fond of renaming local-data-* to cache-*
>> >> > > > > > > > > > > so it
>> >> > > > > > > > > > > doesn't
>> >> > > > > > > > > > > imply that we backup the whole cache content. We only
>> >> > > > > > > > > > > backup and
>> >> > > > > > > > > > > restore
>> >> > > > > > > > > > > data that are local to the client and not present in
>> >> > > > > > > > > > > LDAP.
>> >> > > > > > > > > > > Currently
>> >> > > > > > > > > > > only local overrides, but it may include local users
>> >> > > > > > > > > > > and
>> >> > > > > > > > > > > groups in
>> >> > > > > > > > > > > the
>> >> > > > > > > > > > > future.
>> >> > > > > > > >
>> >> > > > > > > > When we have the files provider there would
>> >> > > > > > > > be a cache as well. Moreover, we store secrets now. The
>> >> > > > > > > > restore
>> >> > > > > > > > command
>> >> > > > > > > > backs up all *.ldb files, right?
>> >> > > > > > >
>> >> > > > > > > This is how I understood it at first, but the current backup
>> >> > > > > > > and restore only work for local overrides. But as Pavel
>> >> > > > > > > mentioned,
>> >> > > > > > > it may work for local users and groups in the future
>> >> > > > > > > (id_provider=local). My original confusion was that I also
>> >> > > > > > > thought it backs-up and restores all ldb files, which is
>> >> > > > > > > not the case.
>> >> > > > > >
>> >> > > > > > No, the intention is to backup only data that are not stored on
>> >> > > > > > server
>> >> > > > > > and would be lost when the cache is removed. In the time, only
>> >> > > > > > local
>> >> > > > > > overrides were local. If secrets creates local data, the tool
>> >> > > > > > should be
>> >> > > > > > modified.
>> >> > > > >
>> >> > > > > Yes, but users and groups from local domain would also be
>> >> > > > > lost if the local data is deleted. So I though we want to backup
>> >> > > > > them as well in the future versions (I mean the local provider,
>> >> > > > > not the files provider).
>> >> > > >
>> >> > > > Well, probably not in the first version, but definitely in a couple
>> >> > > > of
>> >> > > > months we want to add the capability to set extended attributes to
>> >> > > > the
>> >> > > > files provider which we'll want to back up as well. But I also
>> >> > > > think we
>> >> > > > will add the additional data into a new directory, just like we did
>> >> > > > with
>> >> > > > the secrets database.
>> >> > > >
>> >> > > > >
>> >> > > > > Btw. do we want to merge sss_override tool into sssctl?
>> >> > > > > Because if the local-data-* commands work currently with
>> >> > > > > overrides only, then we could make a new topic 'overrides' and add
>> >> > > > > commands
>> >> > > > > like
>> >> > > >
>> >> > > > I would like to merge all tools into sssctl unless there is a strong
>> >> > > > reason to keep them separate (the local domain tools should be kept
>> >> > > > separate IMO).
>> >> > > >
>> >> > > > The reason is simply discoverability, if there is a new sssd
>> >> > > > release,
>> >> > > > the admin would just run sssctl and if there are any new tools,
>> >> > > > their
>> >> > > > topics would be displayed.
>> >> > > >
>> >> > > > (Also I think the code duplication would be reduced as a
>> >> > > > side-effect).
>> >> > > >
>> >> > > > >
>> >> > > > > sssctl overrides-backup
>> >> > > > > sssctl overrides-restore
>> >> > > > >
>> >> > > > > and later also all the functionality of sss_overrides
>> >> > > > >
>> >> > > > > sssctl overrides-user-add
>> >> > > > > sssctl overrides-user-del
>> >> > > >
>> >> > > > Maybe, but later.
>> >> > > >
>> >> > > > >
>> >> > > > > etc.
>> >> > > > >
>> >> > > > > This way we could avoid confusion between local-data and
>> >> > > > > cache. If secrets will also create some local data, we will
>> >> > > > > add topic 'secrets' to deal with that separately.
>> >> > > > >
>> >> > > > > sssctl secrets-backup
>> >> > > > > sssctl secrets-restore
>> >> > > >
>> >> > > > I think I got lost in the thread :-) What is the benefit of having
>> >> > > > more
>> >> > > > backup/restore commands than one that backs up or removes of value
>> >> > > > under
>> >> > > > the /var/lib/sss/ structure? So far I can only think of cache being
>> >> > > > more
>> >> > > > intuitive to admins than local-data.
>> >> > >
>> >> > > How about client-data instead of local-data?
>> >> >
>> >> > SGTM (sounds good to me :-))
>> >>
>> >> SGTM too :)
>> >>
>> >> I will send version with client-data after the meeting.
>> >>
>> >
>> >I forgot to update this after the meeting.
>> >New patch is attached.
>> >
>> >Michal
>> >
>>
>> >From 73c3e97013274ff644aa630547494fd79b1854c0 Mon Sep 17 00:00:00 2001
>> >From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzi...@redhat.com>
>> >Date: Mon, 25 Jul 2016 13:50:13 +0200
>> >Subject: [PATCH 1/3] sssctl: Consistent commands naming
>> >
>> >Fixes:
>> >https://fedorahosted.org/sssd/ticket/3087
>> >
>> >Use TOPIC-ACTION pattern for sssctl command
>> >names.
>> >---
>> > src/tools/common/sss_tools.c | 4 ++++
>> > src/tools/common/sss_tools.h | 8 ++++++--
>> > src/tools/sss_override.c | 26 ++++++++++++------------
>> > src/tools/sssctl/sssctl.c | 33 ++++++++++++++++++++----------
>> > src/tools/sssctl/sssctl.h | 42
>> > +++++++++++++++++++--------------------
>> > src/tools/sssctl/sssctl_cache.c | 18 ++++++++---------
>> > src/tools/sssctl/sssctl_data.c | 16 +++++++--------
>> > src/tools/sssctl/sssctl_domains.c | 6 +++---
>> > src/tools/sssctl/sssctl_logs.c | 4 ++--
>> > 9 files changed, 88 insertions(+), 69 deletions(-)
>> >
>> >diff --git a/src/tools/common/sss_tools.c b/src/tools/common/sss_tools.c
>> >index 686b53a..4977358 100644
>> >--- a/src/tools/common/sss_tools.c
>> >+++ b/src/tools/common/sss_tools.c
>> >@@ -283,6 +283,10 @@ void sss_tool_usage(const char *tool_name, struct
>> >sss_route_cmd *commands)
>> > min_len = sss_tool_max_length(commands);
>> >
>> > for (i = 0; commands[i].command != NULL; i++) {
>> >+ if (commands[i].hidden) {
>> >+ continue;
>> >+ }
>> >+
>> > if (sss_tool_is_delimiter(&commands[i])) {
>> > fprintf(stderr, "\n%s\n", commands[i].description);
>> > continue;
>> >diff --git a/src/tools/common/sss_tools.h b/src/tools/common/sss_tools.h
>> >index a9ebabe..b567ea4 100644
>> >--- a/src/tools/common/sss_tools.h
>> >+++ b/src/tools/common/sss_tools.h
>> >@@ -45,14 +45,18 @@ typedef errno_t
>> > struct sss_tool_ctx *tool_ctx,
>> > void *pvt);
>> >
>> >-#define SSS_TOOL_COMMAND(cmd, msg, err, fn) {cmd, _(msg), err, fn}
>> >-#define SSS_TOOL_DELIMITER(message) {"", (message), 0, NULL}
>> >+#define SSS_TOOL_COMMAND(cmd, msg, err, fn) {cmd, _(msg), err, fn, false}
>> >+#define SSS_TOOL_COMMAND_NOMSG(cmd, err, fn) {cmd, NULL, err, fn, false}
>> >+#define SSS_TOOL_COMMAND_OBSOLETE(cmd, err, fn) {cmd, NULL, err, fn, true}
>> >+#define SSS_TOOL_DELIMITER(message) {"", (message), 0, NULL, false}
>> >+#define SSS_TOOL_LAST {NULL, NULL, 0, NULL, false}
>> >
>> > struct sss_route_cmd {
>> > const char *command;
>> > const char *description;
>> > errno_t handles_init_err;
>> > sss_route_fn fn;
>> >+ bool hidden; /* Do not show this command in sssctl usage */
>> > };
>> >
>> > void sss_tool_usage(const char *tool_name,
>> >diff --git a/src/tools/sss_override.c b/src/tools/sss_override.c
>> >index 45a28fd..d41da52 100644
>> >--- a/src/tools/sss_override.c
>> >+++ b/src/tools/sss_override.c
>> >@@ -1913,19 +1913,19 @@ static int override_group_export(struct sss_cmdline
>> >*cmdline,
>> > int main(int argc, const char **argv)
>> > {
>> > struct sss_route_cmd commands[] = {
>> >- {"user-add", NULL, 0, override_user_add},
>> >- {"user-del", NULL, 0, override_user_del},
>> >- {"user-find", NULL, 0, override_user_find},
>> >- {"user-show", NULL, 0, override_user_show},
>> >- {"user-import", NULL, 0, override_user_import},
>> >- {"user-export", NULL, 0, override_user_export},
>> >- {"group-add", NULL, 0, override_group_add},
>> >- {"group-del", NULL, 0, override_group_del},
>> >- {"group-find", NULL, 0, override_group_find},
>> >- {"group-show", NULL, 0, override_group_show},
>> >- {"group-import", NULL, 0, override_group_import},
>> >- {"group-export", NULL, 0, override_group_export},
>> >- {NULL, NULL, 0, NULL}
>> >+ SSS_TOOL_COMMAND_NOMSG("user-add", 0, override_user_add),
>> >+ SSS_TOOL_COMMAND_NOMSG("user-del", 0, override_user_del),
>> >+ SSS_TOOL_COMMAND_NOMSG("user-find", 0, override_user_find),
>> >+ SSS_TOOL_COMMAND_NOMSG("user-show", 0, override_user_show),
>> >+ SSS_TOOL_COMMAND_NOMSG("user-import", 0, override_user_import),
>> >+ SSS_TOOL_COMMAND_NOMSG("user-export", 0, override_user_export),
>> >+ SSS_TOOL_COMMAND_NOMSG("group-add", 0, override_group_add),
>> >+ SSS_TOOL_COMMAND_NOMSG("group-del", 0, override_group_del),
>> >+ SSS_TOOL_COMMAND_NOMSG("group-find", 0, override_group_find),
>> >+ SSS_TOOL_COMMAND_NOMSG("group-show", 0, override_group_show),
>> >+ SSS_TOOL_COMMAND_NOMSG("group-import", 0, override_group_import),
>> >+ SSS_TOOL_COMMAND_NOMSG("group-export", 0, override_group_export),
>> >+ SSS_TOOL_LAST
>> > };
>> >
>> > return sss_tool_main(argc, argv, commands, NULL);
>> >diff --git a/src/tools/sssctl/sssctl.c b/src/tools/sssctl/sssctl.c
>> >index 86656f1..8ce93b2 100644
>> >--- a/src/tools/sssctl/sssctl.c
>> >+++ b/src/tools/sssctl/sssctl.c
>> >@@ -257,25 +257,36 @@ int main(int argc, const char **argv)
>> > {
>> > struct sss_route_cmd commands[] = {
>> > SSS_TOOL_DELIMITER("SSSD Status:"),
>> >- SSS_TOOL_COMMAND("list-domains", "List available domains", 0,
>> >sssctl_list_domains),
>> >+ SSS_TOOL_COMMAND("domain-list", "List available domains", 0,
>> >sssctl_domain_list),
>> > SSS_TOOL_COMMAND("domain-status", "Print information about
>> > domain", 0, sssctl_domain_status),
>> > SSS_TOOL_DELIMITER("Information about cached content:"),
>> >- SSS_TOOL_COMMAND("user", "Information about cached user", 0,
>> >sssctl_user),
>> >- SSS_TOOL_COMMAND("group", "Information about cached group", 0,
>> >sssctl_group),
>> >- SSS_TOOL_COMMAND("netgroup", "Information about cached netgroup",
>> >0, sssctl_netgroup),
>> >+ SSS_TOOL_COMMAND("user-show", "Information about cached user", 0,
>> >sssctl_user_show),
>> >+ SSS_TOOL_COMMAND("group-show", "Information about cached group",
>> >0, sssctl_group_show),
>> >+ SSS_TOOL_COMMAND("netgroup-show", "Information about cached
>> >netgroup", 0, sssctl_netgroup_show),
>> > SSS_TOOL_DELIMITER("Local data tools:"),
>> >- SSS_TOOL_COMMAND("backup-local-data", "Backup local data", 0,
>> >sssctl_backup_local_data),
>> >- SSS_TOOL_COMMAND("restore-local-data", "Restore local data from
>> >backup", 0, sssctl_restore_local_data),
>> >- SSS_TOOL_COMMAND("remove-cache", "Backup local data and remove
>> >cached content", 0, sssctl_remove_cache),
>> >- SSS_TOOL_COMMAND("upgrade-cache", "Perform cache upgrade",
>> >ERR_SYSDB_VERSION_TOO_OLD, sssctl_upgrade_cache),
>> >+ SSS_TOOL_COMMAND("client-data-backup", "Backup local data", 0,
>> >sssctl_client_data_backup),
>> >+ SSS_TOOL_COMMAND("client-data-restore", "Restore local data from
>> >backup", 0, sssctl_client_data_restore),
>> >+ SSS_TOOL_COMMAND("cache-remove", "Backup local data and remove
>> >cached content", 0, sssctl_cache_remove),
>> >+ SSS_TOOL_COMMAND("cache-upgrade", "Perform cache upgrade",
>> >ERR_SYSDB_VERSION_TOO_OLD, sssctl_cache_upgrade),
>> > SSS_TOOL_DELIMITER("Log files tools:"),
>> >- SSS_TOOL_COMMAND("remove-logs", "Remove existing SSSD log files",
>> >0, sssctl_remove_logs),
>> >- SSS_TOOL_COMMAND("fetch-logs", "Archive SSSD log files in
>> >tarball", 0, sssctl_fetch_logs),
>> >+ SSS_TOOL_COMMAND("logs-remove", "Remove existing SSSD log files",
>> >0, sssctl_logs_remove),
>> >+ SSS_TOOL_COMMAND("logs-fetch", "Archive SSSD log files in
>> >tarball", 0, sssctl_logs_fetch),
>> > #ifdef HAVE_LIBINI_CONFIG_V1_3
>> > SSS_TOOL_DELIMITER("Configuration files tools:"),
>> > SSS_TOOL_COMMAND("config-check", "Perform static analysis of SSSD
>> > configuration", 0, sssctl_config_check),
>> > #endif
>> >- {NULL, NULL, 0, NULL}
>> >+ /* Obsolete commands */
>> >+ SSS_TOOL_COMMAND_OBSOLETE("list-domains", 0, sssctl_domain_list),
>> >+ SSS_TOOL_COMMAND_OBSOLETE("user", 0, sssctl_user_show),
>> >+ SSS_TOOL_COMMAND_OBSOLETE("group", 0, sssctl_group_show),
>> >+ SSS_TOOL_COMMAND_OBSOLETE("netgroup", 0, sssctl_netgroup_show),
>> >+ SSS_TOOL_COMMAND_OBSOLETE("backup-local-data", 0,
>> >sssctl_client_data_backup),
>> >+ SSS_TOOL_COMMAND_OBSOLETE("restore-local-data", 0,
>> >sssctl_client_data_restore),
>> >+ SSS_TOOL_COMMAND_OBSOLETE("remove-cache", 0, sssctl_cache_remove),
>> >+ SSS_TOOL_COMMAND_OBSOLETE("upgrade-cache",
>> >ERR_SYSDB_VERSION_TOO_OLD, sssctl_cache_upgrade),
>> >+ SSS_TOOL_COMMAND_OBSOLETE("remove-logs", 0, sssctl_logs_remove),
>> >+ SSS_TOOL_COMMAND_OBSOLETE("fetch-logs", 0, sssctl_logs_fetch),
>> Do we really need to be so "backward compatible"
>
>I would say yes, we should be. We can remove the commands later, but we
>shouldn't break backwards compatibility if the tarball was already
>released.
>
Nobody will comply if we remove it because
sssd-1.14.0 is used very very rarely
and if somebody complain we can provide old aliases in 1.14.x
IMHO, it does not worth to add complexity for unused feature
because of "backward compatibility"
>>
>> a) there was a desing[1] and nobody complained
>> b) 1.14.0 is used very rarely
>>
>> BTW We should firstly change desing page. Then ask for review and then
>> send a patch. Cond-NACK to patch till desing page will be updated
>
>Fine by me.
>
>> and
>> change will be accepted by others.
>
>Others who? :-)
non developers (The person who requested this change; I assume this
change was not requested by developers)
In other words, the same process as with creating new design document.
LS
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
